[PATCH net-next 0/9] sit: convert configuration to RCU and lockless fill_info

[PATCH net-next 0/9] sit: convert configuration to RCU and lockless fill_info

[Eric Dumazet]

SIT (IPv6-in-IPv4) tunnel configuration and status reporting have
historically relied on the RTNL lock for synchronization. Consequently,
netlink dumps via ipip6_fill_info() had to run with RTNL held, adding
contention during network device dumps.

At the same time, the transmit path (dev->lltx == true), tunnel lookups,
and error handling run locklessly and can race with configuration
updates. This can result in torn reads of multi-word fields (such as the
128-bit 6RD IPv6 prefix) or transiently zeroed encapsulation parameters.
Furthermore, ipip6_tunnel_update() currently unhashes, re-hashes, and
calls synchronize_net() unconditionally, even when the tunnel endpoint
addresses (saddr and daddr) have not changed.

This patch series addresses PRL issues, modernizes SIT parameter
management to use RCU protection, optimizes tunnel updates, and removes
the RTNL requirement from ipip6_fill_info():

- Patch 1 fixes a pre-existing UAF in PRL (Potential Router List)
  deletion where call_rcu() was invoked before unlinking t->prl.
- Patch 2 adds GFP_KERNEL_ACCOUNT to struct ip_tunnel_prl_entry
  allocations in ipip6_tunnel_add_prl().
- Patch 3 removes the unsafe in-place memset() in ip_tunnel_encap_setup()
  and uses WRITE_ONCE() to prevent lockless readers from observing
  transiently zeroed or torn fields.
- Patch 4 annotates data races on tunnel->fwmark with READ_ONCE() and
  WRITE_ONCE().
- Patch 5 converts 6RD configuration (tunnel->ip6rd) to an RCU-protected
  pointer, preventing torn reads on the 128-bit IPv6 prefix.
- Patch 6 implements a dedicated ipip6_get_iflink() callback to decouple
  SIT parameter handling from generic ip_tunnel.
- Patch 7 dynamically allocates struct ip_tunnel_parm_kern (sit_parms)
  as a preparatory step.
- Patch 8 converts tunnel->sit_parms to full RCU protection. Updates
  publish new parameters via rcu_assign_pointer() and free the old ones
  via kfree_rcu(). When saddr and daddr do not change, unhashing,
  re-hashing, and synchronize_net() are completely bypassed.
- Patch 9 wraps attribute serialization in ipip6_fill_info() under
  rcu_read_lock(), eliminating the reliance on the RTNL lock.

Eric Dumazet (9):
  sit: fix UAF in ipip6_tunnel_del_prl()
  sit: charge ip_tunnel_prl_entry allocations to memcg
  ip_tunnel: use WRITE_ONCE in ip_tunnel_encap_setup
  sit: annotate data-races around tunnel->fwmark
  sit: convert 6RD configuration to RCU protection
  sit: implement ipip6_get_iflink()
  sit: dynamically allocate struct ip_tunnel_parm_kern
  sit: convert configuration to RCU protection
  sit: no longer rely on RTNL in ipip6_fill_info()

 include/net/ip_tunnels.h |   5 +-
 net/ipv4/ip_tunnel.c     |  14 +-
 net/ipv6/sit.c           | 477 +++++++++++++++++++++++++++------------
 3 files changed, 339 insertions(+), 157 deletions(-)

-- 
2.55.0.979.g7e5102b832-goog

[PATCH net-next 1/9] sit: fix UAF in ipip6_tunnel_del_prl()

[Eric Dumazet]

When flushing all potential router list (PRL) entries,
ipip6_tunnel_del_prl() called call_rcu(&x->rcu_head, prl_list_destroy_rcu)
before clearing t->prl.

A concurrent reader in isatap_chksrc() could enter an RCU read-side
critical section after call_rcu() but before t->prl is set to NULL,
allowing prl_list_destroy_rcu() to free nodes while the reader traverses
them.

Clear t->prl using RCU_INIT_POINTER() before invoking call_rcu().
Also use rcu_assign_pointer() when unlinking individual entries.

Fixes: ef9a9d1183b3 ("ipv6 sit: RCU conversion phase I")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv6/sit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 19b7fa8d1a2a07991a00b250c5ae58819d104e79..4438b2472b28764f813b9162bdb9f6b6dee5a007 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -436,7 +436,7 @@ ipip6_tunnel_del_prl(struct ip_tunnel *t, struct ip_tunnel_prl *a)
 		     (x = rtnl_dereference(*p)) != NULL;
 		     p = &x->next) {
 			if (x->addr == a->addr) {
-				*p = x->next;
+				rcu_assign_pointer(*p, rtnl_dereference(x->next));
 				kfree_rcu(x, rcu_head);
 				t->prl_count--;
 				goto out;
@@ -447,8 +447,8 @@ ipip6_tunnel_del_prl(struct ip_tunnel *t, struct ip_tunnel_prl *a)
 		x = rtnl_dereference(t->prl);
 		if (x) {
 			t->prl_count = 0;
+			RCU_INIT_POINTER(t->prl, NULL);
 			call_rcu(&x->rcu_head, prl_list_destroy_rcu);
-			t->prl = NULL;
 		}
 	}
 out:
-- 
2.55.0.979.g7e5102b832-goog

Re: [PATCH net-next 1/9] sit: fix UAF in ipip6_tunnel_del_prl()

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1703 bytes --]

> When flushing all potential router list (PRL) entries,
> ipip6_tunnel_del_prl() called call_rcu(&x->rcu_head, prl_list_destroy_rcu)
> before clearing t->prl.
> 
> A concurrent reader in isatap_chksrc() could enter an RCU read-side
> critical section after call_rcu() but before t->prl is set to NULL,
> allowing prl_list_destroy_rcu() to free nodes while the reader traverses
> them.
> 
> Clear t->prl using RCU_INIT_POINTER() before invoking call_rcu().
> Also use rcu_assign_pointer() when unlinking individual entries.
> 
> Fixes: ef9a9d1183b3 ("ipv6 sit: RCU conversion phase I")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Acked-by Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

> ---
>  net/ipv6/sit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index 19b7fa8d1a2a07991a00b250c5ae58819d104e79..4438b2472b28764f813b9162bdb9f6b6dee5a007 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -436,7 +436,7 @@ ipip6_tunnel_del_prl(struct ip_tunnel *t, struct ip_tunnel_prl *a)
>  		     (x = rtnl_dereference(*p)) != NULL;
>  		     p = &x->next) {
>  			if (x->addr == a->addr) {
> -				*p = x->next;
> +				rcu_assign_pointer(*p, rtnl_dereference(x->next));
>  				kfree_rcu(x, rcu_head);
>  				t->prl_count--;
>  				goto out;
> @@ -447,8 +447,8 @@ ipip6_tunnel_del_prl(struct ip_tunnel *t, struct ip_tunnel_prl *a)
>  		x = rtnl_dereference(t->prl);
>  		if (x) {
>  			t->prl_count = 0;
> +			RCU_INIT_POINTER(t->prl, NULL);
>  			call_rcu(&x->rcu_head, prl_list_destroy_rcu);
> -			t->prl = NULL;
>  		}
>  	}
>  out:
> -- 
> 2.55.0.979.g7e5102b832-goog
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH net-next 2/9] sit: charge ip_tunnel_prl_entry allocations to memcg

[Eric Dumazet]

ipip6_tunnel_add_prl() allocates struct ip_tunnel_prl_entry for each
potential router entry added via SIOCADDPRL.
An unprivileged user with CAP_NET_ADMIN in a user namespace can add
an unbounded number of entries, consuming uncharged kernel memory.

Commit 1b51d8271973 ("memcg: ipv6/sit: account and don't WARN on
ip_tunnel_prl structs allocation") added accounting to the temporary
buffer in ipip6_tunnel_get_prl(), but missed the entry allocations
in ipip6_tunnel_add_prl().

Use GFP_KERNEL_ACCOUNT when allocating struct ip_tunnel_prl_entry.

Fixes: 1b51d8271973 ("memcg: ipv6/sit: account and don't WARN on ip_tunnel_prl structs allocation")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv6/sit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 4438b2472b28764f813b9162bdb9f6b6dee5a007..9a2bbd76d3794849cd11c6530ba1303bf53fa388 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -395,7 +395,7 @@ ipip6_tunnel_add_prl(struct ip_tunnel *t, struct ip_tunnel_prl *a, int chg)
 		goto out;
 	}
 
-	p = kzalloc_obj(struct ip_tunnel_prl_entry);
+	p = kzalloc_obj(struct ip_tunnel_prl_entry, GFP_KERNEL_ACCOUNT);
 	if (!p) {
 		err = -ENOBUFS;
 		goto out;
-- 
2.55.0.979.g7e5102b832-goog

Re: [PATCH net-next 2/9] sit: charge ip_tunnel_prl_entry allocations to memcg

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1431 bytes --]

> ipip6_tunnel_add_prl() allocates struct ip_tunnel_prl_entry for each
> potential router entry added via SIOCADDPRL.
> An unprivileged user with CAP_NET_ADMIN in a user namespace can add
> an unbounded number of entries, consuming uncharged kernel memory.
> 
> Commit 1b51d8271973 ("memcg: ipv6/sit: account and don't WARN on
> ip_tunnel_prl structs allocation") added accounting to the temporary
> buffer in ipip6_tunnel_get_prl(), but missed the entry allocations
> in ipip6_tunnel_add_prl().
> 
> Use GFP_KERNEL_ACCOUNT when allocating struct ip_tunnel_prl_entry.
> 
> Fixes: 1b51d8271973 ("memcg: ipv6/sit: account and don't WARN on ip_tunnel_prl structs allocation")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

> ---
>  net/ipv6/sit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index 4438b2472b28764f813b9162bdb9f6b6dee5a007..9a2bbd76d3794849cd11c6530ba1303bf53fa388 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -395,7 +395,7 @@ ipip6_tunnel_add_prl(struct ip_tunnel *t, struct ip_tunnel_prl *a, int chg)
>  		goto out;
>  	}
>  
> -	p = kzalloc_obj(struct ip_tunnel_prl_entry);
> +	p = kzalloc_obj(struct ip_tunnel_prl_entry, GFP_KERNEL_ACCOUNT);
>  	if (!p) {
>  		err = -ENOBUFS;
>  		goto out;
> -- 
> 2.55.0.979.g7e5102b832-goog
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH net-next 3/9] ip_tunnel: use WRITE_ONCE in ip_tunnel_encap_setup

[Eric Dumazet]

Update ip_tunnel_encap_setup() to use WRITE_ONCE() when writing
to encap fields (type, sport, dport, flags) and hlen fields.
This ensures that concurrent lockless readers (like fill_info)
do not see torn writes.

Also remove the unsafe memset() on t->encap which could cause
concurrent readers to transiently see zeroed fields.
Removing it also fixes a bug where t->encap was left cleared
even if ip_encap_hlen() failed, resulting in partial configuration.

Fixes: 56328486539d ("net: Changes to ip_tunnel to support foo-over-udp encapsulation")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/ip_tunnel.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index e6bcf01411d0bcd12cc9a88e449d9283c4a83c64..13b5e35e8790b13ed4c87f28d46aa2bafc9ad72c 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -491,19 +491,17 @@ int ip_tunnel_encap_setup(struct ip_tunnel *t,
 {
 	int hlen;
 
-	memset(&t->encap, 0, sizeof(t->encap));
-
 	hlen = ip_encap_hlen(ipencap);
 	if (hlen < 0)
 		return hlen;
 
-	t->encap.type = ipencap->type;
-	t->encap.sport = ipencap->sport;
-	t->encap.dport = ipencap->dport;
-	t->encap.flags = ipencap->flags;
+	WRITE_ONCE(t->encap.type, ipencap->type);
+	WRITE_ONCE(t->encap.sport, ipencap->sport);
+	WRITE_ONCE(t->encap.dport, ipencap->dport);
+	WRITE_ONCE(t->encap.flags, ipencap->flags);
 
-	t->encap_hlen = hlen;
-	t->hlen = t->encap_hlen + t->tun_hlen;
+	WRITE_ONCE(t->encap_hlen, hlen);
+	WRITE_ONCE(t->hlen, hlen + t->tun_hlen);
 
 	return 0;
 }
-- 
2.55.0.979.g7e5102b832-goog

Re: [PATCH net-next 3/9] ip_tunnel: use WRITE_ONCE in ip_tunnel_encap_setup

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1826 bytes --]

> Update ip_tunnel_encap_setup() to use WRITE_ONCE() when writing
> to encap fields (type, sport, dport, flags) and hlen fields.
> This ensures that concurrent lockless readers (like fill_info)
> do not see torn writes.
> 
> Also remove the unsafe memset() on t->encap which could cause
> concurrent readers to transiently see zeroed fields.
> Removing it also fixes a bug where t->encap was left cleared
> even if ip_encap_hlen() failed, resulting in partial configuration.
> 
> Fixes: 56328486539d ("net: Changes to ip_tunnel to support foo-over-udp encapsulation")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

> ---
>  net/ipv4/ip_tunnel.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index e6bcf01411d0bcd12cc9a88e449d9283c4a83c64..13b5e35e8790b13ed4c87f28d46aa2bafc9ad72c 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -491,19 +491,17 @@ int ip_tunnel_encap_setup(struct ip_tunnel *t,
>  {
>  	int hlen;
>  
> -	memset(&t->encap, 0, sizeof(t->encap));
> -
>  	hlen = ip_encap_hlen(ipencap);
>  	if (hlen < 0)
>  		return hlen;
>  
> -	t->encap.type = ipencap->type;
> -	t->encap.sport = ipencap->sport;
> -	t->encap.dport = ipencap->dport;
> -	t->encap.flags = ipencap->flags;
> +	WRITE_ONCE(t->encap.type, ipencap->type);
> +	WRITE_ONCE(t->encap.sport, ipencap->sport);
> +	WRITE_ONCE(t->encap.dport, ipencap->dport);
> +	WRITE_ONCE(t->encap.flags, ipencap->flags);
>  
> -	t->encap_hlen = hlen;
> -	t->hlen = t->encap_hlen + t->tun_hlen;
> +	WRITE_ONCE(t->encap_hlen, hlen);
> +	WRITE_ONCE(t->hlen, hlen + t->tun_hlen);
>  
>  	return 0;
>  }
> -- 
> 2.55.0.979.g7e5102b832-goog
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next 3/9] ip_tunnel: use WRITE_ONCE in ip_tunnel_encap_setup

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (2) · Low: 0

- [Medium] The mirror helper ip6_tnl_encap_setup()
  (net/ipv6/ip6_tunnel.c:1816) is left unconverted, so the exact bug…

Pre-existing issues:
- [Medium] ip_tunnel_encap_setup() recomputes t->hlen as 'hlen +
  t->tun_hlen' (net/ipv4/ip_tunnel.c:504), which disagrees with the…
- [Medium] The six encap/hlen fields are published as independent
  WRITE_ONCE() stores with no seqcount, RCU pointer swap or release…

--

ip_tunnel: use WRITE_ONCE in ip_tunnel_encap_setup

This converts the encap field and hlen stores in ip_tunnel_encap_setup()
to WRITE_ONCE() so lockless readers such as fill_info cannot observe torn
values.  It also drops the memset() of t->encap, which both removed a
transient all-zero window for readers and stopped leaving t->encap cleared
when ip_encap_hlen() fails.

No Link: tags present in the commit header.

> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index e6bcf01411d0b..13b5e35e8790b 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -491,19 +491,17 @@ int ip_tunnel_encap_setup(struct ip_tunnel *t,
>  {
>  	int hlen;
>  
> -	memset(&t->encap, 0, sizeof(t->encap));
> -
>  	hlen = ip_encap_hlen(ipencap);
>  	if (hlen < 0)
>  		return hlen;

[Severity: Medium]
Should the IPv6 twin get the same treatment?  ip6_tnl_encap_setup() in
net/ipv6/ip6_tunnel.c still has the original shape at this commit:

net/ipv6/ip6_tunnel.c:ip6_tnl_encap_setup() {
	memset(&t->encap, 0, sizeof(t->encap));

	hlen = ip6_encap_hlen(ipencap);
	if (hlen < 0)
		return hlen;

	t->encap.type = ipencap->type;
	t->encap.sport = ipencap->sport;
	t->encap.dport = ipencap->dport;
	t->encap.flags = ipencap->flags;

	t->encap_hlen = hlen;
	t->hlen = t->encap_hlen + t->tun_hlen;
}

So the "t->encap was left cleared even if ip_encap_hlen() failed" case
described in the changelog is still reachable for ip6tnl and ip6gre, on
already configured tunnels:

ip6_tnl_changelink()
    ip6_tnl_encap_setup(t, &ipencap)   /* t is live */
        memset(&t->encap, 0, ...)
        ip6_encap_hlen() returns -EINVAL   /* e.g. fou6 ops not registered,
                                              or type >= MAX_IPTUN_ENCAP_OPS */
        return hlen

ip6gre_changelink_common() reaches the same helper the same way.  The
netlink command fails, but the live tunnel's encapsulation configuration
has already been wiped.

The plain stores after the memset in ip6_tnl_encap_setup() also leave the
transient all-zero encap visible to ip6_tnl_fill_info()/ip6gre_fill_info(),
which run without RTNL since RTM_GETLINK is registered with
RTNL_FLAG_DOIT_UNLOCKED | RTNL_FLAG_DUMP_UNLOCKED in
net/core/rtnetlink.c, and to the ip6 xmit path.

>  
> -	t->encap.type = ipencap->type;
> -	t->encap.sport = ipencap->sport;
> -	t->encap.dport = ipencap->dport;
> -	t->encap.flags = ipencap->flags;
> +	WRITE_ONCE(t->encap.type, ipencap->type);
> +	WRITE_ONCE(t->encap.sport, ipencap->sport);
> +	WRITE_ONCE(t->encap.dport, ipencap->dport);
> +	WRITE_ONCE(t->encap.flags, ipencap->flags);

[Severity: Medium]
This is a pre-existing issue, not something this patch introduces, but is
it worth noting in the changelog that WRITE_ONCE() only removes torn reads
and does not give readers a coherent snapshot?

encap.type, the field that gates encapsulation, is still published first,
with no seqcount or RCU pointer swap, so a reader can pair the new type
with the stale sport/dport/flags or with the stale (smaller) encap_hlen and
hlen.  The same ordering existed before the patch (with zeroes instead of
stale values), so nothing gets worse here.

On the transmit side the two reads are separated:

net/ipv6/sit.c:ipip6_tunnel_xmit() {
	int t_hlen = tunnel->hlen + sizeof(struct iphdr);
	...
	max_headroom = LL_RESERVED_SPACE(tdev) + t_hlen;
	if (skb_headroom(skb) < max_headroom || ...)
		new_skb = skb_realloc_headroom(skb, max_headroom);
	...
	if (ip_tunnel_encap(skb, &tunnel->encap, &protocol, &fl4) < 0) {
}

The headroom comes from the earlier hlen snapshot while ip_tunnel_encap()
reads the live tunnel->encap, and fou_build_udp()/__gue_build_header() use
skb_push().  I could not show that the reserved headroom is actually
exceeded, since skb_realloc_headroom()/__skb_cow() round expansions up, so
this part is speculative.

>  
> -	t->encap_hlen = hlen;
> -	t->hlen = t->encap_hlen + t->tun_hlen;
> +	WRITE_ONCE(t->encap_hlen, hlen);
> +	WRITE_ONCE(t->hlen, hlen + t->tun_hlen);

[Severity: Medium]
This isn't a bug introduced by this patch, since the formula is unchanged
(t->encap_hlen + t->tun_hlen before, hlen + t->tun_hlen after), but does
this hlen formula disagree with the ERSPAN one?

net/ipv4/ip_gre.c:erspan_tunnel_init() {
	tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen +
		       erspan_hdr_len(tunnel->erspan_ver);
}

erspan_changelink() reaches this helper on an already initialised device:

erspan_changelink()
    ipgre_newlink_encap_setup()
        ip_tunnel_encap_setup()      /* last writer of t->hlen */

ndo_init does not run again on that path, so an
"ip link set <erspan> type erspan encap ..." command appears to drop
erspan_hdr_len() (12 bytes for v1, 16 for v2) from t->hlen.
ip_tunnel_bind_dev() then derives both the MTU and

	dev->needed_headroom = ip_tunnel_limit_headroom(t_hlen + hlen);

from the smaller value, while erspan_xmit() only does
skb_cow_head(skb, dev->needed_headroom) before pushing the erspan header,
the GRE header, the encap header and the outer IPv4 header.

>  
>  	return 0;
>  }

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

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

[Eric Dumazet]

ipip6_tunnel_xmit() runs locklessly (dev->lltx == true) and reads
tunnel->fwmark.

In preparation for converting ipip6_fill_info() to run without RTNL,
add READ_ONCE() and WRITE_ONCE() annotations around tunnel->fwmark.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv6/sit.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 9a2bbd76d3794849cd11c6530ba1303bf53fa388..e85fa80e80d1ad2c593b860685b9f5bac21e66d9 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);
 		ipip6_tunnel_bind_dev(t->dev);
 	}
 	dst_cache_reset(&t->dst_cache);
@@ -1706,7 +1706,7 @@ static int ipip6_fill_info(struct sk_buff *skb, const struct net_device *dev)
 	    nla_put_u8(skb, IFLA_IPTUN_PROTO, parm->iph.protocol) ||
 	    nla_put_be16(skb, IFLA_IPTUN_FLAGS,
 			 ip_tunnel_flags_to_be16(parm->i_flags)) ||
-	    nla_put_u32(skb, IFLA_IPTUN_FWMARK, tunnel->fwmark))
+	    nla_put_u32(skb, IFLA_IPTUN_FWMARK, READ_ONCE(tunnel->fwmark)))
 		goto nla_put_failure;
 
 #ifdef CONFIG_IPV6_SIT_6RD
-- 
2.55.0.979.g7e5102b832-goog

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1968 bytes --]

> ipip6_tunnel_xmit() runs locklessly (dev->lltx == true) and reads
> tunnel->fwmark.
> 
> In preparation for converting ipip6_fill_info() to run without RTNL,
> add READ_ONCE() and WRITE_ONCE() annotations around tunnel->fwmark.

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv6/sit.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index 9a2bbd76d3794849cd11c6530ba1303bf53fa388..e85fa80e80d1ad2c593b860685b9f5bac21e66d9 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);
>  		ipip6_tunnel_bind_dev(t->dev);
>  	}
>  	dst_cache_reset(&t->dst_cache);
> @@ -1706,7 +1706,7 @@ static int ipip6_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  	    nla_put_u8(skb, IFLA_IPTUN_PROTO, parm->iph.protocol) ||
>  	    nla_put_be16(skb, IFLA_IPTUN_FLAGS,
>  			 ip_tunnel_flags_to_be16(parm->i_flags)) ||
> -	    nla_put_u32(skb, IFLA_IPTUN_FWMARK, tunnel->fwmark))
> +	    nla_put_u32(skb, IFLA_IPTUN_FWMARK, READ_ONCE(tunnel->fwmark)))
>  		goto nla_put_failure;
>  
>  #ifdef CONFIG_IPV6_SIT_6RD
> -- 
> 2.55.0.979.g7e5102b832-goog
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[netdev-bot+sashiko]

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

[PATCH net-next 5/9] sit: convert 6RD configuration to RCU protection

[Eric Dumazet]

In order to allow lockless readers in future patches, convert
'tunnel->ip6rd' to an RCU protected pointer.

Updating 6RD configuration via ipip6_tunnel_update_6rd() or
ipip6_tunnel_clone_6rd() now allocates a struct ip_tunnel_6rd_parm and
uses rcu_assign_pointer() to publish it, freeing the previous
parameters with kfree_rcu().

Readers in check_6rd() and only_dnatted() use rcu_dereference() under
existing RCU read lock, preventing torn reads on the 128-bit IPv6
prefix.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/ip_tunnels.h |   3 +-
 net/ipv6/sit.c           | 160 ++++++++++++++++++++++++++++-----------
 2 files changed, 117 insertions(+), 46 deletions(-)

diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 7c9aadfe8fe396da10a47e93499a97141ac04f4c..7fff59bab53b682ac0d1efd1f47082ba7d633fd2 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -127,6 +127,7 @@ struct ip_tunnel_6rd_parm {
 	__be32			relay_prefix;
 	u16			prefixlen;
 	u16			relay_prefixlen;
+	struct rcu_head		rcu;
 };
 #endif
 
@@ -185,7 +186,7 @@ struct ip_tunnel {
 
 	/* for SIT */
 #ifdef CONFIG_IPV6_SIT_6RD
-	struct ip_tunnel_6rd_parm ip6rd;
+	struct ip_tunnel_6rd_parm __rcu *ip6rd;
 #endif
 	struct ip_tunnel_prl_entry __rcu *prl;	/* potential router list */
 	unsigned int		prl_count;	/* # of entries in PRL */
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index e85fa80e80d1ad2c593b860685b9f5bac21e66d9..7cabcd3afbc4f06d9a08374e6d8df2fb1bfb9676 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -183,21 +183,47 @@ static void ipip6_tunnel_link(struct sit_net *sitn, struct ip_tunnel *t)
 	rcu_assign_pointer(*tp, t);
 }
 
-static void ipip6_tunnel_clone_6rd(struct net_device *dev, struct sit_net *sitn)
+static int ipip6_tunnel_clone_6rd(struct net_device *dev, struct sit_net *sitn)
 {
 #ifdef CONFIG_IPV6_SIT_6RD
 	struct ip_tunnel *t = netdev_priv(dev);
+	struct ip_tunnel_6rd_parm *new_6rd, *old_6rd;
+
+	new_6rd = kmalloc_obj(*new_6rd);
+	if (!new_6rd)
+		return -ENOMEM;
 
 	if (dev == sitn->fb_tunnel_dev || !sitn->fb_tunnel_dev) {
-		ipv6_addr_set(&t->ip6rd.prefix, htonl(0x20020000), 0, 0, 0);
-		t->ip6rd.relay_prefix = 0;
-		t->ip6rd.prefixlen = 16;
-		t->ip6rd.relay_prefixlen = 0;
+		ipv6_addr_set(&new_6rd->prefix, htonl(0x20020000), 0, 0, 0);
+		new_6rd->relay_prefix = 0;
+		new_6rd->prefixlen = 16;
+		new_6rd->relay_prefixlen = 0;
 	} else {
 		struct ip_tunnel *t0 = netdev_priv(sitn->fb_tunnel_dev);
-		memcpy(&t->ip6rd, &t0->ip6rd, sizeof(t->ip6rd));
+		struct ip_tunnel_6rd_parm *t0_6rd;
+
+		t0_6rd = rtnl_dereference(t0->ip6rd);
+		if (t0_6rd) {
+			*new_6rd = *t0_6rd;
+		} else {
+			ipv6_addr_set(&new_6rd->prefix, htonl(0x20020000), 0, 0, 0);
+			new_6rd->relay_prefix = 0;
+			new_6rd->prefixlen = 16;
+			new_6rd->relay_prefixlen = 0;
+		}
+	}
+
+	old_6rd = rcu_dereference_protected(t->ip6rd,
+					    lockdep_rtnl_is_held() ||
+					    dev->reg_state == NETREG_UNINITIALIZED);
+	rcu_assign_pointer(t->ip6rd, new_6rd);
+	if (old_6rd) {
+		dst_cache_reset(&t->dst_cache);
+		netdev_state_change(t->dev);
+		kfree_rcu(old_6rd, rcu);
 	}
 #endif
+	return 0;
 }
 
 static int ipip6_tunnel_create(struct net_device *dev)
@@ -206,6 +232,10 @@ static int ipip6_tunnel_create(struct net_device *dev)
 	struct sit_net *sitn = net_generic(t->net, sit_net_id);
 	int err;
 
+	err = ipip6_tunnel_clone_6rd(dev, sitn);
+	if (err < 0)
+		goto out;
+
 	__dev_addr_set(dev, &t->parms.iph.saddr, 4);
 	memcpy(dev->broadcast, &t->parms.iph.daddr, 4);
 
@@ -218,8 +248,6 @@ static int ipip6_tunnel_create(struct net_device *dev)
 	if (err < 0)
 		goto out;
 
-	ipip6_tunnel_clone_6rd(dev, sitn);
-
 	ipip6_tunnel_link(sitn, t);
 	return 0;
 
@@ -281,6 +309,7 @@ static struct ip_tunnel *ipip6_tunnel_locate(struct net *net,
 	return nt;
 
 failed_free:
+	ipip6_dev_free(dev);
 	free_netdev(dev);
 failed:
 	return NULL;
@@ -631,8 +660,13 @@ static bool only_dnatted(const struct ip_tunnel *tunnel,
 	int prefix_len;
 
 #ifdef CONFIG_IPV6_SIT_6RD
-	prefix_len = tunnel->ip6rd.prefixlen + 32
-		- tunnel->ip6rd.relay_prefixlen;
+	const struct ip_tunnel_6rd_parm *ip6rd;
+
+	ip6rd = rcu_dereference(tunnel->ip6rd);
+	if (ip6rd)
+		prefix_len = ip6rd->prefixlen + 32 - ip6rd->relay_prefixlen;
+	else
+		prefix_len = 48;
 #else
 	prefix_len = 48;
 #endif
@@ -810,25 +844,28 @@ static bool check_6rd(struct ip_tunnel *tunnel, const struct in6_addr *v6dst,
 		      __be32 *v4dst)
 {
 #ifdef CONFIG_IPV6_SIT_6RD
-	if (ipv6_prefix_equal(v6dst, &tunnel->ip6rd.prefix,
-			      tunnel->ip6rd.prefixlen)) {
+	const struct ip_tunnel_6rd_parm *ip6rd;
+
+	ip6rd = rcu_dereference(tunnel->ip6rd);
+	if (ip6rd && ipv6_prefix_equal(v6dst, &ip6rd->prefix,
+				       ip6rd->prefixlen)) {
 		unsigned int pbw0, pbi0;
 		int pbi1;
 		u32 d;
 
-		pbw0 = tunnel->ip6rd.prefixlen >> 5;
-		pbi0 = tunnel->ip6rd.prefixlen & 0x1f;
+		pbw0 = ip6rd->prefixlen >> 5;
+		pbi0 = ip6rd->prefixlen & 0x1f;
 
-		d = tunnel->ip6rd.relay_prefixlen < 32 ?
+		d = ip6rd->relay_prefixlen < 32 ?
 			(ntohl(v6dst->s6_addr32[pbw0]) << pbi0) >>
-		    tunnel->ip6rd.relay_prefixlen : 0;
+		    ip6rd->relay_prefixlen : 0;
 
-		pbi1 = pbi0 - tunnel->ip6rd.relay_prefixlen;
+		pbi1 = pbi0 - ip6rd->relay_prefixlen;
 		if (pbi1 > 0)
 			d |= ntohl(v6dst->s6_addr32[pbw0 + 1]) >>
 			     (32 - pbi1);
 
-		*v4dst = tunnel->ip6rd.relay_prefix | htonl(d);
+		*v4dst = ip6rd->relay_prefix | htonl(d);
 		return true;
 	}
 #else
@@ -1164,6 +1201,7 @@ static void ipip6_tunnel_update(struct ip_tunnel *t,
 static int ipip6_tunnel_update_6rd(struct ip_tunnel *t,
 				   struct ip_tunnel_6rd *ip6rd)
 {
+	struct ip_tunnel_6rd_parm *new_6rd, *old_6rd;
 	struct in6_addr prefix;
 	__be32 relay_prefix;
 
@@ -1183,10 +1221,20 @@ static int ipip6_tunnel_update_6rd(struct ip_tunnel *t,
 	if (relay_prefix != ip6rd->relay_prefix)
 		return -EINVAL;
 
-	t->ip6rd.prefix = prefix;
-	t->ip6rd.relay_prefix = relay_prefix;
-	t->ip6rd.prefixlen = ip6rd->prefixlen;
-	t->ip6rd.relay_prefixlen = ip6rd->relay_prefixlen;
+	new_6rd = kmalloc_obj(*new_6rd);
+	if (!new_6rd)
+		return -ENOMEM;
+
+	new_6rd->prefix = prefix;
+	new_6rd->relay_prefix = relay_prefix;
+	new_6rd->prefixlen = ip6rd->prefixlen;
+	new_6rd->relay_prefixlen = ip6rd->relay_prefixlen;
+
+	old_6rd = rtnl_dereference(t->ip6rd);
+	rcu_assign_pointer(t->ip6rd, new_6rd);
+	if (old_6rd)
+		kfree_rcu(old_6rd, rcu);
+
 	dst_cache_reset(&t->dst_cache);
 	netdev_state_change(t->dev);
 	return 0;
@@ -1195,6 +1243,7 @@ static int ipip6_tunnel_update_6rd(struct ip_tunnel *t,
 static int
 ipip6_tunnel_get6rd(struct net_device *dev, struct ip_tunnel_parm __user *data)
 {
+	const struct ip_tunnel_6rd_parm *ip6rd_parm;
 	struct ip_tunnel *t = netdev_priv(dev);
 	struct ip_tunnel_parm_kern p;
 	struct ip_tunnel_6rd ip6rd;
@@ -1207,10 +1256,15 @@ ipip6_tunnel_get6rd(struct net_device *dev, struct ip_tunnel_parm __user *data)
 	if (!t)
 		t = netdev_priv(dev);
 
-	ip6rd.prefix = t->ip6rd.prefix;
-	ip6rd.relay_prefix = t->ip6rd.relay_prefix;
-	ip6rd.prefixlen = t->ip6rd.prefixlen;
-	ip6rd.relay_prefixlen = t->ip6rd.relay_prefixlen;
+	ip6rd_parm = rtnl_dereference(t->ip6rd);
+	if (ip6rd_parm) {
+		ip6rd.prefix = ip6rd_parm->prefix;
+		ip6rd.relay_prefix = ip6rd_parm->relay_prefix;
+		ip6rd.prefixlen = ip6rd_parm->prefixlen;
+		ip6rd.relay_prefixlen = ip6rd_parm->relay_prefixlen;
+	} else {
+		memset(&ip6rd, 0, sizeof(ip6rd));
+	}
 	if (copy_to_user(data, &ip6rd, sizeof(ip6rd)))
 		return -EFAULT;
 	return 0;
@@ -1222,20 +1276,16 @@ ipip6_tunnel_6rdctl(struct net_device *dev, struct ip_tunnel_6rd __user *data,
 {
 	struct ip_tunnel *t = netdev_priv(dev);
 	struct ip_tunnel_6rd ip6rd;
-	int err;
 
 	if (!ns_capable(t->net->user_ns, CAP_NET_ADMIN))
 		return -EPERM;
 	if (copy_from_user(&ip6rd, data, sizeof(ip6rd)))
 		return -EFAULT;
 
-	if (cmd != SIOCDEL6RD) {
-		err = ipip6_tunnel_update_6rd(t, &ip6rd);
-		if (err < 0)
-			return err;
-	} else
-		ipip6_tunnel_clone_6rd(dev, dev_to_sit_net(dev));
-	return 0;
+	if (cmd != SIOCDEL6RD)
+		return ipip6_tunnel_update_6rd(t, &ip6rd);
+
+	return ipip6_tunnel_clone_6rd(dev, dev_to_sit_net(dev));
 }
 
 #endif /* CONFIG_IPV6_SIT_6RD */
@@ -1406,8 +1456,17 @@ static const struct net_device_ops ipip6_netdev_ops = {
 static void ipip6_dev_free(struct net_device *dev)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
+#ifdef CONFIG_IPV6_SIT_6RD
+	struct ip_tunnel_6rd_parm *ip6rd;
 
-	dst_cache_destroy(&tunnel->dst_cache);
+	ip6rd = rcu_dereference_protected(tunnel->ip6rd, 1);
+	RCU_INIT_POINTER(tunnel->ip6rd, NULL);
+	kfree(ip6rd);
+#endif
+	if (tunnel->dst_cache.cache) {
+		dst_cache_destroy(&tunnel->dst_cache);
+		tunnel->dst_cache.cache = NULL;
+	}
 }
 
 #define SIT_FEATURES (NETIF_F_SG	   | \
@@ -1576,8 +1635,10 @@ static int ipip6_newlink(struct net_device *dev,
 		return -EEXIST;
 
 	err = ipip6_tunnel_create(dev);
-	if (err < 0)
+	if (err < 0) {
+		ipip6_dev_free(dev);
 		return err;
+	}
 
 	if (tb[IFLA_MTU]) {
 		u32 mtu = nla_get_u32(tb[IFLA_MTU]);
@@ -1695,6 +1756,9 @@ static int ipip6_fill_info(struct sk_buff *skb, const struct net_device *dev)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
 	struct ip_tunnel_parm_kern *parm = &tunnel->parms;
+#ifdef CONFIG_IPV6_SIT_6RD
+	const struct ip_tunnel_6rd_parm *ip6rd;
+#endif
 
 	if (nla_put_u32(skb, IFLA_IPTUN_LINK, parm->link) ||
 	    nla_put_in_addr(skb, IFLA_IPTUN_LOCAL, parm->iph.saddr) ||
@@ -1710,14 +1774,16 @@ static int ipip6_fill_info(struct sk_buff *skb, const struct net_device *dev)
 		goto nla_put_failure;
 
 #ifdef CONFIG_IPV6_SIT_6RD
-	if (nla_put_in6_addr(skb, IFLA_IPTUN_6RD_PREFIX,
-			     &tunnel->ip6rd.prefix) ||
-	    nla_put_in_addr(skb, IFLA_IPTUN_6RD_RELAY_PREFIX,
-			    tunnel->ip6rd.relay_prefix) ||
-	    nla_put_u16(skb, IFLA_IPTUN_6RD_PREFIXLEN,
-			tunnel->ip6rd.prefixlen) ||
-	    nla_put_u16(skb, IFLA_IPTUN_6RD_RELAY_PREFIXLEN,
-			tunnel->ip6rd.relay_prefixlen))
+	ip6rd = rcu_dereference_rtnl(tunnel->ip6rd);
+	if (ip6rd &&
+	    (nla_put_in6_addr(skb, IFLA_IPTUN_6RD_PREFIX,
+			      &ip6rd->prefix) ||
+	     nla_put_in_addr(skb, IFLA_IPTUN_6RD_RELAY_PREFIX,
+			     ip6rd->relay_prefix) ||
+	     nla_put_u16(skb, IFLA_IPTUN_6RD_PREFIXLEN,
+			 ip6rd->prefixlen) ||
+	     nla_put_u16(skb, IFLA_IPTUN_6RD_RELAY_PREFIXLEN,
+			 ip6rd->relay_prefixlen)))
 		goto nla_put_failure;
 #endif
 
@@ -1863,17 +1929,21 @@ static int __net_init sit_init_net(struct net *net)
 	t = netdev_priv(sitn->fb_tunnel_dev);
 	t->net = net;
 
+	err = ipip6_tunnel_clone_6rd(sitn->fb_tunnel_dev, sitn);
+	if (err < 0)
+		goto err_reg_dev;
+
 	err = register_netdev(sitn->fb_tunnel_dev);
 	if (err)
 		goto err_reg_dev;
 
-	ipip6_tunnel_clone_6rd(sitn->fb_tunnel_dev, sitn);
 	ipip6_fb_tunnel_init(sitn->fb_tunnel_dev);
 
 	strscpy(t->parms.name, sitn->fb_tunnel_dev->name);
 	return 0;
 
 err_reg_dev:
+	ipip6_dev_free(sitn->fb_tunnel_dev);
 	free_netdev(sitn->fb_tunnel_dev);
 err_alloc_dev:
 	return err;
-- 
2.55.0.979.g7e5102b832-goog

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 12418 bytes --]

> In order to allow lockless readers in future patches, convert
> 'tunnel->ip6rd' to an RCU protected pointer.
> 
> Updating 6RD configuration via ipip6_tunnel_update_6rd() or
> ipip6_tunnel_clone_6rd() now allocates a struct ip_tunnel_6rd_parm and
> uses rcu_assign_pointer() to publish it, freeing the previous
> parameters with kfree_rcu().
> 
> Readers in check_6rd() and only_dnatted() use rcu_dereference() under
> existing RCU read lock, preventing torn reads on the 128-bit IPv6
> prefix.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Hi Eric,

just few nits inline.

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

Regards,
Lorenzo

> ---
>  include/net/ip_tunnels.h |   3 +-
>  net/ipv6/sit.c           | 160 ++++++++++++++++++++++++++++-----------
>  2 files changed, 117 insertions(+), 46 deletions(-)
> 
> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index 7c9aadfe8fe396da10a47e93499a97141ac04f4c..7fff59bab53b682ac0d1efd1f47082ba7d633fd2 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -127,6 +127,7 @@ struct ip_tunnel_6rd_parm {
>  	__be32			relay_prefix;
>  	u16			prefixlen;
>  	u16			relay_prefixlen;
> +	struct rcu_head		rcu;
>  };
>  #endif
>  
> @@ -185,7 +186,7 @@ struct ip_tunnel {
>  
>  	/* for SIT */
>  #ifdef CONFIG_IPV6_SIT_6RD
> -	struct ip_tunnel_6rd_parm ip6rd;
> +	struct ip_tunnel_6rd_parm __rcu *ip6rd;
>  #endif
>  	struct ip_tunnel_prl_entry __rcu *prl;	/* potential router list */
>  	unsigned int		prl_count;	/* # of entries in PRL */
> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index e85fa80e80d1ad2c593b860685b9f5bac21e66d9..7cabcd3afbc4f06d9a08374e6d8df2fb1bfb9676 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -183,21 +183,47 @@ static void ipip6_tunnel_link(struct sit_net *sitn, struct ip_tunnel *t)
>  	rcu_assign_pointer(*tp, t);
>  }
>  
> -static void ipip6_tunnel_clone_6rd(struct net_device *dev, struct sit_net *sitn)
> +static int ipip6_tunnel_clone_6rd(struct net_device *dev, struct sit_net *sitn)
>  {
>  #ifdef CONFIG_IPV6_SIT_6RD
>  	struct ip_tunnel *t = netdev_priv(dev);
> +	struct ip_tunnel_6rd_parm *new_6rd, *old_6rd;

RCT

> +
> +	new_6rd = kmalloc_obj(*new_6rd);

what about using kzalloc_obj() and get rid of the 0 initialization? This is
just control path, so I guess it is fine.

> +	if (!new_6rd)
> +		return -ENOMEM;
>  
>  	if (dev == sitn->fb_tunnel_dev || !sitn->fb_tunnel_dev) {
> -		ipv6_addr_set(&t->ip6rd.prefix, htonl(0x20020000), 0, 0, 0);
> -		t->ip6rd.relay_prefix = 0;
> -		t->ip6rd.prefixlen = 16;
> -		t->ip6rd.relay_prefixlen = 0;
> +		ipv6_addr_set(&new_6rd->prefix, htonl(0x20020000), 0, 0, 0);
> +		new_6rd->relay_prefix = 0;
> +		new_6rd->prefixlen = 16;
> +		new_6rd->relay_prefixlen = 0;
>  	} else {
>  		struct ip_tunnel *t0 = netdev_priv(sitn->fb_tunnel_dev);
> -		memcpy(&t->ip6rd, &t0->ip6rd, sizeof(t->ip6rd));
> +		struct ip_tunnel_6rd_parm *t0_6rd;
> +
> +		t0_6rd = rtnl_dereference(t0->ip6rd);
> +		if (t0_6rd) {
> +			*new_6rd = *t0_6rd;
> +		} else {
> +			ipv6_addr_set(&new_6rd->prefix, htonl(0x20020000), 0, 0, 0);
> +			new_6rd->relay_prefix = 0;
> +			new_6rd->prefixlen = 16;
> +			new_6rd->relay_prefixlen = 0;
> +		}
> +	}
> +
> +	old_6rd = rcu_dereference_protected(t->ip6rd,
> +					    lockdep_rtnl_is_held() ||
> +					    dev->reg_state == NETREG_UNINITIALIZED);
> +	rcu_assign_pointer(t->ip6rd, new_6rd);

what about using rcu_replace_pointer()?

> +	if (old_6rd) {
> +		dst_cache_reset(&t->dst_cache);
> +		netdev_state_change(t->dev);
> +		kfree_rcu(old_6rd, rcu);
>  	}
>  #endif
> +	return 0;
>  }
>  
>  static int ipip6_tunnel_create(struct net_device *dev)
> @@ -206,6 +232,10 @@ static int ipip6_tunnel_create(struct net_device *dev)
>  	struct sit_net *sitn = net_generic(t->net, sit_net_id);
>  	int err;
>  
> +	err = ipip6_tunnel_clone_6rd(dev, sitn);
> +	if (err < 0)
> +		goto out;

I guess you can just return err here.

> +
>  	__dev_addr_set(dev, &t->parms.iph.saddr, 4);
>  	memcpy(dev->broadcast, &t->parms.iph.daddr, 4);
>  
> @@ -218,8 +248,6 @@ static int ipip6_tunnel_create(struct net_device *dev)
>  	if (err < 0)
>  		goto out;
>  
> -	ipip6_tunnel_clone_6rd(dev, sitn);
> -
>  	ipip6_tunnel_link(sitn, t);
>  	return 0;
>  
> @@ -281,6 +309,7 @@ static struct ip_tunnel *ipip6_tunnel_locate(struct net *net,
>  	return nt;
>  
>  failed_free:
> +	ipip6_dev_free(dev);
>  	free_netdev(dev);
>  failed:
>  	return NULL;
> @@ -631,8 +660,13 @@ static bool only_dnatted(const struct ip_tunnel *tunnel,
>  	int prefix_len;
>  
>  #ifdef CONFIG_IPV6_SIT_6RD
> -	prefix_len = tunnel->ip6rd.prefixlen + 32
> -		- tunnel->ip6rd.relay_prefixlen;
> +	const struct ip_tunnel_6rd_parm *ip6rd;
> +
> +	ip6rd = rcu_dereference(tunnel->ip6rd);
> +	if (ip6rd)
> +		prefix_len = ip6rd->prefixlen + 32 - ip6rd->relay_prefixlen;
> +	else
> +		prefix_len = 48;
>  #else
>  	prefix_len = 48;
>  #endif
> @@ -810,25 +844,28 @@ static bool check_6rd(struct ip_tunnel *tunnel, const struct in6_addr *v6dst,
>  		      __be32 *v4dst)
>  {
>  #ifdef CONFIG_IPV6_SIT_6RD
> -	if (ipv6_prefix_equal(v6dst, &tunnel->ip6rd.prefix,
> -			      tunnel->ip6rd.prefixlen)) {
> +	const struct ip_tunnel_6rd_parm *ip6rd;
> +
> +	ip6rd = rcu_dereference(tunnel->ip6rd);
> +	if (ip6rd && ipv6_prefix_equal(v6dst, &ip6rd->prefix,
> +				       ip6rd->prefixlen)) {
>  		unsigned int pbw0, pbi0;
>  		int pbi1;
>  		u32 d;
>  
> -		pbw0 = tunnel->ip6rd.prefixlen >> 5;
> -		pbi0 = tunnel->ip6rd.prefixlen & 0x1f;
> +		pbw0 = ip6rd->prefixlen >> 5;
> +		pbi0 = ip6rd->prefixlen & 0x1f;
>  
> -		d = tunnel->ip6rd.relay_prefixlen < 32 ?
> +		d = ip6rd->relay_prefixlen < 32 ?
>  			(ntohl(v6dst->s6_addr32[pbw0]) << pbi0) >>
> -		    tunnel->ip6rd.relay_prefixlen : 0;
> +		    ip6rd->relay_prefixlen : 0;
>  
> -		pbi1 = pbi0 - tunnel->ip6rd.relay_prefixlen;
> +		pbi1 = pbi0 - ip6rd->relay_prefixlen;
>  		if (pbi1 > 0)
>  			d |= ntohl(v6dst->s6_addr32[pbw0 + 1]) >>
>  			     (32 - pbi1);
>  
> -		*v4dst = tunnel->ip6rd.relay_prefix | htonl(d);
> +		*v4dst = ip6rd->relay_prefix | htonl(d);
>  		return true;
>  	}
>  #else
> @@ -1164,6 +1201,7 @@ static void ipip6_tunnel_update(struct ip_tunnel *t,
>  static int ipip6_tunnel_update_6rd(struct ip_tunnel *t,
>  				   struct ip_tunnel_6rd *ip6rd)
>  {
> +	struct ip_tunnel_6rd_parm *new_6rd, *old_6rd;
>  	struct in6_addr prefix;
>  	__be32 relay_prefix;
>  
> @@ -1183,10 +1221,20 @@ static int ipip6_tunnel_update_6rd(struct ip_tunnel *t,
>  	if (relay_prefix != ip6rd->relay_prefix)
>  		return -EINVAL;
>  
> -	t->ip6rd.prefix = prefix;
> -	t->ip6rd.relay_prefix = relay_prefix;
> -	t->ip6rd.prefixlen = ip6rd->prefixlen;
> -	t->ip6rd.relay_prefixlen = ip6rd->relay_prefixlen;
> +	new_6rd = kmalloc_obj(*new_6rd);
> +	if (!new_6rd)
> +		return -ENOMEM;
> +
> +	new_6rd->prefix = prefix;
> +	new_6rd->relay_prefix = relay_prefix;
> +	new_6rd->prefixlen = ip6rd->prefixlen;
> +	new_6rd->relay_prefixlen = ip6rd->relay_prefixlen;
> +
> +	old_6rd = rtnl_dereference(t->ip6rd);
> +	rcu_assign_pointer(t->ip6rd, new_6rd);

rcu_replace_pointer()?

> +	if (old_6rd)
> +		kfree_rcu(old_6rd, rcu);
> +
>  	dst_cache_reset(&t->dst_cache);
>  	netdev_state_change(t->dev);
>  	return 0;
> @@ -1195,6 +1243,7 @@ static int ipip6_tunnel_update_6rd(struct ip_tunnel *t,
>  static int
>  ipip6_tunnel_get6rd(struct net_device *dev, struct ip_tunnel_parm __user *data)
>  {
> +	const struct ip_tunnel_6rd_parm *ip6rd_parm;
>  	struct ip_tunnel *t = netdev_priv(dev);
>  	struct ip_tunnel_parm_kern p;
>  	struct ip_tunnel_6rd ip6rd;
> @@ -1207,10 +1256,15 @@ ipip6_tunnel_get6rd(struct net_device *dev, struct ip_tunnel_parm __user *data)
>  	if (!t)
>  		t = netdev_priv(dev);
>  
> -	ip6rd.prefix = t->ip6rd.prefix;
> -	ip6rd.relay_prefix = t->ip6rd.relay_prefix;
> -	ip6rd.prefixlen = t->ip6rd.prefixlen;
> -	ip6rd.relay_prefixlen = t->ip6rd.relay_prefixlen;
> +	ip6rd_parm = rtnl_dereference(t->ip6rd);
> +	if (ip6rd_parm) {
> +		ip6rd.prefix = ip6rd_parm->prefix;
> +		ip6rd.relay_prefix = ip6rd_parm->relay_prefix;
> +		ip6rd.prefixlen = ip6rd_parm->prefixlen;
> +		ip6rd.relay_prefixlen = ip6rd_parm->relay_prefixlen;
> +	} else {
> +		memset(&ip6rd, 0, sizeof(ip6rd));
> +	}
>  	if (copy_to_user(data, &ip6rd, sizeof(ip6rd)))
>  		return -EFAULT;
>  	return 0;
> @@ -1222,20 +1276,16 @@ ipip6_tunnel_6rdctl(struct net_device *dev, struct ip_tunnel_6rd __user *data,
>  {
>  	struct ip_tunnel *t = netdev_priv(dev);
>  	struct ip_tunnel_6rd ip6rd;
> -	int err;
>  
>  	if (!ns_capable(t->net->user_ns, CAP_NET_ADMIN))
>  		return -EPERM;
>  	if (copy_from_user(&ip6rd, data, sizeof(ip6rd)))
>  		return -EFAULT;
>  
> -	if (cmd != SIOCDEL6RD) {
> -		err = ipip6_tunnel_update_6rd(t, &ip6rd);
> -		if (err < 0)
> -			return err;
> -	} else
> -		ipip6_tunnel_clone_6rd(dev, dev_to_sit_net(dev));
> -	return 0;
> +	if (cmd != SIOCDEL6RD)
> +		return ipip6_tunnel_update_6rd(t, &ip6rd);
> +
> +	return ipip6_tunnel_clone_6rd(dev, dev_to_sit_net(dev));
>  }
>  
>  #endif /* CONFIG_IPV6_SIT_6RD */
> @@ -1406,8 +1456,17 @@ static const struct net_device_ops ipip6_netdev_ops = {
>  static void ipip6_dev_free(struct net_device *dev)
>  {
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
> +#ifdef CONFIG_IPV6_SIT_6RD
> +	struct ip_tunnel_6rd_parm *ip6rd;
>  
> -	dst_cache_destroy(&tunnel->dst_cache);
> +	ip6rd = rcu_dereference_protected(tunnel->ip6rd, 1);
> +	RCU_INIT_POINTER(tunnel->ip6rd, NULL);
> +	kfree(ip6rd);
> +#endif
> +	if (tunnel->dst_cache.cache) {
> +		dst_cache_destroy(&tunnel->dst_cache);
> +		tunnel->dst_cache.cache = NULL;
> +	}
>  }
>  
>  #define SIT_FEATURES (NETIF_F_SG	   | \
> @@ -1576,8 +1635,10 @@ static int ipip6_newlink(struct net_device *dev,
>  		return -EEXIST;
>  
>  	err = ipip6_tunnel_create(dev);
> -	if (err < 0)
> +	if (err < 0) {
> +		ipip6_dev_free(dev);
>  		return err;
> +	}
>  
>  	if (tb[IFLA_MTU]) {
>  		u32 mtu = nla_get_u32(tb[IFLA_MTU]);
> @@ -1695,6 +1756,9 @@ static int ipip6_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  {
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
>  	struct ip_tunnel_parm_kern *parm = &tunnel->parms;
> +#ifdef CONFIG_IPV6_SIT_6RD
> +	const struct ip_tunnel_6rd_parm *ip6rd;
> +#endif
>  
>  	if (nla_put_u32(skb, IFLA_IPTUN_LINK, parm->link) ||
>  	    nla_put_in_addr(skb, IFLA_IPTUN_LOCAL, parm->iph.saddr) ||
> @@ -1710,14 +1774,16 @@ static int ipip6_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  		goto nla_put_failure;
>  
>  #ifdef CONFIG_IPV6_SIT_6RD
> -	if (nla_put_in6_addr(skb, IFLA_IPTUN_6RD_PREFIX,
> -			     &tunnel->ip6rd.prefix) ||
> -	    nla_put_in_addr(skb, IFLA_IPTUN_6RD_RELAY_PREFIX,
> -			    tunnel->ip6rd.relay_prefix) ||
> -	    nla_put_u16(skb, IFLA_IPTUN_6RD_PREFIXLEN,
> -			tunnel->ip6rd.prefixlen) ||
> -	    nla_put_u16(skb, IFLA_IPTUN_6RD_RELAY_PREFIXLEN,
> -			tunnel->ip6rd.relay_prefixlen))
> +	ip6rd = rcu_dereference_rtnl(tunnel->ip6rd);
> +	if (ip6rd &&
> +	    (nla_put_in6_addr(skb, IFLA_IPTUN_6RD_PREFIX,
> +			      &ip6rd->prefix) ||
> +	     nla_put_in_addr(skb, IFLA_IPTUN_6RD_RELAY_PREFIX,
> +			     ip6rd->relay_prefix) ||
> +	     nla_put_u16(skb, IFLA_IPTUN_6RD_PREFIXLEN,
> +			 ip6rd->prefixlen) ||
> +	     nla_put_u16(skb, IFLA_IPTUN_6RD_RELAY_PREFIXLEN,
> +			 ip6rd->relay_prefixlen)))
>  		goto nla_put_failure;
>  #endif
>  
> @@ -1863,17 +1929,21 @@ static int __net_init sit_init_net(struct net *net)
>  	t = netdev_priv(sitn->fb_tunnel_dev);
>  	t->net = net;
>  
> +	err = ipip6_tunnel_clone_6rd(sitn->fb_tunnel_dev, sitn);
> +	if (err < 0)
> +		goto err_reg_dev;
> +
>  	err = register_netdev(sitn->fb_tunnel_dev);
>  	if (err)
>  		goto err_reg_dev;
>  
> -	ipip6_tunnel_clone_6rd(sitn->fb_tunnel_dev, sitn);
>  	ipip6_fb_tunnel_init(sitn->fb_tunnel_dev);
>  
>  	strscpy(t->parms.name, sitn->fb_tunnel_dev->name);
>  	return 0;
>  
>  err_reg_dev:
> +	ipip6_dev_free(sitn->fb_tunnel_dev);
>  	free_netdev(sitn->fb_tunnel_dev);
>  err_alloc_dev:
>  	return err;
> -- 
> 2.55.0.979.g7e5102b832-goog
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[netdev-bot+sashiko]

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

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

- [Medium] check_6rd() now performs `ip6rd =
  rcu_dereference(tunnel->ip6rd)` (net/ipv6/sit.c:849), but check_6rd()…
- [Low] The patch changes SIOCDEL6RD behaviour beyond the advertised RCU
  conversion: ipip6_tunnel_clone_6rd() now, when an old 6RD…

--

sit: convert 6RD configuration to RCU protection

This converts tunnel->ip6rd from an embedded struct into an RCU protected
pointer, so updates via ipip6_tunnel_update_6rd() and
ipip6_tunnel_clone_6rd() allocate a new struct ip_tunnel_6rd_parm, publish
it with rcu_assign_pointer() and free the old one with kfree_rcu().
Readers in check_6rd(), only_dnatted(), ipip6_tunnel_get6rd() and
ipip6_fill_info() are updated accordingly.

> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index e85fa80e80d1a..7cabcd3afbc4f 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -183,21 +183,47 @@ static void ipip6_tunnel_link(struct sit_net *sitn, struct ip_tunnel *t)
>  	rcu_assign_pointer(*tp, t);
>  }
>  
> -static void ipip6_tunnel_clone_6rd(struct net_device *dev, struct sit_net *sitn)
> +static int ipip6_tunnel_clone_6rd(struct net_device *dev, struct sit_net *sitn)
>  {
>  #ifdef CONFIG_IPV6_SIT_6RD
>  	struct ip_tunnel *t = netdev_priv(dev);
> +	struct ip_tunnel_6rd_parm *new_6rd, *old_6rd;
> +
> +	new_6rd = kmalloc_obj(*new_6rd);
> +	if (!new_6rd)
> +		return -ENOMEM;
>  
>  	if (dev == sitn->fb_tunnel_dev || !sitn->fb_tunnel_dev) {
> -		ipv6_addr_set(&t->ip6rd.prefix, htonl(0x20020000), 0, 0, 0);
> -		t->ip6rd.relay_prefix = 0;
> -		t->ip6rd.prefixlen = 16;
> -		t->ip6rd.relay_prefixlen = 0;
> +		ipv6_addr_set(&new_6rd->prefix, htonl(0x20020000), 0, 0, 0);
> +		new_6rd->relay_prefix = 0;
> +		new_6rd->prefixlen = 16;
> +		new_6rd->relay_prefixlen = 0;
>  	} else {
>  		struct ip_tunnel *t0 = netdev_priv(sitn->fb_tunnel_dev);
> -		memcpy(&t->ip6rd, &t0->ip6rd, sizeof(t->ip6rd));
> +		struct ip_tunnel_6rd_parm *t0_6rd;
> +
> +		t0_6rd = rtnl_dereference(t0->ip6rd);
> +		if (t0_6rd) {
> +			*new_6rd = *t0_6rd;
> +		} else {
> +			ipv6_addr_set(&new_6rd->prefix, htonl(0x20020000), 0, 0, 0);
> +			new_6rd->relay_prefix = 0;
> +			new_6rd->prefixlen = 16;
> +			new_6rd->relay_prefixlen = 0;
> +		}
> +	}
> +
> +	old_6rd = rcu_dereference_protected(t->ip6rd,
> +					    lockdep_rtnl_is_held() ||
> +					    dev->reg_state == NETREG_UNINITIALIZED);
> +	rcu_assign_pointer(t->ip6rd, new_6rd);
> +	if (old_6rd) {
> +		dst_cache_reset(&t->dst_cache);
> +		netdev_state_change(t->dev);
> +		kfree_rcu(old_6rd, rcu);
>  	}
>  #endif
> +	return 0;
>  }

[Severity: Low]
Is the new dst_cache_reset() plus netdev_state_change() in
ipip6_tunnel_clone_6rd() intended as part of this conversion?

Before this patch, the SIOCDEL6RD path did neither:

  ipip6_tunnel_6rdctl()
    ipip6_tunnel_clone_6rd()   /* just overwrote t->ip6rd fields */

Only ipip6_tunnel_update_6rd() invalidated the destination cache and
notified userspace.  Since ipip6_tunnel_xmit() derives the IPv4
destination from the 6RD configuration via try_6rd() and then consults
dst_cache_get_ip4(&tunnel->dst_cache, ...), the added reset changes where
packets go after a 6RD deletion, and netdev_state_change() ->
netif_state_change() emits an extra NETDEV_CHANGE plus RTM_NEWLINK that
link watchers will see.

Would it make sense to split that out into its own patch, with a Fixes:
tag if it is fixing stale routing after SIOCDEL6RD?  The changelog
currently only describes the allocation and publish conversion.

[ ... ]

> @@ -810,25 +844,28 @@ static bool check_6rd(struct ip_tunnel *tunnel, const struct in6_addr *v6dst,
>  		      __be32 *v4dst)
>  {
>  #ifdef CONFIG_IPV6_SIT_6RD
> -	if (ipv6_prefix_equal(v6dst, &tunnel->ip6rd.prefix,
> -			      tunnel->ip6rd.prefixlen)) {
> +	const struct ip_tunnel_6rd_parm *ip6rd;
> +
> +	ip6rd = rcu_dereference(tunnel->ip6rd);
> +	if (ip6rd && ipv6_prefix_equal(v6dst, &ip6rd->prefix,
> +				       ip6rd->prefixlen)) {

[Severity: Medium]
Is plain rcu_dereference() the right accessor here?  The changelog says:

  "Readers in check_6rd() and only_dnatted() use rcu_dereference() under
   existing RCU read lock"

That holds for the receive path, but check_6rd() is also reached from
transmit:

  netdev_start_xmit()
    sit_tunnel_xmit()
      ipip6_tunnel_xmit()
        try_6rd()
          check_6rd()
            rcu_dereference(tunnel->ip6rd)

and the xmit callers do not hold rcu_read_lock().  __dev_queue_xmit()
takes rcu_read_lock_bh(), which acquires rcu_bh_lock_map, and the
AF_PACKET qdisc bypass path __dev_direct_xmit() only does
local_bh_disable():

net/core/dev.c:__dev_direct_xmit() {
	...
	local_bh_disable();

	dev_xmit_recursion_inc();
	HARD_TX_LOCK(dev, txq, smp_processor_id());
	...
}

rcu_dereference() expands to rcu_dereference_check(p, 0), whose condition
is rcu_read_lock_held():

kernel/rcu/update.c:rcu_read_lock_held() {
	...
	return lock_is_held(&rcu_lock_map);
}

so on a CONFIG_PROVE_RCU kernel, would sending a packet on a sit device
from these contexts produce a "suspicious rcu_dereference_check() usage"
splat?  Object lifetime looks fine, since BH-disabled regions are RCU
read-side sections, but the annotation seems off.

Would rcu_dereference_bh() (or rcu_dereference_check() with
rcu_read_lock_bh_held()) be more accurate, or alternatively an explicit
rcu_read_lock() in ipip6_tunnel_xmit(), like vxlan_xmit() does before its
RCU lookups?  The same question applies to the new rcu_dereference() in
only_dnatted().

[ ... ]

> @@ -1222,20 +1276,16 @@ ipip6_tunnel_6rdctl(struct net_device *dev, struct ip_tunnel_6rd __user *data,
>  {
>  	struct ip_tunnel *t = netdev_priv(dev);
>  	struct ip_tunnel_6rd ip6rd;
> -	int err;
>  
>  	if (!ns_capable(t->net->user_ns, CAP_NET_ADMIN))
>  		return -EPERM;
>  	if (copy_from_user(&ip6rd, data, sizeof(ip6rd)))
>  		return -EFAULT;
>  
> -	if (cmd != SIOCDEL6RD) {
> -		err = ipip6_tunnel_update_6rd(t, &ip6rd);
> -		if (err < 0)
> -			return err;
> -	} else
> -		ipip6_tunnel_clone_6rd(dev, dev_to_sit_net(dev));
> -	return 0;
> +	if (cmd != SIOCDEL6RD)
> +		return ipip6_tunnel_update_6rd(t, &ip6rd);
> +
> +	return ipip6_tunnel_clone_6rd(dev, dev_to_sit_net(dev));
>  }

Related to the comment above: SIOCDEL6RD can now also fail with -ENOMEM,
where it previously always returned 0.  Is that intentional?

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

[PATCH net-next 6/9] sit: implement ipip6_get_iflink()

[Eric Dumazet]

SIT currently uses ip_tunnel_get_iflink() which reads tunnel->parms.link.
In order to decouple SIT parameter handling from ip_tunnel, implement
its own ndo_get_iflink callback.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv6/sit.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 7cabcd3afbc4f06d9a08374e6d8df2fb1bfb9676..35c6695909014a2c335f1de7afb48f07b7c91c39 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -1444,12 +1444,19 @@ ipip6_tunnel_siocdevprivate(struct net_device *dev, struct ifreq *ifr,
 	}
 }
 
+static int ipip6_get_iflink(const struct net_device *dev)
+{
+	struct ip_tunnel *tunnel = netdev_priv(dev);
+
+	return READ_ONCE(tunnel->parms.link);
+}
+
 static const struct net_device_ops ipip6_netdev_ops = {
 	.ndo_init	= ipip6_tunnel_init,
 	.ndo_uninit	= ipip6_tunnel_uninit,
 	.ndo_start_xmit	= sit_tunnel_xmit,
 	.ndo_siocdevprivate = ipip6_tunnel_siocdevprivate,
-	.ndo_get_iflink = ip_tunnel_get_iflink,
+	.ndo_get_iflink = ipip6_get_iflink,
 	.ndo_tunnel_ctl = ipip6_tunnel_ctl,
 };
 
-- 
2.55.0.979.g7e5102b832-goog

Re: [PATCH net-next 6/9] sit: implement ipip6_get_iflink()

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1307 bytes --]

> SIT currently uses ip_tunnel_get_iflink() which reads tunnel->parms.link.
> In order to decouple SIT parameter handling from ip_tunnel, implement
> its own ndo_get_iflink callback.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

> ---
>  net/ipv6/sit.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index 7cabcd3afbc4f06d9a08374e6d8df2fb1bfb9676..35c6695909014a2c335f1de7afb48f07b7c91c39 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -1444,12 +1444,19 @@ ipip6_tunnel_siocdevprivate(struct net_device *dev, struct ifreq *ifr,
>  	}
>  }
>  
> +static int ipip6_get_iflink(const struct net_device *dev)
> +{
> +	struct ip_tunnel *tunnel = netdev_priv(dev);
> +
> +	return READ_ONCE(tunnel->parms.link);
> +}
> +
>  static const struct net_device_ops ipip6_netdev_ops = {
>  	.ndo_init	= ipip6_tunnel_init,
>  	.ndo_uninit	= ipip6_tunnel_uninit,
>  	.ndo_start_xmit	= sit_tunnel_xmit,
>  	.ndo_siocdevprivate = ipip6_tunnel_siocdevprivate,
> -	.ndo_get_iflink = ip_tunnel_get_iflink,
> +	.ndo_get_iflink = ipip6_get_iflink,
>  	.ndo_tunnel_ctl = ipip6_tunnel_ctl,
>  };
>  
> -- 
> 2.55.0.979.g7e5102b832-goog
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH net-next 7/9] sit: dynamically allocate struct ip_tunnel_parm_kern

[Eric Dumazet]

In preparation for converting SIT configuration parameters to RCU,
dynamically allocate struct ip_tunnel_parm_kern as sit_parms in
struct ip_tunnel.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/ip_tunnels.h |   1 +
 net/ipv6/sit.c           | 152 +++++++++++++++++++++++----------------
 2 files changed, 92 insertions(+), 61 deletions(-)

diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 7fff59bab53b682ac0d1efd1f47082ba7d633fd2..f464c4480edaf35f9ace1c5081c564ce51fed636 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -190,6 +190,7 @@ struct ip_tunnel {
 #endif
 	struct ip_tunnel_prl_entry __rcu *prl;	/* potential router list */
 	unsigned int		prl_count;	/* # of entries in PRL */
+	struct ip_tunnel_parm_kern *sit_parms;
 	unsigned int		ip_tnl_net_id;
 	struct gro_cells	gro_cells;
 	__u32			fwmark;
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 35c6695909014a2c335f1de7afb48f07b7c91c39..dc37c7109af5324f2ced0301b289e7622dd1a55f 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -108,24 +108,24 @@ static struct ip_tunnel *ipip6_tunnel_lookup(struct net *net,
 	int ifindex = dev ? dev->ifindex : 0;
 
 	for_each_ip_tunnel_rcu(t, sitn->tunnels_r_l[h0 ^ h1]) {
-		if (local == t->parms.iph.saddr &&
-		    remote == t->parms.iph.daddr &&
-		    (!dev || !t->parms.link || ifindex == t->parms.link ||
-		     sifindex == t->parms.link) &&
+		if (local == t->sit_parms->iph.saddr &&
+		    remote == t->sit_parms->iph.daddr &&
+		    (!dev || !t->sit_parms->link || ifindex == t->sit_parms->link ||
+		     sifindex == t->sit_parms->link) &&
 		    (t->dev->flags & IFF_UP))
 			return t;
 	}
 	for_each_ip_tunnel_rcu(t, sitn->tunnels_r[h0]) {
-		if (remote == t->parms.iph.daddr &&
-		    (!dev || !t->parms.link || ifindex == t->parms.link ||
-		     sifindex == t->parms.link) &&
+		if (remote == t->sit_parms->iph.daddr &&
+		    (!dev || !t->sit_parms->link || ifindex == t->sit_parms->link ||
+		     sifindex == t->sit_parms->link) &&
 		    (t->dev->flags & IFF_UP))
 			return t;
 	}
 	for_each_ip_tunnel_rcu(t, sitn->tunnels_l[h1]) {
-		if (local == t->parms.iph.saddr &&
-		    (!dev || !t->parms.link || ifindex == t->parms.link ||
-		     sifindex == t->parms.link) &&
+		if (local == t->sit_parms->iph.saddr &&
+		    (!dev || !t->sit_parms->link || ifindex == t->sit_parms->link ||
+		     sifindex == t->sit_parms->link) &&
 		    (t->dev->flags & IFF_UP))
 			return t;
 	}
@@ -157,7 +157,7 @@ __ipip6_bucket(struct sit_net *sitn, struct ip_tunnel_parm_kern *parms)
 static inline struct ip_tunnel __rcu **ipip6_bucket(struct sit_net *sitn,
 		struct ip_tunnel *t)
 {
-	return __ipip6_bucket(sitn, &t->parms);
+	return __ipip6_bucket(sitn, t->sit_parms);
 }
 
 static void ipip6_tunnel_unlink(struct sit_net *sitn, struct ip_tunnel *t)
@@ -236,10 +236,11 @@ static int ipip6_tunnel_create(struct net_device *dev)
 	if (err < 0)
 		goto out;
 
-	__dev_addr_set(dev, &t->parms.iph.saddr, 4);
-	memcpy(dev->broadcast, &t->parms.iph.daddr, 4);
+	t->parms = *t->sit_parms;
+	__dev_addr_set(dev, &t->sit_parms->iph.saddr, 4);
+	memcpy(dev->broadcast, &t->sit_parms->iph.daddr, 4);
 
-	if (test_bit(IP_TUNNEL_SIT_ISATAP_BIT, t->parms.i_flags))
+	if (test_bit(IP_TUNNEL_SIT_ISATAP_BIT, t->sit_parms->i_flags))
 		dev->priv_flags |= IFF_ISATAP;
 
 	dev->rtnl_link_ops = &sit_link_ops;
@@ -270,9 +271,9 @@ static struct ip_tunnel *ipip6_tunnel_locate(struct net *net,
 	for (tp = __ipip6_bucket(sitn, parms);
 	    (t = rtnl_dereference(*tp)) != NULL;
 	     tp = &t->next) {
-		if (local == t->parms.iph.saddr &&
-		    remote == t->parms.iph.daddr &&
-		    parms->link == t->parms.link) {
+		if (local == t->sit_parms->iph.saddr &&
+		    remote == t->sit_parms->iph.daddr &&
+		    parms->link == t->sit_parms->link) {
 			if (create)
 				return NULL;
 			else
@@ -299,7 +300,10 @@ static struct ip_tunnel *ipip6_tunnel_locate(struct net *net,
 	nt = netdev_priv(dev);
 
 	nt->net = net;
-	nt->parms = *parms;
+	nt->sit_parms = kmalloc_obj(*nt->sit_parms);
+	if (!nt->sit_parms)
+		goto failed_free;
+	*nt->sit_parms = *parms;
 	if (ipip6_tunnel_create(dev) < 0)
 		goto failed_free;
 
@@ -602,12 +606,12 @@ static int ipip6_err(struct sk_buff *skb, u32 info)
 
 	if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED) {
 		ipv4_update_pmtu(skb, dev_net(skb->dev), info,
-				 t->parms.link, iph->protocol);
+				 t->sit_parms->link, iph->protocol);
 		err = 0;
 		goto out;
 	}
 	if (type == ICMP_REDIRECT) {
-		ipv4_redirect(skb, dev_net(skb->dev), t->parms.link,
+		ipv4_redirect(skb, dev_net(skb->dev), t->sit_parms->link,
 			      iph->protocol);
 		err = 0;
 		goto out;
@@ -618,10 +622,10 @@ static int ipip6_err(struct sk_buff *skb, u32 info)
 	    !ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len))
 		goto out;
 
-	if (t->parms.iph.daddr == 0)
+	if (t->sit_parms->iph.daddr == 0)
 		goto out;
 
-	if (t->parms.iph.ttl == 0 && type == ICMP_TIME_EXCEEDED)
+	if (t->sit_parms->iph.ttl == 0 && type == ICMP_TIME_EXCEEDED)
 		goto out;
 
 	if (time_before(jiffies, READ_ONCE(t->err_time) + IPTUNNEL_ERR_TIMEO))
@@ -722,8 +726,8 @@ static int ipip6_rcv(struct sk_buff *skb)
 	tunnel = ipip6_tunnel_lookup(dev_net(skb->dev), skb->dev,
 				     iph->saddr, iph->daddr, sifindex);
 	if (tunnel) {
-		if (tunnel->parms.iph.protocol != IPPROTO_IPV6 &&
-		    tunnel->parms.iph.protocol != 0)
+		if (tunnel->sit_parms->iph.protocol != IPPROTO_IPV6 &&
+		    tunnel->sit_parms->iph.protocol != 0)
 			goto out;
 
 		skb->mac_header = skb->network_header;
@@ -798,8 +802,8 @@ static int sit_tunnel_rcv(struct sk_buff *skb, u8 ipproto)
 	if (tunnel) {
 		const struct tnl_ptk_info *tpi;
 
-		if (tunnel->parms.iph.protocol != ipproto &&
-		    tunnel->parms.iph.protocol != 0)
+		if (tunnel->sit_parms->iph.protocol != ipproto &&
+		    tunnel->sit_parms->iph.protocol != 0)
 			goto drop;
 
 		if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
@@ -938,9 +942,9 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
 				     struct net_device *dev)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
-	const struct iphdr  *tiph = &tunnel->parms.iph;
+	const struct iphdr  *tiph = &tunnel->sit_parms->iph;
 	const struct ipv6hdr *iph6 = ipv6_hdr(skb);
-	u8     tos = tunnel->parms.iph.tos;
+	u8     tos = tunnel->sit_parms->iph.tos;
 	__be16 df = tiph->frag_off;
 	struct rtable *rt;		/* Route to the other host */
 	struct net_device *tdev;	/* Device to other host */
@@ -966,7 +970,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, READ_ONCE(tunnel->fwmark),
+	flowi4_init_output(&fl4, tunnel->sit_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));
@@ -1014,7 +1018,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
 			df = 0;
 		}
 
-		if (tunnel->parms.iph.daddr)
+		if (tunnel->sit_parms->iph.daddr)
 			skb_dst_update_pmtu_no_confirm(skb, mtu);
 
 		if (skb->len > mtu && !skb_is_gso(skb)) {
@@ -1083,7 +1087,7 @@ 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->parms.iph;
+	const struct iphdr  *tiph = &tunnel->sit_parms->iph;
 
 	if (iptunnel_handle_offloads(skb, SKB_GSO_IPXIP4))
 		goto tx_error;
@@ -1138,7 +1142,7 @@ static void ipip6_tunnel_bind_dev(struct net_device *dev)
 	const struct iphdr *iph;
 	struct flowi4 fl4;
 
-	iph = &tunnel->parms.iph;
+	iph = &tunnel->sit_parms->iph;
 
 	if (iph->daddr) {
 		struct rtable *rt = ip_route_output_ports(tunnel->net, &fl4,
@@ -1147,7 +1151,7 @@ static void ipip6_tunnel_bind_dev(struct net_device *dev)
 							  0, 0,
 							  IPPROTO_IPV6,
 							  iph->tos & INET_DSCP_MASK,
-							  tunnel->parms.link);
+							  tunnel->sit_parms->link);
 
 		if (!IS_ERR(rt)) {
 			tdev = rt->dst.dev;
@@ -1156,8 +1160,8 @@ static void ipip6_tunnel_bind_dev(struct net_device *dev)
 		dev->flags |= IFF_POINTOPOINT;
 	}
 
-	if (!tdev && tunnel->parms.link)
-		tdev = __dev_get_by_index(tunnel->net, tunnel->parms.link);
+	if (!tdev && tunnel->sit_parms->link)
+		tdev = __dev_get_by_index(tunnel->net, tunnel->sit_parms->link);
 
 	if (tdev && !netif_is_l3_master(tdev)) {
 		int mtu;
@@ -1171,30 +1175,41 @@ static void ipip6_tunnel_bind_dev(struct net_device *dev)
 	dev->needed_headroom = ip_tunnel_limit_headroom(t_hlen + hlen);
 }
 
-static void ipip6_tunnel_update(struct ip_tunnel *t,
-				struct ip_tunnel_parm_kern *p,
-				__u32 fwmark)
+static int ipip6_tunnel_update(struct ip_tunnel *t,
+			       struct ip_tunnel_parm_kern *p,
+			       __u32 fwmark)
 {
 	struct net *net = t->net;
 	struct sit_net *sitn = net_generic(net, sit_net_id);
+	struct ip_tunnel_parm_kern *new_p, *old_p;
 
+	old_p = t->sit_parms;
+	new_p = kmalloc_obj(*new_p);
+	if (!new_p)
+		return -ENOMEM;
+	*new_p = *old_p;
+	new_p->iph.saddr = p->iph.saddr;
+	new_p->iph.daddr = p->iph.daddr;
+	new_p->iph.ttl = p->iph.ttl;
+	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->parms.iph.saddr = p->iph.saddr;
-	t->parms.iph.daddr = p->iph.daddr;
+	t->sit_parms = new_p;
+	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);
-	t->parms.iph.ttl = p->iph.ttl;
-	t->parms.iph.tos = p->iph.tos;
-	t->parms.iph.frag_off = p->iph.frag_off;
-	if (t->parms.link != p->link || t->fwmark != fwmark) {
-		t->parms.link = p->link;
+	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);
+	return 0;
 }
 
 #ifdef CONFIG_IPV6_SIT_6RD
@@ -1326,7 +1341,7 @@ ipip6_tunnel_get(struct net_device *dev, struct ip_tunnel_parm_kern *p)
 		t = ipip6_tunnel_locate(t->net, p, 0);
 	if (!t)
 		t = netdev_priv(dev);
-	memcpy(p, &t->parms, sizeof(*p));
+	memcpy(p, t->sit_parms, sizeof(*p));
 	return 0;
 }
 
@@ -1371,7 +1386,9 @@ ipip6_tunnel_change(struct net_device *dev, struct ip_tunnel_parm_kern *p)
 			t = netdev_priv(dev);
 		}
 
-		ipip6_tunnel_update(t, p, t->fwmark);
+		err = ipip6_tunnel_update(t, p, t->fwmark);
+		if (err)
+			return err;
 	}
 
 	return 0;
@@ -1448,7 +1465,7 @@ static int ipip6_get_iflink(const struct net_device *dev)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
 
-	return READ_ONCE(tunnel->parms.link);
+	return READ_ONCE(tunnel->sit_parms->link);
 }
 
 static const struct net_device_ops ipip6_netdev_ops = {
@@ -1470,6 +1487,8 @@ static void ipip6_dev_free(struct net_device *dev)
 	RCU_INIT_POINTER(tunnel->ip6rd, NULL);
 	kfree(ip6rd);
 #endif
+	kfree(tunnel->sit_parms);
+	tunnel->sit_parms = NULL;
 	if (tunnel->dst_cache.cache) {
 		dst_cache_destroy(&tunnel->dst_cache);
 		tunnel->dst_cache.cache = NULL;
@@ -1512,7 +1531,7 @@ static int ipip6_tunnel_init(struct net_device *dev)
 	int err;
 
 	tunnel->dev = dev;
-	strscpy(tunnel->parms.name, dev->name);
+	strscpy(tunnel->sit_parms->name, dev->name);
 
 	ipip6_tunnel_bind_dev(dev);
 
@@ -1528,15 +1547,9 @@ static int ipip6_tunnel_init(struct net_device *dev)
 static void __net_init ipip6_fb_tunnel_init(struct net_device *dev)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
-	struct iphdr *iph = &tunnel->parms.iph;
 	struct net *net = dev_net(dev);
 	struct sit_net *sitn = net_generic(net, sit_net_id);
 
-	iph->version		= 4;
-	iph->protocol		= IPPROTO_IPV6;
-	iph->ihl		= 5;
-	iph->ttl		= 64;
-
 	rcu_assign_pointer(sitn->tunnels_wc[0], tunnel);
 }
 
@@ -1623,6 +1636,7 @@ static int ipip6_newlink(struct net_device *dev,
 #ifdef CONFIG_IPV6_SIT_6RD
 	struct ip_tunnel_6rd ip6rd;
 #endif
+	struct ip_tunnel_parm_kern p;
 	struct net *net;
 	int err;
 
@@ -1636,11 +1650,16 @@ static int ipip6_newlink(struct net_device *dev,
 			return err;
 	}
 
-	ipip6_netlink_parms(data, &nt->parms, &nt->fwmark);
+	ipip6_netlink_parms(data, &p, &nt->fwmark);
 
-	if (ipip6_tunnel_locate(net, &nt->parms, 0))
+	if (ipip6_tunnel_locate(net, &p, 0))
 		return -EEXIST;
 
+	nt->sit_parms = kmalloc_obj(*nt->sit_parms);
+	if (!nt->sit_parms)
+		return -ENOMEM;
+	*nt->sit_parms = p;
+
 	err = ipip6_tunnel_create(dev);
 	if (err < 0) {
 		ipip6_dev_free(dev);
@@ -1707,7 +1726,9 @@ static int ipip6_changelink(struct net_device *dev, struct nlattr *tb[],
 	} else
 		t = netdev_priv(dev);
 
-	ipip6_tunnel_update(t, &p, fwmark);
+	err = ipip6_tunnel_update(t, &p, fwmark);
+	if (err)
+		return err;
 
 #ifdef CONFIG_IPV6_SIT_6RD
 	if (ipip6_netlink_6rd_parms(data, &ip6rd))
@@ -1762,7 +1783,7 @@ static size_t ipip6_get_size(const struct net_device *dev)
 static int ipip6_fill_info(struct sk_buff *skb, const struct net_device *dev)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
-	struct ip_tunnel_parm_kern *parm = &tunnel->parms;
+	struct ip_tunnel_parm_kern *parm = tunnel->sit_parms;
 #ifdef CONFIG_IPV6_SIT_6RD
 	const struct ip_tunnel_6rd_parm *ip6rd;
 #endif
@@ -1935,6 +1956,17 @@ static int __net_init sit_init_net(struct net *net)
 
 	t = netdev_priv(sitn->fb_tunnel_dev);
 	t->net = net;
+	t->sit_parms = kzalloc_obj(*t->sit_parms);
+	if (!t->sit_parms) {
+		err = -ENOMEM;
+		goto err_reg_dev;
+	}
+	t->sit_parms->iph.version	= 4;
+	t->sit_parms->iph.protocol	= IPPROTO_IPV6;
+	t->sit_parms->iph.ihl		= 5;
+	t->sit_parms->iph.ttl		= 64;
+	strscpy(t->sit_parms->name, sitn->fb_tunnel_dev->name);
+	t->parms = *t->sit_parms;
 
 	err = ipip6_tunnel_clone_6rd(sitn->fb_tunnel_dev, sitn);
 	if (err < 0)
@@ -1945,8 +1977,6 @@ static int __net_init sit_init_net(struct net *net)
 		goto err_reg_dev;
 
 	ipip6_fb_tunnel_init(sitn->fb_tunnel_dev);
-
-	strscpy(t->parms.name, sitn->fb_tunnel_dev->name);
 	return 0;
 
 err_reg_dev:
-- 
2.55.0.979.g7e5102b832-goog

Re: [PATCH net-next 7/9] sit: dynamically allocate struct ip_tunnel_parm_kern

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 15434 bytes --]

> In preparation for converting SIT configuration parameters to RCU,
> dynamically allocate struct ip_tunnel_parm_kern as sit_parms in
> struct ip_tunnel.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

> ---
>  include/net/ip_tunnels.h |   1 +
>  net/ipv6/sit.c           | 152 +++++++++++++++++++++++----------------
>  2 files changed, 92 insertions(+), 61 deletions(-)
> 
> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index 7fff59bab53b682ac0d1efd1f47082ba7d633fd2..f464c4480edaf35f9ace1c5081c564ce51fed636 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -190,6 +190,7 @@ struct ip_tunnel {
>  #endif
>  	struct ip_tunnel_prl_entry __rcu *prl;	/* potential router list */
>  	unsigned int		prl_count;	/* # of entries in PRL */
> +	struct ip_tunnel_parm_kern *sit_parms;
>  	unsigned int		ip_tnl_net_id;
>  	struct gro_cells	gro_cells;
>  	__u32			fwmark;
> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index 35c6695909014a2c335f1de7afb48f07b7c91c39..dc37c7109af5324f2ced0301b289e7622dd1a55f 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -108,24 +108,24 @@ static struct ip_tunnel *ipip6_tunnel_lookup(struct net *net,
>  	int ifindex = dev ? dev->ifindex : 0;
>  
>  	for_each_ip_tunnel_rcu(t, sitn->tunnels_r_l[h0 ^ h1]) {
> -		if (local == t->parms.iph.saddr &&
> -		    remote == t->parms.iph.daddr &&
> -		    (!dev || !t->parms.link || ifindex == t->parms.link ||
> -		     sifindex == t->parms.link) &&
> +		if (local == t->sit_parms->iph.saddr &&
> +		    remote == t->sit_parms->iph.daddr &&
> +		    (!dev || !t->sit_parms->link || ifindex == t->sit_parms->link ||
> +		     sifindex == t->sit_parms->link) &&
>  		    (t->dev->flags & IFF_UP))
>  			return t;
>  	}
>  	for_each_ip_tunnel_rcu(t, sitn->tunnels_r[h0]) {
> -		if (remote == t->parms.iph.daddr &&
> -		    (!dev || !t->parms.link || ifindex == t->parms.link ||
> -		     sifindex == t->parms.link) &&
> +		if (remote == t->sit_parms->iph.daddr &&
> +		    (!dev || !t->sit_parms->link || ifindex == t->sit_parms->link ||
> +		     sifindex == t->sit_parms->link) &&
>  		    (t->dev->flags & IFF_UP))
>  			return t;
>  	}
>  	for_each_ip_tunnel_rcu(t, sitn->tunnels_l[h1]) {
> -		if (local == t->parms.iph.saddr &&
> -		    (!dev || !t->parms.link || ifindex == t->parms.link ||
> -		     sifindex == t->parms.link) &&
> +		if (local == t->sit_parms->iph.saddr &&
> +		    (!dev || !t->sit_parms->link || ifindex == t->sit_parms->link ||
> +		     sifindex == t->sit_parms->link) &&
>  		    (t->dev->flags & IFF_UP))
>  			return t;
>  	}
> @@ -157,7 +157,7 @@ __ipip6_bucket(struct sit_net *sitn, struct ip_tunnel_parm_kern *parms)
>  static inline struct ip_tunnel __rcu **ipip6_bucket(struct sit_net *sitn,
>  		struct ip_tunnel *t)
>  {
> -	return __ipip6_bucket(sitn, &t->parms);
> +	return __ipip6_bucket(sitn, t->sit_parms);
>  }
>  
>  static void ipip6_tunnel_unlink(struct sit_net *sitn, struct ip_tunnel *t)
> @@ -236,10 +236,11 @@ static int ipip6_tunnel_create(struct net_device *dev)
>  	if (err < 0)
>  		goto out;
>  
> -	__dev_addr_set(dev, &t->parms.iph.saddr, 4);
> -	memcpy(dev->broadcast, &t->parms.iph.daddr, 4);
> +	t->parms = *t->sit_parms;
> +	__dev_addr_set(dev, &t->sit_parms->iph.saddr, 4);
> +	memcpy(dev->broadcast, &t->sit_parms->iph.daddr, 4);
>  
> -	if (test_bit(IP_TUNNEL_SIT_ISATAP_BIT, t->parms.i_flags))
> +	if (test_bit(IP_TUNNEL_SIT_ISATAP_BIT, t->sit_parms->i_flags))
>  		dev->priv_flags |= IFF_ISATAP;
>  
>  	dev->rtnl_link_ops = &sit_link_ops;
> @@ -270,9 +271,9 @@ static struct ip_tunnel *ipip6_tunnel_locate(struct net *net,
>  	for (tp = __ipip6_bucket(sitn, parms);
>  	    (t = rtnl_dereference(*tp)) != NULL;
>  	     tp = &t->next) {
> -		if (local == t->parms.iph.saddr &&
> -		    remote == t->parms.iph.daddr &&
> -		    parms->link == t->parms.link) {
> +		if (local == t->sit_parms->iph.saddr &&
> +		    remote == t->sit_parms->iph.daddr &&
> +		    parms->link == t->sit_parms->link) {
>  			if (create)
>  				return NULL;
>  			else
> @@ -299,7 +300,10 @@ static struct ip_tunnel *ipip6_tunnel_locate(struct net *net,
>  	nt = netdev_priv(dev);
>  
>  	nt->net = net;
> -	nt->parms = *parms;
> +	nt->sit_parms = kmalloc_obj(*nt->sit_parms);
> +	if (!nt->sit_parms)
> +		goto failed_free;
> +	*nt->sit_parms = *parms;
>  	if (ipip6_tunnel_create(dev) < 0)
>  		goto failed_free;
>  
> @@ -602,12 +606,12 @@ static int ipip6_err(struct sk_buff *skb, u32 info)
>  
>  	if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED) {
>  		ipv4_update_pmtu(skb, dev_net(skb->dev), info,
> -				 t->parms.link, iph->protocol);
> +				 t->sit_parms->link, iph->protocol);
>  		err = 0;
>  		goto out;
>  	}
>  	if (type == ICMP_REDIRECT) {
> -		ipv4_redirect(skb, dev_net(skb->dev), t->parms.link,
> +		ipv4_redirect(skb, dev_net(skb->dev), t->sit_parms->link,
>  			      iph->protocol);
>  		err = 0;
>  		goto out;
> @@ -618,10 +622,10 @@ static int ipip6_err(struct sk_buff *skb, u32 info)
>  	    !ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len))
>  		goto out;
>  
> -	if (t->parms.iph.daddr == 0)
> +	if (t->sit_parms->iph.daddr == 0)
>  		goto out;
>  
> -	if (t->parms.iph.ttl == 0 && type == ICMP_TIME_EXCEEDED)
> +	if (t->sit_parms->iph.ttl == 0 && type == ICMP_TIME_EXCEEDED)
>  		goto out;
>  
>  	if (time_before(jiffies, READ_ONCE(t->err_time) + IPTUNNEL_ERR_TIMEO))
> @@ -722,8 +726,8 @@ static int ipip6_rcv(struct sk_buff *skb)
>  	tunnel = ipip6_tunnel_lookup(dev_net(skb->dev), skb->dev,
>  				     iph->saddr, iph->daddr, sifindex);
>  	if (tunnel) {
> -		if (tunnel->parms.iph.protocol != IPPROTO_IPV6 &&
> -		    tunnel->parms.iph.protocol != 0)
> +		if (tunnel->sit_parms->iph.protocol != IPPROTO_IPV6 &&
> +		    tunnel->sit_parms->iph.protocol != 0)
>  			goto out;
>  
>  		skb->mac_header = skb->network_header;
> @@ -798,8 +802,8 @@ static int sit_tunnel_rcv(struct sk_buff *skb, u8 ipproto)
>  	if (tunnel) {
>  		const struct tnl_ptk_info *tpi;
>  
> -		if (tunnel->parms.iph.protocol != ipproto &&
> -		    tunnel->parms.iph.protocol != 0)
> +		if (tunnel->sit_parms->iph.protocol != ipproto &&
> +		    tunnel->sit_parms->iph.protocol != 0)
>  			goto drop;
>  
>  		if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
> @@ -938,9 +942,9 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
>  				     struct net_device *dev)
>  {
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
> -	const struct iphdr  *tiph = &tunnel->parms.iph;
> +	const struct iphdr  *tiph = &tunnel->sit_parms->iph;
>  	const struct ipv6hdr *iph6 = ipv6_hdr(skb);
> -	u8     tos = tunnel->parms.iph.tos;
> +	u8     tos = tunnel->sit_parms->iph.tos;
>  	__be16 df = tiph->frag_off;
>  	struct rtable *rt;		/* Route to the other host */
>  	struct net_device *tdev;	/* Device to other host */
> @@ -966,7 +970,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, READ_ONCE(tunnel->fwmark),
> +	flowi4_init_output(&fl4, tunnel->sit_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));
> @@ -1014,7 +1018,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
>  			df = 0;
>  		}
>  
> -		if (tunnel->parms.iph.daddr)
> +		if (tunnel->sit_parms->iph.daddr)
>  			skb_dst_update_pmtu_no_confirm(skb, mtu);
>  
>  		if (skb->len > mtu && !skb_is_gso(skb)) {
> @@ -1083,7 +1087,7 @@ 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->parms.iph;
> +	const struct iphdr  *tiph = &tunnel->sit_parms->iph;
>  
>  	if (iptunnel_handle_offloads(skb, SKB_GSO_IPXIP4))
>  		goto tx_error;
> @@ -1138,7 +1142,7 @@ static void ipip6_tunnel_bind_dev(struct net_device *dev)
>  	const struct iphdr *iph;
>  	struct flowi4 fl4;
>  
> -	iph = &tunnel->parms.iph;
> +	iph = &tunnel->sit_parms->iph;
>  
>  	if (iph->daddr) {
>  		struct rtable *rt = ip_route_output_ports(tunnel->net, &fl4,
> @@ -1147,7 +1151,7 @@ static void ipip6_tunnel_bind_dev(struct net_device *dev)
>  							  0, 0,
>  							  IPPROTO_IPV6,
>  							  iph->tos & INET_DSCP_MASK,
> -							  tunnel->parms.link);
> +							  tunnel->sit_parms->link);
>  
>  		if (!IS_ERR(rt)) {
>  			tdev = rt->dst.dev;
> @@ -1156,8 +1160,8 @@ static void ipip6_tunnel_bind_dev(struct net_device *dev)
>  		dev->flags |= IFF_POINTOPOINT;
>  	}
>  
> -	if (!tdev && tunnel->parms.link)
> -		tdev = __dev_get_by_index(tunnel->net, tunnel->parms.link);
> +	if (!tdev && tunnel->sit_parms->link)
> +		tdev = __dev_get_by_index(tunnel->net, tunnel->sit_parms->link);
>  
>  	if (tdev && !netif_is_l3_master(tdev)) {
>  		int mtu;
> @@ -1171,30 +1175,41 @@ static void ipip6_tunnel_bind_dev(struct net_device *dev)
>  	dev->needed_headroom = ip_tunnel_limit_headroom(t_hlen + hlen);
>  }
>  
> -static void ipip6_tunnel_update(struct ip_tunnel *t,
> -				struct ip_tunnel_parm_kern *p,
> -				__u32 fwmark)
> +static int ipip6_tunnel_update(struct ip_tunnel *t,
> +			       struct ip_tunnel_parm_kern *p,
> +			       __u32 fwmark)
>  {
>  	struct net *net = t->net;
>  	struct sit_net *sitn = net_generic(net, sit_net_id);
> +	struct ip_tunnel_parm_kern *new_p, *old_p;
>  
> +	old_p = t->sit_parms;
> +	new_p = kmalloc_obj(*new_p);
> +	if (!new_p)
> +		return -ENOMEM;
> +	*new_p = *old_p;
> +	new_p->iph.saddr = p->iph.saddr;
> +	new_p->iph.daddr = p->iph.daddr;
> +	new_p->iph.ttl = p->iph.ttl;
> +	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->parms.iph.saddr = p->iph.saddr;
> -	t->parms.iph.daddr = p->iph.daddr;
> +	t->sit_parms = new_p;
> +	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);
> -	t->parms.iph.ttl = p->iph.ttl;
> -	t->parms.iph.tos = p->iph.tos;
> -	t->parms.iph.frag_off = p->iph.frag_off;
> -	if (t->parms.link != p->link || t->fwmark != fwmark) {
> -		t->parms.link = p->link;
> +	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);
> +	return 0;
>  }
>  
>  #ifdef CONFIG_IPV6_SIT_6RD
> @@ -1326,7 +1341,7 @@ ipip6_tunnel_get(struct net_device *dev, struct ip_tunnel_parm_kern *p)
>  		t = ipip6_tunnel_locate(t->net, p, 0);
>  	if (!t)
>  		t = netdev_priv(dev);
> -	memcpy(p, &t->parms, sizeof(*p));
> +	memcpy(p, t->sit_parms, sizeof(*p));
>  	return 0;
>  }
>  
> @@ -1371,7 +1386,9 @@ ipip6_tunnel_change(struct net_device *dev, struct ip_tunnel_parm_kern *p)
>  			t = netdev_priv(dev);
>  		}
>  
> -		ipip6_tunnel_update(t, p, t->fwmark);
> +		err = ipip6_tunnel_update(t, p, t->fwmark);
> +		if (err)
> +			return err;
>  	}
>  
>  	return 0;
> @@ -1448,7 +1465,7 @@ static int ipip6_get_iflink(const struct net_device *dev)
>  {
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
>  
> -	return READ_ONCE(tunnel->parms.link);
> +	return READ_ONCE(tunnel->sit_parms->link);
>  }
>  
>  static const struct net_device_ops ipip6_netdev_ops = {
> @@ -1470,6 +1487,8 @@ static void ipip6_dev_free(struct net_device *dev)
>  	RCU_INIT_POINTER(tunnel->ip6rd, NULL);
>  	kfree(ip6rd);
>  #endif
> +	kfree(tunnel->sit_parms);
> +	tunnel->sit_parms = NULL;
>  	if (tunnel->dst_cache.cache) {
>  		dst_cache_destroy(&tunnel->dst_cache);
>  		tunnel->dst_cache.cache = NULL;
> @@ -1512,7 +1531,7 @@ static int ipip6_tunnel_init(struct net_device *dev)
>  	int err;
>  
>  	tunnel->dev = dev;
> -	strscpy(tunnel->parms.name, dev->name);
> +	strscpy(tunnel->sit_parms->name, dev->name);
>  
>  	ipip6_tunnel_bind_dev(dev);
>  
> @@ -1528,15 +1547,9 @@ static int ipip6_tunnel_init(struct net_device *dev)
>  static void __net_init ipip6_fb_tunnel_init(struct net_device *dev)
>  {
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
> -	struct iphdr *iph = &tunnel->parms.iph;
>  	struct net *net = dev_net(dev);
>  	struct sit_net *sitn = net_generic(net, sit_net_id);
>  
> -	iph->version		= 4;
> -	iph->protocol		= IPPROTO_IPV6;
> -	iph->ihl		= 5;
> -	iph->ttl		= 64;
> -
>  	rcu_assign_pointer(sitn->tunnels_wc[0], tunnel);
>  }
>  
> @@ -1623,6 +1636,7 @@ static int ipip6_newlink(struct net_device *dev,
>  #ifdef CONFIG_IPV6_SIT_6RD
>  	struct ip_tunnel_6rd ip6rd;
>  #endif
> +	struct ip_tunnel_parm_kern p;
>  	struct net *net;
>  	int err;
>  
> @@ -1636,11 +1650,16 @@ static int ipip6_newlink(struct net_device *dev,
>  			return err;
>  	}
>  
> -	ipip6_netlink_parms(data, &nt->parms, &nt->fwmark);
> +	ipip6_netlink_parms(data, &p, &nt->fwmark);
>  
> -	if (ipip6_tunnel_locate(net, &nt->parms, 0))
> +	if (ipip6_tunnel_locate(net, &p, 0))
>  		return -EEXIST;
>  
> +	nt->sit_parms = kmalloc_obj(*nt->sit_parms);
> +	if (!nt->sit_parms)
> +		return -ENOMEM;
> +	*nt->sit_parms = p;
> +
>  	err = ipip6_tunnel_create(dev);
>  	if (err < 0) {
>  		ipip6_dev_free(dev);
> @@ -1707,7 +1726,9 @@ static int ipip6_changelink(struct net_device *dev, struct nlattr *tb[],
>  	} else
>  		t = netdev_priv(dev);
>  
> -	ipip6_tunnel_update(t, &p, fwmark);
> +	err = ipip6_tunnel_update(t, &p, fwmark);
> +	if (err)
> +		return err;
>  
>  #ifdef CONFIG_IPV6_SIT_6RD
>  	if (ipip6_netlink_6rd_parms(data, &ip6rd))
> @@ -1762,7 +1783,7 @@ static size_t ipip6_get_size(const struct net_device *dev)
>  static int ipip6_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  {
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
> -	struct ip_tunnel_parm_kern *parm = &tunnel->parms;
> +	struct ip_tunnel_parm_kern *parm = tunnel->sit_parms;
>  #ifdef CONFIG_IPV6_SIT_6RD
>  	const struct ip_tunnel_6rd_parm *ip6rd;
>  #endif
> @@ -1935,6 +1956,17 @@ static int __net_init sit_init_net(struct net *net)
>  
>  	t = netdev_priv(sitn->fb_tunnel_dev);
>  	t->net = net;
> +	t->sit_parms = kzalloc_obj(*t->sit_parms);
> +	if (!t->sit_parms) {
> +		err = -ENOMEM;
> +		goto err_reg_dev;
> +	}
> +	t->sit_parms->iph.version	= 4;
> +	t->sit_parms->iph.protocol	= IPPROTO_IPV6;
> +	t->sit_parms->iph.ihl		= 5;
> +	t->sit_parms->iph.ttl		= 64;
> +	strscpy(t->sit_parms->name, sitn->fb_tunnel_dev->name);
> +	t->parms = *t->sit_parms;
>  
>  	err = ipip6_tunnel_clone_6rd(sitn->fb_tunnel_dev, sitn);
>  	if (err < 0)
> @@ -1945,8 +1977,6 @@ static int __net_init sit_init_net(struct net *net)
>  		goto err_reg_dev;
>  
>  	ipip6_fb_tunnel_init(sitn->fb_tunnel_dev);
> -
> -	strscpy(t->parms.name, sitn->fb_tunnel_dev->name);
>  	return 0;
>  
>  err_reg_dev:
> -- 
> 2.55.0.979.g7e5102b832-goog
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next 7/9] sit: dynamically allocate struct ip_tunnel_parm_kern

[Artem Lytkin]

On Mon, Sep 07, 2026 at 07:58:44AM +0000, Eric Dumazet wrote:
>  	ipip6_tunnel_unlink(sitn, t);
>  	synchronize_net();
> -	t->parms.iph.saddr = p->iph.saddr;
> -	t->parms.iph.daddr = p->iph.daddr;
> +	t->sit_parms = new_p;
[...]
> +	kfree(old_p);

At this point in the series old_p is freed with no grace period: the
synchronize_net() runs before the pointer switch and only covers the
hash readers. ipip6_tunnel_xmit() doesn't go through the hash and
keeps tiph = &tunnel->sit_parms->iph across the route lookup, and
ipip6_get_iflink() is reached from iflink_show() with no lock at all.
8/9 turns this into kfree_rcu(), so the end result is fine, but this
commit on its own is a UAF and breaks bisection. Moving the rcu_head
and kfree_rcu() here (or squashing 7 and 8) would keep each step safe.

Smaller thing of the same kind in 5/9: check_6rd() gets
rcu_dereference() but the rcu_read_lock() around the xmit path only
arrives in 8/9.

Artem

Re: [PATCH net-next 7/9] sit: dynamically allocate struct ip_tunnel_parm_kern

[Eric Dumazet]

On Mon, Sep 7, 2026 at 3:35 PM Artem Lytkin <iprintercanon@gmail.com> wrote:
>
> On Mon, Sep 07, 2026 at 07:58:44AM +0000, Eric Dumazet wrote:
> >       ipip6_tunnel_unlink(sitn, t);
> >       synchronize_net();
> > -     t->parms.iph.saddr = p->iph.saddr;
> > -     t->parms.iph.daddr = p->iph.daddr;
> > +     t->sit_parms = new_p;
> [...]
> > +     kfree(old_p);
>
> At this point in the series old_p is freed with no grace period: the
> synchronize_net() runs before the pointer switch and only covers the
> hash readers. ipip6_tunnel_xmit() doesn't go through the hash and
> keeps tiph = &tunnel->sit_parms->iph across the route lookup, and
> ipip6_get_iflink() is reached from iflink_show() with no lock at all.
> 8/9 turns this into kfree_rcu(), so the end result is fine, but this
> commit on its own is a UAF and breaks bisection. Moving the rcu_head
> and kfree_rcu() here (or squashing 7 and 8) would keep each step safe.
>
> Smaller thing of the same kind in 5/9: check_6rd() gets
> rcu_dereference() but the rcu_read_lock() around the xmit path only
> arrives in 8/9.


Yeah, I had to split my original patch into three parts because our
sashiko 5,000,000 token limit was hit.

Re: [PATCH net-next 7/9] sit: dynamically allocate struct ip_tunnel_parm_kern

[netdev-bot+sashiko]

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

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

- [Medium] Intermediate-state use-after-free in net/ipv6/sit.c:
  ipip6_tunnel_update() publishes the freshly allocated parameter block…
- [Low] Missing publication barrier / unannotated lockless pointer: in
  this commit `sit_parms` is a plain (non-__rcu) pointer in struct…

--

sit: dynamically allocate struct ip_tunnel_parm_kern

This moves the SIT configuration parameters out of the embedded
ip_tunnel.parms into a separately allocated struct ip_tunnel_parm_kern
reachable through the new ip_tunnel.sit_parms pointer, in preparation for
converting them to RCU.  All readers and writers in net/ipv6/sit.c are
switched over to the new pointer.

A couple of questions about the intermediate state this commit leaves the
tree in.

> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index 7fff59bab53b6..f464c4480edaf 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -190,6 +190,7 @@ struct ip_tunnel {
>  #endif
>  	struct ip_tunnel_prl_entry __rcu *prl;	/* potential router list */
>  	unsigned int		prl_count;	/* # of entries in PRL */
> +	struct ip_tunnel_parm_kern *sit_parms;
      ^^^^

[Severity: Low]
Should this member already carry the __rcu annotation in this commit?  It
is read from softirq packet paths (ipip6_tunnel_lookup(), ipip6_rcv(),
sit_tunnel_rcv(), ipip6_tunnel_xmit(), sit_tunnel_xmit__()) without RTNL,
while ipip6_tunnel_update() replaces it with a plain store.  As a plain
pointer, sparse cannot flag the unprotected dereferences, the publish has
no release semantics, and the loads are plain data races for KCSAN.

The following patch in the series, "sit: convert configuration to RCU
protection", changes this to struct ip_tunnel_parm_kern __rcu *sit_parms
and converts publication to rcu_assign_pointer(), so the window is limited
to this single commit.  Would it be worth folding the annotation in here?

>  	unsigned int		ip_tnl_net_id;
>  	struct gro_cells	gro_cells;
>  	__u32			fwmark;
> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index 35c6695909014..dc37c7109af53 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -108,24 +108,24 @@ static struct ip_tunnel *ipip6_tunnel_lookup(struct net *net,
>  	int ifindex = dev ? dev->ifindex : 0;
>  
>  	for_each_ip_tunnel_rcu(t, sitn->tunnels_r_l[h0 ^ h1]) {
> -		if (local == t->parms.iph.saddr &&
> -		    remote == t->parms.iph.daddr &&
> -		    (!dev || !t->parms.link || ifindex == t->parms.link ||
> -		     sifindex == t->parms.link) &&
> +		if (local == t->sit_parms->iph.saddr &&
> +		    remote == t->sit_parms->iph.daddr &&
> +		    (!dev || !t->sit_parms->link || ifindex == t->sit_parms->link ||
> +		     sifindex == t->sit_parms->link) &&
        ^^^^

[Severity: Low]
Can this mix fields from two different parameter blocks?  t->sit_parms is
reloaded up to four times inside one match decision here, and
ipip6_tunnel_update() swaps the pointer concurrently, so saddr/daddr could
come from the old block and link from the new one.

The next patch caches a single rcu_dereference(t->sit_parms) in a local
parms variable per candidate, which removes this; could that be done here
instead?

>  		    (t->dev->flags & IFF_UP))
>  			return t;
>  	}

[ ... ]

> @@ -938,9 +942,9 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
>  				     struct net_device *dev)
>  {
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
> -	const struct iphdr  *tiph = &tunnel->parms.iph;
> +	const struct iphdr  *tiph = &tunnel->sit_parms->iph;
>  	const struct ipv6hdr *iph6 = ipv6_hdr(skb);
> -	u8     tos = tunnel->parms.iph.tos;
> +	u8     tos = tunnel->sit_parms->iph.tos;
        ^^^^

[Severity: Medium]
Can this read freed memory?  ipip6_tunnel_xmit() reaches the tunnel via
netdev_priv(dev), never through the sitn->tunnels[] buckets, and it keeps
the interior pointer tiph across the route lookup, the PMTU handling, the
headroom reallocation and iptunnel_xmit().

Meanwhile ipip6_tunnel_update() frees the block the pointer points into:

net/ipv6/sit.c:ipip6_tunnel_update() {
	...
	ipip6_tunnel_unlink(sitn, t);
	synchronize_net();
	t->sit_parms = new_p;
	...
	kfree(old_p);
	return 0;
}

The synchronize_net() runs before the new pointer is published and only
drains readers that came in through the hash buckets, so it does not cover
this path.  dev->lltx is set for sit, so nothing else serializes a
SIOCCHGTUNNEL or RTM_NEWLINK change against traffic on the same device.
The values read out of the freed block (tiph->saddr, tos, ttl, frag_off)
end up in the outer IPv4 header.

The immediately following patch, "sit: convert configuration to RCU
protection", wraps the transmit path in rcu_read_lock() and uses
kfree_rcu(old_p, rcu), so this hazard exists only between the two commits
(a concern for bisection and partial backports).

>  	__be16 df = tiph->frag_off;
>  	struct rtable *rt;		/* Route to the other host */
>  	struct net_device *tdev;	/* Device to other host */

[ ... ]

> @@ -1083,7 +1087,7 @@ 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->parms.iph;
> +	const struct iphdr  *tiph = &tunnel->sit_parms->iph;
        ^^^^

[Severity: Medium]
Same question here: this hands an interior pointer of the freeable
parameter block to ip_tunnel_xmit() with no reference and no RCU section
held, so a concurrent ipip6_tunnel_update() can kfree() it underneath.

>  
>  	if (iptunnel_handle_offloads(skb, SKB_GSO_IPXIP4))
>  		goto tx_error;

[ ... ]

> @@ -1171,30 +1175,41 @@ static void ipip6_tunnel_bind_dev(struct net_device *dev)
>  	dev->needed_headroom = ip_tunnel_limit_headroom(t_hlen + hlen);
>  }
>  
> -static void ipip6_tunnel_update(struct ip_tunnel *t,
> -				struct ip_tunnel_parm_kern *p,
> -				__u32 fwmark)
> +static int ipip6_tunnel_update(struct ip_tunnel *t,
> +			       struct ip_tunnel_parm_kern *p,
> +			       __u32 fwmark)
>  {
>  	struct net *net = t->net;
>  	struct sit_net *sitn = net_generic(net, sit_net_id);
> +	struct ip_tunnel_parm_kern *new_p, *old_p;
>  
> +	old_p = t->sit_parms;
> +	new_p = kmalloc_obj(*new_p);
> +	if (!new_p)
> +		return -ENOMEM;
> +	*new_p = *old_p;
> +	new_p->iph.saddr = p->iph.saddr;
> +	new_p->iph.daddr = p->iph.daddr;
> +	new_p->iph.ttl = p->iph.ttl;
> +	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->parms.iph.saddr = p->iph.saddr;
> -	t->parms.iph.daddr = p->iph.daddr;
> +	t->sit_parms = new_p;
        ^^^^

[Severity: Medium]
Is the ordering here intentional?  The grace period happens before the new
pointer is published, so nothing separates the store of new_p from the
kfree(old_p) below for readers that already loaded the old pointer.

> +	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);
> -	t->parms.iph.ttl = p->iph.ttl;
> -	t->parms.iph.tos = p->iph.tos;
> -	t->parms.iph.frag_off = p->iph.frag_off;
> -	if (t->parms.link != p->link || t->fwmark != fwmark) {
> -		t->parms.link = p->link;
> +	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);
        ^^^^

[Severity: Medium]
Would kfree_rcu() be needed here, given that ipip6_tunnel_xmit(),
sit_tunnel_xmit__() and ipip6_get_iflink() all reach sit_parms through
netdev_priv(dev) rather than through the hash buckets?

The next patch in the series adds a struct rcu_head to
ip_tunnel_parm_kern and turns this into kfree_rcu(old_p, rcu).

> +	return 0;
>  }

[ ... ]

> @@ -1448,7 +1465,7 @@ static int ipip6_get_iflink(const struct net_device *dev)
>  {
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
>  
> -	return READ_ONCE(tunnel->parms.link);
> +	return READ_ONCE(tunnel->sit_parms->link);
        ^^^^

[Severity: Medium]
Here the READ_ONCE() now covers only the link field, not the pointer load
itself.  Since this accessor can run without RTNL, can it dereference the
block that ipip6_tunnel_update() is about to kfree()?  The following patch
adds an rcu_read_lock() section and an rcu_dereference() here.

>  }
>  
>  static const struct net_device_ops ipip6_netdev_ops = {

[ ... ]

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

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

[Eric Dumazet]

Now that SIT parameters are dynamically allocated, convert
tunnel->sit_parms to an RCU-protected pointer.

Updates in ipip6_tunnel_update() allocate a new parameter block,
publish it using rcu_assign_pointer(), and free the old one
via kfree_rcu().

We only need to unlink and re-link the tunnel in the hash table
if either saddr or daddr changed. When neither address changes,
the unhash/re-hash and synchronize_net() can be completely skipped.

Readers in ipip6_tunnel_lookup(), ipip6_tunnel_xmit(), ipip6_err(),
and ipip6_rcv() now safely dereference tunnel->sit_parms under RCU.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/ip_tunnels.h |   3 +-
 net/ipv6/sit.c           | 245 +++++++++++++++++++++++++--------------
 2 files changed, 157 insertions(+), 91 deletions(-)

diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index f464c4480edaf35f9ace1c5081c564ce51fed636..be4cc10f88ed64114cebac7f2e01bea21f5419da 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -149,6 +149,7 @@ struct ip_tunnel_parm_kern {
 	__be32			o_key;
 	int			link;
 	struct iphdr		iph;
+	struct rcu_head		rcu;
 };
 
 struct ip_tunnel {
@@ -190,7 +191,7 @@ struct ip_tunnel {
 #endif
 	struct ip_tunnel_prl_entry __rcu *prl;	/* potential router list */
 	unsigned int		prl_count;	/* # of entries in PRL */
-	struct ip_tunnel_parm_kern *sit_parms;
+	struct ip_tunnel_parm_kern __rcu *sit_parms;
 	unsigned int		ip_tnl_net_id;
 	struct gro_cells	gro_cells;
 	__u32			fwmark;
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index dc37c7109af5324f2ced0301b289e7622dd1a55f..c9049ab87e010eed5f53a342460ed10006083cd7 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -108,24 +108,33 @@ static struct ip_tunnel *ipip6_tunnel_lookup(struct net *net,
 	int ifindex = dev ? dev->ifindex : 0;
 
 	for_each_ip_tunnel_rcu(t, sitn->tunnels_r_l[h0 ^ h1]) {
-		if (local == t->sit_parms->iph.saddr &&
-		    remote == t->sit_parms->iph.daddr &&
-		    (!dev || !t->sit_parms->link || ifindex == t->sit_parms->link ||
-		     sifindex == t->sit_parms->link) &&
+		const struct ip_tunnel_parm_kern *parms;
+
+		parms = rcu_dereference(t->sit_parms);
+		if (local == parms->iph.saddr &&
+		    remote == parms->iph.daddr &&
+		    (!dev || !parms->link || ifindex == parms->link ||
+		     sifindex == parms->link) &&
 		    (t->dev->flags & IFF_UP))
 			return t;
 	}
 	for_each_ip_tunnel_rcu(t, sitn->tunnels_r[h0]) {
-		if (remote == t->sit_parms->iph.daddr &&
-		    (!dev || !t->sit_parms->link || ifindex == t->sit_parms->link ||
-		     sifindex == t->sit_parms->link) &&
+		const struct ip_tunnel_parm_kern *parms;
+
+		parms = rcu_dereference(t->sit_parms);
+		if (remote == parms->iph.daddr &&
+		    (!dev || !parms->link || ifindex == parms->link ||
+		     sifindex == parms->link) &&
 		    (t->dev->flags & IFF_UP))
 			return t;
 	}
 	for_each_ip_tunnel_rcu(t, sitn->tunnels_l[h1]) {
-		if (local == t->sit_parms->iph.saddr &&
-		    (!dev || !t->sit_parms->link || ifindex == t->sit_parms->link ||
-		     sifindex == t->sit_parms->link) &&
+		const struct ip_tunnel_parm_kern *parms;
+
+		parms = rcu_dereference(t->sit_parms);
+		if (local == parms->iph.saddr &&
+		    (!dev || !parms->link || ifindex == parms->link ||
+		     sifindex == parms->link) &&
 		    (t->dev->flags & IFF_UP))
 			return t;
 	}
@@ -157,7 +166,7 @@ __ipip6_bucket(struct sit_net *sitn, struct ip_tunnel_parm_kern *parms)
 static inline struct ip_tunnel __rcu **ipip6_bucket(struct sit_net *sitn,
 		struct ip_tunnel *t)
 {
-	return __ipip6_bucket(sitn, t->sit_parms);
+	return __ipip6_bucket(sitn, rtnl_dereference(t->sit_parms));
 }
 
 static void ipip6_tunnel_unlink(struct sit_net *sitn, struct ip_tunnel *t)
@@ -230,17 +239,19 @@ static int ipip6_tunnel_create(struct net_device *dev)
 {
 	struct ip_tunnel *t = netdev_priv(dev);
 	struct sit_net *sitn = net_generic(t->net, sit_net_id);
+	struct ip_tunnel_parm_kern *parms;
 	int err;
 
 	err = ipip6_tunnel_clone_6rd(dev, sitn);
 	if (err < 0)
 		goto out;
 
-	t->parms = *t->sit_parms;
-	__dev_addr_set(dev, &t->sit_parms->iph.saddr, 4);
-	memcpy(dev->broadcast, &t->sit_parms->iph.daddr, 4);
+	parms = rtnl_dereference(t->sit_parms);
+	t->parms = *parms;
+	__dev_addr_set(dev, &parms->iph.saddr, 4);
+	memcpy(dev->broadcast, &parms->iph.daddr, 4);
 
-	if (test_bit(IP_TUNNEL_SIT_ISATAP_BIT, t->sit_parms->i_flags))
+	if (test_bit(IP_TUNNEL_SIT_ISATAP_BIT, parms->i_flags))
 		dev->priv_flags |= IFF_ISATAP;
 
 	dev->rtnl_link_ops = &sit_link_ops;
@@ -264,6 +275,7 @@ static struct ip_tunnel *ipip6_tunnel_locate(struct net *net,
 	__be32 local = parms->iph.saddr;
 	struct ip_tunnel *t, *nt;
 	struct ip_tunnel __rcu **tp;
+	struct ip_tunnel_parm_kern *nt_parms;
 	struct net_device *dev;
 	char name[IFNAMSIZ];
 	struct sit_net *sitn = net_generic(net, sit_net_id);
@@ -271,9 +283,12 @@ static struct ip_tunnel *ipip6_tunnel_locate(struct net *net,
 	for (tp = __ipip6_bucket(sitn, parms);
 	    (t = rtnl_dereference(*tp)) != NULL;
 	     tp = &t->next) {
-		if (local == t->sit_parms->iph.saddr &&
-		    remote == t->sit_parms->iph.daddr &&
-		    parms->link == t->sit_parms->link) {
+		const struct ip_tunnel_parm_kern *tparms;
+
+		tparms = rtnl_dereference(t->sit_parms);
+		if (local == tparms->iph.saddr &&
+		    remote == tparms->iph.daddr &&
+		    parms->link == tparms->link) {
 			if (create)
 				return NULL;
 			else
@@ -300,10 +315,11 @@ static struct ip_tunnel *ipip6_tunnel_locate(struct net *net,
 	nt = netdev_priv(dev);
 
 	nt->net = net;
-	nt->sit_parms = kmalloc_obj(*nt->sit_parms);
-	if (!nt->sit_parms)
+	nt_parms = kmalloc_obj(*nt_parms);
+	if (!nt_parms)
 		goto failed_free;
-	*nt->sit_parms = *parms;
+	*nt_parms = *parms;
+	rcu_assign_pointer(nt->sit_parms, nt_parms);
 	if (ipip6_tunnel_create(dev) < 0)
 		goto failed_free;
 
@@ -599,41 +615,45 @@ static int ipip6_err(struct sk_buff *skb, u32 info)
 	err = -ENOENT;
 
 	sifindex = netif_is_l3_master(skb->dev) ? IPCB(skb)->iif : 0;
+	rcu_read_lock();
 	t = ipip6_tunnel_lookup(dev_net(skb->dev), skb->dev,
 				iph->daddr, iph->saddr, sifindex);
-	if (!t)
-		goto out;
+	if (t) {
+		const struct ip_tunnel_parm_kern *parms;
 
-	if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED) {
-		ipv4_update_pmtu(skb, dev_net(skb->dev), info,
-				 t->sit_parms->link, iph->protocol);
-		err = 0;
-		goto out;
-	}
-	if (type == ICMP_REDIRECT) {
-		ipv4_redirect(skb, dev_net(skb->dev), t->sit_parms->link,
-			      iph->protocol);
-		err = 0;
-		goto out;
-	}
+		parms = rcu_dereference(t->sit_parms);
+		if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED) {
+			ipv4_update_pmtu(skb, dev_net(skb->dev), info,
+					 parms->link, iph->protocol);
+			err = 0;
+			goto out;
+		}
+		if (type == ICMP_REDIRECT) {
+			ipv4_redirect(skb, dev_net(skb->dev), parms->link,
+				      iph->protocol);
+			err = 0;
+			goto out;
+		}
 
-	err = 0;
-	if (__in6_dev_get(skb->dev) &&
-	    !ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len))
-		goto out;
+		err = 0;
+		if (__in6_dev_get(skb->dev) &&
+		    !ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len))
+			goto out;
 
-	if (t->sit_parms->iph.daddr == 0)
-		goto out;
+		if (parms->iph.daddr == 0)
+			goto out;
 
-	if (t->sit_parms->iph.ttl == 0 && type == ICMP_TIME_EXCEEDED)
-		goto out;
+		if (parms->iph.ttl == 0 && type == ICMP_TIME_EXCEEDED)
+			goto out;
 
-	if (time_before(jiffies, READ_ONCE(t->err_time) + IPTUNNEL_ERR_TIMEO))
-		WRITE_ONCE(t->err_count, READ_ONCE(t->err_count) + 1);
-	else
-		WRITE_ONCE(t->err_count, 1);
-	WRITE_ONCE(t->err_time, jiffies);
+		if (time_before(jiffies, READ_ONCE(t->err_time) + IPTUNNEL_ERR_TIMEO))
+			WRITE_ONCE(t->err_count, READ_ONCE(t->err_count) + 1);
+		else
+			WRITE_ONCE(t->err_count, 1);
+		WRITE_ONCE(t->err_time, jiffies);
+	}
 out:
+	rcu_read_unlock();
 	return err;
 }
 
@@ -726,8 +746,11 @@ static int ipip6_rcv(struct sk_buff *skb)
 	tunnel = ipip6_tunnel_lookup(dev_net(skb->dev), skb->dev,
 				     iph->saddr, iph->daddr, sifindex);
 	if (tunnel) {
-		if (tunnel->sit_parms->iph.protocol != IPPROTO_IPV6 &&
-		    tunnel->sit_parms->iph.protocol != 0)
+		const struct ip_tunnel_parm_kern *parms;
+
+		parms = rcu_dereference(tunnel->sit_parms);
+		if (parms->iph.protocol != IPPROTO_IPV6 &&
+		    parms->iph.protocol != 0)
 			goto out;
 
 		skb->mac_header = skb->network_header;
@@ -800,10 +823,12 @@ static int sit_tunnel_rcv(struct sk_buff *skb, u8 ipproto)
 	tunnel = ipip6_tunnel_lookup(dev_net(skb->dev), skb->dev,
 				     iph->saddr, iph->daddr, sifindex);
 	if (tunnel) {
+		const struct ip_tunnel_parm_kern *parms;
 		const struct tnl_ptk_info *tpi;
 
-		if (tunnel->sit_parms->iph.protocol != ipproto &&
-		    tunnel->sit_parms->iph.protocol != 0)
+		parms = rcu_dereference(tunnel->sit_parms);
+		if (parms->iph.protocol != ipproto &&
+		    parms->iph.protocol != 0)
 			goto drop;
 
 		if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
@@ -942,20 +967,27 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
 				     struct net_device *dev)
 {
 	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;
 	const struct ipv6hdr *iph6 = ipv6_hdr(skb);
-	u8     tos = tunnel->sit_parms->iph.tos;
-	__be16 df = tiph->frag_off;
+	u8     tos;
+	__be16 df;
 	struct rtable *rt;		/* Route to the other host */
 	struct net_device *tdev;	/* Device to other host */
 	unsigned int max_headroom;	/* The extra header space needed */
-	__be32 dst = tiph->daddr;
+	__be32 dst;
 	int err_count, mtu;
 	struct flowi4 fl4;
 	u8 ttl;
 	u8 protocol = IPPROTO_IPV6;
 	int t_hlen = tunnel->hlen + sizeof(struct iphdr);
 
+	parms = rcu_dereference(tunnel->sit_parms);
+	tiph = &parms->iph;
+	tos = parms->iph.tos;
+	df = tiph->frag_off;
+	dst = tiph->daddr;
+
 	if (tos == 1)
 		tos = ipv6_get_dsfield(iph6);
 
@@ -970,7 +1002,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->sit_parms->link, READ_ONCE(tunnel->fwmark),
+	flowi4_init_output(&fl4, 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));
@@ -1018,7 +1050,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
 			df = 0;
 		}
 
-		if (tunnel->sit_parms->iph.daddr)
+		if (parms->iph.daddr)
 			skb_dst_update_pmtu_no_confirm(skb, mtu);
 
 		if (skb->len > mtu && !skb_is_gso(skb)) {
@@ -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:
@@ -1108,6 +1144,7 @@ static netdev_tx_t sit_tunnel_xmit(struct sk_buff *skb,
 	if (!pskb_inet_may_pull(skb))
 		goto tx_err;
 
+	rcu_read_lock();
 	switch (skb->protocol) {
 	case htons(ETH_P_IP):
 		sit_tunnel_xmit__(skb, dev, IPPROTO_IPIP);
@@ -1121,8 +1158,10 @@ static netdev_tx_t sit_tunnel_xmit(struct sk_buff *skb,
 		break;
 #endif
 	default:
+		rcu_read_unlock();
 		goto tx_err;
 	}
+	rcu_read_unlock();
 
 	return NETDEV_TX_OK;
 
@@ -1130,19 +1169,20 @@ static netdev_tx_t sit_tunnel_xmit(struct sk_buff *skb,
 	DEV_STATS_INC(dev, tx_errors);
 	kfree_skb(skb);
 	return NETDEV_TX_OK;
-
 }
 
 static void ipip6_tunnel_bind_dev(struct net_device *dev)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
 	int t_hlen = tunnel->hlen + sizeof(struct iphdr);
+	const struct ip_tunnel_parm_kern *parms;
 	struct net_device *tdev = NULL;
 	int hlen = LL_MAX_HEADER;
 	const struct iphdr *iph;
 	struct flowi4 fl4;
 
-	iph = &tunnel->sit_parms->iph;
+	parms = rtnl_dereference(tunnel->sit_parms);
+	iph = &parms->iph;
 
 	if (iph->daddr) {
 		struct rtable *rt = ip_route_output_ports(tunnel->net, &fl4,
@@ -1151,7 +1191,7 @@ static void ipip6_tunnel_bind_dev(struct net_device *dev)
 							  0, 0,
 							  IPPROTO_IPV6,
 							  iph->tos & INET_DSCP_MASK,
-							  tunnel->sit_parms->link);
+							  parms->link);
 
 		if (!IS_ERR(rt)) {
 			tdev = rt->dst.dev;
@@ -1160,8 +1200,8 @@ static void ipip6_tunnel_bind_dev(struct net_device *dev)
 		dev->flags |= IFF_POINTOPOINT;
 	}
 
-	if (!tdev && tunnel->sit_parms->link)
-		tdev = __dev_get_by_index(tunnel->net, tunnel->sit_parms->link);
+	if (!tdev && parms->link)
+		tdev = __dev_get_by_index(tunnel->net, parms->link);
 
 	if (tdev && !netif_is_l3_master(tdev)) {
 		int mtu;
@@ -1182,8 +1222,9 @@ static int ipip6_tunnel_update(struct ip_tunnel *t,
 	struct net *net = t->net;
 	struct sit_net *sitn = net_generic(net, sit_net_id);
 	struct ip_tunnel_parm_kern *new_p, *old_p;
+	bool move;
 
-	old_p = t->sit_parms;
+	old_p = rtnl_dereference(t->sit_parms);
 	new_p = kmalloc_obj(*new_p);
 	if (!new_p)
 		return -ENOMEM;
@@ -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);
+
+	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;
 }
 
@@ -1336,12 +1385,14 @@ static int
 ipip6_tunnel_get(struct net_device *dev, struct ip_tunnel_parm_kern *p)
 {
 	struct ip_tunnel *t = netdev_priv(dev);
+	const struct ip_tunnel_parm_kern *parms;
 
 	if (dev == dev_to_sit_net(dev)->fb_tunnel_dev)
 		t = ipip6_tunnel_locate(t->net, p, 0);
 	if (!t)
 		t = netdev_priv(dev);
-	memcpy(p, t->sit_parms, sizeof(*p));
+	parms = rtnl_dereference(t->sit_parms);
+	memcpy(p, parms, sizeof(*p));
 	return 0;
 }
 
@@ -1464,8 +1515,15 @@ ipip6_tunnel_siocdevprivate(struct net_device *dev, struct ifreq *ifr,
 static int ipip6_get_iflink(const struct net_device *dev)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
+	const struct ip_tunnel_parm_kern *parms;
+	int link;
 
-	return READ_ONCE(tunnel->sit_parms->link);
+	rcu_read_lock();
+	parms = rcu_dereference(tunnel->sit_parms);
+	link = parms ? parms->link : 0;
+	rcu_read_unlock();
+
+	return link;
 }
 
 static const struct net_device_ops ipip6_netdev_ops = {
@@ -1480,6 +1538,7 @@ static const struct net_device_ops ipip6_netdev_ops = {
 static void ipip6_dev_free(struct net_device *dev)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
+	struct ip_tunnel_parm_kern *parms;
 #ifdef CONFIG_IPV6_SIT_6RD
 	struct ip_tunnel_6rd_parm *ip6rd;
 
@@ -1487,8 +1546,9 @@ static void ipip6_dev_free(struct net_device *dev)
 	RCU_INIT_POINTER(tunnel->ip6rd, NULL);
 	kfree(ip6rd);
 #endif
-	kfree(tunnel->sit_parms);
-	tunnel->sit_parms = NULL;
+	parms = rcu_dereference_protected(tunnel->sit_parms, 1);
+	RCU_INIT_POINTER(tunnel->sit_parms, NULL);
+	kfree(parms);
 	if (tunnel->dst_cache.cache) {
 		dst_cache_destroy(&tunnel->dst_cache);
 		tunnel->dst_cache.cache = NULL;
@@ -1528,10 +1588,12 @@ static void ipip6_tunnel_setup(struct net_device *dev)
 static int ipip6_tunnel_init(struct net_device *dev)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
+	struct ip_tunnel_parm_kern *parms;
 	int err;
 
 	tunnel->dev = dev;
-	strscpy(tunnel->sit_parms->name, dev->name);
+	parms = rtnl_dereference(tunnel->sit_parms);
+	strscpy(parms->name, dev->name);
 
 	ipip6_tunnel_bind_dev(dev);
 
@@ -1549,7 +1611,6 @@ static void __net_init ipip6_fb_tunnel_init(struct net_device *dev)
 	struct ip_tunnel *tunnel = netdev_priv(dev);
 	struct net *net = dev_net(dev);
 	struct sit_net *sitn = net_generic(net, sit_net_id);
-
 	rcu_assign_pointer(sitn->tunnels_wc[0], tunnel);
 }
 
@@ -1636,6 +1697,7 @@ static int ipip6_newlink(struct net_device *dev,
 #ifdef CONFIG_IPV6_SIT_6RD
 	struct ip_tunnel_6rd ip6rd;
 #endif
+	struct ip_tunnel_parm_kern *nt_parms;
 	struct ip_tunnel_parm_kern p;
 	struct net *net;
 	int err;
@@ -1655,10 +1717,11 @@ static int ipip6_newlink(struct net_device *dev,
 	if (ipip6_tunnel_locate(net, &p, 0))
 		return -EEXIST;
 
-	nt->sit_parms = kmalloc_obj(*nt->sit_parms);
-	if (!nt->sit_parms)
+	nt_parms = kmalloc_obj(*nt_parms);
+	if (!nt_parms)
 		return -ENOMEM;
-	*nt->sit_parms = p;
+	*nt_parms = p;
+	rcu_assign_pointer(nt->sit_parms, nt_parms);
 
 	err = ipip6_tunnel_create(dev);
 	if (err < 0) {
@@ -1783,7 +1846,7 @@ static size_t ipip6_get_size(const struct net_device *dev)
 static int ipip6_fill_info(struct sk_buff *skb, const struct net_device *dev)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
-	struct ip_tunnel_parm_kern *parm = tunnel->sit_parms;
+	const struct ip_tunnel_parm_kern *parm = rtnl_dereference(tunnel->sit_parms);
 #ifdef CONFIG_IPV6_SIT_6RD
 	const struct ip_tunnel_6rd_parm *ip6rd;
 #endif
@@ -1929,6 +1992,7 @@ static void __net_exit sit_exit_rtnl_net(struct net *net, struct list_head *head
 static int __net_init sit_init_net(struct net *net)
 {
 	struct sit_net *sitn = net_generic(net, sit_net_id);
+	struct ip_tunnel_parm_kern *nt_parms;
 	struct ip_tunnel *t;
 	int err;
 
@@ -1956,17 +2020,18 @@ static int __net_init sit_init_net(struct net *net)
 
 	t = netdev_priv(sitn->fb_tunnel_dev);
 	t->net = net;
-	t->sit_parms = kzalloc_obj(*t->sit_parms);
-	if (!t->sit_parms) {
+	nt_parms = kzalloc_obj(*nt_parms);
+	if (!nt_parms) {
 		err = -ENOMEM;
 		goto err_reg_dev;
 	}
-	t->sit_parms->iph.version	= 4;
-	t->sit_parms->iph.protocol	= IPPROTO_IPV6;
-	t->sit_parms->iph.ihl		= 5;
-	t->sit_parms->iph.ttl		= 64;
-	strscpy(t->sit_parms->name, sitn->fb_tunnel_dev->name);
-	t->parms = *t->sit_parms;
+	nt_parms->iph.version	= 4;
+	nt_parms->iph.protocol	= IPPROTO_IPV6;
+	nt_parms->iph.ihl	= 5;
+	nt_parms->iph.ttl	= 64;
+	strscpy(nt_parms->name, sitn->fb_tunnel_dev->name);
+	t->parms = *nt_parms;
+	rcu_assign_pointer(t->sit_parms, nt_parms);
 
 	err = ipip6_tunnel_clone_6rd(sitn->fb_tunnel_dev, sitn);
 	if (err < 0)
-- 
2.55.0.979.g7e5102b832-goog

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 21008 bytes --]

> Now that SIT parameters are dynamically allocated, convert
> tunnel->sit_parms to an RCU-protected pointer.
> 
> Updates in ipip6_tunnel_update() allocate a new parameter block,
> publish it using rcu_assign_pointer(), and free the old one
> via kfree_rcu().
> 
> We only need to unlink and re-link the tunnel in the hash table
> if either saddr or daddr changed. When neither address changes,
> the unhash/re-hash and synchronize_net() can be completely skipped.
> 
> Readers in ipip6_tunnel_lookup(), ipip6_tunnel_xmit(), ipip6_err(),
> and ipip6_rcv() now safely dereference tunnel->sit_parms under RCU.

I think this patch is fine, I am just wondering if we can use more generic name
with respect to 'sit_parms' since I guess we have the same issue for IPIP and
IP6IP6 tunnels. Do you prefer to have dedicated pointers for them?

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

Regards,
Lorenzo

> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  include/net/ip_tunnels.h |   3 +-
>  net/ipv6/sit.c           | 245 +++++++++++++++++++++++++--------------
>  2 files changed, 157 insertions(+), 91 deletions(-)
> 
> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index f464c4480edaf35f9ace1c5081c564ce51fed636..be4cc10f88ed64114cebac7f2e01bea21f5419da 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -149,6 +149,7 @@ struct ip_tunnel_parm_kern {
>  	__be32			o_key;
>  	int			link;
>  	struct iphdr		iph;
> +	struct rcu_head		rcu;
>  };
>  
>  struct ip_tunnel {
> @@ -190,7 +191,7 @@ struct ip_tunnel {
>  #endif
>  	struct ip_tunnel_prl_entry __rcu *prl;	/* potential router list */
>  	unsigned int		prl_count;	/* # of entries in PRL */
> -	struct ip_tunnel_parm_kern *sit_parms;
> +	struct ip_tunnel_parm_kern __rcu *sit_parms;
>  	unsigned int		ip_tnl_net_id;
>  	struct gro_cells	gro_cells;
>  	__u32			fwmark;
> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index dc37c7109af5324f2ced0301b289e7622dd1a55f..c9049ab87e010eed5f53a342460ed10006083cd7 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -108,24 +108,33 @@ static struct ip_tunnel *ipip6_tunnel_lookup(struct net *net,
>  	int ifindex = dev ? dev->ifindex : 0;
>  
>  	for_each_ip_tunnel_rcu(t, sitn->tunnels_r_l[h0 ^ h1]) {
> -		if (local == t->sit_parms->iph.saddr &&
> -		    remote == t->sit_parms->iph.daddr &&
> -		    (!dev || !t->sit_parms->link || ifindex == t->sit_parms->link ||
> -		     sifindex == t->sit_parms->link) &&
> +		const struct ip_tunnel_parm_kern *parms;
> +
> +		parms = rcu_dereference(t->sit_parms);
> +		if (local == parms->iph.saddr &&
> +		    remote == parms->iph.daddr &&
> +		    (!dev || !parms->link || ifindex == parms->link ||
> +		     sifindex == parms->link) &&
>  		    (t->dev->flags & IFF_UP))
>  			return t;
>  	}
>  	for_each_ip_tunnel_rcu(t, sitn->tunnels_r[h0]) {
> -		if (remote == t->sit_parms->iph.daddr &&
> -		    (!dev || !t->sit_parms->link || ifindex == t->sit_parms->link ||
> -		     sifindex == t->sit_parms->link) &&
> +		const struct ip_tunnel_parm_kern *parms;
> +
> +		parms = rcu_dereference(t->sit_parms);
> +		if (remote == parms->iph.daddr &&
> +		    (!dev || !parms->link || ifindex == parms->link ||
> +		     sifindex == parms->link) &&
>  		    (t->dev->flags & IFF_UP))
>  			return t;
>  	}
>  	for_each_ip_tunnel_rcu(t, sitn->tunnels_l[h1]) {
> -		if (local == t->sit_parms->iph.saddr &&
> -		    (!dev || !t->sit_parms->link || ifindex == t->sit_parms->link ||
> -		     sifindex == t->sit_parms->link) &&
> +		const struct ip_tunnel_parm_kern *parms;
> +
> +		parms = rcu_dereference(t->sit_parms);
> +		if (local == parms->iph.saddr &&
> +		    (!dev || !parms->link || ifindex == parms->link ||
> +		     sifindex == parms->link) &&
>  		    (t->dev->flags & IFF_UP))
>  			return t;
>  	}
> @@ -157,7 +166,7 @@ __ipip6_bucket(struct sit_net *sitn, struct ip_tunnel_parm_kern *parms)
>  static inline struct ip_tunnel __rcu **ipip6_bucket(struct sit_net *sitn,
>  		struct ip_tunnel *t)
>  {
> -	return __ipip6_bucket(sitn, t->sit_parms);
> +	return __ipip6_bucket(sitn, rtnl_dereference(t->sit_parms));
>  }
>  
>  static void ipip6_tunnel_unlink(struct sit_net *sitn, struct ip_tunnel *t)
> @@ -230,17 +239,19 @@ static int ipip6_tunnel_create(struct net_device *dev)
>  {
>  	struct ip_tunnel *t = netdev_priv(dev);
>  	struct sit_net *sitn = net_generic(t->net, sit_net_id);
> +	struct ip_tunnel_parm_kern *parms;
>  	int err;
>  
>  	err = ipip6_tunnel_clone_6rd(dev, sitn);
>  	if (err < 0)
>  		goto out;
>  
> -	t->parms = *t->sit_parms;
> -	__dev_addr_set(dev, &t->sit_parms->iph.saddr, 4);
> -	memcpy(dev->broadcast, &t->sit_parms->iph.daddr, 4);
> +	parms = rtnl_dereference(t->sit_parms);
> +	t->parms = *parms;
> +	__dev_addr_set(dev, &parms->iph.saddr, 4);
> +	memcpy(dev->broadcast, &parms->iph.daddr, 4);
>  
> -	if (test_bit(IP_TUNNEL_SIT_ISATAP_BIT, t->sit_parms->i_flags))
> +	if (test_bit(IP_TUNNEL_SIT_ISATAP_BIT, parms->i_flags))
>  		dev->priv_flags |= IFF_ISATAP;
>  
>  	dev->rtnl_link_ops = &sit_link_ops;
> @@ -264,6 +275,7 @@ static struct ip_tunnel *ipip6_tunnel_locate(struct net *net,
>  	__be32 local = parms->iph.saddr;
>  	struct ip_tunnel *t, *nt;
>  	struct ip_tunnel __rcu **tp;
> +	struct ip_tunnel_parm_kern *nt_parms;
>  	struct net_device *dev;
>  	char name[IFNAMSIZ];
>  	struct sit_net *sitn = net_generic(net, sit_net_id);
> @@ -271,9 +283,12 @@ static struct ip_tunnel *ipip6_tunnel_locate(struct net *net,
>  	for (tp = __ipip6_bucket(sitn, parms);
>  	    (t = rtnl_dereference(*tp)) != NULL;
>  	     tp = &t->next) {
> -		if (local == t->sit_parms->iph.saddr &&
> -		    remote == t->sit_parms->iph.daddr &&
> -		    parms->link == t->sit_parms->link) {
> +		const struct ip_tunnel_parm_kern *tparms;
> +
> +		tparms = rtnl_dereference(t->sit_parms);
> +		if (local == tparms->iph.saddr &&
> +		    remote == tparms->iph.daddr &&
> +		    parms->link == tparms->link) {
>  			if (create)
>  				return NULL;
>  			else
> @@ -300,10 +315,11 @@ static struct ip_tunnel *ipip6_tunnel_locate(struct net *net,
>  	nt = netdev_priv(dev);
>  
>  	nt->net = net;
> -	nt->sit_parms = kmalloc_obj(*nt->sit_parms);
> -	if (!nt->sit_parms)
> +	nt_parms = kmalloc_obj(*nt_parms);
> +	if (!nt_parms)
>  		goto failed_free;
> -	*nt->sit_parms = *parms;
> +	*nt_parms = *parms;
> +	rcu_assign_pointer(nt->sit_parms, nt_parms);
>  	if (ipip6_tunnel_create(dev) < 0)
>  		goto failed_free;
>  
> @@ -599,41 +615,45 @@ static int ipip6_err(struct sk_buff *skb, u32 info)
>  	err = -ENOENT;
>  
>  	sifindex = netif_is_l3_master(skb->dev) ? IPCB(skb)->iif : 0;
> +	rcu_read_lock();
>  	t = ipip6_tunnel_lookup(dev_net(skb->dev), skb->dev,
>  				iph->daddr, iph->saddr, sifindex);
> -	if (!t)
> -		goto out;
> +	if (t) {
> +		const struct ip_tunnel_parm_kern *parms;
>  
> -	if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED) {
> -		ipv4_update_pmtu(skb, dev_net(skb->dev), info,
> -				 t->sit_parms->link, iph->protocol);
> -		err = 0;
> -		goto out;
> -	}
> -	if (type == ICMP_REDIRECT) {
> -		ipv4_redirect(skb, dev_net(skb->dev), t->sit_parms->link,
> -			      iph->protocol);
> -		err = 0;
> -		goto out;
> -	}
> +		parms = rcu_dereference(t->sit_parms);
> +		if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED) {
> +			ipv4_update_pmtu(skb, dev_net(skb->dev), info,
> +					 parms->link, iph->protocol);
> +			err = 0;
> +			goto out;
> +		}
> +		if (type == ICMP_REDIRECT) {
> +			ipv4_redirect(skb, dev_net(skb->dev), parms->link,
> +				      iph->protocol);
> +			err = 0;
> +			goto out;
> +		}
>  
> -	err = 0;
> -	if (__in6_dev_get(skb->dev) &&
> -	    !ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len))
> -		goto out;
> +		err = 0;
> +		if (__in6_dev_get(skb->dev) &&
> +		    !ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len))
> +			goto out;
>  
> -	if (t->sit_parms->iph.daddr == 0)
> -		goto out;
> +		if (parms->iph.daddr == 0)
> +			goto out;
>  
> -	if (t->sit_parms->iph.ttl == 0 && type == ICMP_TIME_EXCEEDED)
> -		goto out;
> +		if (parms->iph.ttl == 0 && type == ICMP_TIME_EXCEEDED)
> +			goto out;
>  
> -	if (time_before(jiffies, READ_ONCE(t->err_time) + IPTUNNEL_ERR_TIMEO))
> -		WRITE_ONCE(t->err_count, READ_ONCE(t->err_count) + 1);
> -	else
> -		WRITE_ONCE(t->err_count, 1);
> -	WRITE_ONCE(t->err_time, jiffies);
> +		if (time_before(jiffies, READ_ONCE(t->err_time) + IPTUNNEL_ERR_TIMEO))
> +			WRITE_ONCE(t->err_count, READ_ONCE(t->err_count) + 1);
> +		else
> +			WRITE_ONCE(t->err_count, 1);
> +		WRITE_ONCE(t->err_time, jiffies);
> +	}
>  out:
> +	rcu_read_unlock();
>  	return err;
>  }
>  
> @@ -726,8 +746,11 @@ static int ipip6_rcv(struct sk_buff *skb)
>  	tunnel = ipip6_tunnel_lookup(dev_net(skb->dev), skb->dev,
>  				     iph->saddr, iph->daddr, sifindex);
>  	if (tunnel) {
> -		if (tunnel->sit_parms->iph.protocol != IPPROTO_IPV6 &&
> -		    tunnel->sit_parms->iph.protocol != 0)
> +		const struct ip_tunnel_parm_kern *parms;
> +
> +		parms = rcu_dereference(tunnel->sit_parms);
> +		if (parms->iph.protocol != IPPROTO_IPV6 &&
> +		    parms->iph.protocol != 0)
>  			goto out;
>  
>  		skb->mac_header = skb->network_header;
> @@ -800,10 +823,12 @@ static int sit_tunnel_rcv(struct sk_buff *skb, u8 ipproto)
>  	tunnel = ipip6_tunnel_lookup(dev_net(skb->dev), skb->dev,
>  				     iph->saddr, iph->daddr, sifindex);
>  	if (tunnel) {
> +		const struct ip_tunnel_parm_kern *parms;
>  		const struct tnl_ptk_info *tpi;
>  
> -		if (tunnel->sit_parms->iph.protocol != ipproto &&
> -		    tunnel->sit_parms->iph.protocol != 0)
> +		parms = rcu_dereference(tunnel->sit_parms);
> +		if (parms->iph.protocol != ipproto &&
> +		    parms->iph.protocol != 0)
>  			goto drop;
>  
>  		if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
> @@ -942,20 +967,27 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
>  				     struct net_device *dev)
>  {
>  	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;
>  	const struct ipv6hdr *iph6 = ipv6_hdr(skb);
> -	u8     tos = tunnel->sit_parms->iph.tos;
> -	__be16 df = tiph->frag_off;
> +	u8     tos;
> +	__be16 df;
>  	struct rtable *rt;		/* Route to the other host */
>  	struct net_device *tdev;	/* Device to other host */
>  	unsigned int max_headroom;	/* The extra header space needed */
> -	__be32 dst = tiph->daddr;
> +	__be32 dst;
>  	int err_count, mtu;
>  	struct flowi4 fl4;
>  	u8 ttl;
>  	u8 protocol = IPPROTO_IPV6;
>  	int t_hlen = tunnel->hlen + sizeof(struct iphdr);
>  
> +	parms = rcu_dereference(tunnel->sit_parms);
> +	tiph = &parms->iph;
> +	tos = parms->iph.tos;
> +	df = tiph->frag_off;
> +	dst = tiph->daddr;
> +
>  	if (tos == 1)
>  		tos = ipv6_get_dsfield(iph6);
>  
> @@ -970,7 +1002,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->sit_parms->link, READ_ONCE(tunnel->fwmark),
> +	flowi4_init_output(&fl4, 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));
> @@ -1018,7 +1050,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
>  			df = 0;
>  		}
>  
> -		if (tunnel->sit_parms->iph.daddr)
> +		if (parms->iph.daddr)
>  			skb_dst_update_pmtu_no_confirm(skb, mtu);
>  
>  		if (skb->len > mtu && !skb_is_gso(skb)) {
> @@ -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:
> @@ -1108,6 +1144,7 @@ static netdev_tx_t sit_tunnel_xmit(struct sk_buff *skb,
>  	if (!pskb_inet_may_pull(skb))
>  		goto tx_err;
>  
> +	rcu_read_lock();
>  	switch (skb->protocol) {
>  	case htons(ETH_P_IP):
>  		sit_tunnel_xmit__(skb, dev, IPPROTO_IPIP);
> @@ -1121,8 +1158,10 @@ static netdev_tx_t sit_tunnel_xmit(struct sk_buff *skb,
>  		break;
>  #endif
>  	default:
> +		rcu_read_unlock();
>  		goto tx_err;
>  	}
> +	rcu_read_unlock();
>  
>  	return NETDEV_TX_OK;
>  
> @@ -1130,19 +1169,20 @@ static netdev_tx_t sit_tunnel_xmit(struct sk_buff *skb,
>  	DEV_STATS_INC(dev, tx_errors);
>  	kfree_skb(skb);
>  	return NETDEV_TX_OK;
> -
>  }
>  
>  static void ipip6_tunnel_bind_dev(struct net_device *dev)
>  {
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
>  	int t_hlen = tunnel->hlen + sizeof(struct iphdr);
> +	const struct ip_tunnel_parm_kern *parms;
>  	struct net_device *tdev = NULL;
>  	int hlen = LL_MAX_HEADER;
>  	const struct iphdr *iph;
>  	struct flowi4 fl4;
>  
> -	iph = &tunnel->sit_parms->iph;
> +	parms = rtnl_dereference(tunnel->sit_parms);
> +	iph = &parms->iph;
>  
>  	if (iph->daddr) {
>  		struct rtable *rt = ip_route_output_ports(tunnel->net, &fl4,
> @@ -1151,7 +1191,7 @@ static void ipip6_tunnel_bind_dev(struct net_device *dev)
>  							  0, 0,
>  							  IPPROTO_IPV6,
>  							  iph->tos & INET_DSCP_MASK,
> -							  tunnel->sit_parms->link);
> +							  parms->link);
>  
>  		if (!IS_ERR(rt)) {
>  			tdev = rt->dst.dev;
> @@ -1160,8 +1200,8 @@ static void ipip6_tunnel_bind_dev(struct net_device *dev)
>  		dev->flags |= IFF_POINTOPOINT;
>  	}
>  
> -	if (!tdev && tunnel->sit_parms->link)
> -		tdev = __dev_get_by_index(tunnel->net, tunnel->sit_parms->link);
> +	if (!tdev && parms->link)
> +		tdev = __dev_get_by_index(tunnel->net, parms->link);
>  
>  	if (tdev && !netif_is_l3_master(tdev)) {
>  		int mtu;
> @@ -1182,8 +1222,9 @@ static int ipip6_tunnel_update(struct ip_tunnel *t,
>  	struct net *net = t->net;
>  	struct sit_net *sitn = net_generic(net, sit_net_id);
>  	struct ip_tunnel_parm_kern *new_p, *old_p;
> +	bool move;
>  
> -	old_p = t->sit_parms;
> +	old_p = rtnl_dereference(t->sit_parms);
>  	new_p = kmalloc_obj(*new_p);
>  	if (!new_p)
>  		return -ENOMEM;
> @@ -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);
> +
> +	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;
>  }
>  
> @@ -1336,12 +1385,14 @@ static int
>  ipip6_tunnel_get(struct net_device *dev, struct ip_tunnel_parm_kern *p)
>  {
>  	struct ip_tunnel *t = netdev_priv(dev);
> +	const struct ip_tunnel_parm_kern *parms;
>  
>  	if (dev == dev_to_sit_net(dev)->fb_tunnel_dev)
>  		t = ipip6_tunnel_locate(t->net, p, 0);
>  	if (!t)
>  		t = netdev_priv(dev);
> -	memcpy(p, t->sit_parms, sizeof(*p));
> +	parms = rtnl_dereference(t->sit_parms);
> +	memcpy(p, parms, sizeof(*p));
>  	return 0;
>  }
>  
> @@ -1464,8 +1515,15 @@ ipip6_tunnel_siocdevprivate(struct net_device *dev, struct ifreq *ifr,
>  static int ipip6_get_iflink(const struct net_device *dev)
>  {
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
> +	const struct ip_tunnel_parm_kern *parms;
> +	int link;
>  
> -	return READ_ONCE(tunnel->sit_parms->link);
> +	rcu_read_lock();
> +	parms = rcu_dereference(tunnel->sit_parms);
> +	link = parms ? parms->link : 0;
> +	rcu_read_unlock();
> +
> +	return link;
>  }
>  
>  static const struct net_device_ops ipip6_netdev_ops = {
> @@ -1480,6 +1538,7 @@ static const struct net_device_ops ipip6_netdev_ops = {
>  static void ipip6_dev_free(struct net_device *dev)
>  {
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
> +	struct ip_tunnel_parm_kern *parms;
>  #ifdef CONFIG_IPV6_SIT_6RD
>  	struct ip_tunnel_6rd_parm *ip6rd;
>  
> @@ -1487,8 +1546,9 @@ static void ipip6_dev_free(struct net_device *dev)
>  	RCU_INIT_POINTER(tunnel->ip6rd, NULL);
>  	kfree(ip6rd);
>  #endif
> -	kfree(tunnel->sit_parms);
> -	tunnel->sit_parms = NULL;
> +	parms = rcu_dereference_protected(tunnel->sit_parms, 1);
> +	RCU_INIT_POINTER(tunnel->sit_parms, NULL);
> +	kfree(parms);
>  	if (tunnel->dst_cache.cache) {
>  		dst_cache_destroy(&tunnel->dst_cache);
>  		tunnel->dst_cache.cache = NULL;
> @@ -1528,10 +1588,12 @@ static void ipip6_tunnel_setup(struct net_device *dev)
>  static int ipip6_tunnel_init(struct net_device *dev)
>  {
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
> +	struct ip_tunnel_parm_kern *parms;
>  	int err;
>  
>  	tunnel->dev = dev;
> -	strscpy(tunnel->sit_parms->name, dev->name);
> +	parms = rtnl_dereference(tunnel->sit_parms);
> +	strscpy(parms->name, dev->name);
>  
>  	ipip6_tunnel_bind_dev(dev);
>  
> @@ -1549,7 +1611,6 @@ static void __net_init ipip6_fb_tunnel_init(struct net_device *dev)
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
>  	struct net *net = dev_net(dev);
>  	struct sit_net *sitn = net_generic(net, sit_net_id);
> -
>  	rcu_assign_pointer(sitn->tunnels_wc[0], tunnel);
>  }
>  
> @@ -1636,6 +1697,7 @@ static int ipip6_newlink(struct net_device *dev,
>  #ifdef CONFIG_IPV6_SIT_6RD
>  	struct ip_tunnel_6rd ip6rd;
>  #endif
> +	struct ip_tunnel_parm_kern *nt_parms;
>  	struct ip_tunnel_parm_kern p;
>  	struct net *net;
>  	int err;
> @@ -1655,10 +1717,11 @@ static int ipip6_newlink(struct net_device *dev,
>  	if (ipip6_tunnel_locate(net, &p, 0))
>  		return -EEXIST;
>  
> -	nt->sit_parms = kmalloc_obj(*nt->sit_parms);
> -	if (!nt->sit_parms)
> +	nt_parms = kmalloc_obj(*nt_parms);
> +	if (!nt_parms)
>  		return -ENOMEM;
> -	*nt->sit_parms = p;
> +	*nt_parms = p;
> +	rcu_assign_pointer(nt->sit_parms, nt_parms);
>  
>  	err = ipip6_tunnel_create(dev);
>  	if (err < 0) {
> @@ -1783,7 +1846,7 @@ static size_t ipip6_get_size(const struct net_device *dev)
>  static int ipip6_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  {
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
> -	struct ip_tunnel_parm_kern *parm = tunnel->sit_parms;
> +	const struct ip_tunnel_parm_kern *parm = rtnl_dereference(tunnel->sit_parms);
>  #ifdef CONFIG_IPV6_SIT_6RD
>  	const struct ip_tunnel_6rd_parm *ip6rd;
>  #endif
> @@ -1929,6 +1992,7 @@ static void __net_exit sit_exit_rtnl_net(struct net *net, struct list_head *head
>  static int __net_init sit_init_net(struct net *net)
>  {
>  	struct sit_net *sitn = net_generic(net, sit_net_id);
> +	struct ip_tunnel_parm_kern *nt_parms;
>  	struct ip_tunnel *t;
>  	int err;
>  
> @@ -1956,17 +2020,18 @@ static int __net_init sit_init_net(struct net *net)
>  
>  	t = netdev_priv(sitn->fb_tunnel_dev);
>  	t->net = net;
> -	t->sit_parms = kzalloc_obj(*t->sit_parms);
> -	if (!t->sit_parms) {
> +	nt_parms = kzalloc_obj(*nt_parms);
> +	if (!nt_parms) {
>  		err = -ENOMEM;
>  		goto err_reg_dev;
>  	}
> -	t->sit_parms->iph.version	= 4;
> -	t->sit_parms->iph.protocol	= IPPROTO_IPV6;
> -	t->sit_parms->iph.ihl		= 5;
> -	t->sit_parms->iph.ttl		= 64;
> -	strscpy(t->sit_parms->name, sitn->fb_tunnel_dev->name);
> -	t->parms = *t->sit_parms;
> +	nt_parms->iph.version	= 4;
> +	nt_parms->iph.protocol	= IPPROTO_IPV6;
> +	nt_parms->iph.ihl	= 5;
> +	nt_parms->iph.ttl	= 64;
> +	strscpy(nt_parms->name, sitn->fb_tunnel_dev->name);
> +	t->parms = *nt_parms;
> +	rcu_assign_pointer(t->sit_parms, nt_parms);
>  
>  	err = ipip6_tunnel_clone_6rd(sitn->fb_tunnel_dev, sitn);
>  	if (err < 0)
> -- 
> 2.55.0.979.g7e5102b832-goog
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Eric Dumazet]

On Mon, Sep 7, 2026 at 4:33 PM Lorenzo Bianconi
<lorenzo.bianconi@oss.qualcomm.com> wrote:
>
> > Now that SIT parameters are dynamically allocated, convert
> > tunnel->sit_parms to an RCU-protected pointer.
> >
> > Updates in ipip6_tunnel_update() allocate a new parameter block,
> > publish it using rcu_assign_pointer(), and free the old one
> > via kfree_rcu().
> >
> > We only need to unlink and re-link the tunnel in the hash table
> > if either saddr or daddr changed. When neither address changes,
> > the unhash/re-hash and synchronize_net() can be completely skipped.
> >
> > Readers in ipip6_tunnel_lookup(), ipip6_tunnel_xmit(), ipip6_err(),
> > and ipip6_rcv() now safely dereference tunnel->sit_parms under RCU.
>
> I think this patch is fine, I am just wondering if we can use more generic name
> with respect to 'sit_parms' since I guess we have the same issue for IPIP and
> IP6IP6 tunnels. Do you prefer to have dedicated pointers for them?

Name could be more generic indeed.

I was thinking of using a union for future tunnel conversions.

>
> Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
>
> Regards,
> Lorenzo

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

[netdev-bot+sashiko]

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

[PATCH net-next 9/9] sit: no longer rely on RTNL in ipip6_fill_info()

[Eric Dumazet]

Now that SIT parameters and 6RD parameters are RCU-protected,
and fwmark/encap are annotated with READ_ONCE()/WRITE_ONCE(),
ipip6_fill_info() no longer requires RTNL to be held.

Wrap the attribute serialization in rcu_read_lock() and rcu_read_unlock().

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv6/sit.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index c9049ab87e010eed5f53a342460ed10006083cd7..14ba3cfb304a83ee37e3be5b09a57c371f1977c3 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -1846,11 +1846,18 @@ static size_t ipip6_get_size(const struct net_device *dev)
 static int ipip6_fill_info(struct sk_buff *skb, const struct net_device *dev)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
-	const struct ip_tunnel_parm_kern *parm = rtnl_dereference(tunnel->sit_parms);
+	const struct ip_tunnel_parm_kern *parm;
 #ifdef CONFIG_IPV6_SIT_6RD
 	const struct ip_tunnel_6rd_parm *ip6rd;
 #endif
 
+	rcu_read_lock();
+	parm = rcu_dereference(tunnel->sit_parms);
+	if (!parm) {
+		rcu_read_unlock();
+		return -ENODEV;
+	}
+
 	if (nla_put_u32(skb, IFLA_IPTUN_LINK, parm->link) ||
 	    nla_put_in_addr(skb, IFLA_IPTUN_LOCAL, parm->iph.saddr) ||
 	    nla_put_in_addr(skb, IFLA_IPTUN_REMOTE, parm->iph.daddr) ||
@@ -1865,7 +1872,7 @@ static int ipip6_fill_info(struct sk_buff *skb, const struct net_device *dev)
 		goto nla_put_failure;
 
 #ifdef CONFIG_IPV6_SIT_6RD
-	ip6rd = rcu_dereference_rtnl(tunnel->ip6rd);
+	ip6rd = rcu_dereference(tunnel->ip6rd);
 	if (ip6rd &&
 	    (nla_put_in6_addr(skb, IFLA_IPTUN_6RD_PREFIX,
 			      &ip6rd->prefix) ||
@@ -1879,18 +1886,20 @@ static int ipip6_fill_info(struct sk_buff *skb, const struct net_device *dev)
 #endif
 
 	if (nla_put_u16(skb, IFLA_IPTUN_ENCAP_TYPE,
-			tunnel->encap.type) ||
+			READ_ONCE(tunnel->encap.type)) ||
 	    nla_put_be16(skb, IFLA_IPTUN_ENCAP_SPORT,
-			tunnel->encap.sport) ||
+			 READ_ONCE(tunnel->encap.sport)) ||
 	    nla_put_be16(skb, IFLA_IPTUN_ENCAP_DPORT,
-			tunnel->encap.dport) ||
+			 READ_ONCE(tunnel->encap.dport)) ||
 	    nla_put_u16(skb, IFLA_IPTUN_ENCAP_FLAGS,
-			tunnel->encap.flags))
+			READ_ONCE(tunnel->encap.flags)))
 		goto nla_put_failure;
 
+	rcu_read_unlock();
 	return 0;
 
 nla_put_failure:
+	rcu_read_unlock();
 	return -EMSGSIZE;
 }
 
-- 
2.55.0.979.g7e5102b832-goog

Re: [PATCH net-next 9/9] sit: no longer rely on RTNL in ipip6_fill_info()

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2660 bytes --]

> Now that SIT parameters and 6RD parameters are RCU-protected,
> and fwmark/encap are annotated with READ_ONCE()/WRITE_ONCE(),
> ipip6_fill_info() no longer requires RTNL to be held.
> 
> Wrap the attribute serialization in rcu_read_lock() and rcu_read_unlock().

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv6/sit.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index c9049ab87e010eed5f53a342460ed10006083cd7..14ba3cfb304a83ee37e3be5b09a57c371f1977c3 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -1846,11 +1846,18 @@ static size_t ipip6_get_size(const struct net_device *dev)
>  static int ipip6_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  {
>  	struct ip_tunnel *tunnel = netdev_priv(dev);
> -	const struct ip_tunnel_parm_kern *parm = rtnl_dereference(tunnel->sit_parms);
> +	const struct ip_tunnel_parm_kern *parm;
>  #ifdef CONFIG_IPV6_SIT_6RD
>  	const struct ip_tunnel_6rd_parm *ip6rd;
>  #endif
>  
> +	rcu_read_lock();
> +	parm = rcu_dereference(tunnel->sit_parms);
> +	if (!parm) {
> +		rcu_read_unlock();
> +		return -ENODEV;
> +	}
> +
>  	if (nla_put_u32(skb, IFLA_IPTUN_LINK, parm->link) ||
>  	    nla_put_in_addr(skb, IFLA_IPTUN_LOCAL, parm->iph.saddr) ||
>  	    nla_put_in_addr(skb, IFLA_IPTUN_REMOTE, parm->iph.daddr) ||
> @@ -1865,7 +1872,7 @@ static int ipip6_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  		goto nla_put_failure;
>  
>  #ifdef CONFIG_IPV6_SIT_6RD
> -	ip6rd = rcu_dereference_rtnl(tunnel->ip6rd);
> +	ip6rd = rcu_dereference(tunnel->ip6rd);
>  	if (ip6rd &&
>  	    (nla_put_in6_addr(skb, IFLA_IPTUN_6RD_PREFIX,
>  			      &ip6rd->prefix) ||
> @@ -1879,18 +1886,20 @@ static int ipip6_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  #endif
>  
>  	if (nla_put_u16(skb, IFLA_IPTUN_ENCAP_TYPE,
> -			tunnel->encap.type) ||
> +			READ_ONCE(tunnel->encap.type)) ||
>  	    nla_put_be16(skb, IFLA_IPTUN_ENCAP_SPORT,
> -			tunnel->encap.sport) ||
> +			 READ_ONCE(tunnel->encap.sport)) ||
>  	    nla_put_be16(skb, IFLA_IPTUN_ENCAP_DPORT,
> -			tunnel->encap.dport) ||
> +			 READ_ONCE(tunnel->encap.dport)) ||
>  	    nla_put_u16(skb, IFLA_IPTUN_ENCAP_FLAGS,
> -			tunnel->encap.flags))
> +			READ_ONCE(tunnel->encap.flags)))
>  		goto nla_put_failure;
>  
> +	rcu_read_unlock();
>  	return 0;
>  
>  nla_put_failure:
> +	rcu_read_unlock();
>  	return -EMSGSIZE;
>  }
>  
> -- 
> 2.55.0.979.g7e5102b832-goog
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next 0/9] sit: convert configuration to RCU and lockless fill_info

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon,  7 Sep 2026 07:58:37 +0000 you wrote:
> SIT (IPv6-in-IPv4) tunnel configuration and status reporting have
> historically relied on the RTNL lock for synchronization. Consequently,
> netlink dumps via ipip6_fill_info() had to run with RTNL held, adding
> contention during network device dumps.
> 
> At the same time, the transmit path (dev->lltx == true), tunnel lookups,
> and error handling run locklessly and can race with configuration
> updates. This can result in torn reads of multi-word fields (such as the
> 128-bit 6RD IPv6 prefix) or transiently zeroed encapsulation parameters.
> Furthermore, ipip6_tunnel_update() currently unhashes, re-hashes, and
> calls synchronize_net() unconditionally, even when the tunnel endpoint
> addresses (saddr and daddr) have not changed.
> 
> [...]

Here is the summary with links:
  - [net-next,1/9] sit: fix UAF in ipip6_tunnel_del_prl()
    https://git.kernel.org/netdev/net-next/c/5145bb6a2c26
  - [net-next,2/9] sit: charge ip_tunnel_prl_entry allocations to memcg
    https://git.kernel.org/netdev/net-next/c/40a2b90f5190
  - [net-next,3/9] ip_tunnel: use WRITE_ONCE in ip_tunnel_encap_setup
    https://git.kernel.org/netdev/net-next/c/88b84cae6b94
  - [net-next,4/9] sit: annotate data-races around tunnel->fwmark
    https://git.kernel.org/netdev/net-next/c/b5e8eadb1c8a
  - [net-next,5/9] sit: convert 6RD configuration to RCU protection
    https://git.kernel.org/netdev/net-next/c/eaa2098a94cd
  - [net-next,6/9] sit: implement ipip6_get_iflink()
    https://git.kernel.org/netdev/net-next/c/e3434672b7ad
  - [net-next,7/9] sit: dynamically allocate struct ip_tunnel_parm_kern
    https://git.kernel.org/netdev/net-next/c/3cd52b7c0bbf
  - [net-next,8/9] sit: convert configuration to RCU protection
    https://git.kernel.org/netdev/net-next/c/0301127e4d98
  - [net-next,9/9] sit: no longer rely on RTNL in ipip6_fill_info()
    https://git.kernel.org/netdev/net-next/c/f712bf6f17d8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html