Re: [PATCH net-next v11 1/6] netlink: specs: rt-addr: fix the type of target-netnsid

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit fixes a real type mismatch in
  Documentation/netlink/specs/rt-addr.yaml.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but the getaddr
  operation in Documentation/netlink/specs/rt-addr.yaml still doesn't…
- [Low] This problem wasn't introduced by this patch, but
  Documentation/netlink/specs/ovs_vport.yaml has the same kind of…

--

netlink: specs: rt-addr: fix the type of target-netnsid

This patch changes the type of target-netnsid in the rt-addr YNL spec from
binary to s32. That matches the NLA_S32 policy the kernel uses for
IFA_TARGET_NETNSID and the s32 type rt-link.yaml already uses.

> The kernel parses IFA_TARGET_NETNSID as NLA_S32 and rt-link.yaml
> declares its target-netnsid as s32, but rt-addr.yaml has it as binary.
>
> Signed-off-by: Yuyang Huang <sigefriedhyy@gmail.com>
> Reviewed-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

[Severity: Low]
Should this have a Fixes: tag? The type: binary line comes from the
commit that first added the spec. The later rename commit only changed the
attribute name:

Fixes: dfb0f7d9d979 ("doc/netlink: Add spec for rt addr messages")

> diff --git a/Documentation/netlink/specs/rt-addr.yaml b/Documentation/netlink/specs/rt-addr.yaml
> index 0ecbd24c890c6..17ead22034515 100644
> --- a/Documentation/netlink/specs/rt-addr.yaml
> +++ b/Documentation/netlink/specs/rt-addr.yaml
> @@ -119,7 +119,7 @@ attribute-sets:
>          type: u32
>        -
>          name: target-netnsid
> -        type: binary
> +        type: s32

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. The
getaddr operation in this spec still doesn't list target-netnsid anywhere:

      name: getaddr
      doc: Dump address information.
      attribute-set: addr-attrs
      dump:
        request:
          value: 22
          attributes: []
        reply:
          value: 20
          attributes: *ifaddr-all

Here *ifaddr-all contains only address, label, local and cacheinfo.

The kernel accepts the attribute in RTM_GETADDR dump requests. This is
from net/ipv4/devinet.c:inet_valid_dump_ifaddr_req():

	if (i == IFA_TARGET_NETNSID) {
		struct net *net;

		fillargs->netnsid = nla_get_s32(tb[i]);

The IPv6 dump request handling in net/ipv6/addrconf.c does the same. The
kernel also puts the attribute in replies, from inet_fill_ifaddr() and
inet6_fill_ifaddr():

	if (args->netnsid >= 0 &&
	    nla_put_s32(skb, IFA_TARGET_NETNSID, args->netnsid))

The spec also has no do block for getaddr. IPv6 registers RTM_GETADDR
with .doit = inet6_rtm_getaddr, and inet6_rtm_valid_getaddr_req() accepts
IFA_TARGET_NETNSID, IFA_ADDRESS and IFA_LOCAL.

Could target-netnsid be added to the getaddr request and reply lists,
along with a do block? Right now, generated per-op C code can't set or
parse the attribute whose type this patch fixes. The Python YNL tools
still work because they encode and decode against the whole attribute set.

A later patch in this series fixes the getmulticast case: "netlink: specs:
rt-addr: document AF_PACKET multicast dumps" adds target-netnsid to the
getmulticast dump request and to &mcaddr-attrs. getaddr is still
unchanged at the end of the series.

>        -
>          name: proto
>          type: u8

[Severity: Low]
This isn't a bug introduced by this patch, but
Documentation/netlink/specs/ovs_vport.yaml seems to have the same kind of
netnsid type mismatch. Should it be s32 as well? That spec declares the
attribute as unsigned:

        name: netnsid
        type: u32

But vport_policy in net/openvswitch/datapath.c uses a signed type:

	[OVS_VPORT_ATTR_NETNSID] = { .type = NLA_S32 },

ovs_vport_cmd_fill_info() sends the value with nla_put_s32(). The value
comes from peernet2id_alloc(), which can return NETNSA_NSID_NOT_ASSIGNED
(-1). A YNL client using the ovs_vport spec would decode -1 as 4294967295.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012011707.3378-1-sigefriedhyy%40gmail.com