Re: [PATCH v7 net-next 3/5] psp: add a new netdev event for dev unregister

[Wei Wang <weibunny.kernel@gmail.com>]

On Mon, Mar 23, 2026 at 11:24 AM Simon Horman <horms@kernel.org> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
>
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> psp: add a new netdev event for dev unregister
>
> This commit adds a netdev notifier to handle device unregistration events
> and clean up PSP device associations when the associated network device is
> removed.
>
> > diff --git a/net/psp/psp.h b/net/psp/psp.h
> > index 0f9c4e4e52cb..fd7457dedd30 100644
> > --- a/net/psp/psp.h
> > +++ b/net/psp/psp.h
> > @@ -15,6 +15,7 @@ extern struct mutex psp_devs_lock;
> >
> >  void psp_dev_free(struct psp_dev *psd);
> >  int psp_dev_check_access(struct psp_dev *psd, struct net *net, bool admin);
> > +void psp_attach_netdev_notifier(void);
> >
> >  void psp_nl_notify_dev(struct psp_dev *psd, u32 cmd);
> >
> > diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
> > index 178b848989f1..db4593e76fa7 100644
> > --- a/net/psp/psp_main.c
> > +++ b/net/psp/psp_main.c
>
> [ ... ]
>
> > +/**
> > + * psp_attach_netdev_notifier() - register netdev notifier on first use
> > + *
> > + * Register the netdevice notifier when the first device association
> > + * is created. In many installations no associations will be created and
> > + * the notifier won't be needed.
> > + *
> > + * Must be called without psd->lock held, due to lock ordering:
> > + * rtnl_lock -> psd->lock (the notifier callback runs under rtnl_lock
> > + * and takes psd->lock).
> > + */
> > +void psp_attach_netdev_notifier(void)
> > +{
> > +     if (READ_ONCE(psp_notifier_registered))
> > +             return;
> > +
> > +     mutex_lock(&psp_devs_lock);
> > +     if (!psp_notifier_registered) {
> > +             register_netdevice_notifier(&psp_netdev_notifier);
> > +             WRITE_ONCE(psp_notifier_registered, true);
>                                                              ^^^^
>
> What happens if register_netdevice_notifier() fails? The function ignores
> the return value and sets psp_notifier_registered to true anyway.
>
> register_netdevice_notifier() can fail with errors like -ENOMEM or errors
> from replaying existing device registrations. If registration fails,
> psp_notifier_registered will be permanently set to true, preventing any
> future registration attempts.
>
> This means the notifier will never be registered, so NETDEV_UNREGISTER
> events will be silently missed and PSP device associations will never be
> cleaned up on device removal.

Adding the failing check in the next version.

>
> > +     }
> > +     mutex_unlock(&psp_devs_lock);
> > +}
> > +
> >  static int __init psp_init(void)
> >  {
> >       mutex_init(&psp_devs_lock);
>
> [ ... ]
>
> > diff --git a/net/psp/psp_nl.c b/net/psp/psp_nl.c
> > index aa60a8277829..44e00add4211 100644
> > --- a/net/psp/psp_nl.c
> > +++ b/net/psp/psp_nl.c
> > @@ -515,6 +515,13 @@ int psp_nl_dev_assoc_doit(struct sk_buff *skb, struct genl_info *info)
> >
> >       psp_nl_notify_dev(psd, PSP_CMD_DEV_CHANGE_NTF);
> >
> > +     /* Register netdev notifier for assoc cleanup on success.
> > +      * Must drop psd->lock to ensure lock ordering: rtnl_lock -> psd->lock
> > +      */
> > +     mutex_unlock(&psd->lock);
> > +     psp_attach_netdev_notifier();
> > +     mutex_lock(&psd->lock);
> > +
>
> Can dropping psd->lock here create a use-after-free window?
>
> The pre_doit callback psp_device_get_and_lock() obtains psd via xa_load()
> and holds psd->lock, but does NOT take a refcount on psd.
>
> When psd->lock is dropped here, psp_dev_unregister() can acquire both
> psp_devs_lock and psd->lock, complete teardown, and call psp_dev_put()
> which drops the refcount to 0:
>
> net/psp/psp_main.c:psp_dev_unregister() {
>     ...
>     mutex_lock(&psp_devs_lock);
>     ...
>     mutex_lock(&psd->lock);
>     ...
>     xa_erase(&psp_devs, psd->id);
>     ...
>     mutex_unlock(&psd->lock);
>     mutex_unlock(&psp_devs_lock);
>
>     psp_dev_put(psd);  // can trigger psp_dev_free()
> }
>
> This would trigger psp_dev_free() which calls mutex_destroy(&psd->lock)
> and kfree_rcu(psd, rcu).
>
> When this function then calls mutex_lock(&psd->lock), it operates on a
> destroyed mutex and logically freed memory. The post_doit callback
> psp_device_unlock() would then call mutex_unlock(&psd->lock) on the same
> destroyed mutex.
>
> Should a refcount be taken on psd before dropping the lock, or should the
> notifier registration be moved outside the doit handler?
>

Will move psp_attach_netdev_notifier() to pre_doit handler.

> >       return psp_nl_reply_send(rsp, info);
> >  }