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