Re: [PATCH net-next 4/9] sit: annotate data-races around tunnel->fwmark

[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: 0 · Low: 1

- [Low] Incomplete fwmark annotation for sit devices:
  ipip6_tunnel_update() now publishes tunnel->fwmark with WRITE_ONCE()…

--

sit: annotate data-races around tunnel->fwmark

This adds READ_ONCE()/WRITE_ONCE() annotations around tunnel->fwmark in
the sit driver, since ipip6_tunnel_xmit() runs locklessly with
dev->lltx == true. It is preparation for running ipip6_fill_info()
without RTNL.

> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index 9a2bbd76d3794..e85fa80e80d1a 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -929,7 +929,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
>  	if (!dst && !ipip6_tunnel_dst_find(skb, &dst, false))
>  		goto tx_error;
>  
> -	flowi4_init_output(&fl4, tunnel->parms.link, tunnel->fwmark,
> +	flowi4_init_output(&fl4, tunnel->parms.link, READ_ONCE(tunnel->fwmark),
>  			   tos & INET_DSCP_MASK, RT_SCOPE_UNIVERSE,
>  			   IPPROTO_IPV6, 0, dst, tiph->saddr, 0, 0,
>  			   sock_net_uid(tunnel->net, NULL));

[ ... ]

> @@ -1153,7 +1153,7 @@ static void ipip6_tunnel_update(struct ip_tunnel *t,
>  	t->parms.iph.frag_off = p->iph.frag_off;
>  	if (t->parms.link != p->link || t->fwmark != fwmark) {
>  		t->parms.link = p->link;
> -		t->fwmark = fwmark;
> +		WRITE_ONCE(t->fwmark, fwmark);

[Severity: Low]
Should the other lockless transmit path of the same sit device also be
annotated?

ipip6_tunnel_xmit() is only one of the two xmit paths.  For IPv4 and MPLS
payloads, sit_tunnel_xmit() dispatches elsewhere:

net/ipv6/sit.c:sit_tunnel_xmit() {
	switch (skb->protocol) {
	case htons(ETH_P_IP):
		sit_tunnel_xmit__(skb, dev, IPPROTO_IPIP);
		break;
	case htons(ETH_P_IPV6):
		ipip6_tunnel_xmit(skb, dev);
		break;
#if IS_ENABLED(CONFIG_MPLS)
	case htons(ETH_P_MPLS_UC):
		sit_tunnel_xmit__(skb, dev, IPPROTO_MPLS);
		break;
#endif
	...
}

and sit_tunnel_xmit__() -> ip_tunnel_xmit() reads the same field with a
plain load, right next to an already annotated read:

net/ipv4/ip_tunnel.c:ip_tunnel_xmit() {
	...
	ip_tunnel_init_flow(&fl4, protocol, dst, tnl_params->saddr,
			    tunnel->parms.o_key, tos & INET_DSCP_MASK,
			    tunnel->net, READ_ONCE(tunnel->parms.link),
			    tunnel->fwmark, skb_get_hash(skb), 0);
	...
}

So for a sit tunnel carrying IPv4 or MPLS frames, an RTNL writer running
ipip6_changelink()/ioctl -> ipip6_tunnel_update() -> WRITE_ONCE(t->fwmark,
fwmark) still races with an unmarked read, and KCSAN can still report it.
The field is a naturally aligned u32, so the read cannot tear; the visible
effects are the KCSAN report plus one route lookup using the pre-update
mark.

Note that the generic writer in net/ipv4/ip_tunnel.c:ip_tunnel_update() is
also still a plain store to t->fwmark, so closing this would mean touching
the generic ip_tunnel code as well, which may be outside the intended scope
of this series.  It was checked at the end of the series and the plain read
in ip_tunnel_xmit() is still there.

[ ... ]

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