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

["Gerd Hoffmann" <kraxel@redhat.com>]

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, but
yes, it's not guaranteed to happen, the compiler can choose to generate
a series of load + and + store instructions instead.

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

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107058): https://edk2.groups.io/g/devel/message/107058
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]
-=-=-=-=-=-=-=-=-=-=-=-