Re: [PATCH net-next 2/6] psp: don't report the main netdevice's ifindex to associated namespaces

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] psp_nl_dev_fill() (net/psp/psp_nl.c:294-312) stops emitting
  PSP_A_DEV_IFINDEX in every dev-get reply / dump entry / dev-…
- [Low] The emitter contract changed (PSP_A_DEV_IFINDEX is now optional
  in dev-get replies/dumps) but the in-tree consumers still treat it as…
- [Low] As of this patch alone, psp.yaml documents that PSP_A_DEV_IFINDEX
  is 'Only reported to the network namespace the main netdevice lives…
- [Low] psp_nl_dev_fill() (net/psp/psp_nl.c:301-305) decides whether to
  report PSP_A_DEV_IFINDEX with `cur_net == dev_net(psd->main_netdev)`…

--

psp: don't report the main netdevice's ifindex to associated namespaces

PSP_A_DEV_IFINDEX is moved inside the branch that handles the main
netdevice's namespace in psp_nl_dev_fill(), so namespaces which only see
the PSP device by association no longer get the main netdev's ifindex.
The psp.yaml spec documentation for the attribute is updated to describe
the new rule.

A few questions below.

> diff --git a/Documentation/netlink/specs/psp.yaml b/Documentation/netlink/specs/psp.yaml
> index e9c2ee7e28e02..f3266763c3253 100644
> --- a/Documentation/netlink/specs/psp.yaml
> +++ b/Documentation/netlink/specs/psp.yaml
> @@ -38,6 +38,8 @@ attribute-sets:
>          doc: |
>            ifindex of the main netdevice linked to the PSP device,
>            or the ifindex to associate with the PSP device.
> +          Only reported to the network namespace the main netdevice
> +          lives in, an ifindex has no meaning outside of it.
>          type: u32

[Severity: Low]
As of this commit, is the newly documented rule verified anywhere in
tree?  The only test which inspects dev-get output taken from an
associated namespace is _psp_dev_get_check_netkit_psp_assoc() (Check 2)
in tools/testing/selftests/drivers/net/psp.py, and at this commit it only
looks for the by-association entry and validates the assoc-list.

Note the following patch in the series, "selftests: drv-net: psp: check
the ifindex an associated netns sees", adds exactly the missing
assertions:

        ksft_true('ifindex' not in peer_dev,
                  "ifindex reported to an associated namespace")

and the equivalent check in _check_disassoc_ntf(), so the gap is closed
before the end of the series.  Mentioning this only for completeness.

> diff --git a/net/psp/psp_nl.c b/net/psp/psp_nl.c
> index f91665748dde0..b57366b5e032b 100644
> --- a/net/psp/psp_nl.c
> +++ b/net/psp/psp_nl.c
> @@ -294,13 +294,16 @@ psp_nl_dev_fill(struct psp_dev *psd, struct sk_buff *rsp,
>  		return -EMSGSIZE;
>  
>  	if (nla_put_u32(rsp, PSP_A_DEV_ID, psd->id) ||
> -	    nla_put_u32(rsp, PSP_A_DEV_IFINDEX, psd->main_netdev->ifindex) ||
>  	    nla_put_u32(rsp, PSP_A_DEV_PSP_VERSIONS_CAP, psd->caps->versions) ||
>  	    nla_put_u32(rsp, PSP_A_DEV_PSP_VERSIONS_ENA, psd->config.versions))
>  		goto err_cancel_msg;
>  
>  	if (cur_net == dev_net(psd->main_netdev)) {
> -		/* Primary device - dump assoc list */
> +		/* Primary device - report the netdev, dump assoc list. */
> +		if (nla_put_u32(rsp, PSP_A_DEV_IFINDEX,
> +				psd->main_netdev->ifindex))
> +			goto err_cancel_msg;
> +

[Severity: Medium]
Should this carry a Fixes: tag?

The unconditional PSP_A_DEV_IFINDEX in psp_nl_dev_fill() has been in the
dev-get reply, the dump entries and the dev add/change/del notifications
since 06c2dce2d0f69 ("psp: add new netlink cmd for dev-assoc and
dev-disassoc"), which was based on v7.1-rc7 and therefore shipped in the
released v7.2.  The tree here is v7.3.0-rc2.

That means the message layout narrowed between two released kernels for
the same command: an application built against v7.2 sees the attribute in
by-association messages, while v7.3 and later omit it, and netlink gives
userspace no way to probe for the difference.

Would adding

  Fixes: 06c2dce2d0f69 ("psp: add new netlink cmd for dev-assoc and dev-disassoc")

plus a sentence in the changelog about the userspace-visible narrowing be
appropriate, so the correction lands in the same stable trees as the
feature?

[Severity: Low]
With the attribute now optional, do the in-tree consumers still handle a
dump that mixes entries with and without it?

tools/testing/selftests/drivers/net/psp.py, _init_psp_dev():

        for dev in cfg.pspnl.dev_get({}, dump=True):
            if dev['ifindex'] == target_ifindex:

ynl omits absent attributes, so a dump containing a by-association entry
raises a Python KeyError here and aborts the test file, where before this
patch it fell through to the clean:

        raise KsftSkipEx("No PSP devices found")

Would dev.get('ifindex') be better?  In the suite as configured every
_init_psp_dev() call uses the host-namespace socket, so the current tests
should not hit it, but the dump is no longer guaranteed to contain the
key.

The same applies more mildly to tools/testing/selftests/drivers/net/
psp_responder.c, in the -i selection loop in main():

        if (opts.ifindex) {
            if (d->ifindex != opts.ifindex)
                continue;

With the attribute absent d->ifindex is 0, so such a device is silently
skipped and the responder reports the device as not found.  This one is
not a behaviour regression as such, since the top-level ifindex it
compared previously belonged to another namespace anyway.

[Severity: Low]
Can the namespace test and the ifindex read disagree here?

The branch predicate reads dev_net(psd->main_netdev) and the attribute
then performs a second, independent read of psd->main_netdev->ifindex.
The psp family is registered with parallel_ops and the dev-get and
notification paths take only psd->lock, no RTNL, while a concurrent
migration in netif_change_net_namespace() publishes the netns before the
new ifindex:

net/core/dev.c:netif_change_net_namespace() {
    ...
    netdev_lock(dev);
    dev_net_set(dev, net);
    netdev_unlock(dev);
    dev->ifindex = new_ifindex;
    ...
}

Nothing in psp re-pins the psd on a netns move - psp_netdev_event() bails
out for anything other than NETDEV_UNREGISTER:

net/psp/psp_main.c:psp_netdev_event() {
    if (event != NETDEV_UNREGISTER)
        return NOTIFY_DONE;
    ...
}

so a reader can pass the test with the old netns and then serialize the
ifindex the device has in its new namespace, which is the case the doc
hunk declares meaningless.  Symmetrically, a notification built for the
main_net already pinned by psp_nl_multicast_per_ns() can flip to the
by-association encoding, since psp_nl_dev_fill() re-reads dev_net()
rather than using that pinned net:

net/psp/psp_nl.c:psp_nl_multicast_per_ns() {
    /* device may be changing netns in parallel */
    rcu_read_lock();
    main_net = maybe_get_net(dev_net_rcu(psd->main_netdev));
    rcu_read_unlock();
    ...
}

Would reading the netns and the ifindex once under a single
rcu_read_lock() with dev_net_rcu(), or passing the already-pinned
main_net down into psp_nl_dev_fill(), close the window?  The
consequence is only a stale or foreign u32 in one message.

>  		err = psp_nl_fill_assoc_dev_list(psd, rsp, cur_net, NULL);
>  		if (err)
>  			goto err_cancel_msg;

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