Re: [PATCH net-next 4/6] psp: notify about a disassociation once it has happened
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-15 06:04:26
Also in:
linux-kselftest
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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 {
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
+ int psp_nl_dev_get_doit(struct sk_buff *req, struct genl_info *info) { struct psp_dev *psd = info->user_ptr[0];
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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