Re: [PATCH v4 1/3] memory: prevent dma-reentracy issues

[Alexander Bulekov]

On 230120 1447, Peter Maydell wrote:
> On Fri, 20 Jan 2023 at 14:42, Darren Kenny <darren.kenny@oracle.com> wrote:
> > Generally, this looks good, but I do have a comment below...
> >
> > On Thursday, 2023-01-19 at 02:00:02 -05, Alexander Bulekov wrote:
> > > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
> > > This flag is set/checked prior to calling a device's MemoryRegion
> > > handlers, and set when device code initiates DMA.  The purpose of this
> > > flag is to prevent two types of DMA-based reentrancy issues:
> 
> > > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > > index e05332d07f..90ffaaa4f5 100644
> > > --- a/softmmu/memory.c
> > > +++ b/softmmu/memory.c
> > > @@ -533,6 +533,7 @@ static MemTxResult access_with_adjusted_size(hwaddr 
> > > addr,
> > >      uint64_t access_mask;
> > >      unsigned access_size;
> > >      unsigned i;
> > > +    DeviceState *dev = NULL;
> > >      MemTxResult r = MEMTX_OK;
> > >
> > >      if (!access_size_min) {
> > > @@ -542,6 +543,17 @@ static MemTxResult access_with_adjusted_size(hwaddr 
> > > addr,
> > >          access_size_max = 4;
> > >      }
> > >
> > > +    /* Do not allow more than one simultanous access to a device's IO 
> > > Regions */
> > > +    if (mr->owner &&
> > > +        !mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) 
> > > {
> > > +        dev = (DeviceState *) object_dynamic_cast(mr->owner, 
> > > TYPE_DEVICE);
> >
> > I don't know how likely this is to happen, but according to:
> >
> > - https://qemu-project.gitlab.io/qemu/devel/qom.html#c.object_dynamic_cast
> >
> > it is possible for the object_dynamic_cast() function to return NULL,
> > so it might make sense to wrap the subsequent calls in a test of dev !=
> > NULL.
> 
> Yes. This came up in a previous version of this:
> https://lore.kernel.org/qemu-devel/CAFEAcA8E4nDoAWcj-v-dED-0hDtXGjJNSp3A=kdGF8UOCw0DrQ@mail.gmail.com/
> 
> It's generally a bug to call object_dynamic_cast() and then not check
> the return value.
> 

Sorry I missed that - Will be fixed in V5.
-Alex

> thanks
> -- PMM