Re: [PATCH net-next] ipv6: check mc_forwarding before calling ip6mr_get_route()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · 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] The fix only works when no multicast routing daemon is
  running.
- [Medium] The patch says it aligns rt6_fill_node() with IPv4's
  rt_fill_info(), but it copies only half of the IPv4 condition.
- [Low] The commit fixes a user-visible netlink failure ('RTNETLINK
  answers: Message too long' from RTM_GETROUTE for an IPv6 multicast…

--

ipv6: check mc_forwarding before calling ip6mr_get_route()

With this patch, rt6_fill_node() calls ip6mr_get_route() only when
mc_forwarding is non-zero. A route get for a multicast destination with an
input interface then no longer fails with -EMSGSIZE when no multicast
routing daemon is running. The commit message says this aligns IPv6 with
the IPv4 rt_fill_info() behavior.

> Signed-off-by: Wenbo Sun <sunwenbo@cmss.chinamobile.com>

[Severity: Low]
Should this have a Fixes: tag, and possibly a Cc to stable?

The patch fixes a user-visible "RTNETLINK answers: Message too long" error
from RTM_GETROUTE. git blame shows that the ungated ip6mr_get_route() call
in rt6_fill_node() was added by:

commit 7bc570c8b4f7 ("[IPV6] MROUTE: Support multicast forwarding.")

The current err < 0 to nla_put_failure mapping came later, from
fd61c6ba313d ("net: ipv6: remove nowait arg to rt6_fill_node").
inet6_rtm_getroute() has never set skb->dev on the reply skb, so the
failure has been reachable since then.

Without a Fixes: tag, the stable tooling will not pick this up.

> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 04098cf32eabe..cb8cfd8b5c8b7 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -5862,7 +5862,8 @@ static int rt6_fill_node(struct net *net, struct sk_buff *skb,
>  #endif
>  	if (iif) {
>  #ifdef CONFIG_IPV6_MROUTE
> -		if (ipv6_addr_is_multicast(&rt6_dst->addr)) {
> +		if (ipv6_addr_is_multicast(&rt6_dst->addr) &&
> +		    atomic_read(&net->ipv6.devconf_all->mc_forwarding)) {

[Severity: Medium]
Doesn't this still return -EMSGSIZE whenever a multicast routing daemon is
running?

The commit message links "skb->dev is NULL" to multicast routing being
inactive. However, inet6_rtm_getroute() never sets skb->dev on the reply
skb, whatever the value of mc_forwarding:

net/ipv6/route.c:inet6_rtm_getroute() {
    ...
	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
    ...
	skb_dst_set(skb, &rt->dst);
    ...
}

The iif device is resolved earlier with dev_get_by_index_rcu(), but it is
only used for ip6_route_input_lookup().

Because skb->dev is NULL, ip6mr_get_route() skips the (*,G) fallback, and
every cache miss ends up here:

net/ipv6/ip6mr.c:ip6mr_get_route() {
    ...
	cache = ip6mr_cache_find(mrt, &rt->rt6i_src.addr, &rt->rt6i_dst.addr);
	if (!cache && skb->dev) {
    ...
		dev = skb->dev;
		if (!dev || (vif = ip6mr_find_vif(mrt, dev)) < 0) {
			rcu_read_unlock();
			return -ENODEV;
		}
    ...
}

A cache hit also looks unlikely. The lookup uses rt6i_src and rt6i_dst of
the pcpu route. ip6_rt_copy_init() copies those from the FIB prefix (for
example ff00::/8), not from the queried S,G:

	rt->rt6i_dst = f6i->fib6_dst;

So when mc_forwarding != 0, the path is still:

inet6_rtm_getroute()
  rt6_fill_node()
    ip6mr_get_route()      -> -ENODEV
    goto nla_put_failure  -> -EMSGSIZE

The IPv4 code that the commit says it aligns with sets skb->dev in
inet_rtm_getroute():

	fl4.flowi4_iif = iif; /* for rt_fill_info */
	skb->dev	= dev;

For output lookups it uses net->loopback_dev. rt_fill_info() also passes
fl4->saddr and fl4->daddr to ipmr_get_route().

Would it make sense to also set skb->dev in inet6_rtm_getroute(), look up
the queried S,G, or stop mapping ip6mr_get_route() errors to -EMSGSIZE?
Any of these would also cover the case where a daemon is running.

[Severity: Medium]
Should this also exclude link-local and node-local multicast, as the IPv4
condition does?

rt_fill_info() in net/ipv4/route.c uses:

			if (ipv4_is_multicast(dst) &&
			    !ipv4_is_local_multicast(dst) &&
			    IPV4_DEVCONF_ALL_RO(net, MC_FORWARDING)) {

On IPv6, ip6_mc_input() never forwards loopback-scope or link-local
groups:

	if (atomic_read(&dev_net_rcu(skb->dev)->ipv6.devconf_all->mc_forwarding) &&
	    !(ipv6_addr_type(&hdr->daddr) &
	      (IPV6_ADDR_LOOPBACK|IPV6_ADDR_LINKLOCAL)) &&

This means no MFC state can ever exist for those groups.

With this patch, a query such as "ip -6 route get ff02::1 from fe80::1 iif
eth0" still reaches ip6mr_get_route() whenever mc_forwarding != 0. It gets
-ENODEV there and returns -EMSGSIZE. For the same kind of query, IPv4
returns RTA_IIF and a normal local route.

Also, rt6_dst->addr is the route prefix (typically ff00::/8 from
addrconf), not the queried group. Would a scope check need to test dest
instead in order to work?

>  			int err = ip6mr_get_route(net, skb, rtm, portid);
>  
>  			if (err == 0)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111094528.425826-1-sunwenbo%40cmss.chinamobile.com