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

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.

[ ... ]

[ ... ]

[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