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

[Simon Horman <horms@kernel.org>]

On Tue, Nov 10, 2026 at 05:53:22AM +0000, netdev-bot+sashiko@kernel.org wrote:
> 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.

This one also looks like it should be addressed.

-- 
pw-bot: changes-requested