Re: [PATCH net-next v2 3/9] netlink: specs: devlink: describe DEVLINK_ATTR_NESTED_DEVLINK

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The new `dl-nested-devlink` fractional set added by "netlink:
  specs: devlink: describe DEVLINK_ATTR_NESTED_DEVLINK"…

--

netlink: specs: devlink: describe DEVLINK_ATTR_NESTED_DEVLINK

This fills in the single attribute hidden by the "# TODO: fill in the
attributes in between" gap before selftests (176), adding nested-devlink
(175) as a multi-attr nest, plus a new dl-nested-devlink fractional set
and the attribute in the dev-get reply.

> diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml
> index 0e0791d4e2c74..7ec52f81c3230 100644
> --- a/Documentation/netlink/specs/devlink.yaml
> +++ b/Documentation/netlink/specs/devlink.yaml

[ ... ]

> @@ -1351,6 +1352,19 @@ attribute-sets:
>        -
>          name: index
>  
> +  -
> +    name: dl-nested-devlink
> +    subset-of: devlink
> +    attributes:
> +      -
> +        name: bus-name
> +      -
> +        name: dev-name
> +      -
> +        name: index
> +      -
> +        name: netns-id
                ^^^^^^^^^

[Severity: Medium]
Is the inherited type right for netns-id here?  Since this is a
subset-of set, it picks up the main devlink set declaration:

      -
        name: netns-id
        type: u32

But the kernel emits this attribute signed, in
devlink_nl_put_nested_handle() (net/devlink/netlink.c), which is the
function reached by devlink_nl_fill() -> devlink_nl_nested_fill() for
each nested_rels entry:

	if (!net_eq(net, devl_net)) {
		int id = peernet2id_alloc(net, devl_net, GFP_ATOMIC);

		rcu_read_unlock();
		if (nla_put_s32(msg, DEVLINK_ATTR_NETNS_ID, id))
			return -EMSGSIZE;

peernet2id_alloc() in net/core/net_namespace.c returns the negative
sentinel NETNSA_NSID_NOT_ASSIGNED (-1, include/uapi/linux/net_namespace.h)
in three cases:

	if (!check_net(net))
		return NETNSA_NSID_NOT_ASSIGNED;
	...
	if (!maybe_get_net(peer)) {
		spin_unlock(&net->nsid_lock);
		return NETNSA_NSID_NOT_ASSIGNED;
	}
	...
	if (id < 0)
		return NETNSA_NSID_NOT_ASSIGNED;

The wire width is 4 bytes either way, so nothing breaks in the kernel,
but would ynl-generated decoders and C structs now render the "nsid not
assigned" case as 4294967295, with no way to tell it apart from a valid
id?

The request side does read the attribute unsigned, in devlink_netns_get()
(net/devlink/dev.c):

	} else if (netns_id_attr) {
		net = get_net_ns_by_id(sock_net(skb->sk),
				       nla_get_u32(netns_id_attr));

so flipping the shared declaration would change the request typing too.
Would it be better to declare the reply-side attribute as s32, or
otherwise narrow the type per-use, so the documented type matches
nla_put_s32()?

One more note on scope: a later patch in this series, "netlink: specs:
devlink: describe the netns id in the parent-dev nest", reuses this same
subset for DEVLINK_ATTR_PARENT_DEV replies, and at 0fe71b34ba2e
("devlink: validate the port index in the rate set request") netns-id is
still type: u32 while the emitter still uses nla_put_s32(), so the
mismatch persists through the end of the series.

> +
>  operations:
>    enum-model: directional
>    list:
> @@ -1376,6 +1390,7 @@ operations:
>              - index
>              - reload-failed
>              - dev-stats
> +            - nested-devlink
>        dump:
>          reply: *get-reply
>

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