[PATCH] ipv4: icmp: use skb_dst_dev_net_rcu() in icmp_build_probe

[PATCH] ipv4: icmp: use skb_dst_dev_net_rcu() in icmp_build_probe

[Haoran Wang]

icmp_build_probe() derives the network namespace from the ingress device.

    /* net/ipv4/icmp.c:1296 */
    struct net *net = dev_net_rcu(skb->dev);

The ICMP Extended Echo (RFC 8335) PROBE replies identify the queried
interface by name/index/address and should look it up and apply policy
in the netns the packet is destined to, not the netns of the ingress
device.  Under a VRF, skb->dev is the VRF master, so dev_net_rcu(skb->dev)
returns the master's netns and the device lookup / reply policy run in
the wrong namespace.

The rest of the IPv4 ICMP echo path already uses skb_dst_dev_net_rcu(skb)
(net/ipv4/icmp.c:1123, :1267, :1437), while icmp_build_probe() is the
exception.


Signed-off-by: Haoran Wang <haoranwangsec@gmail.com>
---
 net/ipv4/icmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 90c0e22c2..a577dfc65 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -1293,7 +1293,7 @@ static enum skb_drop_reason icmp_echo(struct sk_buff *skb)
 
 bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
 {
-	struct net *net = dev_net_rcu(skb->dev);
+	struct net *net = skb_dst_dev_net_rcu(skb);
 	struct icmp_ext_hdr *ext_hdr, _ext_hdr;
 	struct icmp_ext_echo_iio *iio, _iio;
 	struct inet6_dev *in6_dev;

Re: [PATCH] ipv4: icmp: use skb_dst_dev_net_rcu() in icmp_build_probe

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] ipv4: icmp: use skb_dst_dev_net_rcu() in icmp_build_probe

[netdev-bot+sashiko]

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 commit message describes a netns bug that does not exist.
- [Medium] icmp_build_probe() is a shared helper; its comment says
  'Helper for icmp_echo and icmpv6_echo_reply'.
- [Low] icmp_build_probe() now takes the netns for the remote-controlled
  interface lookups from skb_dst(skb)->dev_rcu, and that pointer can…

--

ipv4: icmp: use skb_dst_dev_net_rcu() in icmp_build_probe

This commit changes icmp_build_probe() so that it gets the network
namespace from the skb's dst device via skb_dst_dev_net_rcu(), not from
skb->dev. The commit message says this fixes PROBE device lookups and
reply policy running in the wrong namespace under a VRF.

> The ICMP Extended Echo (RFC 8335) PROBE replies identify the queried
> interface by name/index/address and should look it up and apply policy
> in the netns the packet is destined to, not the netns of the ingress
> device.  Under a VRF, skb->dev is the VRF master, so dev_net_rcu(skb->dev)
> returns the master's netns and the device lookup / reply policy run in
> the wrong namespace.
>
> The rest of the IPv4 ICMP echo path already uses skb_dst_dev_net_rcu(skb)
> (net/ipv4/icmp.c:1123, :1267, :1437), while icmp_build_probe() is the
> exception.

[Severity: Medium]
Can the VRF master actually be in a different netns from the dst device
here?

vrf_ip_rcv() does set skb->dev = vrf_dev. But the master always sits in
the same netns as its slaves, because do_set_master() looks it up in the
slave's own netns:

net/core/rtnetlink.c:do_set_master() {
    ...
	upper_dev = __dev_get_by_index(dev_net(dev), ifindex);
    ...
}

A slave that moves to another netns gets NETDEV_UNREGISTER, and
vrf_device_event() unlinks it.

For local-input routes, ip_rt_get_dev() returns either the l3mdev master
or the loopback device of the same netns:

net/ipv4/route.c:ip_rt_get_dev() {
    ...
	dev = l3mdev_master_dev_rcu(nhc->nhc_dev);
	return dev ? : net->loopback_dev;
}

ip6_rt_get_dev_rcu() does the same thing with dev_net(dev)->loopback_dev.

So on normal receive paths, dev_net_rcu(skb->dev) and
skb_dst_dev_net_rcu(skb) appear to return the same struct net.

Would it be more accurate to describe this as a consistency cleanup? The
other option is to show a setup where the two pointers really differ. As
written, the change reads like a VRF netns fix and could be queued for
net or stable on that basis.

The cited precedent also looks inaccurate. Of the three call sites, only
the one in icmp_echo() is on the echo path. The first is in
icmp_unreach(), which handles ICMP_DEST_UNREACH and similar errors. The
last is the error stats path in icmp_timestamp():

	__ICMP_INC_STATS(skb_dst_dev_net_rcu(skb), ICMP_MIB_INERRORS);

> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index 90c0e22c29be..a577dfc652b2 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -1293,7 +1293,7 @@ static enum skb_drop_reason icmp_echo(struct sk_buff *skb)
>  
>  bool icmp_build_probe(struct sk_buff *skb, struct icmphdr *icmphdr)
>  {
> -	struct net *net = dev_net_rcu(skb->dev);
> +	struct net *net = skb_dst_dev_net_rcu(skb);

[Severity: Medium]
icmp_build_probe() is shared with IPv6; the comment above it says
"Helper for icmp_echo and icmpv6_echo_reply". Is the change in IPv6
behaviour intended? The subject and commit message only mention IPv4.

The IPv6 caller still gets its netns from skb->dev:

net/ipv6/icmp.c:icmpv6_echo_reply() {
	struct net *net = dev_net_rcu(skb->dev);
    ...
	if (icmph->icmp6_type == ICMPV6_EXT_ECHO_REQUEST)
		if (!icmp_build_probe(skb, (struct icmphdr *)&tmp_hdr))
			goto out_dst_release;
    ...
}

icmpv6_rcv() does the same. After this patch, a single ICMPv6 Extended
Echo request uses two netns sources:

The dst netns controls the sysctl_icmp_echo_enable_probe check and the
dev_get_by_name(), dev_get_by_index(), ip_dev_find() and ipv6_dev_find()
lookups.

The skb->dev netns controls everything else:
icmpv6_echo_ignore_multicast/anycast, IP6_REPLY_MARK, sock_net_uid(),
icmpv6_xmit_lock(), the route and xfrm lookup, icmpv6_global_allow() and
the ICMP6 MIB counters.

Before the patch, the helper and its IPv6 caller used the same netns.

If the two netns can differ, as the commit message says, wouldn't IPv6
keep the problem the patch claims to fix, and also split one reply
across two namespaces? Would it be cleaner to pass net in from each
caller, or to convert both callers the same way?

[Severity: Low]
Can skb_dst_dev_net_rcu() return init_net here for a packet that was
received in another netns?

dst_dev_put() switches the dst device to blackhole_netdev, even though
skbs may still hold the dst:

net/core/dst.c:dst_dev_put() {
    ...
	rcu_assign_pointer(dst->dev_rcu, blackhole_netdev);
    ...
}

rt_fibinfo_free() and fib6_nh_release_dsts() call it.
rt_flush_dev() and rt6_uncached_list_flush_dev() make the same switch
when a device is unregistered. blackhole_netdev is allocated through
alloc_netdev, which calls dev_net_set(dev, &init_net), so it belongs to
init_net.

A PROBE skb in netns X can keep its dst across this switch through
NFQUEUE, because __nf_queue() calls skb_dst_force(). Suppose the local
address or route is deleted while the packet is queued, and the packet
is then reinjected at LOCAL_IN:

icmpv6_rcv()
  icmpv6_echo_reply()     net = dev_net_rcu(skb->dev)      /* netns X */
    icmp_build_probe()    net = skb_dst_dev_net_rcu(skb)   /* init_net */

icmp_build_probe() would then check init_net's
sysctl_icmp_echo_enable_probe and resolve the name, index or address
chosen by the sender in init_net. The reply would say whether that
init_net interface exists, whether it is up, and whether it has IPv4 or
IPv6 addresses.

icmpv6_echo_reply() still builds and routes the reply in netns X, so the
sender would receive it. Before the patch the helper used skb->dev, so
these lookups stayed in netns X.

This only happens if init_net has icmp_echo_enable_probe=1, which is off
by default, and the window is narrow. On IPv4, icmp_rcv(), icmp_echo()
and icmp_reply() already used the dst netns, so this patch extends that
existing exposure to the probe lookups.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179241614083.5916.6651154069510905781%40whr