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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] sctp: missing READ_ONCE() on cookie_auth_enable in
  proc_sctp_do_hmac_alg()
- [Low] sctp: missing READ_ONCE() in proc_sctp_do_rto_min() and
  proc_sctp_do_rto_max()
--

commit 93a2b54452b64ee91dbf610597cfa8b06cc871d2
Author: Jiayuan Chen <jiayuan.chen@linux.dev>

sctp: annotate data-races around sctp sysctls

This commit adds READ_ONCE() and WRITE_ONCE() annotations to per-netns SCTP
sysctl variables accessed concurrently by readers and sysctl handlers.

> 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;

[Severity: Low]
Should the read path right below in proc_sctp_do_hmac_alg() also use
READ_ONCE(net->sctp.cookie_auth_enable)?

net/sctp/sysctl.c:proc_sctp_do_hmac_alg() {
    ...
	if (net->sctp.cookie_auth_enable)
		tbl.data = (char *)"sha256";
    ...
}

Unlike proc_dointvec(), proc_dostring() only sees the selected string
literal, so a concurrent sysctl read and write of cookie_hmac_alg still
race on net->sctp.cookie_auth_enable.

> @@ -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: Low]
Do proc_sctp_do_rto_min() and proc_sctp_do_rto_max() also need READ_ONCE()
when dereferencing ctl->extra2 and ctl->extra1?

sctp_sysctl_net_register() points rto_min's extra2 at &net->sctp.rto_max
and rto_max's extra1 at &net->sctp.rto_min:

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

Reading or writing rto_min while rto_max is being updated (or vice versa)
still performs an unannotated plain read against the peer handler's
WRITE_ONCE().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109025425.328445-1-jiayuan.chen@linux.dev?part=6