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

["Michael Brown" <mcb30@ipxe.org>]

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);

Thanks,

Michael



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