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 "netlink: specs: rt-addr: fix the type of target-
netnsid" fixes a bug in one specific earlier commit, but its message…
Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but after this
commit rt-addr.yaml still lists target-netnsid in no operation's…
--
netlink: specs: rt-addr: fix the type of target-netnsid
This commit changes the type of the target-netnsid attribute in the rt-addr
netlink spec from binary to s32. That matches the NLA_S32 policy the kernel
uses for IFA_TARGET_NETNSID and the type rt-link.yaml already declares.
> 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]
Should this patch carry a Fixes: tag?
The binary type for target-netnsid has been there since the spec was first
added, when the file was still named rt_addr.yaml. Commit 0c8e30252d9f
later renamed the attribute but did not change its type.
The kernel has always treated this attribute as s32. In net/ipv4/devinet.c
and net/ipv6/addrconf.c the policy entry is:
[IFA_TARGET_NETNSID] = { .type = NLA_S32 },
The value is written with nla_put_s32() and read with nla_get_s32().
Perhaps:
Fixes: dfb0f7d9d979 ("doc/netlink: Add spec for rt addr messages")
[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. Even with
the type corrected, no operation in rt-addr.yaml lists target-netnsid. The
getaddr dump in particular leaves it out:
name: getaddr
doc: Dump address information.
attribute-set: addr-attrs
dump:
request:
value: 22
attributes: []
reply:
value: 20
attributes: *ifaddr-all
The kernel accepts IFA_TARGET_NETNSID in RTM_GETADDR dump requests:
net/ipv4/devinet.c:inet_valid_dump_ifaddr_req() {
...
if (i == IFA_TARGET_NETNSID) {
struct net *net;
fillargs->netnsid = nla_get_s32(tb[i]);
...
}
inet6_rtm_valid_getaddr_req() and inet6_rtm_getaddr() in
net/ipv6/addrconf.c accept it too.
The kernel also puts it in replies, from inet_fill_ifaddr() and
inet6_fill_ifaddr():
if (args->netnsid >= 0 &&
nla_put_s32(skb, IFA_TARGET_NETNSID, args->netnsid))
Could getaddr's request and reply attribute lists include target-netnsid?
Without it, YNL-generated code cannot dump addresses from another netns
through getaddr.
A later commit in this series, "netlink: specs: rt-addr: document AF_PACKET
multicast dumps", adds target-netnsid to the getmulticast request and reply
lists. getaddr is still unchanged at the end of the series.
> -
> name: proto
> type: u8
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930112842.21323-1-sigefriedhyy%40gmail.com
