Re: [PATCH v3 2/5] iommufd: Destroy vdevice on idevice destroy

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Jul 07, 2025 at 06:58:45PM +0800, Xu Yilun wrote:

> I still see concerns here. The pre_destroy() and wait_event is before the
> idev's users count decreasing to 0, which means a new user could get
> in.

Hum, yes it seems we could get a race with another vdevice being
created during idevice destruction, but I think this could be fixed in
a straightforward way with a flag 'being destroyed' in the idev under
the lock if it is the only issue

> The worse thing is the new user could hold the shortterm_users until
> pre_destroy(), but there isn't a second pre_destroy()...

This is OK, the point of pre_destroy is to remove the reference
cycle. So it needs to prevent new vdevs from establishing references
and remove the reference from any existing vdev.

refcounts outside of this cycle are handled by the existing
mechanisms - pre_destroy is only about the refcount loop.

> I think extending the life of shortterm_users at runtime makes things
> complex. 

Well, it keeps the refcounting working the way it is supposed
to. Holding a pointer without a reference is nasty and is creating
alot of these weird problems and strange flows. Hold the refcount
along with the pointer and things follow the normal patterns.

There is nothing inherently wrong with shortterm users for this
besides its name. Think of it is as 'wait for users', and we clearly
want to wait for the vdev user to put back its idev reference.

I think there are too many gyrations with refcounts in this version
below. It is hard to reason about if zero refcounts are re-elevated to
non-zero.

>         vdev = iommufd_get_vdevice(idev->ictx, idev->vdev->obj.id);
> -       if (IS_ERR(vdev))
> +       if (IS_ERR(vdev)) {
> +               /*
> +               * vdev is removed from xarray by userspace, but is not
> +               * destroyed. We should wait for vdev real destruction before
> +               * idev destruction.
> +               */
> +               iommufd_object_destroy_wait_prepare(&idev->obj);
>                 goto out_unlock;

This is now really weird, we are mangling the refcounts and the
reasoning why it is safe with the concurrent vdev thread is tricky..

> +/*
> + * iommufd_object_destroy_wait_complete() should be called by first-destroyer
> + * in its destroy() ops. The first-destroyer should not access the
> + * later-destroyer after calling this function.
> + */
> +static inline void
> +iommufd_object_destroy_wait_complete(struct iommufd_ctx *ictx,
> +                                    struct iommufd_object *to_destroy)
> +{
> +       if (refcount_read(&to_destroy->users) != 0 ||
> +           refcount_read(&to_destroy->shortterm_users) != 1)
> +               return;
> +
> +       refcount_dec(&to_destroy->shortterm_users);
> +       wake_up_interruptible_all(&ictx->destroy_wait);
> +}

This looks weird too, why is refcount_read safe? That is usually not
how this kind of logic is written.

I'm not sure this is OK, it is not an obvious known-good pattern for
lifetime management.

Jason