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

[Simon Horman <horms@kernel.org>]

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.

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

>  	return psp_nl_reply_send(rsp, info);
>  }