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

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

On Wed, 19 Jul 2023 at 18:31, Gerd Hoffmann <kraxel@redhat.com> 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
>
> v2:
>  - add locking to IoMmuFreeBounceBuffer too, clearing bits in
>    mReservedMemBitmap is not guaranteed to be atomic (Michael Brown).
>

Please put this under the --- so I don't have to remove manually it
when applying.

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Pushed as #4665

Thanks,

> ---
>  OvmfPkg/IoMmuDxe/IoMmuBuffer.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
> index c8f6cf4818e8..103003cae376 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);
>
> @@ -395,6 +398,8 @@ IoMmuFreeBounceBuffer (
>    IN OUT     MAP_INFO  *MapInfo
>    )
>  {
> +  EFI_TPL  OldTpl;
> +
>    if (MapInfo->ReservedMemBitmap == 0) {
>      gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);
>    } else {
> @@ -407,9 +412,11 @@ IoMmuFreeBounceBuffer (
>        mReservedMemBitmap,
>        mReservedMemBitmap & ((UINT32)(~MapInfo->ReservedMemBitmap))
>        ));
> +    OldTpl                     = gBS->RaiseTPL (TPL_NOTIFY);
>      MapInfo->PlainTextAddress  = 0;
>      mReservedMemBitmap        &= (UINT32)(~MapInfo->ReservedMemBitmap);
>      MapInfo->ReservedMemBitmap = 0;
> +    gBS->RestoreTPL (OldTpl);
>    }
>
>    return EFI_SUCCESS;
> --
> 2.41.0
>
>
>
> 
>
>


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