Re: [PATCH 00/14] Fix memory region use-after-finalization

[Peter Xu <peterx@redhat.com>]

On Fri, Oct 10, 2025 at 07:20:04PM +0900, Akihiko Odaki wrote:
> On 2025/10/03 0:03, Paolo Bonzini wrote:
> > On 9/17/25 12:32, Akihiko Odaki wrote:
> > > Based-on: <20250917-use-v3-0-72c2a6887c6c@rsg.ci.i.u-tokyo.ac.jp>
> > > ("[PATCH v3 0/7] Do not unparent in instance_finalize()")
> > > 
> > > This patch series was spun off from "[PATCH v2 00/15] Fix memory region
> > > leaks and use-after-finalization":
> > > https://lore.kernel.org/qemu-devel/20250915-use-v2-0-
> > > f4c7ff13bfe9@rsg.ci.i.u-tokyo.ac.jp/
> > > 
> > > When developing the next version of "[PATCH 00/16] memory: Stop
> > > piggybacking on memory region owners*", I faced multiple memory region
> > > leaks and use-after-finalization. This series extracts their fixes so
> > > that the number of Cc: won't explode.
> > > 
> > > Patch "qdev: Automatically delete memory subregions" and the succeeding
> > > patches are for refactoring, but patch "vfio-user: Do not delete the
> > > subregion" does fix use-after-finalization.
> > > 
> > > * https://lore.kernel.org/qemu-devel/20250901-mr-v1-0-
> > > dd7cb6b1480b@rsg.ci.i.u-tokyo.ac.jp/
> > > 
> > > Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> > 
> > This makes sense, but I think it is not bisectable, because of this in
> > memory_region_del_subregion():
> > 
> >      assert(subregion->container == mr);
> >      subregion->container = NULL;
> > 
> > You would need to add a temporary
> > 
> >      if (subregion->container == NULL) {
> >          return;
> >      }
> > 
> > and undo it at the end of the series.  Do you agree?  With this change I
> > can apply it.
> 
> It is unnecessary because patch "qdev: Automatically delete memory
> subregions" satisfies the following:
> 
> 1. the device-specific code can assume that subregions they added are
> present until it finishes unrealization. The unrealize() callback can also
> assume the subregions are present and delete them. qdev satisfies this by
> deleting subregions only after calling the unrealize().
> 
> 2. qdev should delete the remaining subregions before it finishes
> unrealization to ensure that the devices are hidden from the guest. qdev
> satisfies this by checking if memory regions have containers before
> deleting.

There's another option, which is to have the auto-detach patch for qdev
merged, meanwhile we can still keep the "good citizens" who do explicit
detachments in unrealize(), so that it won't confuse the reader on a
specific device about when did the MR went away.  I believe there will be
developers get confused by them otherwise.

The auto detach will only be a final safety belt to make sure MRs won't get
leaked.

If that is an option, it may also explain my original question on what
problem we're fixing... the extreme case is what this series removed
afterwards are exactly all such uses that may need an explicit detachment
of MRs (ignoring there can be owner-shared MR links, which is even out of
the picture).  Then the auto-detach feature may fix nothing, so we merge a
patch, looks good, logically sensible, but not functioning.

The reality might be, we may still have some qdev that should unlink the
MRs in unrealize(), but they didn't.  However again I suspect it's rare,
especially considering the recently merged auto-detach in finalize().  I'd
just go and fix them, but only if they're a real problem (aka,
unpluggable). Then, we know why we introduce a change, and especially, when
the change can go away.

OTOH, if we merge auto-detach of qdev MRs, we likely need to stick it
forever because justifying it not needed will be extremely hard if not
impossible.. after all we do not know what it fixes.  So if we can at least
list the devices that may be fixed with it in the commit message, it would
be a benefit.

Once again, feel free to treat my comment as a pure question.

Thanks,

-- 
Peter Xu