Re: [PATCH net v2 4/4] ipv4: replace obsolete routing comment

[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 comment added to ip_route_output_key_hash_rcu()
  (net/ipv4/route.c, in the fib_lookup() error branch) is wrong for one…

--

ipv4: replace obsolete routing comment

This replaces the comment in the fib_lookup() error fallback path of
ip_route_output_key_hash_rcu(). The old comment described obsolete
fib_lookup(), MSG_DONTROUTE and IPv6 behavior. The new one describes
when the on-link fallback is taken.

> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 80a2c8a3c571f..ac94d8262b6ed 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -2858,24 +2858,19 @@ struct rtable *ip_route_output_key_hash_rcu(struct net *net, struct flowi4 *fl4,
>  		if (fl4->flowi4_oif &&
>  		    (ipv4_is_multicast(fl4->daddr) ||
>  		     (!fl4->flowi4_l3mdev && err == -ESRCH))) {
> -			/* Apparently, routing tables are wrong. Assume,
> -			 * that the destination is on link.
> +			/* When oif is specified, fib_lookup() is called
> +			 * primarily to determine if the destination is
> +			 * accessible directly or through a gateway. It also
> +			 * checks whether there are any traffic restrictions
> +			 * (blackhole, prohibit, unreachable), or if there's a
> +			 * routing lookup miss.
>  			 *

[ ... ]

> +			 * This fallback path is always taken for multicast,
> +			 * regardless of any restrictions or misses. Non-vrf
> +			 * unicast does honor restrictions, but falls back if
> +			 * there's a miss. The destination is assumed on-link
> +			 * for both.
>  			 */

[Severity: Low]
Is "always taken for multicast" accurate when the oif is an L3 master
(VRF) device?

With a VRF configured, fib_has_custom_rules is set. That sends
fib_lookup() through __fib_lookup(), which changes the caller's flow in
place:

net/ipv4/fib_rules.c:__fib_lookup() {
    ...
	/* update flow if oif or iif point to device enslaved to l3mdev */
	l3mdev_update_flow(net, flowi4_to_flowi(flp));
    ...
}

net/l3mdev/l3mdev.c:l3mdev_update_flow() {
    ...
			if (netif_is_l3_master(dev))
				fl->flowi_oif = 0;
    ...
}

The fallback condition reads fl4->flowi4_oif after the lookup. Take a
multicast daddr that is not link-local and not IGMP (for example 239.x),
with the oif set to the VRF master. If the VRF table lookup fails, for
example on the usual unreachable default route, the oif test is false
and the lookup error is returned. The on-link fallback is not taken.

This behavior predates the patch. Could the comment mention this
exception, and note that the oif test uses the value left after
fib_lookup()?

The opening sentence has the same problem. For a VRF master oif,
fib_lookup() is used to select the VRF table and the oif match is
dropped. It is not mainly a check for whether the destination is direct
or gatewayed. Should the comment say that as well?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110061536.10811-1-liamsmith827%40grapheneos.org