Re: [PATCH net-next 7/8] netlink: specs: devlink: describe the netns id in the parent-dev nest

[netdev-bot+sashiko@kernel.org]

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

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

- [High] The new `dl-rate-set` subset
  (Documentation/netlink/specs/devlink.yaml:1406-1432), which the patch…
- [Low] This patch makes `netns-id` part of the described contents of the
  `parent-dev` nest (parent-dev now nests `dl-nested-devlink`,…

Pre-existing issues:
- [Low] The purpose of the change is to describe the netns id carried
  inside the `parent-dev` nest emitted by…

--

netlink: specs: devlink: describe the netns id in the parent-dev nest

The patch points the devlink parent-dev nest at the dl-nested-devlink
subset so the netns id emitted by devlink_nl_put_nested_handle() is
described, and adds a new dl-rate-set subset used as the attribute-set of
the rate-set and rate-new operations so their input keeps nesting
dl-parent-dev.

> diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml
> index 9d816a0b7da41..54de1eeb22347 100644
> --- a/Documentation/netlink/specs/devlink.yaml
> +++ b/Documentation/netlink/specs/devlink.yaml
> @@ -912,7 +912,7 @@ attribute-sets:
>        -
>          name: parent-dev
>          type: nest
> -        nested-attributes: dl-parent-dev
> +        nested-attributes: dl-nested-devlink

[Severity: Low]
With parent-dev now nesting dl-nested-devlink, netns-id becomes part of
the described nest contents, but the root devlink attribute set types it
as unsigned:

      -
        name: netns-id
        type: u32

The kernel writes it signed in 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;

and peernet2id_alloc() returns NETNSA_NSID_NOT_ASSIGNED (-1) when
check_net() fails, when maybe_get_net() on the peer fails, or when
alloc_netid() fails.

The width matches so the wire format is unchanged, but should the type be
sint/s32 so spec-driven decoders show the "not assigned" report as -1
rather than 4294967295?

Note the same attribute is consumed as an unsigned input by
devlink_netns_get(), so the output-side typing may need to be expressed
separately from the input.

[Severity: Low]
This is a pre-existing gap rather than something this patch introduces,
but the nest whose netns id is being described here is still not listed in
any reply in the spec. rate-get reuses the request anchor for its reply:

        request:
          value: 74
          attributes: &rate-id-attrs
            - bus-name
            - dev-name
            - index
            - port-index
            - rate-node-name
        reply: &rate-get-reply
          value: 76
          attributes: *rate-id-attrs

while devlink_nl_rate_fill() also emits RATE_TYPE, RATE_TX_SHARE,
RATE_TX_MAX, RATE_TX_PRIORITY, RATE_TX_WEIGHT, RATE_TC_BWS and, through
devlink_nl_rate_parent_fill(), RATE_PARENT_NODE_NAME and PARENT_DEV.

Since parent-dev appears only in the root attribute set, in dl-rate-set
and in the rate-set/rate-new request lists, does the netns id described
here remain unreachable for ynl-gen-c generated consumers, which build the
reply struct from the listed reply attributes?

>          doc: |
>            Identifies the devlink instance which owns the parent rate node.
>            Used with rate-set and rate-new to parent a rate object to a node on

[ ... ]

> @@ -1390,6 +1390,22 @@ attribute-sets:
>    -
>      name: dl-parent-dev
>      subset-of: devlink
> +    doc: |
> +      Devlink handle accepted as the parent-dev input; the netns id the
> +      kernel reports back is not accepted, the parent is always resolved
> +      in the caller's netns.
> +    attributes:
> +      -
> +        name: bus-name
> +      -
> +        name: dev-name
> +      -
> +        name: index
> +
> +  -
> +    name: dl-rate-set
> +    subset-of: devlink
> +    doc: Attributes accepted by the rate-set and rate-new requests.
>      attributes:
>        -
>          name: bus-name
> @@ -1397,6 +1413,23 @@ attribute-sets:
>          name: dev-name
>        -
>          name: index
> +      -
> +        name: rate-node-name
> +      -
> +        name: rate-tx-share
> +      -
> +        name: rate-tx-max
> +      -
> +        name: rate-tx-priority
> +      -
> +        name: rate-tx-weight
> +      -
> +        name: rate-parent-node-name
> +      -
> +        name: rate-tc-bws
> +      -
> +        name: parent-dev
> +        nested-attributes: dl-parent-dev

[Severity: High]
Is port-index missing from dl-rate-set?  It is the only way to address a
leaf (port) rate object, per devlink_rate_get_from_info() in
net/devlink/rate.c:

	if (attrs[DEVLINK_ATTR_PORT_INDEX])
		return devlink_rate_leaf_get_from_info(devlink, info);
	else if (attrs[DEVLINK_ATTR_RATE_NODE_NAME])
		return devlink_rate_node_get_from_info(rate_devlink, devlink, info);
	else
		return ERR_PTR(-EINVAL);

Before this change the two ops used attribute-set: devlink, so pyynl could
encode port-index.  _encode_message() encodes request fields out of the
op's attribute set and _add_attr() raises on an unknown name:

        try:
            attr = self.attr_sets[space][name]
        except KeyError as err:
            raise YnlException(f"Space '{space}' has no attribute '{name}'") from err

Does every leaf rate-set/rate-new now fail client side with "Space
'dl-rate-set' has no attribute 'port-index'" before a message is built?

Two in-tree selftests pass port-index to devnl.rate_set():
tools/testing/selftests/drivers/net/hw/devlink_rate_cross_esw.py
(rate_set_leaf, rate_set_leaf_parent, rate_clear_leaf_parent) and
tools/testing/selftests/drivers/net/hw/devlink_rate_tc_bw.py
(setup_devlink_rate).

The same narrowing also affects extack decoding, which resolves the
offending attribute through op.attr_set via attrs_by_val[attr.type], so a
bad-attr extack naming any devlink attribute outside these eleven members
would now raise a decode exception instead of resolving a name.

Related to this, the doc string says "Attributes accepted by the rate-set
and rate-new requests", but the kernel accepts DEVLINK_ATTR_PORT_INDEX for
these commands, and rate-get's request list does include port-index, so the
two spots describing the same object selector disagree.  Adding port-index
to dl-rate-set (and to the rate-set/rate-new request lists, which also omit
it today and only work because of dont-validate: [strict]) would keep them
consistent.

Checked at the tip of the series (59624e85e9fb): the last patch, "netlink:
specs: devlink: add pad to the subsets carrying padded u64s", only adds pad
members and does not add port-index to dl-rate-set.

>  
>    -
>      name: dl-nested-devlink
> @@ -2379,7 +2412,7 @@ operations:
>      -
>        name: rate-set
>        doc: Set rate instances.
> -      attribute-set: devlink
> +      attribute-set: dl-rate-set
>        dont-validate: [strict]
>        flags: [admin-perm]
>        do:
> @@ -2402,7 +2435,7 @@ operations:
>      -
>        name: rate-new
>        doc: Create rate instances.
> -      attribute-set: devlink
> +      attribute-set: dl-rate-set
>        dont-validate: [strict]
>        flags: [admin-perm]
>        do:

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