Re: [PATCH net-next 8/9] sit: convert configuration to RCU protection

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] SIT tunnel parameters are now read from two objects that live
  in different synchronization domains, and ipip6_tunnel_update()…

--

sit: convert configuration to RCU protection

This converts tunnel->sit_parms into an RCU-protected pointer, so that
ipip6_tunnel_update() publishes a freshly allocated parameter block with
rcu_assign_pointer() and frees the old one with kfree_rcu(). The unlink /
re-link plus synchronize_net() are now skipped when neither saddr nor daddr
changed.

> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index dc37c7109af53..c9049ab87e010 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c

[ ... ]

> @@ -1087,13 +1119,17 @@ static netdev_tx_t sit_tunnel_xmit__(struct sk_buff *skb,
>  				     struct net_device *dev, u8 ipproto)
>  {
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
> -	const struct iphdr  *tiph = &tunnel->sit_parms->iph;
> +	const struct ip_tunnel_parm_kern *parms;
> +	const struct iphdr  *tiph;
>  
>  	if (iptunnel_handle_offloads(skb, SKB_GSO_IPXIP4))
>  		goto tx_error;
>  
>  	skb_set_inner_ipproto(skb, ipproto);
>  
> +	parms = rcu_dereference(tunnel->sit_parms);
> +	tiph = &parms->iph;
> +
>  	ip_tunnel_xmit(skb, dev, tiph, ipproto);
>  	return NETDEV_TX_OK;
>  tx_error:

[ ... ]

> @@ -1194,21 +1235,29 @@ static int ipip6_tunnel_update(struct ip_tunnel *t,
>  	new_p->iph.tos = p->iph.tos;
>  	new_p->iph.frag_off = p->iph.frag_off;
>  	new_p->link = p->link;
> -	ipip6_tunnel_unlink(sitn, t);
> -	synchronize_net();
> -	t->sit_parms = new_p;
> +	move = old_p->iph.saddr != p->iph.saddr ||
> +	       old_p->iph.daddr != p->iph.daddr;
> +
> +	if (move)
> +		ipip6_tunnel_unlink(sitn, t);
> +
>  	t->parms.iph = new_p->iph;
>  	WRITE_ONCE(t->parms.link, new_p->link);
> -	__dev_addr_set(t->dev, &p->iph.saddr, 4);
> -	memcpy(t->dev->broadcast, &p->iph.daddr, 4);
> -	ipip6_tunnel_link(sitn, t);
> +	rcu_assign_pointer(t->sit_parms, new_p);

[Severity: Medium]
After this change, are the SIT transmit parameters read from two objects
that are no longer updated together?

ipip6_tunnel_update() now performs two separate stores here:

	t->parms.iph = new_p->iph;		/* plain struct copy */
	WRITE_ONCE(t->parms.link, new_p->link);
	rcu_assign_pointer(t->sit_parms, new_p);	/* published */

On the transmit side, sit_tunnel_xmit__() takes the header out of the
RCU-published block:

	parms = rcu_dereference(tunnel->sit_parms);
	tiph = &parms->iph;

	ip_tunnel_xmit(skb, dev, tiph, ipproto);

but ip_tunnel_xmit() in net/ipv4/ip_tunnel.c mixes that with the shadow
copy that is not RCU protected:

	connected = (tunnel->parms.iph.daddr != 0);
	...
	dst = tnl_params->daddr;
	...
		rt = connected ? dst_cache_get_ip4(&tunnel->dst_cache,
						&fl4.saddr) : NULL;
	...
		else if (!md && connected)
			dst_cache_set_ip4(&tunnel->dst_cache, &rt->dst,
					  fl4.saddr);

Since sit_tunnel_xmit() runs locklessly, can it observe the two objects
from different generations, e.g. sit_parms with daddr == 0 (so dst is
derived per packet from the skb/neighbour) while tunnel->parms.iph.daddr
already holds the new address, making connected true?  In that case
ip_tunnel_xmit() would consult and populate tunnel->dst_cache for a
destination-specific route on a tunnel that the header says is NBMA, so a
burst of packets could be routed via the cached entry.  The reverse pairing
(old shadow daddr, new sit_parms daddr) looks possible too where the plain
tunnel->parms.iph.daddr load can be reordered against the rcu_dereference().

Independently of the ordering question, is the plain struct assignment
t->parms.iph = new_p->iph a data race?  It writes roughly twenty bytes
without any annotation while the xmit path is reading ttl, tos, frag_off
and daddr from tunnel->parms, which KCSAN should flag and which can hand
the transmit path a mixture of old and new field values.

The synchronize_net() in the move case only runs after publication, so it
bounds but does not close the window for a reader that entered right at the
grace period boundary.

Would it make sense for ip_tunnel_xmit() to derive connected and link from
the same parameter block that tiph came from, so that only one object is
consulted per packet?

>  
> +	if (move) {
> +		synchronize_net();
> +		__dev_addr_set(t->dev, &p->iph.saddr, 4);
> +		memcpy(t->dev->broadcast, &p->iph.daddr, 4);
> +		ipip6_tunnel_link(sitn, t);
> +	}
>  	if (old_p->link != p->link || t->fwmark != fwmark) {
>  		WRITE_ONCE(t->fwmark, fwmark);
>  		ipip6_tunnel_bind_dev(t->dev);
>  	}
>  	dst_cache_reset(&t->dst_cache);
>  	netdev_state_change(t->dev);
> -	kfree(old_p);
> +	kfree_rcu(old_p, rcu);
>  	return 0;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907075846.2913645-1-edumazet%40google.com