[PATCH net-next 00/14] net: annotate data-races around sysctl knobs

[PATCH net-next 00/14] net: annotate data-races around sysctl knobs

[Jiayuan Chen]

While testing the recent sysctl range fixes I noticed that a lot of our
sysctl knobs are read with plain loads while the handlers update them
with WRITE_ONCE() and no lock, so KCSAN reports them as data races. I
then used an LLM to go through every ctl_table under net/ and list the
readers that were left behind. This series annotates them with
READ_ONCE(), and adds WRITE_ONCE() to a few custom handlers and netlink
paths that store to the same fields with plain writes. Most of it is a
plain READ_ONCE() at each read; where a function reads the same knob
twice to make one decision, it now reads it once into a local, so the
comparison and the value it acts on cannot come from two different
writes.

There are no Fixes tags. Done one knob per patch this would be well over
a hundred commits, so I grouped them by subsystem and feature instead to
keep the review manageable. If maintainers prefer, I can split it up
further with a Fixes tag on each one, as several smaller series.

No functional change. KCSAN does hit these, e.g. toggling
net.ipv6.icmp.echo_ignore_all while receiving ICMPv6 echo requests:

  BUG: KCSAN: data-race in icmpv6_rcv / proc_dou8vec_minmax

  write (marked) to 0xffffffff8ee2940d of 1 bytes by task 504 on cpu 0:
   proc_dou8vec_minmax+0x1ab/0x1f0
   proc_sys_call_handler+0x282/0x3a0
   proc_sys_write+0x13/0x20
   ...

  read to 0xffffffff8ee2940d of 1 bytes by interrupt on cpu 3:
   icmpv6_rcv+0x99d/0xd40
   ip6_protocol_deliver_rcu+0x787/0x9a0
   ip6_input_finish+0xb0/0x1b0
   ip6_input+0xa8/0x250
   ipv6_rcv+0x14b/0x1c0
   ...

  value changed: 0x01 -> 0x00

Jiayuan Chen (14):
  ipv4: annotate data-races around ip_rt_* sysctls
  inet: frags: annotate data-races around fqdir->timeout and
    fqdir->max_dist
  ipv6: annotate data-races around addrconf sysctls
  ipv6: annotate data-races around icmpv6 reply sysctls
  calipso: annotate data-races around cache sysctls
  sctp: annotate data-races around sctp sysctls
  mptcp: annotate data-races around pernet sysctls
  rds: annotate data-races around core rds sysctls
  netfilter: conntrack: annotate data-races around conntrack sysctls
  netfilter: bridge: annotate data-races around br_netfilter and nf_log
    sysctls
  ipvs: annotate data-races around lblc_expiration and lblcr_expiration
  net/smc: annotate data-races around smc sysctls
  netfilter: conntrack: annotate data-races around timeout and tcp
    sysctls
  netfilter: cttimeout: annotate data-races around the default timeouts

 include/net/ipv6.h                            |   2 +-
 include/net/netfilter/nf_conntrack_acct.h     |   6 +-
 .../net/netfilter/nf_conntrack_timestamp.h    |   2 +-
 net/bridge/br_netfilter_hooks.c               |  20 ++--
 net/bridge/netfilter/ebt_log.c                |   2 +-
 net/ipv4/inet_fragment.c                      |   2 +-
 net/ipv4/ip_fragment.c                        |   6 +-
 net/ipv4/route.c                              |  34 +++---
 net/ipv6/addrconf.c                           |  22 ++--
 net/ipv6/calipso.c                            |   6 +-
 net/ipv6/icmp.c                               |  10 +-
 net/ipv6/ndisc.c                              |   2 +-
 net/ipv6/route.c                              |   8 +-
 net/mptcp/ctrl.c                              |  18 +--
 net/netfilter/ipvs/ip_vs_lblc.c               |   2 +-
 net/netfilter/ipvs/ip_vs_lblcr.c              |   2 +-
 net/netfilter/nf_conntrack_core.c             |   8 +-
 net/netfilter/nf_conntrack_ecache.c           |   2 +-
 net/netfilter/nf_conntrack_expect.c           |   2 +-
 net/netfilter/nf_conntrack_netlink.c          |   2 +-
 net/netfilter/nf_conntrack_proto.c            |   6 +-
 net/netfilter/nf_conntrack_proto_generic.c    |   8 +-
 net/netfilter/nf_conntrack_proto_gre.c        |  20 ++--
 net/netfilter/nf_conntrack_proto_icmp.c       |  12 +-
 net/netfilter/nf_conntrack_proto_icmpv6.c     |  12 +-
 net/netfilter/nf_conntrack_proto_sctp.c       |  13 ++-
 net/netfilter/nf_conntrack_proto_tcp.c        | 110 +++++++++---------
 net/netfilter/nf_conntrack_proto_udp.c        |  24 ++--
 net/netfilter/nf_flow_table_core.c            |   4 +-
 net/netfilter/nf_log_syslog.c                 |   2 +-
 net/rds/recv.c                                |   2 +-
 net/rds/send.c                                |   8 +-
 net/rds/threads.c                             |   4 +-
 net/sctp/associola.c                          |   2 +-
 net/sctp/bind_addr.c                          |   2 +-
 net/sctp/endpointola.c                        |  14 +--
 net/sctp/input.c                              |   2 +-
 net/sctp/sm_make_chunk.c                      |   6 +-
 net/sctp/sm_sideeffect.c                      |   2 +-
 net/sctp/sm_statefuns.c                       |   6 +-
 net/sctp/socket.c                             |  46 ++++----
 net/sctp/sysctl.c                             |  16 +--
 net/sctp/transport.c                          |   6 +-
 net/smc/af_smc.c                              |   8 +-
 net/smc/smc_clc.c                             |   8 +-
 net/smc/smc_core.c                            |   2 +-
 net/smc/smc_tx.c                              |   2 +-
 47 files changed, 259 insertions(+), 246 deletions(-)

-- 
2.43.0

[PATCH net-next 01/14] ipv4: annotate data-races around ip_rt_* sysctls

[Jiayuan Chen]

ip_rt_redirect_number, ip_rt_redirect_load, ip_rt_redirect_silence,
ip_rt_error_cost, ip_rt_error_burst, ip_rt_gc_timeout, ip_rt_min_pmtu,
ip_rt_mtu_expires and ip_rt_min_advmss are read without any lock while
their sysctl handlers update them with WRITE_ONCE(). Add READ_ONCE() to
the readers.

KCSAN reports it when net.ipv4.route.min_adv_mss is changed while TCP
connections are being opened:

  BUG: KCSAN: data-race in do_proc_int_conv / ipv4_default_advmss

  write (marked) to 0xffffffffb0828f70 of 4 bytes by task 715 on cpu 0:
   do_proc_int_conv+0xa9/0xd0
   proc_vec_conv+0x21/0x40
   proc_vec+0x21e/0x6f0
   proc_dointvec+0x16/0x30
   proc_sys_call_handler+0x282/0x3a0
   proc_sys_write+0x13/0x20
   ...

  read to 0xffffffffb0828f70 of 4 bytes by task 719 on cpu 1:
   ipv4_default_advmss+0x98/0x2c0
   __tcp_transmit_skb+0x1df4/0x28f0
   tcp_connect+0x266b/0x3c30
   tcp_v4_connect+0x9a1/0xae0
   __inet_stream_connect+0x1cc/0x760
   inet_stream_connect+0x49/0x80
   ...

  value changed: 0x00000100 -> 0x00000101

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 net/ipv4/route.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index d0a5f6d77094c..83f49b2d4c36a 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -829,7 +829,7 @@ static void __ip_do_redirect(struct rtable *rt, struct sk_buff *skb, struct flow
 				nhc = FIB_RES_NHC(res);
 				update_or_create_fnhe(nhc, fl4->daddr, new_gw,
 						0, false,
-						jiffies + ip_rt_gc_timeout);
+						jiffies + READ_ONCE(ip_rt_gc_timeout));
 			}
 			if (kill_route)
 				WRITE_ONCE(rt->dst.obsolete, DST_OBSOLETE_KILL);
@@ -930,7 +930,7 @@ void ip_rt_send_redirect(struct sk_buff *skb)
 	/* No redirected packets during ip_rt_redirect_silence;
 	 * reset the algorithm.
 	 */
-	if (time_after(jiffies, peer->rate_last + ip_rt_redirect_silence)) {
+	if (time_after(jiffies, peer->rate_last + READ_ONCE(ip_rt_redirect_silence))) {
 		peer->rate_tokens = 0;
 		peer->n_redirects = 0;
 	}
@@ -938,7 +938,7 @@ void ip_rt_send_redirect(struct sk_buff *skb)
 	/* Too many ignored redirects; do not send anything
 	 * set dst.rate_last to the last seen redirected packet.
 	 */
-	if (peer->n_redirects >= ip_rt_redirect_number) {
+	if (peer->n_redirects >= READ_ONCE(ip_rt_redirect_number)) {
 		peer->rate_last = jiffies;
 		goto out_unlock;
 	}
@@ -946,7 +946,7 @@ void ip_rt_send_redirect(struct sk_buff *skb)
 	/* Check for load limit; set rate_last to the latest sent
 	 * redirect.
 	 */
-	backoff = (u64)ip_rt_redirect_load << min(peer->n_redirects, 31U);
+	backoff = (u64)READ_ONCE(ip_rt_redirect_load) << min(peer->n_redirects, 31U);
 	backoff = min_t(u64, backoff, MAX_JIFFY_OFFSET);
 	if (peer->n_redirects == 0 ||
 	    time_after(jiffies, peer->rate_last + (unsigned long)backoff)) {
@@ -956,7 +956,7 @@ void ip_rt_send_redirect(struct sk_buff *skb)
 		peer->rate_last = jiffies;
 		++peer->n_redirects;
 		if (IS_ENABLED(CONFIG_IP_ROUTE_VERBOSE) && log_martians &&
-		    peer->n_redirects == ip_rt_redirect_number)
+		    peer->n_redirects == READ_ONCE(ip_rt_redirect_number))
 			net_warn_ratelimited("host %pI4/if%d ignores redirects for %pI4 to %pI4\n",
 					     &ip_hdr(skb)->saddr, inet_iif(skb),
 					     &ip_hdr(skb)->daddr, &gw);
@@ -1027,13 +1027,16 @@ static int ip_error(struct sk_buff *skb)
 			       l3mdev_master_ifindex_rcu(skb->dev));
 	send = true;
 	if (peer) {
+		int burst = READ_ONCE(ip_rt_error_burst);
+		int cost = READ_ONCE(ip_rt_error_cost);
+
 		now = jiffies;
 		peer->rate_tokens += now - peer->rate_last;
-		if (peer->rate_tokens > ip_rt_error_burst)
-			peer->rate_tokens = ip_rt_error_burst;
+		if (peer->rate_tokens > burst)
+			peer->rate_tokens = burst;
 		peer->rate_last = now;
-		if (peer->rate_tokens >= ip_rt_error_cost)
-			peer->rate_tokens -= ip_rt_error_cost;
+		if (peer->rate_tokens >= cost)
+			peer->rate_tokens -= cost;
 		else
 			send = false;
 	}
@@ -1063,14 +1066,14 @@ static void __ip_rt_update_pmtu(struct rtable *rt, struct flowi4 *fl4, u32 mtu)
 
 	rcu_read_lock();
 	net = dst_dev_net_rcu(dst);
-	if (mtu < net->ipv4.ip_rt_min_pmtu) {
+	if (mtu < READ_ONCE(net->ipv4.ip_rt_min_pmtu)) {
 		lock = true;
-		mtu = min(old_mtu, net->ipv4.ip_rt_min_pmtu);
+		mtu = min(old_mtu, READ_ONCE(net->ipv4.ip_rt_min_pmtu));
 	}
 
 	if (rt->rt_pmtu == mtu && !lock &&
 	    time_before(jiffies, READ_ONCE(dst->expires) -
-				 net->ipv4.ip_rt_mtu_expires / 2))
+				 READ_ONCE(net->ipv4.ip_rt_mtu_expires) / 2))
 		goto out;
 
 	if (fib_lookup(net, fl4, &res, 0) == 0) {
@@ -1086,14 +1089,15 @@ static void __ip_rt_update_pmtu(struct rtable *rt, struct flowi4 *fl4, u32 mtu)
 				if (!nhc)
 					break;
 				update_or_create_fnhe(nhc, fl4->daddr, 0, mtu, lock,
-						      jiffies + net->ipv4.ip_rt_mtu_expires);
+						      jiffies +
+						      READ_ONCE(net->ipv4.ip_rt_mtu_expires));
 			}
 			goto out;
 		}
 #endif /* CONFIG_IP_ROUTE_MULTIPATH */
 		nhc = FIB_RES_NHC(res);
 		update_or_create_fnhe(nhc, fl4->daddr, 0, mtu, lock,
-				      jiffies + net->ipv4.ip_rt_mtu_expires);
+				      jiffies + READ_ONCE(net->ipv4.ip_rt_mtu_expires));
 	}
 out:
 	rcu_read_unlock();
@@ -1367,7 +1371,7 @@ static unsigned int ipv4_default_advmss(const struct dst_entry *dst)
 	rcu_read_lock();
 	net = dst_dev_net_rcu(dst);
 	advmss = max_t(unsigned int, ipv4_mtu(dst) - header_size,
-				   net->ipv4.ip_rt_min_advmss);
+				   READ_ONCE(net->ipv4.ip_rt_min_advmss));
 	rcu_read_unlock();
 
 	return min(advmss, IPV4_MAX_PMTU - header_size);
-- 
2.43.0

Re: [PATCH net-next 01/14] ipv4: annotate data-races around ip_rt_* sysctls

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In __ip_rt_update_pmtu() (net/ipv4/route.c), the patch adds two
  separate READ_ONCE() loads of net->ipv4.ip_rt_min_pmtu.

--

ipv4: annotate data-races around ip_rt_* sysctls

This adds READ_ONCE() to the lockless readers of several ip_rt_* route
sysctls in net/ipv4/route.c. Their sysctl handlers write them with
WRITE_ONCE(). The change was prompted by a KCSAN report on
net.ipv4.route.min_adv_mss in ipv4_default_advmss().

> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index d0a5f6d77094c..83f49b2d4c36a 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c

[ ... ]

> @@ -1063,14 +1066,14 @@ static void __ip_rt_update_pmtu(struct rtable *rt, struct flowi4 *fl4, u32 mtu)
>  
>  	rcu_read_lock();
>  	net = dst_dev_net_rcu(dst);
> -	if (mtu < net->ipv4.ip_rt_min_pmtu) {
> +	if (mtu < READ_ONCE(net->ipv4.ip_rt_min_pmtu)) {
>  		lock = true;
> -		mtu = min(old_mtu, net->ipv4.ip_rt_min_pmtu);
> +		mtu = min(old_mtu, READ_ONCE(net->ipv4.ip_rt_min_pmtu));

[Severity: Low]
READ_ONCE() is volatile, so __ip_rt_update_pmtu() now always does two
separate loads of ip_rt_min_pmtu. If net.ipv4.route.min_pmtu is written
between them, can the check and the clamp see different values?

For example, take old_mtu=1500 and an ICMP FRAG_NEEDED carrying mtu=300:

  first load sees min_pmtu=552   -> lock = true
  sysctl write sets min_pmtu=68  (accepted, extra1 is ip_min_valid_pmtu)
  second load sees 68            -> mtu = min(1500, 68) = 68

update_or_create_fnhe() would then store pmtu=68 with fnhe_mtu_locked set,
and it stays that way for ip_rt_mtu_expires. The old setting would give
552 locked, and the new setting would give 300 unlocked.

Before this patch, the compiler could merge the two plain loads into one.
Only a privileged writer can trigger this, because min_pmtu is not exported
to netns owned by non-init user namespaces. Later patches in the series
don't seem to change this.

Could the value be read once into a local? This patch already does that
in ip_error() with burst and cost.

A smaller case of the same pattern affects ip_rt_mtu_expires in the same
function. It is re-read for the freshness check:

	    time_before(jiffies, READ_ONCE(dst->expires) -
				 READ_ONCE(net->ipv4.ip_rt_mtu_expires) / 2))

It is read again for each nexthop in the multipath loop, and once more for
the single-path update_or_create_fnhe() call. Could nexthops updated in
one call get different expiries?

ip_rt_redirect_number is also read twice in ip_rt_send_redirect():

	if (peer->n_redirects >= READ_ONCE(ip_rt_redirect_number)) {
	...
		    peer->n_redirects == READ_ONCE(ip_rt_redirect_number))

The second read only gates the "ignores redirects" warning, so there the
only effect is on logging. Should these be loaded into locals once as
well?

>  	}
>  
>  	if (rt->rt_pmtu == mtu && !lock &&

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109025425.328445-1-jiayuan.chen%40linux.dev

[PATCH net-next 02/14] inet: frags: annotate data-races around fqdir->timeout and fqdir->max_dist

[Jiayuan Chen]

fqdir->timeout and fqdir->max_dist are read without any lock while their
sysctl handlers update them with WRITE_ONCE(). Add READ_ONCE() to the
readers.

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 net/ipv4/inet_fragment.c | 2 +-
 net/ipv4/ip_fragment.c   | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 0b8d22ffafca6..ba10aec88cc9b 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -415,7 +415,7 @@ static struct inet_frag_queue *inet_frag_create(struct fqdir *fqdir,
 		inet_frag_putn(q, 2);
 		return NULL;
 	}
-	mod_timer(&q->timer, jiffies + fqdir->timeout);
+	mod_timer(&q->timer, jiffies + READ_ONCE(fqdir->timeout));
 	spin_unlock_bh(&q->lock);
 	return q;
 }
diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 36105dd192823..e70ef3fe5e331 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -89,7 +89,7 @@ static void ip4_frag_init(struct inet_frag_queue *q, const void *a)
 
 	q->key.v4 = *key;
 	qp->ecn = 0;
-	if (q->fqdir->max_dist) {
+	if (READ_ONCE(q->fqdir->max_dist)) {
 		rcu_read_lock();
 		p = inet_getpeer_v4(net->ipv4.peers, key->saddr, key->vif);
 		if (p && !refcount_inc_not_zero(&p->refcnt))
@@ -220,7 +220,7 @@ static struct ipq *ip_find(struct net *net, struct iphdr *iph,
 static int ip_frag_too_far(struct ipq *qp)
 {
 	struct inet_peer *peer = qp->peer;
-	unsigned int max = qp->q.fqdir->max_dist;
+	unsigned int max = READ_ONCE(qp->q.fqdir->max_dist);
 	unsigned int start, end;
 
 	int rc;
@@ -242,7 +242,7 @@ static int ip_frag_too_far(struct ipq *qp)
 
 static int ip_frag_reinit(struct ipq *qp)
 {
-	if (!mod_timer_pending(&qp->q.timer, jiffies + qp->q.fqdir->timeout))
+	if (!mod_timer_pending(&qp->q.timer, jiffies + READ_ONCE(qp->q.fqdir->timeout)))
 		return -ETIMEDOUT;
 
 	inet_frag_queue_flush(&qp->q, SKB_DROP_REASON_FRAG_TOO_FAR);
-- 
2.43.0

[PATCH net-next 03/14] ipv6: annotate data-races around addrconf sysctls

[Jiayuan Chen]

The forwarding setting of conf/all, the per-device autoconf, accept_ra,
accept_ra_min_lft, addr_gen_mode, ra_defrtr_metric, disable_ipv6 and mtu6
settings, and the per-netns idgen_retries and idgen_delay, can be read
concurrently with the sysctl handlers that update them with WRITE_ONCE().
Add READ_ONCE() to the readers.

KCSAN reports it when net.ipv6.conf.<dev>.autoconf is changed while
Router Advertisements are received on the device:

  BUG: KCSAN: data-race in addrconf_prefix_rcv / do_proc_int_conv

  write (marked) to 0xffff8d7250116df4 of 4 bytes by task 767 on cpu 2:
   do_proc_int_conv+0xa9/0xd0
   proc_vec_conv+0x21/0x40
   proc_vec+0x21e/0x6f0
   proc_dointvec+0x16/0x30
   proc_sys_call_handler+0x282/0x3a0
   proc_sys_write+0x13/0x20
   ...

  read to 0xffff8d7250116df4 of 4 bytes by interrupt on cpu 1:
   addrconf_prefix_rcv+0x474/0xf00
   ndisc_router_discovery+0xf4f/0x1f80
   ndisc_rcv+0x2a2/0x3c0
   icmpv6_rcv+0xb46/0xd40
   ip6_protocol_deliver_rcu+0x787/0x9a0
   ip6_input_finish+0xb0/0x1b0
   ip6_input+0xa8/0x250
   ip6_mc_input+0x249/0x780
   ipv6_rcv+0x1b1/0x1c0
   __netif_receive_skb_one_core+0x8f/0xe0
   ...

  value changed: 0x00000001 -> 0x00000000

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 net/ipv6/addrconf.c | 22 ++++++++++++----------
 net/ipv6/ndisc.c    |  2 +-
 net/ipv6/route.c    |  8 ++++----
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index d9d13e78bf13b..40f94124641d4 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1104,7 +1104,7 @@ ipv6_add_addr(struct inet6_dev *idev, struct ifa6_config *cfg,
 		goto out;
 	}
 
-	if (idev->cnf.disable_ipv6) {
+	if (READ_ONCE(idev->cnf.disable_ipv6)) {
 		NL_SET_ERR_MSG_MOD(extack, "IPv6 is disabled on this device");
 		err = -EACCES;
 		goto out;
@@ -2215,7 +2215,7 @@ void addrconf_dad_failure(struct sk_buff *skb, struct inet6_ifaddr *ifp)
 			.scope = ifp->scope,
 		};
 
-		if (retries > net->ipv6.sysctl.idgen_retries) {
+		if (retries > READ_ONCE(net->ipv6.sysctl.idgen_retries)) {
 			net_info_ratelimited("%s: privacy stable address generation failed because of DAD conflicts!\n",
 					     ifp->idev->dev->name);
 			goto errdad;
@@ -2248,7 +2248,7 @@ void addrconf_dad_failure(struct sk_buff *skb, struct inet6_ifaddr *ifp)
 		spin_unlock_bh(&ifp2->lock);
 
 		if (start_dad)
-			addrconf_mod_dad_work(ifp2, net->ipv6.sysctl.idgen_delay);
+			addrconf_mod_dad_work(ifp2, READ_ONCE(net->ipv6.sysctl.idgen_delay));
 		in6_ifa_put(ifp2);
 lock_errdad:
 		spin_lock_bh(&ifp->lock);
@@ -2706,8 +2706,10 @@ static void manage_tempaddrs(struct inet6_dev *idev,
 
 static bool is_addr_mode_generate_stable(struct inet6_dev *idev)
 {
-	return idev->cnf.addr_gen_mode == IN6_ADDR_GEN_MODE_STABLE_PRIVACY ||
-	       idev->cnf.addr_gen_mode == IN6_ADDR_GEN_MODE_RANDOM;
+	u32 mode = READ_ONCE(idev->cnf.addr_gen_mode);
+
+	return mode == IN6_ADDR_GEN_MODE_STABLE_PRIVACY ||
+	       mode == IN6_ADDR_GEN_MODE_RANDOM;
 }
 
 int addrconf_prefix_rcv_add_addr(struct net *net, struct net_device *dev,
@@ -2735,7 +2737,7 @@ int addrconf_prefix_rcv_add_addr(struct net *net, struct net_device *dev,
 #ifdef CONFIG_IPV6_OPTIMISTIC_DAD
 		if ((READ_ONCE(net->ipv6.devconf_all->optimistic_dad) ||
 		     READ_ONCE(in6_dev->cnf.optimistic_dad)) &&
-		    !net->ipv6.devconf_all->forwarding && sllao)
+		    !READ_ONCE(net->ipv6.devconf_all->forwarding) && sllao)
 			cfg.ifa_flags |= IFA_F_OPTIMISTIC;
 #endif
 
@@ -2857,7 +2859,7 @@ void addrconf_prefix_rcv(struct net_device *dev, u8 *opt, int len, bool sllao)
 		return;
 	}
 
-	if (valid_lft != 0 && valid_lft < in6_dev->cnf.accept_ra_min_lft)
+	if (valid_lft != 0 && valid_lft < READ_ONCE(in6_dev->cnf.accept_ra_min_lft))
 		goto put;
 
 	/*
@@ -2928,7 +2930,7 @@ void addrconf_prefix_rcv(struct net_device *dev, u8 *opt, int len, bool sllao)
 	/* Try to figure out our local address for this prefix */
 
 	ignore_autoconf = READ_ONCE(in6_dev->cnf.ra_honor_pio_pflag) && pinfo->preferpd;
-	if (pinfo->autoconf && in6_dev->cnf.autoconf && !ignore_autoconf) {
+	if (pinfo->autoconf && READ_ONCE(in6_dev->cnf.autoconf) && !ignore_autoconf) {
 		struct in6_addr addr;
 		bool tokenized = false, dev_addr_generated = false;
 
@@ -3434,7 +3436,7 @@ static int ipv6_generate_stable_address(struct in6_addr *address,
 
 	if (ipv6_reserved_interfaceid(temp)) {
 		dad_count++;
-		if (dad_count > dev_net(idev->dev)->ipv6.sysctl.idgen_retries)
+		if (dad_count > READ_ONCE(dev_net(idev->dev)->ipv6.sysctl.idgen_retries))
 			return -1;
 		goto retry;
 	}
@@ -3805,7 +3807,7 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 			 * when the interface up, the changed MTU must be
 			 * reflected in the idev as well as routers.
 			 */
-			if (idev->cnf.mtu6 != dev->mtu &&
+			if (READ_ONCE(idev->cnf.mtu6) != dev->mtu &&
 			    dev->mtu >= IPV6_MIN_MTU) {
 				rt6_mtu_change(dev, dev->mtu);
 				WRITE_ONCE(idev->cnf.mtu6, dev->mtu);
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 0ed1a619372bf..b69e6d66e8b1a 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1375,7 +1375,7 @@ static enum skb_drop_reason ndisc_router_discovery(struct sk_buff *skb)
 		}
 	}
 	/* Set default route metric as specified by user */
-	defrtr_usr_metric = in6_dev->cnf.ra_defrtr_metric;
+	defrtr_usr_metric = READ_ONCE(in6_dev->cnf.ra_defrtr_metric);
 	/* delete the route if lifetime is 0 or if metric needs change */
 	if (rt && (lifetime == 0 || rt->fib6_metric != defrtr_usr_metric)) {
 		ip6_del_rt_reason(net, rt,
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 04098cf32eabe..91a003417697c 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2065,7 +2065,7 @@ static bool rt6_mtu_change_route_allowed(struct inet6_dev *idev,
 	if (dmtu >= mtu)
 		return true;
 
-	if (dmtu == idev->cnf.mtu6)
+	if (dmtu == READ_ONCE(idev->cnf.mtu6))
 		return true;
 
 	return false;
@@ -3666,7 +3666,7 @@ int fib6_nh_init(struct net *net, struct fib6_nh *fib6_nh,
 	if (!dev)
 		goto out;
 
-	if (!idev || idev->cnf.disable_ipv6) {
+	if (!idev || READ_ONCE(idev->cnf.disable_ipv6)) {
 		NL_SET_ERR_MSG(extack, "IPv6 is disabled on nexthop device");
 		err = -EACCES;
 		goto out;
@@ -4533,7 +4533,7 @@ static void __rt6_purge_dflt_routers(struct net *net,
 		struct inet6_dev *idev = dev ? __in6_dev_get(dev) : NULL;
 
 		if (rt->fib6_flags & (RTF_DEFAULT | RTF_ADDRCONF) &&
-		    (!idev || idev->cnf.accept_ra != 2) &&
+		    (!idev || READ_ONCE(idev->cnf.accept_ra) != 2) &&
 		    fib6_info_hold_safe(rt)) {
 			rcu_read_unlock();
 			ip6_del_rt(net, rt, false);
@@ -5075,7 +5075,7 @@ static int fib6_nh_mtu_change(struct fib6_nh *nh, void *_arg)
 			return 0;
 
 		if (mtu >= arg->mtu ||
-		    (mtu < arg->mtu && mtu == idev->cnf.mtu6))
+		    (mtu < arg->mtu && mtu == READ_ONCE(idev->cnf.mtu6)))
 			fib6_metric_set(f6i, RTAX_MTU, arg->mtu);
 
 		spin_lock_bh(&rt6_exception_lock);
-- 
2.43.0

[PATCH net-next 04/14] ipv6: annotate data-races around icmpv6 reply sysctls

[Jiayuan Chen]

icmpv6_echo_ignore_all, icmpv6_echo_ignore_multicast,
icmpv6_echo_ignore_anycast, anycast_src_echo_reply and fwmark_reflect are
read without any lock while their sysctl handlers update them with
WRITE_ONCE(). Add READ_ONCE() to the readers.

KCSAN reports it when net.ipv6.icmp.echo_ignore_all is changed while
ICMPv6 echo requests are received:

  BUG: KCSAN: data-race in icmpv6_rcv / proc_dou8vec_minmax

  write (marked) to 0xffffffff8ee2940d of 1 bytes by task 504 on cpu 0:
   proc_dou8vec_minmax+0x1ab/0x1f0
   proc_sys_call_handler+0x282/0x3a0
   proc_sys_write+0x13/0x20
   ...

  read to 0xffffffff8ee2940d of 1 bytes by interrupt on cpu 3:
   icmpv6_rcv+0x99d/0xd40
   ip6_protocol_deliver_rcu+0x787/0x9a0
   ip6_input_finish+0xb0/0x1b0
   ip6_input+0xa8/0x250
   ipv6_rcv+0x14b/0x1c0
   __netif_receive_skb_one_core+0x8f/0xe0
   ...

  value changed: 0x01 -> 0x00

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 include/net/ipv6.h |  2 +-
 net/ipv6/icmp.c    | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 579df1516d251..b77dc5c3ea268 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -199,7 +199,7 @@ struct sk_buff *ip6_frag_next(struct sk_buff *skb,
 			      struct ip6_frag_state *state);
 
 #define IP6_REPLY_MARK(net, mark) \
-	((net)->ipv6.sysctl.fwmark_reflect ? (mark) : 0)
+	(READ_ONCE((net)->ipv6.sysctl.fwmark_reflect) ? (mark) : 0)
 
 #include <net/sock.h>
 
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index a95b0351824f3..3e95b1191c4f2 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -938,17 +938,17 @@ static enum skb_drop_reason icmpv6_echo_reply(struct sk_buff *skb)
 	u8 type;
 
 	if (ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr) &&
-	    net->ipv6.sysctl.icmpv6_echo_ignore_multicast)
+	    READ_ONCE(net->ipv6.sysctl.icmpv6_echo_ignore_multicast))
 		return reason;
 
 	saddr = &ipv6_hdr(skb)->daddr;
 
 	acast = ipv6_anycast_destination(skb_dst(skb), saddr);
-	if (acast && net->ipv6.sysctl.icmpv6_echo_ignore_anycast)
+	if (acast && READ_ONCE(net->ipv6.sysctl.icmpv6_echo_ignore_anycast))
 		return reason;
 
 	if (!ipv6_unicast_destination(skb) &&
-	    !(net->ipv6.sysctl.anycast_src_echo_reply && acast))
+	    !(READ_ONCE(net->ipv6.sysctl.anycast_src_echo_reply) && acast))
 		saddr = NULL;
 
 	if (icmph->icmp6_type == ICMPV6_EXT_ECHO_REQUEST)
@@ -1152,11 +1152,11 @@ static int icmpv6_rcv(struct sk_buff *skb)
 
 	switch (type) {
 	case ICMPV6_ECHO_REQUEST:
-		if (!net->ipv6.sysctl.icmpv6_echo_ignore_all)
+		if (!READ_ONCE(net->ipv6.sysctl.icmpv6_echo_ignore_all))
 			reason = icmpv6_echo_reply(skb);
 		break;
 	case ICMPV6_EXT_ECHO_REQUEST:
-		if (!net->ipv6.sysctl.icmpv6_echo_ignore_all &&
+		if (!READ_ONCE(net->ipv6.sysctl.icmpv6_echo_ignore_all) &&
 		    READ_ONCE(net->ipv4.sysctl_icmp_echo_enable_probe))
 			reason = icmpv6_echo_reply(skb);
 		break;
-- 
2.43.0

[PATCH net-next 05/14] calipso: annotate data-races around cache sysctls

[Jiayuan Chen]

calipso_cache_enabled and calipso_cache_bucketsize are read without any
lock while their sysctl handlers update them with WRITE_ONCE(). Add
READ_ONCE() to the readers.

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 net/ipv6/calipso.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
index e399a42e30839..5e63212b60163 100644
--- a/net/ipv6/calipso.c
+++ b/net/ipv6/calipso.c
@@ -204,7 +204,7 @@ static int calipso_cache_check(const unsigned char *key,
 	struct calipso_map_cache_entry *prev_entry = NULL;
 	u32 hash;
 
-	if (!calipso_cache_enabled)
+	if (!READ_ONCE(calipso_cache_enabled))
 		return -ENOENT;
 
 	hash = calipso_map_cache_hash(key, key_len);
@@ -269,7 +269,7 @@ static int calipso_cache_add(const unsigned char *calipso_ptr,
 	struct calipso_map_cache_entry *old_entry = NULL;
 	u32 calipso_ptr_len;
 
-	if (!calipso_cache_enabled || calipso_cache_bucketsize <= 0)
+	if (!READ_ONCE(calipso_cache_enabled) || READ_ONCE(calipso_cache_bucketsize) <= 0)
 		return 0;
 
 	calipso_ptr_len = calipso_ptr[1];
@@ -289,7 +289,7 @@ static int calipso_cache_add(const unsigned char *calipso_ptr,
 
 	bkt = entry->hash & (CALIPSO_CACHE_BUCKETS - 1);
 	spin_lock_bh(&calipso_cache[bkt].lock);
-	if (calipso_cache[bkt].size < calipso_cache_bucketsize) {
+	if (calipso_cache[bkt].size < READ_ONCE(calipso_cache_bucketsize)) {
 		list_add(&entry->list, &calipso_cache[bkt].list);
 		calipso_cache[bkt].size += 1;
 	} else {
-- 
2.43.0

Re: [PATCH net-next 05/14] calipso: annotate data-races around cache sysctls

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] calipso_cache_add() in net/ipv6/calipso.c still reads
  calipso_cache_bucketsize twice, and the two reads can disagree.

--

calipso: annotate data-races around cache sysctls

This commit adds READ_ONCE() to the lockless readers of
calipso_cache_enabled and calipso_cache_bucketsize in net/ipv6/calipso.c.
The sysctl handlers update these variables with WRITE_ONCE().

> diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
> index e399a42e30839..5e63212b60163 100644
> --- a/net/ipv6/calipso.c
> +++ b/net/ipv6/calipso.c

[ ... ]

> @@ -269,7 +269,7 @@ static int calipso_cache_add(const unsigned char *calipso_ptr,
>  	struct calipso_map_cache_entry *old_entry = NULL;
>  	u32 calipso_ptr_len;
>  
> -	if (!calipso_cache_enabled || calipso_cache_bucketsize <= 0)
> +	if (!READ_ONCE(calipso_cache_enabled) || READ_ONCE(calipso_cache_bucketsize) <= 0)
>  		return 0;
>  
>  	calipso_ptr_len = calipso_ptr[1];
> @@ -289,7 +289,7 @@ static int calipso_cache_add(const unsigned char *calipso_ptr,
>  
>  	bkt = entry->hash & (CALIPSO_CACHE_BUCKETS - 1);
>  	spin_lock_bh(&calipso_cache[bkt].lock);
> -	if (calipso_cache[bkt].size < calipso_cache_bucketsize) {
> +	if (calipso_cache[bkt].size < READ_ONCE(calipso_cache_bucketsize)) {
>  		list_add(&entry->list, &calipso_cache[bkt].list);
>  		calipso_cache[bkt].size += 1;
>  	} else {

[Severity: High]
Can these two separate READ_ONCE() loads of calipso_cache_bucketsize in
calipso_cache_add() return different values?

The first load is unlocked and only guards the early return. The second
load comes later, under the bucket spinlock, after the GFP_ATOMIC
allocations, jhash() and refcount_inc().

The bucket lock does not cover the sysctl. Also, calipso_cache_bucket_size
in net/ipv6/sysctl_net_ipv6.c uses plain proc_dointvec with no
extra1/extra2 bounds, so 0 or a negative value can be written during that
window.

Suppose three things happen together:

  - the first load sees a positive value
  - a concurrent write of 0 lands before the second load
  - the hashed bucket is empty (size == 0)

Then the size check is false, and the eviction branch runs on an empty
list:

    old_entry = list_entry(calipso_cache[bkt].list.prev,
                           struct calipso_map_cache_entry, list);
    list_del(&old_entry->list);
    list_add(&entry->list, &calipso_cache[bkt].list);
    calipso_cache_entry_free(old_entry);

Here old_entry is container_of() applied to the bucket's own list head.
list_del() poisons the head, and list_add() then dereferences
LIST_POISON1.

Wouldn't that oops in softirq context with the bucket lock held?

If execution got past that point, calipso_cache_entry_free() would free
whatever sits at the key and lsm_data offsets of the fake entry. Those
offsets overlap neighbouring buckets. It would then kfree() a pointer into
the middle of the calipso_cache array.

Separately, if only the second load sees a negative value, it is
converted to a huge u32 limit and the bucket size cap is skipped.

This window existed before this patch, since spin_lock_bh() already forced
a reload. However, this commit is meant to fix the racy reads of this
exact variable. Could it take a single snapshot instead? That is what
cipso_v4_cache_add() has done since commit dd44f04b9214ad ("cipso: Fix
data-races around sysctl."):

net/ipv4/cipso_ipv4.c:cipso_v4_cache_add() {
	int bkt_size = READ_ONCE(cipso_v4_cache_bucketsize);
	...
	if (!READ_ONCE(cipso_v4_cache_enabled) || bkt_size <= 0)
		return 0;
	...
	if (cipso_v4_cache[bkt].size < bkt_size) {
	...
}

No later patch in the series changes net/ipv6/calipso.c.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109025425.328445-1-jiayuan.chen%40linux.dev

[PATCH net-next 06/14] sctp: annotate data-races around sctp sysctls

[Jiayuan Chen]

rto_min, rto_max, rto_initial, hb_interval, sack_timeout,
valid_cookie_life, max_burst, max_retrans_path, max_retrans_association,
max_retrans_init, pf_retrans, ps_retrans, pf_expose, pf_enable, udp_port,
encap_port, probe_interval, addip_enable, addip_noauth, auth_enable,
default_auto_asconf, prsctp_enable, reconf_enable, ecn_enable,
intl_enable, cookie_auth_enable, cookie_preserve_enable, sndbuf_policy,
rcvbuf_policy, rwnd_upd_shift, scope_policy and max_autoclose can be
read concurrently with the sysctl handlers that update them, and the
rto_min, rto_max, udp_port, probe_interval, auth_enable and
cookie_hmac_alg handlers store them with plain writes. Add READ_ONCE() to
the readers and WRITE_ONCE() to those handlers.

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 net/sctp/associola.c     |  2 +-
 net/sctp/bind_addr.c     |  2 +-
 net/sctp/endpointola.c   | 14 ++++++------
 net/sctp/input.c         |  2 +-
 net/sctp/sm_make_chunk.c |  6 +++---
 net/sctp/sm_sideeffect.c |  2 +-
 net/sctp/sm_statefuns.c  |  6 +++---
 net/sctp/socket.c        | 46 +++++++++++++++++++++-------------------
 net/sctp/sysctl.c        | 16 +++++++-------
 net/sctp/transport.c     |  6 +++---
 10 files changed, 52 insertions(+), 50 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 4521be3bd85aa..0a814faaea601 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1454,7 +1454,7 @@ static inline bool sctp_peer_needs_update(struct sctp_association *asoc)
 	case SCTP_STATE_SHUTDOWN_SENT:
 		if ((asoc->rwnd > asoc->a_rwnd) &&
 		    ((asoc->rwnd - asoc->a_rwnd) >= max_t(__u32,
-			   (asoc->base.sk->sk_rcvbuf >> net->sctp.rwnd_upd_shift),
+			   (asoc->base.sk->sk_rcvbuf >> READ_ONCE(net->sctp.rwnd_upd_shift)),
 			   asoc->pathmtu)))
 			return true;
 		break;
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index 31737f144c7f0..36cea9ae2ad18 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -530,7 +530,7 @@ int sctp_in_scope(struct net *net, const union sctp_addr *addr,
 	 * Address scoping can be selectively controlled via sysctl
 	 * option
 	 */
-	switch (net->sctp.scope_policy) {
+	switch (READ_ONCE(net->sctp.scope_policy)) {
 	case SCTP_SCOPE_POLICY_DISABLE:
 		return 1;
 	case SCTP_SCOPE_POLICY_ENABLE:
diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
index a15b599b20b76..49d9c5c0babc1 100644
--- a/net/sctp/endpointola.c
+++ b/net/sctp/endpointola.c
@@ -54,8 +54,8 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
 	struct net *net = sock_net(sk);
 	struct sctp_shared_key *null_key;
 
-	ep->asconf_enable = net->sctp.addip_enable;
-	ep->auth_enable = net->sctp.auth_enable;
+	ep->asconf_enable = READ_ONCE(net->sctp.addip_enable);
+	ep->auth_enable = READ_ONCE(net->sctp.auth_enable);
 	if (ep->auth_enable) {
 		if (sctp_auth_init(ep, gfp))
 			goto nomem;
@@ -86,14 +86,14 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
 	INIT_LIST_HEAD(&ep->asocs);
 
 	/* Use SCTP specific send buffer space queues.  */
-	ep->sndbuf_policy = net->sctp.sndbuf_policy;
+	ep->sndbuf_policy = READ_ONCE(net->sctp.sndbuf_policy);
 
 	sk->sk_data_ready = sctp_data_ready;
 	sk->sk_write_space = sctp_write_space;
 	sock_set_flag(sk, SOCK_USE_WRITE_QUEUE);
 
 	/* Get the receive buffer policy for this endpoint */
-	ep->rcvbuf_policy = net->sctp.rcvbuf_policy;
+	ep->rcvbuf_policy = READ_ONCE(net->sctp.rcvbuf_policy);
 
 	/* Generate the cookie authentication key. */
 	gen_cookie_auth_key(&ep->cookie_auth_key);
@@ -109,9 +109,9 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
 	/* Add the null key to the endpoint shared keys list and
 	 * set the hmcas and chunks pointers.
 	 */
-	ep->prsctp_enable = net->sctp.prsctp_enable;
-	ep->reconf_enable = net->sctp.reconf_enable;
-	ep->ecn_enable = net->sctp.ecn_enable;
+	ep->prsctp_enable = READ_ONCE(net->sctp.prsctp_enable);
+	ep->reconf_enable = READ_ONCE(net->sctp.reconf_enable);
+	ep->ecn_enable = READ_ONCE(net->sctp.ecn_enable);
 
 	/* Remember who we are attached to.  */
 	ep->base.sk = sk;
diff --git a/net/sctp/input.c b/net/sctp/input.c
index 9494cfa51106c..ecff82c0d5539 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -1276,7 +1276,7 @@ static struct sctp_association *__sctp_rcv_walk_lookup(struct net *net,
 			break;
 
 		case SCTP_CID_ASCONF:
-			if (have_auth || net->sctp.addip_noauth)
+			if (have_auth || READ_ONCE(net->sctp.addip_noauth))
 				asoc = __sctp_rcv_asconf_lookup(
 						net, ch, laddr,
 						sctp_hdr(skb)->source,
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 90de272842512..43a757d4a69e7 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -2038,7 +2038,7 @@ static int sctp_verify_ext_param(struct net *net,
 	 * only if ADD-IP is turned on and we are not backward-compatible
 	 * mode.
 	 */
-	if (net->sctp.addip_noauth)
+	if (READ_ONCE(net->sctp.addip_noauth))
 		return 1;
 
 	if (ep->asconf_enable && !have_auth && have_asconf)
@@ -2425,7 +2425,7 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,
 	 * also give us an option to silently ignore the packet, which
 	 * is what we'll do here.
 	 */
-	if (!asoc->base.net->sctp.addip_noauth &&
+	if (!READ_ONCE(asoc->base.net->sctp.addip_noauth) &&
 	    (asoc->peer.asconf_capable && !asoc->peer.auth_capable)) {
 		asoc->peer.addip_disabled_mask |= (SCTP_PARAM_ADD_IP |
 						  SCTP_PARAM_DEL_IP |
@@ -2583,7 +2583,7 @@ static int sctp_process_param(struct sctp_association *asoc,
 		break;
 
 	case SCTP_PARAM_COOKIE_PRESERVATIVE:
-		if (!net->sctp.cookie_preserve_enable)
+		if (!READ_ONCE(net->sctp.cookie_preserve_enable))
 			break;
 
 		stale = ntohl(param.life->lifespan_increment);
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 35f540fb15fc1..01ee6c3dd441a 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -591,7 +591,7 @@ static void sctp_do_8_2_transport_strike(struct sctp_cmd_seq *commands,
 	 * is SCTP_ACTIVE, then mark this transport as Partially Failed,
 	 * see SCTP Quick Failover Draft, section 5.1
 	 */
-	if (asoc->base.net->sctp.pf_enable &&
+	if (READ_ONCE(asoc->base.net->sctp.pf_enable) &&
 	    transport->state == SCTP_ACTIVE &&
 	    transport->error_count < transport->pathmaxrxt &&
 	    transport->error_count > transport->pf_retrans) {
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index b2f609f177fb9..b6b95972eea9a 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -653,7 +653,7 @@ static bool sctp_auth_chunk_verify(struct net *net, struct sctp_chunk *chunk,
 	 */
 
 	/* Make sure that we and the peer are AUTH capable */
-	if (!net->sctp.auth_enable || !asoc->peer.auth_capable)
+	if (!READ_ONCE(net->sctp.auth_enable) || !asoc->peer.auth_capable)
 		return false;
 
 	/* set-up our fake chunk so that we can process it */
@@ -3944,7 +3944,7 @@ enum sctp_disposition sctp_sf_do_asconf(struct net *net,
 	 * described in [I-D.ietf-tsvwg-sctp-auth].
 	 */
 	if (!asoc->peer.asconf_capable ||
-	    (!net->sctp.addip_noauth && !chunk->auth))
+	    (!READ_ONCE(net->sctp.addip_noauth) && !chunk->auth))
 		return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
 
 	hdr = (struct sctp_addiphdr *)chunk->skb->data;
@@ -4087,7 +4087,7 @@ enum sctp_disposition sctp_sf_do_asconf_ack(struct net *net,
 	 * described in [I-D.ietf-tsvwg-sctp-auth].
 	 */
 	if (!asoc->peer.asconf_capable ||
-	    (!net->sctp.addip_noauth && !asconf_ack->auth))
+	    (!READ_ONCE(net->sctp.addip_noauth) && !asconf_ack->auth))
 		return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
 
 	addip_hdr = (struct sctp_addiphdr *)asconf_ack->skb->data;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index b5a57f61ee6d1..715ecc25b2866 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -403,7 +403,7 @@ static void sctp_auto_asconf_init(struct sctp_sock *sp)
 {
 	struct net *net = sock_net(&sp->inet.sk);
 
-	if (net->sctp.default_auto_asconf) {
+	if (READ_ONCE(net->sctp.default_auto_asconf)) {
 		spin_lock_bh(&net->sctp.addr_wq_lock);
 		list_add_tail(&sp->auto_asconf_list, &net->sctp.auto_asconf_splist);
 		spin_unlock_bh(&net->sctp.addr_wq_lock);
@@ -2326,6 +2326,7 @@ static int sctp_setsockopt_autoclose(struct sock *sk, u32 *optval,
 {
 	struct sctp_sock *sp = sctp_sk(sk);
 	struct net *net = sock_net(sk);
+	unsigned long max_autoclose;
 
 	/* Applicable to UDP-style socket only */
 	if (sctp_style(sk, TCP))
@@ -2333,9 +2334,10 @@ static int sctp_setsockopt_autoclose(struct sock *sk, u32 *optval,
 	if (optlen != sizeof(int))
 		return -EINVAL;
 
+	max_autoclose = READ_ONCE(net->sctp.max_autoclose);
 	sp->autoclose = *optval;
-	if (sp->autoclose > net->sctp.max_autoclose)
-		sp->autoclose = net->sctp.max_autoclose;
+	if (sp->autoclose > max_autoclose)
+		sp->autoclose = max_autoclose;
 
 	return 0;
 }
@@ -4296,7 +4298,7 @@ static int sctp_setsockopt_interleaving_supported(struct sock *sk,
 	if (!asoc && p->assoc_id != SCTP_FUTURE_ASSOC && sctp_style(sk, UDP))
 		return -EINVAL;
 
-	if (!sock_net(sk)->sctp.intl_enable || !sp->frag_interleave) {
+	if (!READ_ONCE(sock_net(sk)->sctp.intl_enable) || !sp->frag_interleave) {
 		return -EPERM;
 	}
 
@@ -5098,9 +5100,9 @@ static int sctp_init_sock(struct sock *sk)
 	sp->default_timetolive = 0;
 
 	sp->default_rcv_context = 0;
-	sp->max_burst = net->sctp.max_burst;
+	sp->max_burst = READ_ONCE(net->sctp.max_burst);
 
-	sp->cookie_auth_enable = net->sctp.cookie_auth_enable;
+	sp->cookie_auth_enable = READ_ONCE(net->sctp.cookie_auth_enable);
 
 	/* Initialize default setup parameters. These parameters
 	 * can be modified with the SCTP_INITMSG socket option or
@@ -5108,24 +5110,24 @@ static int sctp_init_sock(struct sock *sk)
 	 */
 	sp->initmsg.sinit_num_ostreams   = sctp_max_outstreams;
 	sp->initmsg.sinit_max_instreams  = sctp_max_instreams;
-	sp->initmsg.sinit_max_attempts   = net->sctp.max_retrans_init;
-	sp->initmsg.sinit_max_init_timeo = net->sctp.rto_max;
+	sp->initmsg.sinit_max_attempts   = READ_ONCE(net->sctp.max_retrans_init);
+	sp->initmsg.sinit_max_init_timeo = READ_ONCE(net->sctp.rto_max);
 
 	/* Initialize default RTO related parameters.  These parameters can
 	 * be modified for with the SCTP_RTOINFO socket option.
 	 */
-	sp->rtoinfo.srto_initial = net->sctp.rto_initial;
-	sp->rtoinfo.srto_max     = net->sctp.rto_max;
-	sp->rtoinfo.srto_min     = net->sctp.rto_min;
+	sp->rtoinfo.srto_initial = READ_ONCE(net->sctp.rto_initial);
+	sp->rtoinfo.srto_max     = READ_ONCE(net->sctp.rto_max);
+	sp->rtoinfo.srto_min     = READ_ONCE(net->sctp.rto_min);
 
 	/* Initialize default association related parameters. These parameters
 	 * can be modified with the SCTP_ASSOCINFO socket option.
 	 */
-	sp->assocparams.sasoc_asocmaxrxt = net->sctp.max_retrans_association;
+	sp->assocparams.sasoc_asocmaxrxt = READ_ONCE(net->sctp.max_retrans_association);
 	sp->assocparams.sasoc_number_peer_destinations = 0;
 	sp->assocparams.sasoc_peer_rwnd = 0;
 	sp->assocparams.sasoc_local_rwnd = 0;
-	sp->assocparams.sasoc_cookie_life = net->sctp.valid_cookie_life;
+	sp->assocparams.sasoc_cookie_life = READ_ONCE(net->sctp.valid_cookie_life);
 
 	/* Initialize default event subscriptions. By default, all the
 	 * options are off.
@@ -5135,15 +5137,15 @@ static int sctp_init_sock(struct sock *sk)
 	/* Default Peer Address Parameters.  These defaults can
 	 * be modified via SCTP_PEER_ADDR_PARAMS
 	 */
-	sp->hbinterval  = net->sctp.hb_interval;
-	sp->udp_port    = htons(net->sctp.udp_port);
-	sp->encap_port  = htons(net->sctp.encap_port);
-	sp->pathmaxrxt  = net->sctp.max_retrans_path;
-	sp->pf_retrans  = net->sctp.pf_retrans;
-	sp->ps_retrans  = net->sctp.ps_retrans;
-	sp->pf_expose   = net->sctp.pf_expose;
+	sp->hbinterval  = READ_ONCE(net->sctp.hb_interval);
+	sp->udp_port    = htons(READ_ONCE(net->sctp.udp_port));
+	sp->encap_port  = htons(READ_ONCE(net->sctp.encap_port));
+	sp->pathmaxrxt  = READ_ONCE(net->sctp.max_retrans_path);
+	sp->pf_retrans  = READ_ONCE(net->sctp.pf_retrans);
+	sp->ps_retrans  = READ_ONCE(net->sctp.ps_retrans);
+	sp->pf_expose   = READ_ONCE(net->sctp.pf_expose);
 	sp->pathmtu     = 0; /* allow default discovery */
-	sp->sackdelay   = net->sctp.sack_timeout;
+	sp->sackdelay   = READ_ONCE(net->sctp.sack_timeout);
 	sp->sackfreq	= 2;
 	sp->param_flags = SPP_HB_ENABLE |
 			  SPP_PMTUD_ENABLE |
@@ -5182,7 +5184,7 @@ static int sctp_init_sock(struct sock *sk)
 	atomic_set(&sp->pd_mode, 0);
 	skb_queue_head_init(&sp->pd_lobby);
 	sp->frag_interleave = 0;
-	sp->probe_interval = net->sctp.probe_interval;
+	sp->probe_interval = READ_ONCE(net->sctp.probe_interval);
 
 	/* Create a per socket endpoint structure.  Even if we
 	 * change the data structure relationships, this may still
diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
index 2b94c211427de..8a84ae3272d24 100644
--- a/net/sctp/sysctl.c
+++ b/net/sctp/sysctl.c
@@ -402,11 +402,11 @@ static int proc_sctp_do_hmac_alg(const struct ctl_table *ctl, int write,
 		if (ret)
 			return ret;
 		if (!strcmp(tmp, "sha256")) {
-			net->sctp.cookie_auth_enable = 1;
+			WRITE_ONCE(net->sctp.cookie_auth_enable, 1);
 			return 0;
 		}
 		if (!strcmp(tmp, "none")) {
-			net->sctp.cookie_auth_enable = 0;
+			WRITE_ONCE(net->sctp.cookie_auth_enable, 0);
 			return 0;
 		}
 		return -EINVAL;
@@ -441,7 +441,7 @@ static int proc_sctp_do_rto_min(const struct ctl_table *ctl, int write,
 		if (new_value > max || new_value < min)
 			return -EINVAL;
 
-		net->sctp.rto_min = new_value;
+		WRITE_ONCE(net->sctp.rto_min, new_value);
 	}
 
 	return ret;
@@ -469,7 +469,7 @@ static int proc_sctp_do_rto_max(const struct ctl_table *ctl, int write,
 		if (new_value > max || new_value < min)
 			return -EINVAL;
 
-		net->sctp.rto_max = new_value;
+		WRITE_ONCE(net->sctp.rto_max, new_value);
 	}
 
 	return ret;
@@ -504,7 +504,7 @@ static int proc_sctp_do_auth(const struct ctl_table *ctl, int write,
 	if (write && ret == 0) {
 		struct sock *sk = net->sctp.ctl_sock;
 
-		net->sctp.auth_enable = new_value;
+		WRITE_ONCE(net->sctp.auth_enable, new_value);
 		/* Update the value in the control socket */
 		lock_sock(sk);
 		sctp_sk(sk)->ep->auth_enable = new_value;
@@ -541,12 +541,12 @@ static int proc_sctp_do_udp_port(const struct ctl_table *ctl, int write,
 			return -EINVAL;
 
 		mutex_lock(&sctp_sysctl_mutex);
-		net->sctp.udp_port = new_value;
+		WRITE_ONCE(net->sctp.udp_port, new_value);
 		sctp_udp_sock_stop(net);
 		if (new_value) {
 			ret = sctp_udp_sock_start(net);
 			if (ret)
-				net->sctp.udp_port = 0;
+				WRITE_ONCE(net->sctp.udp_port, 0);
 		}
 
 		/* Update the value in the control socket */
@@ -580,7 +580,7 @@ static int proc_sctp_do_probe_interval(const struct ctl_table *ctl, int write,
 		if (new_value && new_value < SCTP_PROBE_TIMER_MIN)
 			return -EINVAL;
 
-		net->sctp.probe_interval = new_value;
+		WRITE_ONCE(net->sctp.probe_interval, new_value);
 	}
 
 	return ret;
diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index 6ea55b9fbde45..78b87e1b7e64b 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -55,7 +55,7 @@ static void sctp_transport_init(struct net *net,
 	 * given destination transport address, set RTO to the protocol
 	 * parameter 'RTO.Initial'.
 	 */
-	peer->rto = msecs_to_jiffies(net->sctp.rto_initial);
+	peer->rto = msecs_to_jiffies(READ_ONCE(net->sctp.rto_initial));
 
 	peer->last_time_heard = 0;
 	peer->last_time_ecne_reduced = jiffies;
@@ -65,8 +65,8 @@ static void sctp_transport_init(struct net *net,
 			    SPP_SACKDELAY_ENABLE;
 
 	/* Initialize the default path max_retrans.  */
-	peer->pathmaxrxt  = net->sctp.max_retrans_path;
-	peer->pf_retrans  = net->sctp.pf_retrans;
+	peer->pathmaxrxt  = READ_ONCE(net->sctp.max_retrans_path);
+	peer->pf_retrans  = READ_ONCE(net->sctp.pf_retrans);
 
 	INIT_LIST_HEAD(&peer->transmitted);
 	INIT_LIST_HEAD(&peer->send_ready);
-- 
2.43.0

Re: [PATCH net-next 06/14] sctp: annotate data-races around sctp sysctls

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete inside the same sysctl handlers it
  changes.

--

sctp: annotate data-races around sctp sysctls

This commit adds READ_ONCE() to readers of many per-netns sctp sysctl
fields. It also converts the plain stores in the rto_min, rto_max,
udp_port, probe_interval, auth_enable and cookie_hmac_alg handlers to
WRITE_ONCE().

> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> index 2b94c211427de..8a84ae3272d24 100644
> --- a/net/sctp/sysctl.c
> +++ b/net/sctp/sysctl.c
> @@ -402,11 +402,11 @@ static int proc_sctp_do_hmac_alg(const struct ctl_table *ctl, int write,
>  		if (ret)
>  			return ret;
>  		if (!strcmp(tmp, "sha256")) {
> -			net->sctp.cookie_auth_enable = 1;
> +			WRITE_ONCE(net->sctp.cookie_auth_enable, 1);
>  			return 0;
>  		}
>  		if (!strcmp(tmp, "none")) {
> -			net->sctp.cookie_auth_enable = 0;
> +			WRITE_ONCE(net->sctp.cookie_auth_enable, 0);
>  			return 0;
>  		}
>  		return -EINVAL;
> @@ -441,7 +441,7 @@ static int proc_sctp_do_rto_min(const struct ctl_table *ctl, int write,
>  		if (new_value > max || new_value < min)
>  			return -EINVAL;
>  
> -		net->sctp.rto_min = new_value;
> +		WRITE_ONCE(net->sctp.rto_min, new_value);
>  	}
>  
>  	return ret;
> @@ -469,7 +469,7 @@ static int proc_sctp_do_rto_max(const struct ctl_table *ctl, int write,
>  		if (new_value > max || new_value < min)
>  			return -EINVAL;
>  
> -		net->sctp.rto_max = new_value;
> +		WRITE_ONCE(net->sctp.rto_max, new_value);
>  	}
>  
>  	return ret;

[Severity: High]
Do the handlers this patch edits still contain plain loads of these same
fields?

proc_sctp_do_rto_min() reads rto_max through extra2 to bound the new
value. proc_sctp_do_rto_max() reads rto_min through extra1 in the same
way:

net/sctp/sysctl.c:proc_sctp_do_rto_min() {
	...
	unsigned int min = *(unsigned int *) ctl->extra1;
	unsigned int max = *(unsigned int *) ctl->extra2;
	...
}

sctp_sysctl_net_register() points these at the live per-netns fields. The
static sctp_net_table does the same for init_net:

	table[SCTP_RTO_MIN_IDX].extra2 = &net->sctp.rto_max;
	table[SCTP_RTO_MAX_IDX].extra1 = &net->sctp.rto_min;

So when writes to /proc/sys/net/sctp/rto_min and rto_max run at the same
time, proc_sctp_do_rto_min() does a plain read of rto_max while
proc_sctp_do_rto_max() does WRITE_ONCE(net->sctp.rto_max, ...). The same
thing happens in the other direction for rto_min. Nothing serializes
these two handlers against each other.

The read branch of proc_sctp_do_hmac_alg() also still loads
cookie_auth_enable without READ_ONCE():

	if (net->sctp.cookie_auth_enable)
		tbl.data = (char *)"sha256";
	else
		tbl.data = (char *)"none";

This can race with the WRITE_ONCE() stores above when cookie_hmac_alg is
read and written at the same time.

The commit message lists rto_min, rto_max and the cookie_hmac_alg handler
among the races being closed. The readers of these fields in
sctp_init_sock() now use READ_ONCE().

Should these three loads in the handlers use READ_ONCE() too? Otherwise
KCSAN can still report a marked write racing with a plain read on these
fields.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109025425.328445-1-jiayuan.chen%40linux.dev

[PATCH net-next 07/14] mptcp: annotate data-races around pernet sysctls

[Jiayuan Chen]

enabled, add_addr_timeout, checksum_enabled, allow_join_initial_addr_port,
stale_loss_cnt, close_timeout, pm_type and syn_retrans_before_tcp_fallback
are read by their ctrl.c helpers without any lock while the sysctl
handlers update them with WRITE_ONCE(), and the path_manager handler
stores pm_type with a plain write. Add READ_ONCE() to the readers and
WRITE_ONCE() to that handler.

KCSAN reports it when net.mptcp.enabled is changed while MPTCP sockets
are being created:

  BUG: KCSAN: data-race in mptcp_is_enabled / proc_dou8vec_minmax

  write (marked) to 0xffff8d724248abc9 of 1 bytes by task 733 on cpu 3:
   proc_dou8vec_minmax+0x1ab/0x1f0
   proc_sys_call_handler+0x282/0x3a0
   proc_sys_write+0x13/0x20
   ...

  read to 0xffff8d724248abc9 of 1 bytes by task 735 on cpu 1:
   mptcp_is_enabled+0x33/0x50
   mptcp_init_sock+0x35/0x300
   inet_create+0x4a6/0x8e0
   ...

  value changed: 0x00 -> 0x01

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 net/mptcp/ctrl.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index 63c5747f0f63c..164f79b9deb0e 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -50,39 +50,39 @@ static struct mptcp_pernet *mptcp_get_pernet(const struct net *net)
 
 int mptcp_is_enabled(const struct net *net)
 {
-	return mptcp_get_pernet(net)->mptcp_enabled;
+	return READ_ONCE(mptcp_get_pernet(net)->mptcp_enabled);
 }
 
 unsigned int mptcp_get_add_addr_timeout(const struct net *net)
 {
-	return mptcp_get_pernet(net)->add_addr_timeout;
+	return READ_ONCE(mptcp_get_pernet(net)->add_addr_timeout);
 }
 
 int mptcp_is_checksum_enabled(const struct net *net)
 {
-	return mptcp_get_pernet(net)->checksum_enabled;
+	return READ_ONCE(mptcp_get_pernet(net)->checksum_enabled);
 }
 
 int mptcp_allow_join_id0(const struct net *net)
 {
-	return mptcp_get_pernet(net)->allow_join_initial_addr_port;
+	return READ_ONCE(mptcp_get_pernet(net)->allow_join_initial_addr_port);
 }
 
 unsigned int mptcp_stale_loss_cnt(const struct net *net)
 {
-	return mptcp_get_pernet(net)->stale_loss_cnt;
+	return READ_ONCE(mptcp_get_pernet(net)->stale_loss_cnt);
 }
 
 unsigned int mptcp_close_timeout(const struct sock *sk)
 {
 	if (sock_flag(sk, SOCK_DEAD))
 		return TCP_TIMEWAIT_LEN;
-	return mptcp_get_pernet(sock_net(sk))->close_timeout;
+	return READ_ONCE(mptcp_get_pernet(sock_net(sk))->close_timeout);
 }
 
 int mptcp_get_pm_type(const struct net *net)
 {
-	return mptcp_get_pernet(net)->pm_type;
+	return READ_ONCE(mptcp_get_pernet(net)->pm_type);
 }
 
 const char *mptcp_get_path_manager(const struct net *net)
@@ -230,7 +230,7 @@ static int proc_path_manager(const struct ctl_table *ctl, int write,
 				pm_type = MPTCP_PM_TYPE_KERNEL;
 			else if (strncmp(pm_name, "userspace", MPTCP_PM_NAME_MAX) == 0)
 				pm_type = MPTCP_PM_TYPE_USERSPACE;
-			pernet->pm_type = pm_type;
+			WRITE_ONCE(pernet->pm_type, pm_type);
 		}
 	}
 
@@ -551,7 +551,7 @@ void mptcp_active_detect_blackhole(struct sock *ssk, bool expired)
 
 	net = sock_net(ssk);
 	timeouts = inet_csk(ssk)->icsk_retransmits;
-	to_max = mptcp_get_pernet(net)->syn_retrans_before_tcp_fallback;
+	to_max = READ_ONCE(mptcp_get_pernet(net)->syn_retrans_before_tcp_fallback);
 
 	if (timeouts == to_max || (timeouts < to_max && expired)) {
 		subflow->mpc_drop = 1;
-- 
2.43.0

[PATCH net-next 08/14] rds: annotate data-races around core rds sysctls

[Jiayuan Chen]

reconnect_min_delay_ms, reconnect_max_delay_ms, max_unacked_packets,
max_unacked_bytes and ping_enable are read without any lock while their
sysctl handlers update them with WRITE_ONCE(). Add READ_ONCE() to the
readers.

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 net/rds/recv.c    | 2 +-
 net/rds/send.c    | 8 ++++----
 net/rds/threads.c | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/rds/recv.c b/net/rds/recv.c
index 00ab60f2d9969..daccbfc8cb8a2 100644
--- a/net/rds/recv.c
+++ b/net/rds/recv.c
@@ -370,7 +370,7 @@ void rds_recv_incoming(struct rds_connection *conn, struct in6_addr *saddr,
 	}
 	cp->cp_next_rx_seq = be64_to_cpu(inc->i_hdr.h_sequence) + 1;
 
-	if (rds_sysctl_ping_enable && inc->i_hdr.h_dport == 0) {
+	if (READ_ONCE(rds_sysctl_ping_enable) && inc->i_hdr.h_dport == 0) {
 		if (inc->i_hdr.h_sport == 0) {
 			rdsdebug("ignore ping with 0 sport from %pI6c\n",
 				 saddr);
diff --git a/net/rds/send.c b/net/rds/send.c
index f38967f65af18..66e75a382bcc7 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -87,8 +87,8 @@ void rds_send_path_reset(struct rds_conn_path *cp)
 
 	cp->cp_conn->c_map_queued = 0;
 
-	cp->cp_unacked_packets = rds_sysctl_max_unacked_packets;
-	cp->cp_unacked_bytes = rds_sysctl_max_unacked_bytes;
+	cp->cp_unacked_packets = READ_ONCE(rds_sysctl_max_unacked_packets);
+	cp->cp_unacked_bytes = READ_ONCE(rds_sysctl_max_unacked_bytes);
 
 	/* Mark messages as retransmissions, and move them to the send q */
 	spin_lock_irqsave(&cp->cp_lock, flags);
@@ -380,9 +380,9 @@ int rds_send_xmit(struct rds_conn_path *cp)
 				set_bit(RDS_MSG_ACK_REQUIRED, &rm->m_flags);
 
 				cp->cp_unacked_packets =
-					rds_sysctl_max_unacked_packets;
+					READ_ONCE(rds_sysctl_max_unacked_packets);
 				cp->cp_unacked_bytes =
-					rds_sysctl_max_unacked_bytes;
+					READ_ONCE(rds_sysctl_max_unacked_bytes);
 				rds_stats_inc(s_send_ack_required);
 			} else {
 				cp->cp_unacked_bytes -= len;
diff --git a/net/rds/threads.c b/net/rds/threads.c
index 639302bab51ef..115fcff0a5b13 100644
--- a/net/rds/threads.c
+++ b/net/rds/threads.c
@@ -137,7 +137,7 @@ void rds_queue_reconnect(struct rds_conn_path *cp)
 
 	set_bit(RDS_RECONNECT_PENDING, &cp->cp_flags);
 	if (cp->cp_reconnect_jiffies == 0) {
-		cp->cp_reconnect_jiffies = rds_sysctl_reconnect_min_jiffies;
+		cp->cp_reconnect_jiffies = READ_ONCE(rds_sysctl_reconnect_min_jiffies);
 		rcu_read_lock();
 		if (!rds_destroy_pending(cp->cp_conn))
 			queue_delayed_work(cp->cp_wq, &cp->cp_conn_w, 0);
@@ -156,7 +156,7 @@ void rds_queue_reconnect(struct rds_conn_path *cp)
 	rcu_read_unlock();
 
 	cp->cp_reconnect_jiffies = min(cp->cp_reconnect_jiffies * 2,
-					rds_sysctl_reconnect_max_jiffies);
+					READ_ONCE(rds_sysctl_reconnect_max_jiffies));
 }
 
 void rds_connect_worker(struct work_struct *work)
-- 
2.43.0

[PATCH net-next 09/14] netfilter: conntrack: annotate data-races around conntrack sysctls

[Jiayuan Chen]

nf_conntrack_acct, nf_conntrack_events, nf_conntrack_timestamp,
nf_conntrack_checksum, nf_conntrack_log_invalid, nf_conntrack_max and
nf_conntrack_expect_max are read without any lock while their sysctl
handlers update them with WRITE_ONCE(), and nf_ct_set_acct() stores
sysctl_acct with a plain write. Add READ_ONCE() to the readers and
WRITE_ONCE() to nf_ct_set_acct().

KCSAN reports it when nf_conntrack_checksum is changed while packets go
through conntrack:

  BUG: KCSAN: data-race in nf_conntrack_icmpv4_error / proc_dou8vec_minmax

  write (marked) to 0xffffffff8ee29b6d of 1 bytes by task 568 on cpu 0:
   proc_dou8vec_minmax+0x1ab/0x1f0
   proc_sys_call_handler+0x282/0x3a0
   proc_sys_write+0x13/0x20
   ...

  read to 0xffffffff8ee29b6d of 1 bytes by interrupt on cpu 2:
   nf_conntrack_icmpv4_error+0xb3/0x240
   nf_conntrack_in+0xa85/0xb20
   ipv4_conntrack_local+0xb0/0x120
   nf_hook_slow+0x7e/0x180
   nf_hook+0x1fa/0x4e0
   __ip_local_out+0x1ac/0x3f0
   ip_push_pending_frames+0x65/0x160
   icmp_push_reply+0x17a/0x200
   __icmp_send+0xb7d/0x1a10
   udp_rcv+0x95c/0x14e0
   ...

  value changed: 0x00 -> 0x01

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 include/net/netfilter/nf_conntrack_acct.h      | 6 +++---
 include/net/netfilter/nf_conntrack_timestamp.h | 2 +-
 net/netfilter/nf_conntrack_core.c              | 6 +++---
 net/netfilter/nf_conntrack_ecache.c            | 2 +-
 net/netfilter/nf_conntrack_expect.c            | 2 +-
 net/netfilter/nf_conntrack_netlink.c           | 2 +-
 net/netfilter/nf_conntrack_proto.c             | 6 +++---
 net/netfilter/nf_conntrack_proto_icmp.c        | 2 +-
 net/netfilter/nf_conntrack_proto_icmpv6.c      | 2 +-
 net/netfilter/nf_conntrack_proto_sctp.c        | 2 +-
 net/netfilter/nf_conntrack_proto_tcp.c         | 2 +-
 net/netfilter/nf_conntrack_proto_udp.c         | 2 +-
 12 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_acct.h b/include/net/netfilter/nf_conntrack_acct.h
index a120685cac932..4e5a46b628831 100644
--- a/include/net/netfilter/nf_conntrack_acct.h
+++ b/include/net/netfilter/nf_conntrack_acct.h
@@ -33,7 +33,7 @@ struct nf_conn_acct *nf_ct_acct_ext_add(struct nf_conn *ct, gfp_t gfp)
 	struct net *net = nf_ct_net(ct);
 	struct nf_conn_acct *acct;
 
-	if (!net->ct.sysctl_acct)
+	if (!READ_ONCE(net->ct.sysctl_acct))
 		return NULL;
 
 	acct = nf_ct_ext_add(ct, NF_CT_EXT_ACCT, gfp);
@@ -51,7 +51,7 @@ struct nf_conn_acct *nf_ct_acct_ext_add(struct nf_conn *ct, gfp_t gfp)
 static inline bool nf_ct_acct_enabled(struct net *net)
 {
 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
-	return net->ct.sysctl_acct != 0;
+	return READ_ONCE(net->ct.sysctl_acct) != 0;
 #else
 	return false;
 #endif
@@ -61,7 +61,7 @@ static inline bool nf_ct_acct_enabled(struct net *net)
 static inline void nf_ct_set_acct(struct net *net, bool enable)
 {
 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
-	net->ct.sysctl_acct = enable;
+	WRITE_ONCE(net->ct.sysctl_acct, enable);
 #endif
 }
 
diff --git a/include/net/netfilter/nf_conntrack_timestamp.h b/include/net/netfilter/nf_conntrack_timestamp.h
index 57138d974a9f7..59751be856ce9 100644
--- a/include/net/netfilter/nf_conntrack_timestamp.h
+++ b/include/net/netfilter/nf_conntrack_timestamp.h
@@ -29,7 +29,7 @@ struct nf_conn_tstamp *nf_ct_tstamp_ext_add(struct nf_conn *ct, gfp_t gfp)
 #ifdef CONFIG_NF_CONNTRACK_TIMESTAMP
 	struct net *net = nf_ct_net(ct);
 
-	if (!net->ct.sysctl_tstamp)
+	if (!READ_ONCE(net->ct.sysctl_tstamp))
 		return NULL;
 
 	return nf_ct_ext_add(ct, NF_CT_EXT_TSTAMP, gfp);
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 931b76aacd934..5c7054753722e 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1508,7 +1508,7 @@ static void gc_worker(struct work_struct *work)
 
 	i = gc_work->next_bucket;
 	if (gc_work->early_drop)
-		nf_conntrack_max95 = nf_conntrack_max / 100u * 95u;
+		nf_conntrack_max95 = READ_ONCE(nf_conntrack_max) / 100u * 95u;
 
 	if (i == 0) {
 		gc_work->avg_timeout = GC_SCAN_INTERVAL_INIT;
@@ -1658,7 +1658,7 @@ __nf_conntrack_alloc(struct net *net,
 	/* We don't want any race condition at early drop stage */
 	ct_count = atomic_inc_return(&cnet->count);
 
-	if (unlikely(ct_count > nf_conntrack_max)) {
+	if (unlikely(ct_count > READ_ONCE(nf_conntrack_max))) {
 		if (!early_drop(net, hash)) {
 			if (!conntrack_gc_work.early_drop)
 				conntrack_gc_work.early_drop = true;
@@ -1793,7 +1793,7 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,
 #ifdef CONFIG_NF_CONNTRACK_EVENTS
 	ecache = tmpl ? nf_ct_ecache_find(tmpl) : NULL;
 
-	if ((ecache || net->ct.sysctl_events) &&
+	if ((ecache || READ_ONCE(net->ct.sysctl_events)) &&
 	    !nf_ct_ecache_ext_add(ct, ecache ? ecache->ctmask : 0,
 				  ecache ? ecache->expmask : 0,
 				  GFP_ATOMIC)) {
diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
index fb731ee235d89..2846511c5318c 100644
--- a/net/netfilter/nf_conntrack_ecache.c
+++ b/net/netfilter/nf_conntrack_ecache.c
@@ -321,7 +321,7 @@ bool nf_ct_ecache_ext_add(struct nf_conn *ct, u16 ctmask, u16 expmask, gfp_t gfp
 	struct net *net = nf_ct_net(ct);
 	struct nf_conntrack_ecache *e;
 
-	switch (net->ct.sysctl_events) {
+	switch (READ_ONCE(net->ct.sysctl_events)) {
 	case 0:
 		 /* assignment via template / ruleset? ignore sysctl. */
 		if (ctmask || expmask)
diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index 06242c86e1dc8..43594283bcc76 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -518,7 +518,7 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect,
 	}
 
 	cnet = nf_ct_pernet(net);
-	if (cnet->expect_count >= nf_ct_expect_max) {
+	if (cnet->expect_count >= READ_ONCE(nf_ct_expect_max)) {
 		net_warn_ratelimited("nf_conntrack: expectation table full\n");
 		ret = -EMFILE;
 	}
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 1f8d94a052403..0f1d42a10b347 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -2591,7 +2591,7 @@ ctnetlink_stat_ct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
 	if (nla_put_be32(skb, CTA_STATS_GLOBAL_ENTRIES, htonl(nr_conntracks)))
 		goto nla_put_failure;
 
-	if (nla_put_be32(skb, CTA_STATS_GLOBAL_MAX_ENTRIES, htonl(nf_conntrack_max)))
+	if (nla_put_be32(skb, CTA_STATS_GLOBAL_MAX_ENTRIES, htonl(READ_ONCE(nf_conntrack_max))))
 		goto nla_put_failure;
 
 	nlmsg_end(skb, nlh);
diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c
index 7a40e4e0e33e6..83dc169f87df5 100644
--- a/net/netfilter/nf_conntrack_proto.c
+++ b/net/netfilter/nf_conntrack_proto.c
@@ -51,12 +51,12 @@ void nf_l4proto_log_invalid(const struct sk_buff *skb,
 			    u8 protonum,
 			    const char *fmt, ...)
 {
+	u8 log_invalid = READ_ONCE(state->net->ct.sysctl_log_invalid);
 	struct net *net = state->net;
 	struct va_format vaf;
 	va_list args;
 
-	if (net->ct.sysctl_log_invalid != protonum &&
-	    net->ct.sysctl_log_invalid != IPPROTO_RAW)
+	if (log_invalid != protonum && log_invalid != IPPROTO_RAW)
 		return;
 
 	va_start(args, fmt);
@@ -86,7 +86,7 @@ void nf_ct_l4proto_log_invalid(const struct sk_buff *skb,
 	lockdep_assert_not_held(&ct->lock);
 
 	net = nf_ct_net(ct);
-	if (likely(net->ct.sysctl_log_invalid == 0))
+	if (likely(READ_ONCE(net->ct.sysctl_log_invalid) == 0))
 		return;
 
 	va_start(args, fmt);
diff --git a/net/netfilter/nf_conntrack_proto_icmp.c b/net/netfilter/nf_conntrack_proto_icmp.c
index 32148a3a8509a..330c6d83d670a 100644
--- a/net/netfilter/nf_conntrack_proto_icmp.c
+++ b/net/netfilter/nf_conntrack_proto_icmp.c
@@ -217,7 +217,7 @@ int nf_conntrack_icmpv4_error(struct nf_conn *tmpl,
 	}
 
 	/* See nf_conntrack_proto_tcp.c */
-	if (state->net->ct.sysctl_checksum &&
+	if (READ_ONCE(state->net->ct.sysctl_checksum) &&
 	    state->hook == NF_INET_PRE_ROUTING &&
 	    nf_ip_checksum(skb, state->hook, dataoff, IPPROTO_ICMP)) {
 		icmp_error_log(skb, state, "bad hw icmp checksum");
diff --git a/net/netfilter/nf_conntrack_proto_icmpv6.c b/net/netfilter/nf_conntrack_proto_icmpv6.c
index e508b3aa370a7..803221781de39 100644
--- a/net/netfilter/nf_conntrack_proto_icmpv6.c
+++ b/net/netfilter/nf_conntrack_proto_icmpv6.c
@@ -198,7 +198,7 @@ int nf_conntrack_icmpv6_error(struct nf_conn *tmpl,
 	}
 
 	if (state->hook == NF_INET_PRE_ROUTING &&
-	    state->net->ct.sysctl_checksum &&
+	    READ_ONCE(state->net->ct.sysctl_checksum) &&
 	    nf_ip6_checksum(skb, state->hook, dataoff, IPPROTO_ICMPV6)) {
 		icmpv6_error_log(skb, state, "ICMPv6 checksum failed");
 		return -NF_ACCEPT;
diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
index 71cc920f68568..49ec8478746ad 100644
--- a/net/netfilter/nf_conntrack_proto_sctp.c
+++ b/net/netfilter/nf_conntrack_proto_sctp.c
@@ -304,7 +304,7 @@ static bool sctp_error(struct sk_buff *skb,
 		goto out_invalid;
 	}
 	if (state->hook == NF_INET_PRE_ROUTING &&
-	    state->net->ct.sysctl_checksum &&
+	    READ_ONCE(state->net->ct.sysctl_checksum) &&
 	    skb->ip_summed == CHECKSUM_NONE) {
 		if (skb_ensure_writable(skb, dataoff + sizeof(*sh))) {
 			logmsg = "nf_ct_sctp: failed to read header ";
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index ad6f1986d52a6..b859e409f0796 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -856,7 +856,7 @@ static bool tcp_error(const struct tcphdr *th,
 	 * because the checksum is assumed to be correct.
 	 */
 	/* FIXME: Source route IP option packets --RR */
-	if (state->net->ct.sysctl_checksum &&
+	if (READ_ONCE(state->net->ct.sysctl_checksum) &&
 	    state->hook == NF_INET_PRE_ROUTING &&
 	    nf_checksum(skb, state->hook, dataoff, IPPROTO_TCP, state->pf)) {
 		tcp_error_log(skb, state, "bad checksum");
diff --git a/net/netfilter/nf_conntrack_proto_udp.c b/net/netfilter/nf_conntrack_proto_udp.c
index a7edfefd6fd14..9ea4062752238 100644
--- a/net/netfilter/nf_conntrack_proto_udp.c
+++ b/net/netfilter/nf_conntrack_proto_udp.c
@@ -82,7 +82,7 @@ static bool udp_error(struct sk_buff *skb,
 	 * because the checksum is assumed to be correct.
 	 * FIXME: Source route IP option packets --RR */
 	if (state->hook == NF_INET_PRE_ROUTING &&
-	    state->net->ct.sysctl_checksum &&
+	    READ_ONCE(state->net->ct.sysctl_checksum) &&
 	    nf_checksum(skb, state->hook, dataoff, IPPROTO_UDP, state->pf)) {
 		udp_error_log(skb, state, "bad checksum");
 		return true;
-- 
2.43.0

[PATCH net-next 10/14] netfilter: bridge: annotate data-races around br_netfilter and nf_log sysctls

[Jiayuan Chen]

bridge-nf-call-arptables, bridge-nf-call-iptables,
bridge-nf-call-ip6tables, bridge-nf-filter-vlan-tagged,
bridge-nf-filter-pppoe-tagged, bridge-nf-pass-vlan-input-dev and
nf_log_all_netns are read in the packet path without any lock while their
sysctl handlers update them, and brnf_sysctl_call_tables() normalizes the
value with a plain write. Add READ_ONCE() to the readers and WRITE_ONCE()
to that handler.

KCSAN reports it when bridge-nf-call-iptables is changed while packets
are bridged:

  BUG: KCSAN: data-race in br_nf_pre_routing / do_proc_int_conv

  write (marked) to 0xffff8d7242d8ec50 of 4 bytes by task 824 on cpu 0:
   do_proc_int_conv+0xa9/0xd0
   proc_vec_conv+0x21/0x40
   proc_vec+0x21e/0x6f0
   proc_dointvec+0x16/0x30
   brnf_sysctl_call_tables+0x19/0x70
   proc_sys_call_handler+0x282/0x3a0
   proc_sys_write+0x13/0x20
   ...

  read to 0xffff8d7242d8ec50 of 4 bytes by interrupt on cpu 1:
   br_nf_pre_routing+0x1a9/0x9d0
   br_handle_frame+0x523/0x990
   __netif_receive_skb_core.constprop.0+0x3b2/0x1cf0
   __netif_receive_skb_one_core+0x51/0xe0
   __netif_receive_skb+0x23/0xb0
   process_backlog+0x27a/0x950
   ...

  value changed: 0x00000001 -> 0x00000000

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 net/bridge/br_netfilter_hooks.c | 20 ++++++++++----------
 net/bridge/netfilter/ebt_log.c  |  2 +-
 net/netfilter/nf_log_syslog.c   |  2 +-
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
index e15355ff215a7..8a47fe4e8eb61 100644
--- a/net/bridge/br_netfilter_hooks.c
+++ b/net/bridge/br_netfilter_hooks.c
@@ -94,7 +94,7 @@ static inline bool is_vlan_ip(const struct sk_buff *skb, const struct net *net)
 {
 	struct brnf_net *brnet = net_generic(net, brnf_net_id);
 
-	return vlan_proto(skb) == htons(ETH_P_IP) && brnet->filter_vlan_tagged;
+	return vlan_proto(skb) == htons(ETH_P_IP) && READ_ONCE(brnet->filter_vlan_tagged);
 }
 
 static inline bool is_vlan_ipv6(const struct sk_buff *skb,
@@ -103,14 +103,14 @@ static inline bool is_vlan_ipv6(const struct sk_buff *skb,
 	struct brnf_net *brnet = net_generic(net, brnf_net_id);
 
 	return vlan_proto(skb) == htons(ETH_P_IPV6) &&
-	       brnet->filter_vlan_tagged;
+	       READ_ONCE(brnet->filter_vlan_tagged);
 }
 
 static inline bool is_vlan_arp(const struct sk_buff *skb, const struct net *net)
 {
 	struct brnf_net *brnet = net_generic(net, brnf_net_id);
 
-	return vlan_proto(skb) == htons(ETH_P_ARP) && brnet->filter_vlan_tagged;
+	return vlan_proto(skb) == htons(ETH_P_ARP) && READ_ONCE(brnet->filter_vlan_tagged);
 }
 
 static inline __be16 pppoe_proto(const struct sk_buff *skb)
@@ -124,7 +124,7 @@ static inline bool is_pppoe_ip(const struct sk_buff *skb, const struct net *net)
 	struct brnf_net *brnet = net_generic(net, brnf_net_id);
 
 	return skb->protocol == htons(ETH_P_PPP_SES) &&
-	       pppoe_proto(skb) == htons(PPP_IP) && brnet->filter_pppoe_tagged;
+	       pppoe_proto(skb) == htons(PPP_IP) && READ_ONCE(brnet->filter_pppoe_tagged);
 }
 
 static inline bool is_pppoe_ipv6(const struct sk_buff *skb,
@@ -134,7 +134,7 @@ static inline bool is_pppoe_ipv6(const struct sk_buff *skb,
 
 	return skb->protocol == htons(ETH_P_PPP_SES) &&
 	       pppoe_proto(skb) == htons(PPP_IPV6) &&
-	       brnet->filter_pppoe_tagged;
+	       READ_ONCE(brnet->filter_pppoe_tagged);
 }
 
 /* largest possible L2 header, see br_nf_dev_queue_xmit() */
@@ -442,7 +442,7 @@ static struct net_device *brnf_get_logical_dev(struct sk_buff *skb,
 
 	br = bridge_parent(dev);
 
-	if (brnet->pass_vlan_indev == 0 || !skb_vlan_tag_present(skb))
+	if (READ_ONCE(brnet->pass_vlan_indev) == 0 || !skb_vlan_tag_present(skb))
 		return br;
 
 	vlan = __vlan_find_dev_deep_rcu(br, skb->vlan_proto,
@@ -502,7 +502,7 @@ static unsigned int br_nf_pre_routing(void *priv,
 	brnet = net_generic(state->net, brnf_net_id);
 	if (IS_IPV6(skb) || is_vlan_ipv6(skb, state->net) ||
 	    is_pppoe_ipv6(skb, state->net)) {
-		if (!brnet->call_ip6tables &&
+		if (!READ_ONCE(brnet->call_ip6tables) &&
 		    !br_opt_get(br, BROPT_NF_CALL_IP6TABLES))
 			return NF_ACCEPT;
 		if (!ipv6_mod_enabled()) {
@@ -514,7 +514,7 @@ static unsigned int br_nf_pre_routing(void *priv,
 		return br_nf_pre_routing_ipv6(priv, skb, state);
 	}
 
-	if (!brnet->call_iptables && !br_opt_get(br, BROPT_NF_CALL_IPTABLES))
+	if (!READ_ONCE(brnet->call_iptables) && !br_opt_get(br, BROPT_NF_CALL_IPTABLES))
 		return NF_ACCEPT;
 
 	if (!IS_IP(skb) && !is_vlan_ip(skb, state->net) &&
@@ -740,7 +740,7 @@ static unsigned int br_nf_forward_arp(struct sk_buff *skb,
 	br = p->br;
 
 	brnet = net_generic(state->net, brnf_net_id);
-	if (!brnet->call_arptables && !br_opt_get(br, BROPT_NF_CALL_ARPTABLES))
+	if (!READ_ONCE(brnet->call_arptables) && !br_opt_get(br, BROPT_NF_CALL_ARPTABLES))
 		return NF_ACCEPT;
 
 	if (is_vlan_arp(skb, state->net))
@@ -1178,7 +1178,7 @@ int brnf_sysctl_call_tables(const struct ctl_table *ctl, int write,
 	ret = proc_dointvec(ctl, write, buffer, lenp, ppos);
 
 	if (write && *(int *)(ctl->data))
-		*(int *)(ctl->data) = 1;
+		WRITE_ONCE(*(int *)(ctl->data), 1);
 	return ret;
 }
 
diff --git a/net/bridge/netfilter/ebt_log.c b/net/bridge/netfilter/ebt_log.c
index e2eea1daaf8ba..f360414a2814e 100644
--- a/net/bridge/netfilter/ebt_log.c
+++ b/net/bridge/netfilter/ebt_log.c
@@ -79,7 +79,7 @@ ebt_log_packet(struct net *net, u_int8_t pf, unsigned int hooknum,
 	unsigned int bitmask;
 
 	/* FIXME: Disabled from containers until syslog ns is supported */
-	if (!net_eq(net, &init_net) && !sysctl_nf_log_all_netns)
+	if (!net_eq(net, &init_net) && !READ_ONCE(sysctl_nf_log_all_netns))
 		return;
 
 	spin_lock_bh(&ebt_log_lock);
diff --git a/net/netfilter/nf_log_syslog.c b/net/netfilter/nf_log_syslog.c
index c3fd398ffcd77..2907f47c83423 100644
--- a/net/netfilter/nf_log_syslog.c
+++ b/net/netfilter/nf_log_syslog.c
@@ -43,7 +43,7 @@ struct arppayload {
 /* Guard against containers flooding syslog. */
 static bool nf_log_allowed(const struct net *net)
 {
-	return net_eq(net, &init_net) || sysctl_nf_log_all_netns;
+	return net_eq(net, &init_net) || READ_ONCE(sysctl_nf_log_all_netns);
 }
 
 static void nf_log_dump_vlan(struct nf_log_buf *m, const struct sk_buff *skb)
-- 
2.43.0

Re: [PATCH net-next 10/14] netfilter: bridge: annotate data-races around br_netfilter and nf_log sysctls

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] brnf_sysctl_call_tables() in net/bridge/br_netfilter_hooks.c is
  only half annotated.

--

netfilter: bridge: annotate data-races around br_netfilter and nf_log sysctls

This patch adds READ_ONCE() annotations in the packet path for the
bridge-nf-* sysctls and for sysctl_nf_log_all_netns. It also adds a
WRITE_ONCE() to the normalization store in brnf_sysctl_call_tables().
The goal is to silence KCSAN data-race reports that show up when these
sysctls change while packets are being bridged.

> diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
> index e15355ff215a7..8a47fe4e8eb61 100644
> --- a/net/bridge/br_netfilter_hooks.c
> +++ b/net/bridge/br_netfilter_hooks.c

[ ... ]

> @@ -1178,7 +1178,7 @@ int brnf_sysctl_call_tables(const struct ctl_table *ctl, int write,
>  	ret = proc_dointvec(ctl, write, buffer, lenp, ppos);
>  
>  	if (write && *(int *)(ctl->data))
                     ^^^^^^^^^^^^^^^^^^^
> -		*(int *)(ctl->data) = 1;
> +		WRITE_ONCE(*(int *)(ctl->data), 1);

[Severity: Low]
This isn't a bug, but should the load in the condition also be annotated?

The store is now WRITE_ONCE(), but the check just before it is still a
plain read of *(int *)(ctl->data).

proc_sys_call_handler() in fs/proc/proc_sysctl.c calls
table->proc_handler() without taking any lock. Two concurrent writes to
the same knob (bridge-nf-call-iptables, for example) can therefore run
brnf_sysctl_call_tables() in parallel. In that case one handler's plain
read can race with the other writer's marked store in do_proc_int_conv(),
or with its WRITE_ONCE(..., 1).

That is the same plain-read-vs-marked-write pattern as the splat in the
commit message, so KCSAN would report it as a race between
brnf_sysctl_call_tables and do_proc_int_conv.

There is no functional effect here. The final value is always 0 or 1,
and any non-zero value is treated as true. This only affects how
complete the annotation is.

Would it make sense to use READ_ONCE() for the check? Another option is
to parse into a local value and publish it with a single WRITE_ONCE().

>  	return ret;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109025425.328445-1-jiayuan.chen%40linux.dev

[PATCH net-next 11/14] ipvs: annotate data-races around lblc_expiration and lblcr_expiration

[Jiayuan Chen]

sysctl_lblc_expiration and sysctl_lblcr_expiration are read without any
lock while their sysctl handlers update them with WRITE_ONCE(). Add
READ_ONCE() to the readers.

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 net/netfilter/ipvs/ip_vs_lblc.c  | 2 +-
 net/netfilter/ipvs/ip_vs_lblcr.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_lblc.c b/net/netfilter/ipvs/ip_vs_lblc.c
index e1111c3b27211..21bb820e02914 100644
--- a/net/netfilter/ipvs/ip_vs_lblc.c
+++ b/net/netfilter/ipvs/ip_vs_lblc.c
@@ -249,7 +249,7 @@ static void ip_vs_lblc_flush(struct ip_vs_service *svc)
 static int sysctl_lblc_expiration(struct ip_vs_service *svc)
 {
 #ifdef CONFIG_SYSCTL
-	return svc->ipvs->sysctl_lblc_expiration;
+	return READ_ONCE(svc->ipvs->sysctl_lblc_expiration);
 #else
 	return DEFAULT_EXPIRATION;
 #endif
diff --git a/net/netfilter/ipvs/ip_vs_lblcr.c b/net/netfilter/ipvs/ip_vs_lblcr.c
index dfb5bf83750db..33d6e30968a95 100644
--- a/net/netfilter/ipvs/ip_vs_lblcr.c
+++ b/net/netfilter/ipvs/ip_vs_lblcr.c
@@ -413,7 +413,7 @@ static void ip_vs_lblcr_flush(struct ip_vs_service *svc)
 static int sysctl_lblcr_expiration(struct ip_vs_service *svc)
 {
 #ifdef CONFIG_SYSCTL
-	return svc->ipvs->sysctl_lblcr_expiration;
+	return READ_ONCE(svc->ipvs->sysctl_lblcr_expiration);
 #else
 	return DEFAULT_EXPIRATION;
 #endif
-- 
2.43.0

[PATCH net-next 12/14] net/smc: annotate data-races around smc sysctls

[Jiayuan Chen]

autocorking_size, smcr_buf_type, smcr_max_links_per_lgr,
smcr_max_conns_per_lgr and limit_smc_hs are read without any lock while
their sysctl handlers update them with WRITE_ONCE(), and the SMC netlink
handlers toggle limit_smc_hs with plain writes. Add READ_ONCE() to the
readers and WRITE_ONCE() to those handlers.

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 net/smc/af_smc.c   | 8 ++++----
 net/smc/smc_clc.c  | 8 ++++----
 net/smc/smc_core.c | 2 +-
 net/smc/smc_tx.c   | 2 +-
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index d10244af5bd29..9a657172be8f9 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -88,7 +88,7 @@ int smc_nl_dump_hs_limitation(struct sk_buff *skb, struct netlink_callback *cb)
 		return -ENOMEM;
 
 	if (nla_put_u8(skb, SMC_NLA_HS_LIMITATION_ENABLED,
-		       sock_net(skb->sk)->smc.limit_smc_hs))
+		       READ_ONCE(sock_net(skb->sk)->smc.limit_smc_hs)))
 		goto err;
 
 	genlmsg_end(skb, hdr);
@@ -102,13 +102,13 @@ int smc_nl_dump_hs_limitation(struct sk_buff *skb, struct netlink_callback *cb)
 
 int smc_nl_enable_hs_limitation(struct sk_buff *skb, struct genl_info *info)
 {
-	sock_net(skb->sk)->smc.limit_smc_hs = true;
+	WRITE_ONCE(sock_net(skb->sk)->smc.limit_smc_hs, true);
 	return 0;
 }
 
 int smc_nl_disable_hs_limitation(struct sk_buff *skb, struct genl_info *info)
 {
-	sock_net(skb->sk)->smc.limit_smc_hs = false;
+	WRITE_ONCE(sock_net(skb->sk)->smc.limit_smc_hs, false);
 	return 0;
 }
 
@@ -411,7 +411,7 @@ void smc_sk_init(struct net *net, struct sock *sk, int protocol)
 	spin_lock_init(&smc->conn.send_lock);
 	mutex_init(&smc->clcsock_release_lock);
 	smc_init_saved_callbacks(smc);
-	smc->limit_smc_hs = net->smc.limit_smc_hs;
+	smc->limit_smc_hs = READ_ONCE(net->smc.limit_smc_hs);
 	smc->use_fallback = false; /* assume rdma capability first */
 	smc->fallback_rsn = 0;
 	smc_close_init(smc);
diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index 014d527d5462d..df0afeef67cbd 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -990,8 +990,8 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 	}
 	if (smcr_indicated(ini->smc_type_v2)) {
 		memcpy(v2_ext->roce, ini->smcrv2.ib_gid_v2, SMC_GID_SIZE);
-		v2_ext->max_conns = net->smc.sysctl_max_conns_per_lgr;
-		v2_ext->max_links = net->smc.sysctl_max_links_per_lgr;
+		v2_ext->max_conns = READ_ONCE(net->smc.sysctl_max_conns_per_lgr);
+		v2_ext->max_links = READ_ONCE(net->smc.sysctl_max_links_per_lgr);
 	}
 
 	pclc_base->hdr.length = htons(plen);
@@ -1272,12 +1272,12 @@ int smc_clc_srv_v2x_features_validate(struct smc_sock *smc,
 
 	if (ini->smcr_version & SMC_V2) {
 		ini->max_conns = min_t(u8, pclc_v2_ext->max_conns,
-				       net->smc.sysctl_max_conns_per_lgr);
+				       READ_ONCE(net->smc.sysctl_max_conns_per_lgr));
 		if (ini->max_conns < SMC_CONN_PER_LGR_MIN)
 			return SMC_CLC_DECL_MAXCONNERR;
 
 		ini->max_links = min_t(u8, pclc_v2_ext->max_links,
-				       net->smc.sysctl_max_links_per_lgr);
+				       READ_ONCE(net->smc.sysctl_max_links_per_lgr));
 		if (ini->max_links < SMC_LINKS_ADD_LNK_MIN)
 			return SMC_CLC_DECL_MAXLINKERR;
 	}
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 6021322253e93..3d338019a1b8b 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -990,7 +990,7 @@ static int smc_lgr_create(struct smc_sock *smc, struct smc_init_info *ini)
 		lgr->net = smc_ib_net(lnk->smcibdev);
 		lgr_list = &smc_lgr_list.list;
 		lgr_lock = &smc_lgr_list.lock;
-		lgr->buf_type = lgr->net->smc.sysctl_smcr_buf_type;
+		lgr->buf_type = READ_ONCE(lgr->net->smc.sysctl_smcr_buf_type);
 		atomic_inc(&lgr_cnt);
 	}
 	smc->conn.lgr = lgr;
diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
index 0f0c6df3bf2ce..30bffe79d06a3 100644
--- a/net/smc/smc_tx.c
+++ b/net/smc/smc_tx.c
@@ -148,7 +148,7 @@ static bool smc_should_autocork(struct smc_sock *smc)
 	int corking_size;
 
 	corking_size = min_t(unsigned int, conn->sndbuf_desc->len >> 1,
-			     sock_net(&smc->sk)->smc.sysctl_autocorking_size);
+			     READ_ONCE(sock_net(&smc->sk)->smc.sysctl_autocorking_size));
 
 	if (atomic_read(&conn->cdc_pend_tx_wr) == 0 ||
 	    smc_tx_prepared_sends(conn) > corking_size)
-- 
2.43.0

[PATCH net-next 13/14] netfilter: conntrack: annotate data-races around timeout and tcp sysctls

[Jiayuan Chen]

The nf_conntrack_*_timeout* knobs, nf_flowtable_tcp_timeout,
nf_flowtable_udp_timeout and nf_conntrack_tcp_{loose,be_liberal,
max_retrans,ignore_invalid_rst} are read in the packet path without any
lock while their sysctl handlers update them with WRITE_ONCE(). Add
READ_ONCE() to the readers.

KCSAN reports it when nf_conntrack_udp_timeout is changed while UDP
traffic goes through conntrack:

  BUG: KCSAN: data-race in nf_conntrack_udp_packet / proc_int_u2k_conv_uop

  write (marked) to 0xffffffffb0829bc4 of 4 bytes by task 436 on cpu 0:
   proc_int_u2k_conv_uop+0x92/0xa0
   sysctl_u2k_int_conv_hz+0x21/0x30
   proc_int_conv+0xdf/0x130
   do_proc_int_conv_jiffies+0x1b/0x30
   proc_vec_conv+0x21/0x40
   proc_vec+0x21e/0x6f0
   proc_dointvec_conv+0x21/0x40
   proc_dointvec_jiffies+0x15/0x30
   proc_sys_call_handler+0x282/0x3a0
   proc_sys_write+0x13/0x20
   ...

  read to 0xffffffffb0829bc4 of 4 bytes by task 439 on cpu 3:
   nf_conntrack_udp_packet+0x27b/0x550
   nf_conntrack_in+0x767/0xb20
   ipv4_conntrack_local+0xb0/0x120
   nf_hook_slow+0x7e/0x180
   nf_hook+0x1fa/0x4e0
   __ip_local_out+0x1ac/0x3f0
   ip_send_skb+0x31/0x120
   udp_send_skb+0x4bf/0x640
   udp_sendmsg+0xdec/0x18a0
   ...

  value changed: 0x00001d4c -> 0x00001e46

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 net/netfilter/nf_conntrack_core.c         |  2 +-
 net/netfilter/nf_conntrack_proto_gre.c    |  4 +--
 net/netfilter/nf_conntrack_proto_icmp.c   |  2 +-
 net/netfilter/nf_conntrack_proto_icmpv6.c |  2 +-
 net/netfilter/nf_conntrack_proto_sctp.c   |  2 +-
 net/netfilter/nf_conntrack_proto_tcp.c    | 37 ++++++++++++-----------
 net/netfilter/nf_conntrack_proto_udp.c    |  6 ++--
 net/netfilter/nf_flow_table_core.c        |  4 +--
 8 files changed, 31 insertions(+), 28 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 5c7054753722e..8e332e407529e 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1959,7 +1959,7 @@ static int generic_packet(struct nf_conn *ct, struct sk_buff *skb,
 	if (!timeout)
 		timeout = &nf_generic_pernet(nf_ct_net(ct))->timeout;
 
-	nf_ct_refresh_acct(ct, ctinfo, skb, *timeout);
+	nf_ct_refresh_acct(ct, ctinfo, skb, READ_ONCE(*timeout));
 	return NF_ACCEPT;
 }
 
diff --git a/net/netfilter/nf_conntrack_proto_gre.c b/net/netfilter/nf_conntrack_proto_gre.c
index 616ab1e2fc5ed..d8bca1e29797b 100644
--- a/net/netfilter/nf_conntrack_proto_gre.c
+++ b/net/netfilter/nf_conntrack_proto_gre.c
@@ -279,8 +279,8 @@ int nf_conntrack_gre_packet(struct nf_conn *ct,
 
 		/* initialize to sane value.  Ideally a conntrack helper
 		 * (e.g. in case of pptp) is increasing them */
-		ct->proto.gre.stream_timeout = timeouts[GRE_CT_REPLIED];
-		ct->proto.gre.timeout = timeouts[GRE_CT_UNREPLIED];
+		ct->proto.gre.stream_timeout = READ_ONCE(timeouts[GRE_CT_REPLIED]);
+		ct->proto.gre.timeout = READ_ONCE(timeouts[GRE_CT_UNREPLIED]);
 	}
 
 	status = READ_ONCE(ct->status);
diff --git a/net/netfilter/nf_conntrack_proto_icmp.c b/net/netfilter/nf_conntrack_proto_icmp.c
index 330c6d83d670a..0f6ef0443c6fa 100644
--- a/net/netfilter/nf_conntrack_proto_icmp.c
+++ b/net/netfilter/nf_conntrack_proto_icmp.c
@@ -98,7 +98,7 @@ int nf_conntrack_icmp_packet(struct nf_conn *ct,
 	if (!timeout)
 		timeout = &nf_icmp_pernet(nf_ct_net(ct))->timeout;
 
-	nf_ct_refresh_acct(ct, ctinfo, skb, *timeout);
+	nf_ct_refresh_acct(ct, ctinfo, skb, READ_ONCE(*timeout));
 	return NF_ACCEPT;
 }
 
diff --git a/net/netfilter/nf_conntrack_proto_icmpv6.c b/net/netfilter/nf_conntrack_proto_icmpv6.c
index 803221781de39..e4eebf13950cb 100644
--- a/net/netfilter/nf_conntrack_proto_icmpv6.c
+++ b/net/netfilter/nf_conntrack_proto_icmpv6.c
@@ -118,7 +118,7 @@ int nf_conntrack_icmpv6_packet(struct nf_conn *ct,
 	/* Do not immediately delete the connection after the first
 	   successful reply to avoid excessive conntrackd traffic
 	   and also to handle correctly ICMP echo reply duplicates. */
-	nf_ct_refresh_acct(ct, ctinfo, skb, *timeout);
+	nf_ct_refresh_acct(ct, ctinfo, skb, READ_ONCE(*timeout));
 
 	return NF_ACCEPT;
 }
diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
index 49ec8478746ad..047bf69113bf9 100644
--- a/net/netfilter/nf_conntrack_proto_sctp.c
+++ b/net/netfilter/nf_conntrack_proto_sctp.c
@@ -523,7 +523,7 @@ int nf_conntrack_sctp_packet(struct nf_conn *ct,
 	if (!timeouts)
 		timeouts = nf_sctp_pernet(nf_ct_net(ct))->timeouts;
 
-	nf_ct_refresh_acct(ct, ctinfo, skb, timeouts[new_state]);
+	nf_ct_refresh_acct(ct, ctinfo, skb, READ_ONCE(timeouts[new_state]));
 
 	return NF_ACCEPT;
 
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index b859e409f0796..0c61a16fca400 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -505,7 +505,7 @@ nf_tcp_store_invalid(const struct nf_conn *ct,
 	const struct nf_tcp_net *tn = nf_tcp_pernet(nf_ct_net(ct));
 	bool be_liberal;
 
-	be_liberal = sender->flags & IP_CT_TCP_FLAG_BE_LIBERAL || tn->tcp_be_liberal;
+	be_liberal = sender->flags & IP_CT_TCP_FLAG_BE_LIBERAL || READ_ONCE(tn->tcp_be_liberal);
 	if (be_liberal)
 		return NFCT_TCP_ACCEPT;
 
@@ -904,7 +904,7 @@ static noinline bool tcp_new(struct nf_conn *ct, const struct sk_buff *skb,
 			ct->proto.tcp.seen[0].td_end;
 
 		tcp_options(skb, dataoff, th, &ct->proto.tcp.seen[0]);
-	} else if (tn->tcp_loose == 0) {
+	} else if (READ_ONCE(tn->tcp_loose) == 0) {
 		/* Don't try to pick up connections. */
 		return false;
 	} else {
@@ -979,7 +979,7 @@ void nf_conntrack_tcp_set_closing(struct nf_conn *ct)
 		timeouts = tn->timeouts;
 	}
 
-	timeout = timeouts[TCP_CONNTRACK_CLOSE];
+	timeout = READ_ONCE(timeouts[TCP_CONNTRACK_CLOSE]);
 	WRITE_ONCE(ct->timeout, timeout + nfct_time_stamp);
 
 	spin_unlock_bh(&ct->lock);
@@ -1010,6 +1010,7 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 	struct nf_tcp_invalid_log log = {};
 	unsigned int index, *timeouts;
 	bool lowered_timeout = false;
+	unsigned int retrans, unack;
 	enum nf_ct_tcp_action res;
 	enum ip_conntrack_dir dir;
 	const struct tcphdr *th;
@@ -1235,7 +1236,7 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 				break;
 
 			if (before(seq, ct->proto.tcp.seen[!dir].td_maxack) &&
-			    !tn->tcp_ignore_invalid_rst) {
+			    !READ_ONCE(tn->tcp_ignore_invalid_rst)) {
 				/* Invalid RST  */
 				spin_unlock_bh(&ct->lock);
 				nf_ct_l4proto_log_invalid(skb, ct, state, "invalid rst");
@@ -1322,22 +1323,24 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 	if (!timeouts)
 		timeouts = tn->timeouts;
 
-	if (ct->proto.tcp.retrans >= tn->tcp_max_retrans &&
-	    timeouts[new_state] > timeouts[TCP_CONNTRACK_RETRANS])
-		timeout = timeouts[TCP_CONNTRACK_RETRANS];
+	timeout = READ_ONCE(timeouts[new_state]);
+	retrans = READ_ONCE(timeouts[TCP_CONNTRACK_RETRANS]);
+	unack = READ_ONCE(timeouts[TCP_CONNTRACK_UNACK]);
+
+	if (ct->proto.tcp.retrans >= READ_ONCE(tn->tcp_max_retrans) &&
+	    timeout > retrans)
+		timeout = retrans;
 	else if (unlikely(index == TCP_RST_SET &&
 			  new_state == TCP_CONNTRACK_ESTABLISHED) &&
-		 timeouts[new_state] > timeouts[TCP_CONNTRACK_UNACK])
-		timeout = timeouts[TCP_CONNTRACK_UNACK];
+		 timeout > unack)
+		timeout = unack;
 	else if ((ct->proto.tcp.seen[0].flags | ct->proto.tcp.seen[1].flags) &
 		 IP_CT_TCP_FLAG_DATA_UNACKNOWLEDGED &&
-		 timeouts[new_state] > timeouts[TCP_CONNTRACK_UNACK])
-		timeout = timeouts[TCP_CONNTRACK_UNACK];
+		 timeout > unack)
+		timeout = unack;
 	else if (ct->proto.tcp.last_win == 0 &&
-		 timeouts[new_state] > timeouts[TCP_CONNTRACK_RETRANS])
-		timeout = timeouts[TCP_CONNTRACK_RETRANS];
-	else
-		timeout = timeouts[new_state];
+		 timeout > retrans)
+		timeout = retrans;
 	spin_unlock_bh(&ct->lock);
 
 	if (new_state != old_state)
@@ -1366,8 +1369,8 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 		 * pickup with loose=1. Avoid large ESTABLISHED timeout.
 		 */
 		if (new_state == TCP_CONNTRACK_ESTABLISHED &&
-		    timeout > timeouts[TCP_CONNTRACK_UNACK])
-			timeout = timeouts[TCP_CONNTRACK_UNACK];
+		    timeout > unack)
+			timeout = unack;
 	} else if (!test_bit(IPS_ASSURED_BIT, &ct->status)
 		   && (old_state == TCP_CONNTRACK_SYN_RECV
 		       || old_state == TCP_CONNTRACK_ESTABLISHED)
diff --git a/net/netfilter/nf_conntrack_proto_udp.c b/net/netfilter/nf_conntrack_proto_udp.c
index 9ea4062752238..99eedaa2b1183 100644
--- a/net/netfilter/nf_conntrack_proto_udp.c
+++ b/net/netfilter/nf_conntrack_proto_udp.c
@@ -116,12 +116,12 @@ int nf_conntrack_udp_packet(struct nf_conn *ct,
 	 * stream. Set Assured.
 	 */
 	if (status & IPS_SEEN_REPLY) {
-		unsigned long extra = timeouts[UDP_CT_UNREPLIED];
+		unsigned long extra = READ_ONCE(timeouts[UDP_CT_UNREPLIED]);
 		bool stream = false;
 
 		/* Still active after two seconds? Extend timeout. */
 		if (time_after(jiffies, ct->proto.udp.stream_ts)) {
-			extra = timeouts[UDP_CT_REPLIED];
+			extra = READ_ONCE(timeouts[UDP_CT_REPLIED]);
 			stream = (status & IPS_ASSURED) == 0;
 		}
 
@@ -135,7 +135,7 @@ int nf_conntrack_udp_packet(struct nf_conn *ct,
 		if (stream && !test_and_set_bit(IPS_ASSURED_BIT, &ct->status))
 			nf_conntrack_event_cache(IPCT_ASSURED, ct);
 	} else {
-		nf_ct_refresh_acct(ct, ctinfo, skb, timeouts[UDP_CT_UNREPLIED]);
+		nf_ct_refresh_acct(ct, ctinfo, skb, READ_ONCE(timeouts[UDP_CT_UNREPLIED]));
 	}
 	return NF_ACCEPT;
 }
diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index 0d72dc8ae47a1..fa25a641e9c38 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -322,11 +322,11 @@ unsigned long flow_offload_get_timeout(struct flow_offload *flow)
 	if (l4num == IPPROTO_TCP) {
 		struct nf_tcp_net *tn = nf_tcp_pernet(net);
 
-		timeout = tn->offload_timeout;
+		timeout = READ_ONCE(tn->offload_timeout);
 	} else if (l4num == IPPROTO_UDP) {
 		struct nf_udp_net *tn = nf_udp_pernet(net);
 
-		timeout = tn->offload_timeout;
+		timeout = READ_ONCE(tn->offload_timeout);
 	}
 
 	return timeout;
-- 
2.43.0

[PATCH net-next 14/14] netfilter: cttimeout: annotate data-races around the default timeouts

[Jiayuan Chen]

cttimeout_default_set() and cttimeout_default_get() hand the per-netns
default timeout arrays to the l4proto nlattr_to_obj() and obj_to_nlattr()
callbacks, which write and read them with plain accesses while the
nf_conntrack_*_timeout* sysctl handlers update the same fields with
WRITE_ONCE(). Use WRITE_ONCE() and READ_ONCE() in the callbacks.

KCSAN reports both sides when the UDP default timeouts are set over
nfnetlink while UDP traffic goes through conntrack, or while the sysctl
is written:

  BUG: KCSAN: data-race in nf_conntrack_udp_packet / udp_timeout_nlattr_to_obj

  write to 0xffffffffb0829bc4 of 4 bytes by task 457 on cpu 1:
   udp_timeout_nlattr_to_obj+0x3b/0xf0
   ctnl_timeout_parse_policy+0xc0/0x100
   cttimeout_default_set+0x8f/0xc0
   nfnetlink_rcv_msg+0x64c/0xc90
   netlink_rcv_skb+0x9c/0x1b0
   nfnetlink_rcv+0x9b/0x1e0
   netlink_unicast+0x418/0x5c0
   netlink_sendmsg+0x3fc/0x750
   ...

  read to 0xffffffffb0829bc4 of 4 bytes by task 459 on cpu 0:
   nf_conntrack_udp_packet+0x27b/0x550
   nf_conntrack_in+0x767/0xb20
   ipv4_conntrack_local+0xb0/0x120
   nf_hook_slow+0x7e/0x180
   ...

  value changed: 0x00001e46 -> 0x00001d4c

  BUG: KCSAN: data-race in proc_int_u2k_conv_uop / udp_timeout_nlattr_to_obj

  write (marked) to 0xffffffffb0829bc4 of 4 bytes by task 475 on cpu 1:
   proc_int_u2k_conv_uop+0x92/0xa0
   ...
   proc_dointvec_jiffies+0x15/0x30
   proc_sys_call_handler+0x282/0x3a0
   proc_sys_write+0x13/0x20
   ...

  read to 0xffffffffb0829bc4 of 4 bytes by task 474 on cpu 0:
   udp_timeout_nlattr_to_obj+0x2c/0xf0
   ctnl_timeout_parse_policy+0xc0/0x100
   cttimeout_default_set+0x8f/0xc0
   nfnetlink_rcv_msg+0x64c/0xc90
   ...

  value changed: 0x00001d4c -> 0x00001e46

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 net/netfilter/nf_conntrack_proto_generic.c |  8 +--
 net/netfilter/nf_conntrack_proto_gre.c     | 16 ++---
 net/netfilter/nf_conntrack_proto_icmp.c    |  8 +--
 net/netfilter/nf_conntrack_proto_icmpv6.c  |  8 +--
 net/netfilter/nf_conntrack_proto_sctp.c    |  9 +--
 net/netfilter/nf_conntrack_proto_tcp.c     | 71 +++++++++++-----------
 net/netfilter/nf_conntrack_proto_udp.c     | 16 ++---
 7 files changed, 69 insertions(+), 67 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_generic.c b/net/netfilter/nf_conntrack_proto_generic.c
index cb260eb3d012c..08b8d44c9f402 100644
--- a/net/netfilter/nf_conntrack_proto_generic.c
+++ b/net/netfilter/nf_conntrack_proto_generic.c
@@ -27,11 +27,11 @@ static int generic_timeout_nlattr_to_obj(struct nlattr *tb[],
 		timeout = &gn->timeout;
 
 	if (tb[CTA_TIMEOUT_GENERIC_TIMEOUT])
-		*timeout =
-		    ntohl(nla_get_be32(tb[CTA_TIMEOUT_GENERIC_TIMEOUT])) * HZ;
+		WRITE_ONCE(*timeout,
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_GENERIC_TIMEOUT])) * HZ);
 	else {
 		/* Set default generic timeout. */
-		*timeout = gn->timeout;
+		WRITE_ONCE(*timeout, READ_ONCE(gn->timeout));
 	}
 
 	return 0;
@@ -42,7 +42,7 @@ generic_timeout_obj_to_nlattr(struct sk_buff *skb, const void *data)
 {
 	const unsigned int *timeout = data;
 
-	if (nla_put_be32(skb, CTA_TIMEOUT_GENERIC_TIMEOUT, htonl(*timeout / HZ)))
+	if (nla_put_be32(skb, CTA_TIMEOUT_GENERIC_TIMEOUT, htonl(READ_ONCE(*timeout) / HZ)))
 		goto nla_put_failure;
 
 	return 0;
diff --git a/net/netfilter/nf_conntrack_proto_gre.c b/net/netfilter/nf_conntrack_proto_gre.c
index d8bca1e29797b..e1b99c5a7c7a2 100644
--- a/net/netfilter/nf_conntrack_proto_gre.c
+++ b/net/netfilter/nf_conntrack_proto_gre.c
@@ -318,16 +318,16 @@ static int gre_timeout_nlattr_to_obj(struct nlattr *tb[],
 	if (!timeouts)
 		timeouts = gre_get_timeouts(net);
 	/* set default timeouts for GRE. */
-	timeouts[GRE_CT_UNREPLIED] = net_gre->timeouts[GRE_CT_UNREPLIED];
-	timeouts[GRE_CT_REPLIED] = net_gre->timeouts[GRE_CT_REPLIED];
+	WRITE_ONCE(timeouts[GRE_CT_UNREPLIED], READ_ONCE(net_gre->timeouts[GRE_CT_UNREPLIED]));
+	WRITE_ONCE(timeouts[GRE_CT_REPLIED], READ_ONCE(net_gre->timeouts[GRE_CT_REPLIED]));
 
 	if (tb[CTA_TIMEOUT_GRE_UNREPLIED]) {
-		timeouts[GRE_CT_UNREPLIED] =
-			ntohl(nla_get_be32(tb[CTA_TIMEOUT_GRE_UNREPLIED])) * HZ;
+		WRITE_ONCE(timeouts[GRE_CT_UNREPLIED],
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_GRE_UNREPLIED])) * HZ);
 	}
 	if (tb[CTA_TIMEOUT_GRE_REPLIED]) {
-		timeouts[GRE_CT_REPLIED] =
-			ntohl(nla_get_be32(tb[CTA_TIMEOUT_GRE_REPLIED])) * HZ;
+		WRITE_ONCE(timeouts[GRE_CT_REPLIED],
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_GRE_REPLIED])) * HZ);
 	}
 	return 0;
 }
@@ -338,9 +338,9 @@ gre_timeout_obj_to_nlattr(struct sk_buff *skb, const void *data)
 	const unsigned int *timeouts = data;
 
 	if (nla_put_be32(skb, CTA_TIMEOUT_GRE_UNREPLIED,
-			 htonl(timeouts[GRE_CT_UNREPLIED] / HZ)) ||
+			 htonl(READ_ONCE(timeouts[GRE_CT_UNREPLIED]) / HZ)) ||
 	    nla_put_be32(skb, CTA_TIMEOUT_GRE_REPLIED,
-			 htonl(timeouts[GRE_CT_REPLIED] / HZ)))
+			 htonl(READ_ONCE(timeouts[GRE_CT_REPLIED]) / HZ)))
 		goto nla_put_failure;
 	return 0;
 
diff --git a/net/netfilter/nf_conntrack_proto_icmp.c b/net/netfilter/nf_conntrack_proto_icmp.c
index 0f6ef0443c6fa..0d8499b54f878 100644
--- a/net/netfilter/nf_conntrack_proto_icmp.c
+++ b/net/netfilter/nf_conntrack_proto_icmp.c
@@ -327,11 +327,11 @@ static int icmp_timeout_nlattr_to_obj(struct nlattr *tb[],
 	if (tb[CTA_TIMEOUT_ICMP_TIMEOUT]) {
 		if (!timeout)
 			timeout = &in->timeout;
-		*timeout =
-			ntohl(nla_get_be32(tb[CTA_TIMEOUT_ICMP_TIMEOUT])) * HZ;
+		WRITE_ONCE(*timeout,
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_ICMP_TIMEOUT])) * HZ);
 	} else if (timeout) {
 		/* Set default ICMP timeout. */
-		*timeout = in->timeout;
+		WRITE_ONCE(*timeout, READ_ONCE(in->timeout));
 	}
 	return 0;
 }
@@ -341,7 +341,7 @@ icmp_timeout_obj_to_nlattr(struct sk_buff *skb, const void *data)
 {
 	const unsigned int *timeout = data;
 
-	if (nla_put_be32(skb, CTA_TIMEOUT_ICMP_TIMEOUT, htonl(*timeout / HZ)))
+	if (nla_put_be32(skb, CTA_TIMEOUT_ICMP_TIMEOUT, htonl(READ_ONCE(*timeout) / HZ)))
 		goto nla_put_failure;
 	return 0;
 
diff --git a/net/netfilter/nf_conntrack_proto_icmpv6.c b/net/netfilter/nf_conntrack_proto_icmpv6.c
index e4eebf13950cb..6ed0f7ba60c3d 100644
--- a/net/netfilter/nf_conntrack_proto_icmpv6.c
+++ b/net/netfilter/nf_conntrack_proto_icmpv6.c
@@ -305,11 +305,11 @@ static int icmpv6_timeout_nlattr_to_obj(struct nlattr *tb[],
 	if (!timeout)
 		timeout = icmpv6_get_timeouts(net);
 	if (tb[CTA_TIMEOUT_ICMPV6_TIMEOUT]) {
-		*timeout =
-		    ntohl(nla_get_be32(tb[CTA_TIMEOUT_ICMPV6_TIMEOUT])) * HZ;
+		WRITE_ONCE(*timeout,
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_ICMPV6_TIMEOUT])) * HZ);
 	} else {
 		/* Set default ICMPv6 timeout. */
-		*timeout = in->timeout;
+		WRITE_ONCE(*timeout, READ_ONCE(in->timeout));
 	}
 	return 0;
 }
@@ -319,7 +319,7 @@ icmpv6_timeout_obj_to_nlattr(struct sk_buff *skb, const void *data)
 {
 	const unsigned int *timeout = data;
 
-	if (nla_put_be32(skb, CTA_TIMEOUT_ICMPV6_TIMEOUT, htonl(*timeout / HZ)))
+	if (nla_put_be32(skb, CTA_TIMEOUT_ICMPV6_TIMEOUT, htonl(READ_ONCE(*timeout) / HZ)))
 		goto nla_put_failure;
 	return 0;
 
diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
index 047bf69113bf9..ea6dd1ef3df89 100644
--- a/net/netfilter/nf_conntrack_proto_sctp.c
+++ b/net/netfilter/nf_conntrack_proto_sctp.c
@@ -650,16 +650,17 @@ static int sctp_timeout_nlattr_to_obj(struct nlattr *tb[],
 
 	/* set default SCTP timeouts. */
 	for (i=0; i<SCTP_CONNTRACK_MAX; i++)
-		timeouts[i] = sn->timeouts[i];
+		WRITE_ONCE(timeouts[i], READ_ONCE(sn->timeouts[i]));
 
 	/* there's a 1:1 mapping between attributes and protocol states. */
 	for (i=CTA_TIMEOUT_SCTP_UNSPEC+1; i<CTA_TIMEOUT_SCTP_MAX+1; i++) {
 		if (tb[i]) {
-			timeouts[i] = ntohl(nla_get_be32(tb[i])) * HZ;
+			WRITE_ONCE(timeouts[i], ntohl(nla_get_be32(tb[i])) * HZ);
 		}
 	}
 
-	timeouts[CTA_TIMEOUT_SCTP_UNSPEC] = timeouts[CTA_TIMEOUT_SCTP_CLOSED];
+	WRITE_ONCE(timeouts[CTA_TIMEOUT_SCTP_UNSPEC],
+		   READ_ONCE(timeouts[CTA_TIMEOUT_SCTP_CLOSED]));
 	return 0;
 }
 
@@ -670,7 +671,7 @@ sctp_timeout_obj_to_nlattr(struct sk_buff *skb, const void *data)
 	int i;
 
 	for (i=CTA_TIMEOUT_SCTP_UNSPEC+1; i<CTA_TIMEOUT_SCTP_MAX+1; i++) {
-	        if (nla_put_be32(skb, i, htonl(timeouts[i] / HZ)))
+		if (nla_put_be32(skb, i, htonl(READ_ONCE(timeouts[i]) / HZ)))
 			goto nla_put_failure;
 	}
         return 0;
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 0c61a16fca400..e1db372b9e530 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -1523,55 +1523,56 @@ static int tcp_timeout_nlattr_to_obj(struct nlattr *tb[],
 		timeouts = tn->timeouts;
 	/* set default TCP timeouts. */
 	for (i=0; i<TCP_CONNTRACK_TIMEOUT_MAX; i++)
-		timeouts[i] = tn->timeouts[i];
+		WRITE_ONCE(timeouts[i], READ_ONCE(tn->timeouts[i]));
 
 	if (tb[CTA_TIMEOUT_TCP_SYN_SENT]) {
-		timeouts[TCP_CONNTRACK_SYN_SENT] =
-			ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_SYN_SENT]))*HZ;
+		WRITE_ONCE(timeouts[TCP_CONNTRACK_SYN_SENT],
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_SYN_SENT])) * HZ);
 	}
 
 	if (tb[CTA_TIMEOUT_TCP_SYN_RECV]) {
-		timeouts[TCP_CONNTRACK_SYN_RECV] =
-			ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_SYN_RECV]))*HZ;
+		WRITE_ONCE(timeouts[TCP_CONNTRACK_SYN_RECV],
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_SYN_RECV])) * HZ);
 	}
 	if (tb[CTA_TIMEOUT_TCP_ESTABLISHED]) {
-		timeouts[TCP_CONNTRACK_ESTABLISHED] =
-			ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_ESTABLISHED]))*HZ;
+		WRITE_ONCE(timeouts[TCP_CONNTRACK_ESTABLISHED],
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_ESTABLISHED])) * HZ);
 	}
 	if (tb[CTA_TIMEOUT_TCP_FIN_WAIT]) {
-		timeouts[TCP_CONNTRACK_FIN_WAIT] =
-			ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_FIN_WAIT]))*HZ;
+		WRITE_ONCE(timeouts[TCP_CONNTRACK_FIN_WAIT],
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_FIN_WAIT])) * HZ);
 	}
 	if (tb[CTA_TIMEOUT_TCP_CLOSE_WAIT]) {
-		timeouts[TCP_CONNTRACK_CLOSE_WAIT] =
-			ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_CLOSE_WAIT]))*HZ;
+		WRITE_ONCE(timeouts[TCP_CONNTRACK_CLOSE_WAIT],
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_CLOSE_WAIT])) * HZ);
 	}
 	if (tb[CTA_TIMEOUT_TCP_LAST_ACK]) {
-		timeouts[TCP_CONNTRACK_LAST_ACK] =
-			ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_LAST_ACK]))*HZ;
+		WRITE_ONCE(timeouts[TCP_CONNTRACK_LAST_ACK],
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_LAST_ACK])) * HZ);
 	}
 	if (tb[CTA_TIMEOUT_TCP_TIME_WAIT]) {
-		timeouts[TCP_CONNTRACK_TIME_WAIT] =
-			ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_TIME_WAIT]))*HZ;
+		WRITE_ONCE(timeouts[TCP_CONNTRACK_TIME_WAIT],
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_TIME_WAIT])) * HZ);
 	}
 	if (tb[CTA_TIMEOUT_TCP_CLOSE]) {
-		timeouts[TCP_CONNTRACK_CLOSE] =
-			ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_CLOSE]))*HZ;
+		WRITE_ONCE(timeouts[TCP_CONNTRACK_CLOSE],
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_CLOSE])) * HZ);
 	}
 	if (tb[CTA_TIMEOUT_TCP_SYN_SENT2]) {
-		timeouts[TCP_CONNTRACK_SYN_SENT2] =
-			ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_SYN_SENT2]))*HZ;
+		WRITE_ONCE(timeouts[TCP_CONNTRACK_SYN_SENT2],
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_SYN_SENT2])) * HZ);
 	}
 	if (tb[CTA_TIMEOUT_TCP_RETRANS]) {
-		timeouts[TCP_CONNTRACK_RETRANS] =
-			ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_RETRANS]))*HZ;
+		WRITE_ONCE(timeouts[TCP_CONNTRACK_RETRANS],
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_RETRANS])) * HZ);
 	}
 	if (tb[CTA_TIMEOUT_TCP_UNACK]) {
-		timeouts[TCP_CONNTRACK_UNACK] =
-			ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_UNACK]))*HZ;
+		WRITE_ONCE(timeouts[TCP_CONNTRACK_UNACK],
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_TCP_UNACK])) * HZ);
 	}
 
-	timeouts[CTA_TIMEOUT_TCP_UNSPEC] = timeouts[CTA_TIMEOUT_TCP_SYN_SENT];
+	WRITE_ONCE(timeouts[CTA_TIMEOUT_TCP_UNSPEC],
+		   READ_ONCE(timeouts[CTA_TIMEOUT_TCP_SYN_SENT]));
 	return 0;
 }
 
@@ -1581,27 +1582,27 @@ tcp_timeout_obj_to_nlattr(struct sk_buff *skb, const void *data)
 	const unsigned int *timeouts = data;
 
 	if (nla_put_be32(skb, CTA_TIMEOUT_TCP_SYN_SENT,
-			htonl(timeouts[TCP_CONNTRACK_SYN_SENT] / HZ)) ||
+			htonl(READ_ONCE(timeouts[TCP_CONNTRACK_SYN_SENT]) / HZ)) ||
 	    nla_put_be32(skb, CTA_TIMEOUT_TCP_SYN_RECV,
-			 htonl(timeouts[TCP_CONNTRACK_SYN_RECV] / HZ)) ||
+			 htonl(READ_ONCE(timeouts[TCP_CONNTRACK_SYN_RECV]) / HZ)) ||
 	    nla_put_be32(skb, CTA_TIMEOUT_TCP_ESTABLISHED,
-			 htonl(timeouts[TCP_CONNTRACK_ESTABLISHED] / HZ)) ||
+			 htonl(READ_ONCE(timeouts[TCP_CONNTRACK_ESTABLISHED]) / HZ)) ||
 	    nla_put_be32(skb, CTA_TIMEOUT_TCP_FIN_WAIT,
-			 htonl(timeouts[TCP_CONNTRACK_FIN_WAIT] / HZ)) ||
+			 htonl(READ_ONCE(timeouts[TCP_CONNTRACK_FIN_WAIT]) / HZ)) ||
 	    nla_put_be32(skb, CTA_TIMEOUT_TCP_CLOSE_WAIT,
-			 htonl(timeouts[TCP_CONNTRACK_CLOSE_WAIT] / HZ)) ||
+			 htonl(READ_ONCE(timeouts[TCP_CONNTRACK_CLOSE_WAIT]) / HZ)) ||
 	    nla_put_be32(skb, CTA_TIMEOUT_TCP_LAST_ACK,
-			 htonl(timeouts[TCP_CONNTRACK_LAST_ACK] / HZ)) ||
+			 htonl(READ_ONCE(timeouts[TCP_CONNTRACK_LAST_ACK]) / HZ)) ||
 	    nla_put_be32(skb, CTA_TIMEOUT_TCP_TIME_WAIT,
-			 htonl(timeouts[TCP_CONNTRACK_TIME_WAIT] / HZ)) ||
+			 htonl(READ_ONCE(timeouts[TCP_CONNTRACK_TIME_WAIT]) / HZ)) ||
 	    nla_put_be32(skb, CTA_TIMEOUT_TCP_CLOSE,
-			 htonl(timeouts[TCP_CONNTRACK_CLOSE] / HZ)) ||
+			 htonl(READ_ONCE(timeouts[TCP_CONNTRACK_CLOSE]) / HZ)) ||
 	    nla_put_be32(skb, CTA_TIMEOUT_TCP_SYN_SENT2,
-			 htonl(timeouts[TCP_CONNTRACK_SYN_SENT2] / HZ)) ||
+			 htonl(READ_ONCE(timeouts[TCP_CONNTRACK_SYN_SENT2]) / HZ)) ||
 	    nla_put_be32(skb, CTA_TIMEOUT_TCP_RETRANS,
-			 htonl(timeouts[TCP_CONNTRACK_RETRANS] / HZ)) ||
+			 htonl(READ_ONCE(timeouts[TCP_CONNTRACK_RETRANS]) / HZ)) ||
 	    nla_put_be32(skb, CTA_TIMEOUT_TCP_UNACK,
-			 htonl(timeouts[TCP_CONNTRACK_UNACK] / HZ)))
+			 htonl(READ_ONCE(timeouts[TCP_CONNTRACK_UNACK]) / HZ)))
 		goto nla_put_failure;
 	return 0;
 
diff --git a/net/netfilter/nf_conntrack_proto_udp.c b/net/netfilter/nf_conntrack_proto_udp.c
index 99eedaa2b1183..777b21da3bcff 100644
--- a/net/netfilter/nf_conntrack_proto_udp.c
+++ b/net/netfilter/nf_conntrack_proto_udp.c
@@ -155,16 +155,16 @@ static int udp_timeout_nlattr_to_obj(struct nlattr *tb[],
 		timeouts = un->timeouts;
 
 	/* set default timeouts for UDP. */
-	timeouts[UDP_CT_UNREPLIED] = un->timeouts[UDP_CT_UNREPLIED];
-	timeouts[UDP_CT_REPLIED] = un->timeouts[UDP_CT_REPLIED];
+	WRITE_ONCE(timeouts[UDP_CT_UNREPLIED], READ_ONCE(un->timeouts[UDP_CT_UNREPLIED]));
+	WRITE_ONCE(timeouts[UDP_CT_REPLIED], READ_ONCE(un->timeouts[UDP_CT_REPLIED]));
 
 	if (tb[CTA_TIMEOUT_UDP_UNREPLIED]) {
-		timeouts[UDP_CT_UNREPLIED] =
-			ntohl(nla_get_be32(tb[CTA_TIMEOUT_UDP_UNREPLIED])) * HZ;
+		WRITE_ONCE(timeouts[UDP_CT_UNREPLIED],
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_UDP_UNREPLIED])) * HZ);
 	}
 	if (tb[CTA_TIMEOUT_UDP_REPLIED]) {
-		timeouts[UDP_CT_REPLIED] =
-			ntohl(nla_get_be32(tb[CTA_TIMEOUT_UDP_REPLIED])) * HZ;
+		WRITE_ONCE(timeouts[UDP_CT_REPLIED],
+			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_UDP_REPLIED])) * HZ);
 	}
 	return 0;
 }
@@ -175,9 +175,9 @@ udp_timeout_obj_to_nlattr(struct sk_buff *skb, const void *data)
 	const unsigned int *timeouts = data;
 
 	if (nla_put_be32(skb, CTA_TIMEOUT_UDP_UNREPLIED,
-			 htonl(timeouts[UDP_CT_UNREPLIED] / HZ)) ||
+			 htonl(READ_ONCE(timeouts[UDP_CT_UNREPLIED]) / HZ)) ||
 	    nla_put_be32(skb, CTA_TIMEOUT_UDP_REPLIED,
-			 htonl(timeouts[UDP_CT_REPLIED] / HZ)))
+			 htonl(READ_ONCE(timeouts[UDP_CT_REPLIED]) / HZ)))
 		goto nla_put_failure;
 	return 0;
 
-- 
2.43.0

Re: [PATCH net-next 14/14] netfilter: cttimeout: annotate data-races around the default timeouts

[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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] A concurrent sysctl write to a default conntrack timeout can
  still be silently undone.

--

netfilter: cttimeout: annotate data-races around the default timeouts

This commit changes the l4proto nlattr_to_obj() and obj_to_nlattr()
timeout callbacks to use WRITE_ONCE() and READ_ONCE() instead of plain
accesses. It targets races between cttimeout_default_set() and
cttimeout_default_get(), the conntrack packet path, and the
nf_conntrack_*_timeout* sysctl handlers, which all touch the same
per-netns default timeout arrays.

> diff --git a/net/netfilter/nf_conntrack_proto_udp.c b/net/netfilter/nf_conntrack_proto_udp.c
> index 99eedaa2b1183..777b21da3bcff 100644
> --- a/net/netfilter/nf_conntrack_proto_udp.c
> +++ b/net/netfilter/nf_conntrack_proto_udp.c
> @@ -155,16 +155,16 @@ static int udp_timeout_nlattr_to_obj(struct nlattr *tb[],
>  		timeouts = un->timeouts;
>  
>  	/* set default timeouts for UDP. */
> -	timeouts[UDP_CT_UNREPLIED] = un->timeouts[UDP_CT_UNREPLIED];
> -	timeouts[UDP_CT_REPLIED] = un->timeouts[UDP_CT_REPLIED];
> +	WRITE_ONCE(timeouts[UDP_CT_UNREPLIED], READ_ONCE(un->timeouts[UDP_CT_UNREPLIED]));
> +	WRITE_ONCE(timeouts[UDP_CT_REPLIED], READ_ONCE(un->timeouts[UDP_CT_REPLIED]));

[Severity: Medium]

Can a concurrent sysctl write still be lost here?

cttimeout_default_set() passes a NULL destination:

net/netfilter/nfnetlink_cttimeout.c:cttimeout_default_set() {
    ...
	ret = ctnl_timeout_parse_policy(NULL, l4proto, info->net,
					cda[CTA_TIMEOUT_DATA]);
    ...
}

So udp_timeout_nlattr_to_obj() sets timeouts to un->timeouts, and the
default copy turns into a self-copy of the live per-netns array:

	WRITE_ONCE(un->timeouts[UDP_CT_REPLIED],
		   READ_ONCE(un->timeouts[UDP_CT_REPLIED]));

That is still a separate load and store. The sysctl handler writes the
same field through proc_dointvec_jiffies() and does not take the nfnl
mutex. Take a request that carries only CTA_TIMEOUT_UDP_UNREPLIED, sent
while nf_conntrack_udp_timeout_stream is being written:

CPU0 (IPCTNL_MSG_TIMEOUT_DEFAULT_SET)
  old = READ_ONCE(un->timeouts[UDP_CT_REPLIED])

CPU1 (sysctl write)
  WRITE_ONCE(un->timeouts[UDP_CT_REPLIED], new)

CPU0
  WRITE_ONCE(un->timeouts[UDP_CT_REPLIED], old)

The sysctl value gets reverted, even though the netlink request never
named that attribute.

The second KCSAN splat in the commit message (proc_int_u2k_conv_uop vs
udp_timeout_nlattr_to_obj) comes from this access pair. Once both sides
are marked, KCSAN stops reporting it, but the update can still be lost.

The same self-copy happens in these places:

  tcp_timeout_nlattr_to_obj(), where timeouts = tn->timeouts and the
  loop covers all TCP_CONNTRACK_TIMEOUT_MAX entries
  sctp_timeout_nlattr_to_obj(), where timeouts = sn->timeouts
  gre_timeout_nlattr_to_obj(), where timeouts = gre_get_timeouts(net)
  the else branch of generic_timeout_nlattr_to_obj(), when timeout is
  &gn->timeout
  the else branch of icmpv6_timeout_nlattr_to_obj(), when timeout
  falls back to icmpv6_get_timeouts(net)

When the destination already points at the per-netns array, could the
default copy be skipped, so that fields missing from the request are
never stored?

>  
>  	if (tb[CTA_TIMEOUT_UDP_UNREPLIED]) {
> -		timeouts[UDP_CT_UNREPLIED] =
> -			ntohl(nla_get_be32(tb[CTA_TIMEOUT_UDP_UNREPLIED])) * HZ;
> +		WRITE_ONCE(timeouts[UDP_CT_UNREPLIED],
> +			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_UDP_UNREPLIED])) * HZ);
>  	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109025425.328445-1-jiayuan.chen%40linux.dev

Re: [PATCH net-next 00/14] net: annotate data-races around sysctl knobs

[Jason Xing]

On Mon, Nov 9, 2026 at 10:55 AM Jiayuan Chen <jiayuan.chen@linux.dev> wrote:
>
> While testing the recent sysctl range fixes I noticed that a lot of our
> sysctl knobs are read with plain loads while the handlers update them
> with WRITE_ONCE() and no lock, so KCSAN reports them as data races. I
> then used an LLM to go through every ctl_table under net/ and list the
> readers that were left behind. This series annotates them with
> READ_ONCE(), and adds WRITE_ONCE() to a few custom handlers and netlink
> paths that store to the same fields with plain writes. Most of it is a
> plain READ_ONCE() at each read; where a function reads the same knob
> twice to make one decision, it now reads it once into a local, so the
> comparison and the value it acts on cannot come from two different
> writes.
>
> There are no Fixes tags. Done one knob per patch this would be well over
> a hundred commits, so I grouped them by subsystem and feature instead to
> keep the review manageable. If maintainers prefer, I can split it up
> further with a Fixes tag on each one, as several smaller series.

Over the last year, I've been thinking about the annotation stuff
especially around sysctls, which, I suppose, shouldn't actually affect
the real workload but does affect the outcome of the syzbot.

Out of sheer curiosity, my question would be what is the point of
trying to change all of them? Or what kind of benefits can we get from
the big series?

Thanks,
Jason

>
> No functional change. KCSAN does hit these, e.g. toggling
> net.ipv6.icmp.echo_ignore_all while receiving ICMPv6 echo requests:
>
>   BUG: KCSAN: data-race in icmpv6_rcv / proc_dou8vec_minmax
>
>   write (marked) to 0xffffffff8ee2940d of 1 bytes by task 504 on cpu 0:
>    proc_dou8vec_minmax+0x1ab/0x1f0
>    proc_sys_call_handler+0x282/0x3a0
>    proc_sys_write+0x13/0x20
>    ...
>
>   read to 0xffffffff8ee2940d of 1 bytes by interrupt on cpu 3:
>    icmpv6_rcv+0x99d/0xd40
>    ip6_protocol_deliver_rcu+0x787/0x9a0
>    ip6_input_finish+0xb0/0x1b0
>    ip6_input+0xa8/0x250
>    ipv6_rcv+0x14b/0x1c0
>    ...
>
>   value changed: 0x01 -> 0x00
>
> Jiayuan Chen (14):
>   ipv4: annotate data-races around ip_rt_* sysctls
>   inet: frags: annotate data-races around fqdir->timeout and
>     fqdir->max_dist
>   ipv6: annotate data-races around addrconf sysctls
>   ipv6: annotate data-races around icmpv6 reply sysctls
>   calipso: annotate data-races around cache sysctls
>   sctp: annotate data-races around sctp sysctls
>   mptcp: annotate data-races around pernet sysctls
>   rds: annotate data-races around core rds sysctls
>   netfilter: conntrack: annotate data-races around conntrack sysctls
>   netfilter: bridge: annotate data-races around br_netfilter and nf_log
>     sysctls
>   ipvs: annotate data-races around lblc_expiration and lblcr_expiration
>   net/smc: annotate data-races around smc sysctls
>   netfilter: conntrack: annotate data-races around timeout and tcp
>     sysctls
>   netfilter: cttimeout: annotate data-races around the default timeouts
>
>  include/net/ipv6.h                            |   2 +-
>  include/net/netfilter/nf_conntrack_acct.h     |   6 +-
>  .../net/netfilter/nf_conntrack_timestamp.h    |   2 +-
>  net/bridge/br_netfilter_hooks.c               |  20 ++--
>  net/bridge/netfilter/ebt_log.c                |   2 +-
>  net/ipv4/inet_fragment.c                      |   2 +-
>  net/ipv4/ip_fragment.c                        |   6 +-
>  net/ipv4/route.c                              |  34 +++---
>  net/ipv6/addrconf.c                           |  22 ++--
>  net/ipv6/calipso.c                            |   6 +-
>  net/ipv6/icmp.c                               |  10 +-
>  net/ipv6/ndisc.c                              |   2 +-
>  net/ipv6/route.c                              |   8 +-
>  net/mptcp/ctrl.c                              |  18 +--
>  net/netfilter/ipvs/ip_vs_lblc.c               |   2 +-
>  net/netfilter/ipvs/ip_vs_lblcr.c              |   2 +-
>  net/netfilter/nf_conntrack_core.c             |   8 +-
>  net/netfilter/nf_conntrack_ecache.c           |   2 +-
>  net/netfilter/nf_conntrack_expect.c           |   2 +-
>  net/netfilter/nf_conntrack_netlink.c          |   2 +-
>  net/netfilter/nf_conntrack_proto.c            |   6 +-
>  net/netfilter/nf_conntrack_proto_generic.c    |   8 +-
>  net/netfilter/nf_conntrack_proto_gre.c        |  20 ++--
>  net/netfilter/nf_conntrack_proto_icmp.c       |  12 +-
>  net/netfilter/nf_conntrack_proto_icmpv6.c     |  12 +-
>  net/netfilter/nf_conntrack_proto_sctp.c       |  13 ++-
>  net/netfilter/nf_conntrack_proto_tcp.c        | 110 +++++++++---------
>  net/netfilter/nf_conntrack_proto_udp.c        |  24 ++--
>  net/netfilter/nf_flow_table_core.c            |   4 +-
>  net/netfilter/nf_log_syslog.c                 |   2 +-
>  net/rds/recv.c                                |   2 +-
>  net/rds/send.c                                |   8 +-
>  net/rds/threads.c                             |   4 +-
>  net/sctp/associola.c                          |   2 +-
>  net/sctp/bind_addr.c                          |   2 +-
>  net/sctp/endpointola.c                        |  14 +--
>  net/sctp/input.c                              |   2 +-
>  net/sctp/sm_make_chunk.c                      |   6 +-
>  net/sctp/sm_sideeffect.c                      |   2 +-
>  net/sctp/sm_statefuns.c                       |   6 +-
>  net/sctp/socket.c                             |  46 ++++----
>  net/sctp/sysctl.c                             |  16 +--
>  net/sctp/transport.c                          |   6 +-
>  net/smc/af_smc.c                              |   8 +-
>  net/smc/smc_clc.c                             |   8 +-
>  net/smc/smc_core.c                            |   2 +-
>  net/smc/smc_tx.c                              |   2 +-
>  47 files changed, 259 insertions(+), 246 deletions(-)
>
> --
> 2.43.0
>
>

Re: [PATCH net-next 00/14] net: annotate data-races around sysctl knobs

[Jiayuan Chen]

On 11/9/26 11:11 AM, Jason Xing wrote:
> On Mon, Nov 9, 2026 at 10:55 AM Jiayuan Chen <jiayuan.chen@linux.dev> wrote:
>> While testing the recent sysctl range fixes I noticed that a lot of our
>> sysctl knobs are read with plain loads while the handlers update them
>> with WRITE_ONCE() and no lock, so KCSAN reports them as data races. I
>> then used an LLM to go through every ctl_table under net/ and list the
>> readers that were left behind. This series annotates them with
>> READ_ONCE(), and adds WRITE_ONCE() to a few custom handlers and netlink
>> paths that store to the same fields with plain writes. Most of it is a
>> plain READ_ONCE() at each read; where a function reads the same knob
>> twice to make one decision, it now reads it once into a local, so the
>> comparison and the value it acts on cannot come from two different
>> writes.
>>
>> There are no Fixes tags. Done one knob per patch this would be well over
>> a hundred commits, so I grouped them by subsystem and feature instead to
>> keep the review manageable. If maintainers prefer, I can split it up
>> further with a Fixes tag on each one, as several smaller series.
> Over the last year, I've been thinking about the annotation stuff
> especially around sysctls, which, I suppose, shouldn't actually affect
> the real workload but does affect the outcome of the syzbot.
>
> Out of sheer curiosity, my question would be what is the point of
> trying to change all of them? Or what kind of benefits can we get from
> the big series?


Three things:

1. A plain load may be emitted twice by the compiler. ip_error() compares
peer->rate_tokens against ip_rt_error_cost and then subtracts it again;
if the sysctl changes in between, the u32 wraps. A few readers have that
shape and now read the knob once into a local.

2. Tearing is not involved here, all the fields are at most word sized.

3. The rest is the memory model: a lockless read of a field written with
WRITE_ONCE() is a data race unless it is marked
(tools/memory-model/Documentation/access-marking.txt). That is what KCSAN
reports, and why most of the series is plain READ_ONCE().


I think the benefit is doing it in one pass.

Re: [PATCH net-next 00/14] net: annotate data-races around sysctl knobs

[Jason Xing]

On Mon, Nov 9, 2026 at 11:33 AM Jiayuan Chen <jiayuan.chen@linux.dev> wrote:
>
>
> On 11/9/26 11:11 AM, Jason Xing wrote:
> > On Mon, Nov 9, 2026 at 10:55 AM Jiayuan Chen <jiayuan.chen@linux.dev> wrote:
> >> While testing the recent sysctl range fixes I noticed that a lot of our
> >> sysctl knobs are read with plain loads while the handlers update them
> >> with WRITE_ONCE() and no lock, so KCSAN reports them as data races. I
> >> then used an LLM to go through every ctl_table under net/ and list the
> >> readers that were left behind. This series annotates them with
> >> READ_ONCE(), and adds WRITE_ONCE() to a few custom handlers and netlink
> >> paths that store to the same fields with plain writes. Most of it is a
> >> plain READ_ONCE() at each read; where a function reads the same knob
> >> twice to make one decision, it now reads it once into a local, so the
> >> comparison and the value it acts on cannot come from two different
> >> writes.
> >>
> >> There are no Fixes tags. Done one knob per patch this would be well over
> >> a hundred commits, so I grouped them by subsystem and feature instead to
> >> keep the review manageable. If maintainers prefer, I can split it up
> >> further with a Fixes tag on each one, as several smaller series.
> > Over the last year, I've been thinking about the annotation stuff
> > especially around sysctls, which, I suppose, shouldn't actually affect
> > the real workload but does affect the outcome of the syzbot.
> >
> > Out of sheer curiosity, my question would be what is the point of
> > trying to change all of them? Or what kind of benefits can we get from
> > the big series?
>
>
> Three things:
>
> 1. A plain load may be emitted twice by the compiler. ip_error() compares
> peer->rate_tokens against ip_rt_error_cost and then subtracts it again;
> if the sysctl changes in between, the u32 wraps. A few readers have that
> shape and now read the knob once into a local.
>
> 2. Tearing is not involved here, all the fields are at most word sized.
>
> 3. The rest is the memory model: a lockless read of a field written with
> WRITE_ONCE() is a data race unless it is marked
> (tools/memory-model/Documentation/access-marking.txt). That is what KCSAN
> reports, and why most of the series is plain READ_ONCE().
>
>
> I think the benefit is doing it in one pass.
>
>

I was curious about the REAL impact. If this thing really matters, we
have a lot of work to do even not only in the networking subsystem.
Sorry, I still think none of those points are convincing enough, but
that doesn't mean I object to this series. Anyway, it's just my
2-cents.

Thanks,
Jason