Re: [PATCH net-next 6/6] selftests: drv-net: psp: check the PSP disassociation notifications

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] `_dev_disassoc_notify_one_of_two_netkit()` in
  tools/testing/selftests/drivers/net/psp.py does not verify the…

--

selftests: drv-net: psp: check the PSP disassociation notifications

Adds two new psp.py subtests which subscribe to the mgmt multicast group in
the main and guest namespaces, then verify the notifications generated by
dev-disassoc and by netdevice removal.  Also extends
_psp_dev_assoc_cleanup_on_netkit_del() to check the same notifications.

[ ... ]

[ ... ]

+def _get_dev_ntf(cfg, pspnl, label):
+    """Wait for the next notification about the PSP device under test."""
+    for ntf in pspnl.poll_ntf(duration=10):
+        if ntf['msg'].get('id') == cfg.psp_dev_id:
+            return ntf
+    raise KsftFailEx(f"No notification received in the {label} namespace")
+
+
+def _check_disassoc_ntf(cfg, main_pspnl, peer_pspnl, ifindex):
+    """Check the notifications for a netns losing its last association."""
+    ntf = _get_dev_ntf(cfg, main_pspnl, "main")
+    ksft_eq(ntf['name'], 'dev-change-ntf')
+    for assoc in ntf['msg'].get('assoc-list', []):
+        if assoc['nsid'] != cfg.psp_dev_peer_nsid:
+            continue
+        ksft_ne(assoc['ifindex'], ifindex,
+                "Disassociated device still listed in the notification")
+
+    # The device is gone as far as the disassociated namespace is concerned
+    ntf = _get_dev_ntf(cfg, peer_pspnl, "guest")
+    ksft_eq(ntf['name'], 'dev-del-ntf')
+    ksft_true('ifindex' not in ntf['msg'],
+              "ifindex reported to an associated namespace")

[ ... ]

+def _dev_disassoc_notify_one_of_two_netkit(cfg):
+    """Test the notifications with two netkits associated in one netns.
+
+    Disassociating the first netkit leaves the PSP device visible in the
+    guest namespace, generates a dev-change-ntf.
+    Disassociating the second one takes the device out of its view,
+    generates 'dev-del-ntf'.
+    """

[ ... ]

+    main_pspnl, peer_pspnl = _subscribe_mgmt(cfg)
+
+    # One of the two goes away, the device stays visible in the guest netns
+    cfg.pspnl.dev_disassoc({'id': cfg.psp_dev_id, 'ifindex': tmp_ifindex,
+                            'nsid': cfg.psp_dev_peer_nsid})
+
+    ntf = _get_dev_ntf(cfg, main_pspnl, "main")
+    ksft_eq(ntf['name'], 'dev-change-ntf')
+
+    ntf = _get_dev_ntf(cfg, peer_pspnl, "guest")
+    ksft_eq(ntf['name'], 'dev-change-ntf')
+    found = False
+    for assoc in ntf['msg'].get('assoc-list', []):
+        ksft_ne(assoc['ifindex'], tmp_ifindex,
+                "Disassociated device still listed in the notification")
+        found |= assoc['ifindex'] == cfg.nk_guest_ifindex
+    ksft_true(found, "Remaining association missing from the notification")

[Severity: Medium]

Does this block actually check the property the commit message describes,
that "a namespace which still has another association is only told about
the change"?

_get_dev_ntf() returns as soon as it sees one message for the device, so
only the first guest notification is inspected.  If the kernel also sent a
dev-del-ntf to the guest here (i.e. still_visible in
psp_nl_notify_disassoc() were computed wrongly), nothing in this phase
would notice it.

The stale message is not just ignored, it is later consumed as if it were
fresh.  ynl queues notifications that poll_ntf() did not hand out:

tools/net/ynl/pyynl/lib/ynl.py:poll_ntf() {
	...
	while True:
		try:
			yield self.async_msg_queue.get_nowait()
	...
}

so a bogus dev-del-ntf from the first dev_disassoc() stays in
peer_pspnl's queue, and the second phase picks it up:

_check_disassoc_ntf() {
	...
	ntf = _get_dev_ntf(cfg, peer_pspnl, "guest")
	ksft_eq(ntf['name'], 'dev-del-ntf')
	ksft_true('ifindex' not in ntf['msg'],
		  "ifindex reported to an associated namespace")
}

Both assertions pass on the stale message, because psp_nl_dev_fill() only
emits PSP_A_DEV_IFINDEX when cur_net == dev_net(psd->main_netdev), so any
del-ntf destined for a non-main netns lacks ifindex.

That leaves the test only able to catch a missing notification, not an
extra one.  Would it make sense to drain both sockets after the first
change-ntf and assert that no further notification for this device arrives
(a short poll_ntf() that must yield nothing), before running the second
dev_disassoc()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260912200426.121025-1-kuba%40kernel.org