Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 · Low: 2
- [Low] The patch establishes 'PSP device stopped being visible in a
netns' => PSP_CMD_DEV_DEL_NTF (net/psp/psp_nl.c:381-388), but the…
- [Low] The commit fixes user-visible Generic Netlink behaviour ('the
assoc-list it carried still contained the device which was going away…
--
psp: notify about a disassociation once it has happened
The disassociation notification is moved so it is generated after the
association entry has been unlinked, and a netns which loses its last
associated device is sent a dev-del-ntf instead (using the netns taken
from the netdev). Both the netlink dev-disassoc path and the
NETDEV_UNREGISTER path are converted to the new helper.
> diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
> index 91473f96ad211..273b010d2355c 100644
> --- a/net/psp/psp_main.c
> +++ b/net/psp/psp_main.c
[ ... ]
> @@ -438,9 +440,13 @@ static int psp_netdev_event(struct notifier_block *nb,
> unsigned long event,
> if (psd && psp_dev_tryget(psd)) {
> rcu_read_unlock();
> mutex_lock(&psd->lock);
> - if (psp_dev_is_registered(psd))
> - psp_nl_notify_dev(psd, PSP_CMD_DEV_CHANGE_NTF);
> - psp_dev_disassoc_one(psd, dev);
> + /* Nothing to report if the device was never on the list,
> + * dev-assoc may have failed after publishing dev->psp_dev,
> + * and this is also the main netdevice's path.
> + */
> + if (psp_dev_disassoc_one(psd, dev) &&
> + psp_dev_is_registered(psd))
> + psp_nl_notify_disassoc(psd, dev_net(dev));
> mutex_unlock(&psd->lock);
> psp_dev_put(psd);
> } else {
> diff --git a/net/psp/psp_nl.c b/net/psp/psp_nl.c
> index b57366b5e032b..cdfc2d72fb390 100644
> --- a/net/psp/psp_nl.c
> +++ b/net/psp/psp_nl.c
> @@ -356,6 +356,40 @@ void psp_nl_notify_dev(struct psp_dev *psd, u32 cmd)
> psp_nl_build_dev_ntf, &cmd);
> }
>
> +/**
> + * psp_nl_notify_disassoc() - notify about a device losing an association
> + * @psd: PSP device (must be locked)
> + * @net: netns of the netdevice which got disassociated
> + *
> + * Must be called once @psd no longer has the association, so that the
> + * notifications carry the state after the change.
> + */
> +void psp_nl_notify_disassoc(struct psp_dev *psd, struct net *net)
> +{
> + struct sk_buff *ntf;
> + bool still_visible;
> + u32 cmd;
> +
> + lockdep_assert_held(&psd->lock);
> +
> + psp_nl_notify_dev(psd, PSP_CMD_DEV_CHANGE_NTF);
> +
> + /* psp_nl_notify_dev() reaches the main netdevice's netns and every
> + * netns which still has an associated device. If @net is neither,
> + * the device is gone from @net and we should send a delete ntf.
> + */
> + still_visible = !psp_dev_check_access(psd, net, false);
> + if (still_visible || !maybe_get_net(net))
> + return;
> +
> + cmd = PSP_CMD_DEV_DEL_NTF;
> + ntf = psp_nl_build_dev_ntf(psd, net, &cmd);
> + if (ntf)
> + genlmsg_multicast_netns(&psp_nl_family, net, ntf, 0,
> + PSP_NLGRP_MGMT, GFP_KERNEL);
> + put_net(net);
> +}
[Severity: Low]
This isn't a bug, but should the assoc side be made symmetric with this?
After this change, losing visibility of a PSP device in a netns produces
PSP_CMD_DEV_DEL_NTF, while gaining visibility for the first time still
produces only a change notification in psp_nl_dev_assoc_doit():
list_add_tail(&psp_assoc_dev->dev_list, &psd->assoc_dev_list);
psd->assoc_dev_cnt++;
put_net(net);
psp_nl_notify_dev(psd, PSP_CMD_DEV_CHANGE_NTF);
For a netns where this is the first association, psp_dev_check_access(psd,
net, false) would have returned -ENOENT just before the list_add_tail(),
i.e. the device was not visible there at all, yet the netns is only told
about it with a change notification.
Documentation/netlink/specs/psp.yaml describes dev-add-ntf as "Notification
about device appearing" and dev-del-ntf as "Notification about device
disappearing", and psp_dev_create()/psp_dev_unregister() use that add/del
pair. A mgmt listener in an associated namespace would then get a change-ntf
for an object it was never told about, followed later by a del-ntf for it.
Since notification semantics become uAPI once released, would it make sense
to emit PSP_CMD_DEV_ADD_NTF to a namespace which becomes able to see the
device for the first time?
> +
> int psp_nl_dev_get_doit(struct sk_buff *req, struct genl_info *info)
> {
> struct psp_dev *psd = info->user_ptr[0];
[ ... ]
> @@ -620,13 +654,6 @@ int psp_nl_dev_disassoc_doit(struct sk_buff *skb, struct
> genl_info *info)
> return -ENOMEM;
> }
>
> - put_net(net);
> -
> - /* Notify before removal so listeners in the disassociated namespace
> - * still receive the notification.
> - */
> - psp_nl_notify_dev(psd, PSP_CMD_DEV_CHANGE_NTF);
> -
> /* Remove from the association list */
> list_del(&found->dev_list);
> psd->assoc_dev_cnt--;
> @@ -634,6 +661,9 @@ int psp_nl_dev_disassoc_doit(struct sk_buff *skb, struct
> genl_info *info)
> netdev_put(found->assoc_dev, &found->dev_tracker);
> kfree(found);
>
> + psp_nl_notify_disassoc(psd, net);
> + put_net(net);
> +
> return psp_nl_reply_send(rsp, info);
> }
[Severity: Low]
Should this carry Fixes: tags?
The changelog says the old ordering "is still wrong", and the corrected
behaviour is user-visible netlink content in two paths that both exist in
released code:
- psp_nl_dev_disassoc_doit(), the notify-before-removal block removed here,
came from commit 06c2dce2d0f6 ("psp: add new netlink cmd for dev-assoc
and dev-disassoc")
- psp_netdev_event(), converted to psp_nl_notify_disassoc(), came from
commit 0ddb69e2406e ("psp: add a new netdev event for dev unregister")
Documentation/process/maintainer-netdev.rst says "for fixes the ``Fixes:``
tag is required, regardless of the tree", so adding the two trailers would
make stable selection and backporting reliable.
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260912200426.121025-1-kuba%40kernel.org
