Re: [PATCH net-next v11 4/6] net: add AF_PACKET multicast dumps

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] dev_mc_dump() is registered with RTNL_FLAG_DUMP_UNLOCKED.
- [Low] Under this patch alone, the uAPI and the YAML spec disagree.

--

net: add AF_PACKET multicast dumps

This adds an RTM_GETMULTICAST dump handler for ifa_family AF_PACKET. The
handler reports every entry of dev->mc in the ifaddrmsg format, using
IFA_MULTICAST, IFA_MC_USERS and a new IFA_F_GLOBAL bit in IFA_FLAGS. The
dump runs without RTNL, under RCU and netif_addr_lock_bh(), and uses
dev_mc_genid plus dev_base_seq to set NLM_F_DUMP_INTR.

> diff --git a/include/uapi/linux/if_addr.h b/include/uapi/linux/if_addr.h
> index 7fb630b7fe311..0a1ad9ebb47be 100644
> --- a/include/uapi/linux/if_addr.h
> +++ b/include/uapi/linux/if_addr.h
> @@ -57,6 +57,7 @@ enum {
>  #define IFA_F_NOPREFIXROUTE	0x200
>  #define IFA_F_MCAUTOJOIN	0x400
>  #define IFA_F_STABLE_PRIVACY	0x800
> +#define IFA_F_GLOBAL		0x1000

[Severity: Low]
This isn't a bug, but with only this patch applied, the uAPI and
Documentation/netlink/specs/rt-addr.yaml don't match.

The ifa-flags enum in the spec still ends at stable-privacy and has no
entry for IFA_F_GLOBAL.

The getmulticast reply in the spec lists only multicast, mc-users and
cacheinfo. dev_mc_fill_addr() now also emits IFA_FLAGS and
IFA_TARGET_NETNSID.

The dump request attributes in the spec are still empty. The op doc still
covers only IPv4/IPv6.

The next patch in the series, "netlink: specs: rt-addr: document AF_PACKET
multicast dumps", adds the global flag, the flags and target-netnsid
attributes and the AF_PACKET doc. After it, the series is consistent.

Could the spec update be folded into this patch, so the uAPI and the spec
land together?

> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> index 3ab053cbf8b99..d827cc21867bc 100644
> --- a/net/core/dev_addr_lists.c
> +++ b/net/core/dev_addr_lists.c

[ ... ]

> @@ -1215,6 +1219,194 @@ void dev_mc_init(struct net_device *dev)
>  }
>  EXPORT_SYMBOL(dev_mc_init);
>  
> +static int dev_mc_fill_addr(struct sk_buff *skb, const struct net_device *dev,
> +			    const struct netdev_hw_addr *ha, u32 portid,
> +			    u32 seq, unsigned int flags, int netnsid)
> +{

[ ... ]

> +	if ((netnsid >= 0 &&
> +	     nla_put_s32(skb, IFA_TARGET_NETNSID, netnsid)) ||
> +	    nla_put(skb, IFA_MULTICAST, dev->addr_len, ha->addr) ||

[Severity: Medium]
Is it safe to take the IFA_MULTICAST length from the live dev->addr_len
here?

dev_mc_dump() is registered with RTNL_FLAG_DUMP_UNLOCKED. While it calls
this function it holds only RCU and addr_list_lock. Code that writes
addr_len at runtime holds RTNL but not addr_list_lock. One example is
TUNSETLINK in __tun_chr_ioctl():

drivers/net/tun.c:__tun_chr_ioctl() {
    ...
			tun->dev->type = (int) arg;
			tun->dev->addr_len = tun_get_addr_len(tun->dev->type);
    ...
}

Nothing flushes dev->mc when the type changes. Entries added through
SIOCADDMULTI and dev_mc_add_global() remain after the change, because
that path doesn't require IFF_UP.

Those entries were created by __hw_addr_create(), which initializes only
as many bytes as the address length at creation time:

net/core/dev_addr_lists.c:__hw_addr_create() {
    ...
	ha = kmalloc(alloc_size, GFP_ATOMIC);
	if (!ha)
		return NULL;
	memcpy(ha->addr, addr, addr_len);
    ...
}

Suppose a TAP device is switched from ARPHRD_ETHER (addr_len 6) to
ARPHRD_IP6GRE (addr_len 16). Would this dump then copy the uninitialized
bytes in ha->addr[6..15] to userspace?

There are two related points.

First, if TUNSETLINK runs during a dump, its plain write to dev->addr_len
races with the read here.

Second, in a dump that takes several rounds, entries from the same device
can be emitted with different lengths. A type change doesn't touch
dev_mc_genid or dev_base_seq, so NLM_F_DUMP_INTR isn't set in that case.

dev_mc_seq_show() for /proc/net/dev_mcast already reads dev->addr_len
the same way. This patch adds a second path to existing behavior rather
than a new exposure, but the new uAPI picks up the same problem.

> +	    nla_put_u32(skb, IFA_MC_USERS, ha->refcount) ||
> +	    nla_put_u32(skb, IFA_FLAGS, ha->global_use ? IFA_F_GLOBAL : 0)) {
> +		nlmsg_cancel(skb, nlh);
> +		return -EMSGSIZE;
> +	}

[ ... ]

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