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

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

[Gerd Hoffmann]

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



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

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

[Michael Brown]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Gerd Hoffmann]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ard Biesheuvel]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Michael Brown]

On 19/07/2023 17:52, Ard Biesheuvel wrote:
> 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:
>>> 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?

Even with a single orl/andl instruction, the operation is unlocked. 
It's guaranteed atomic against interrupts (since interrupts always occur 
at instruction boundaries) but it's not guaranteed atomic against 
concurrent accesses to the same global variable from other processors.

(I have no idea if the UEFI model allows APs to call into the IOMMU 
protocol or not, so I don't know if this is a real problem.)

On a quick review of the code, there appear to be other points that also 
modify mReservedMemBitmap (IoMmuAllocateCommonBuffer() and 
IoMmuFreeCommonBuffer()).  I'd guess that these also need to raise to 
TPL_NOTIFY, but I'm not familiar with the code so I don't know if 
there's anything that makes this unnecessary.

Sorry not to be more help.

Thanks,

Michael



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

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

[Ard Biesheuvel]

On Wed, 19 Jul 2023 at 19:40, Michael Brown <mcb30@ipxe.org> wrote:
>
> On 19/07/2023 17:52, Ard Biesheuvel wrote:
> > 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:
> >>> 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?
>
> Even with a single orl/andl instruction, the operation is unlocked.
> It's guaranteed atomic against interrupts (since interrupts always occur
> at instruction boundaries) but it's not guaranteed atomic against
> concurrent accesses to the same global variable from other processors.
>
> (I have no idea if the UEFI model allows APs to call into the IOMMU
> protocol or not, so I don't know if this is a real problem.)
>

No, it's not really a problem. While there is a notion of 'run
function F() on AP #n' in the MpServices protocol, there is no real
SMP support where the same code and data are being used concurrently
on multiple cores in parallel.

> On a quick review of the code, there appear to be other points that also
> modify mReservedMemBitmap (IoMmuAllocateCommonBuffer() and
> IoMmuFreeCommonBuffer()).  I'd guess that these also need to raise to
> TPL_NOTIFY, but I'm not familiar with the code so I don't know if
> there's anything that makes this unnecessary.
>

Thanks for mentioning that - perhaps Gerd could have another look? (I
merged the v2 already)


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

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

[Gerd Hoffmann]

  Hi,

> > On a quick review of the code, there appear to be other points that also
> > modify mReservedMemBitmap (IoMmuAllocateCommonBuffer() and
> > IoMmuFreeCommonBuffer()).  I'd guess that these also need to raise to
> > TPL_NOTIFY, but I'm not familiar with the code so I don't know if
> > there's anything that makes this unnecessary.
> >
> 
> Thanks for mentioning that - perhaps Gerd could have another look? (I
> merged the v2 already)

Patch sent.  Same pattern.  Didn't notice we have that twice (once for
explicitly allocating buffers and once for internally used bounce
buffers).

take care,
  Gerd



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

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

[Gerd Hoffmann]

> > >   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?

Yes.

> > 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.

ARM means v7 only or both v7+v8?

take care,
  Gerd



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

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

[Ard Biesheuvel]

On Thu, 20 Jul 2023 at 10:28, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > > >   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?
>
> Yes.
>
> > > 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.
>
> ARM means v7 only or both v7+v8?
>

All of it.


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