Re: [edk2-devel] [PATCH 1/1] OvmfPkg/IoMmuDxe: add locking to IoMmuAllocateBounceBuffer

["Ard Biesheuvel" <ardb@kernel.org>]

On Wed, 19 Jul 2023 at 18:32, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Wed, Jul 19, 2023 at 04:04:28PM +0000, Michael Brown wrote:
> > On 19/07/2023 12:33, Gerd Hoffmann wrote:
> > > Searching for an unused bounce buffer in mReservedMemBitmap and
> > > reserving the buffer by flipping the bit is a critical section
> > > which must not be interrupted.  Raise the TPL level to ensure
> > > that.
> > >
> > > Without this fix it can happen that IoMmuDxe hands out the same
> > > bounce buffer twice, causing trouble down the road.  Seen happening
> > > in practice with VirtioNetDxe setting up the network interface (and
> > > calling into IoMmuDxe from a polling timer callback) in parallel with
> > > Boot Manager doing some disk I/O.  An ASSERT() in VirtioNet caught
> > > the buffer inconsistency.
> > >
> > > Full story with lots of details and discussions is available here:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=2211060
> > >
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > ---
> > >   OvmfPkg/IoMmuDxe/IoMmuBuffer.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > >
> > > diff --git a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
> > > index c8f6cf4818e8..7f8a0368ab5d 100644
> > > --- a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
> > > +++ b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
> > > @@ -367,7 +367,9 @@ IoMmuAllocateBounceBuffer (
> > >   {
> > >     EFI_STATUS  Status;
> > >     UINT32      ReservedMemBitmap;
> > > +  EFI_TPL     OldTpl;
> > > +  OldTpl            = gBS->RaiseTPL (TPL_NOTIFY);
> > >     ReservedMemBitmap = 0;
> > >     Status            = InternalAllocateBuffer (
> > >                           Type,
> > > @@ -378,6 +380,7 @@ IoMmuAllocateBounceBuffer (
> > >                           );
> > >     MapInfo->ReservedMemBitmap = ReservedMemBitmap;
> > >     mReservedMemBitmap        |= ReservedMemBitmap;
> > > +  gBS->RestoreTPL (OldTpl);
> > >     ASSERT (Status == EFI_SUCCESS);
> >
> > It looks as though IoMmuFreeBounceBuffer() should also raise to TPL_NOTIFY
> > while modifying mReservedMemBitmap, since the modification made in
> > IoMmuFreeBounceBuffer() is not an atomic operation:
> >
> >   mReservedMemBitmap &= (UINT32)(~MapInfo->ReservedMemBitmap);
>
> I'd expect modern compilers optimize that to a single instruction,

You mean something along the lines of

  andl %reg, mReservedMemBitmap(%rip)

right?


> but
> yes, it's not guaranteed to happen, the compiler can choose to generate
> a series of load + and + store instructions instead.
>

That is sadly all we have on ARM, unless you use LSE atomics, which
are optional in the architecture so we never use those in EDK2.

And this observation makes me slightly uneasy, given there are
probably many other places across the codebase where we rely on such
atomicity, which is only guaranteed in practice on non-NOOPT builds
that target IA32 or X64

> Let's play safe, I'll send v2.
>

Good choice.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107059): https://edk2.groups.io/g/devel/message/107059
Mute This Topic: https://groups.io/mt/100233359/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-