Re: [PATCH net-next 4/8] netlink: specs: devlink: complete the port function nest

[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: 0 · Low: 1 (1)

- [Low] The patch fixes a user-visible tooling breakage (any ynl/spec-
  driven decoder aborts on a devlink port-get do/dump reply with "Space…

Pre-existing issues:
- [Low] The nest newly referenced by the added `devlink` attribute (`dl-
  nested-devlink`, subset-of devlink) declares `netns-id` as `type: u32`…

--

netlink: specs: devlink: complete the port function nest

The dl-port-function attribute set in the devlink spec stopped at caps,
while the kernel also emits DEVLINK_PORT_FN_ATTR_DEVLINK and
DEVLINK_PORT_FN_ATTR_MAX_IO_EQS on port-get do and dump.  The patch adds
both attributes to the spec and regenerates the netlink policy.

> diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml
> index 7ec52f81c3230..962789dfbfacf 100644
> --- a/Documentation/netlink/specs/devlink.yaml
> +++ b/Documentation/netlink/specs/devlink.yaml
> @@ -992,6 +992,14 @@ attribute-sets:
>          type: bitfield32
>          enum: port-fn-attr-cap
>          enum-as-flags: true
> +      -
> +        name: devlink
> +        type: nest
> +        nested-attributes: dl-nested-devlink
> +        doc: Handle of the peer devlink instance instantiated for this function.
> +      -
> +        name: max-io-eqs
> +        type: u32

[Severity: Low]
Two attributes are added here, but the commit message attributes the spec
gap to a single commit:

  Commit 5af3e3876d56 ("devlink: Support setting max_io_eqs") added the
  uAPI value and the hand written policy but never touched the spec.

git blame on enum devlink_port_function_attr in
include/uapi/linux/devlink.h shows the two values come from different
commits:

  0b7a2721e36c11 (Jiri Pirko   2023-09-13) DEVLINK_PORT_FN_ATTR_DEVLINK,  /* nested */
  5af3e3876d567f (Parav Pandit 2024-04-06) DEVLINK_PORT_FN_ATTR_MAX_IO_EQS, /* u32 */

Should the message also mention 0b7a2721e36c1 as the origin of the
missing devlink nest attribute?

Since devlink_nl_port_function_attrs_put() emits both attributes on every
port fill:

  net/devlink/port.c:devlink_nl_port_function_attrs_put() {
	err = devlink_port_fn_max_io_eqs_fill(port, msg, extack, &msg_updated);
	if (err)
		goto out;
	err = devlink_rel_devlink_handle_put(msg, port->devlink,
					     port->rel_index,
					     DEVLINK_PORT_FN_ATTR_DEVLINK,
					     &msg_updated);
  }

any spec-driven decoder fails on such a reply.  Would Fixes: tags for both
commits be appropriate so this gets picked up for the affected stable
trees?

> diff --git a/net/devlink/netlink_gen.c b/net/devlink/netlink_gen.c
> index 30f01901b5877..17d1edcdb935e 100644
> --- a/net/devlink/netlink_gen.c
> +++ b/net/devlink/netlink_gen.c
> @@ -46,17 +46,26 @@ devlink_attr_param_type_validate(const struct nlattr *attr,
>  }
>  
>  /* Common nested types */
> +const struct nla_policy devlink_dl_nested_devlink_nl_policy[DEVLINK_ATTR_INDEX + 1] = {
> +	[DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING, },
> +	[DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING, },
> +	[DEVLINK_ATTR_INDEX] = NLA_POLICY_FULL_RANGE(NLA_UINT, &devlink_attr_index_range),
> +	[DEVLINK_ATTR_NETNS_ID] = { .type = NLA_U32, },
> +};

[Severity: Low]
This is a pre-existing issue and not introduced by this patch, but the new
devlink attribute points at dl-nested-devlink, which makes the netns-id
member reachable from the port-function reply, and the spec declares it
unsigned:

  Documentation/netlink/specs/devlink.yaml:
      -
        name: netns-id
        type: u32

The kernel writes that member as signed, and the value can be the
negative sentinel:

  net/devlink/netlink.c:devlink_nl_put_nested_handle() {
	int id = peernet2id_alloc(net, devl_net, GFP_ATOMIC);

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

  net/core/net_namespace.c:peernet2id_alloc() {
	if (!check_net(net))
		return NETNSA_NSID_NOT_ASSIGNED;
  }

So for a peer devlink instance in another namespace whose nsid is not
assigned, a spec-driven decoder prints 4294967295 instead of -1 and cannot
recognise the not-assigned sentinel.  Length validation is the same for
NLA_U32 and NLA_S32, so kernel side behaviour is unaffected.

Documentation/netlink/specs/netdev.yaml declares the equivalent attribute
as s32:

      -
        name: netns-id
        doc: The network namespace id of the netdev.
        type: s32

Would it make sense to change netns-id in devlink.yaml to s32 (or sint)
here as well?  It is still type: u32 at the end of this series.

[ ... ]

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