Re: [PATCH net-next] ipvs: annotate the lockless read of sysctl_sync_threshold[0]

[Simon Horman <horms@kernel.org>]

On Mon, Oct 26, 2026 at 04:37:08PM +0800, Linkui Xiao wrote:
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> sysctl_sync_threshold() reads ipvs->sysctl_sync_threshold[0] from the
> packet path without any synchronization: ip_vs_sync_conn_needed(),
> ip_vs_sync_conn_v0() and ip_vs_sync_conn() run in softirq context on
> behalf of ip_vs_in_hook(), ip_vs_conn_expire() is a timer callback and
> ip_vs_proc_conn() runs in the backup sync kthread. The only writer,
> proc_do_sync_threshold(), updates the array with a plain memcpy() while
> holding ipvs->sync_mutex, a mutex none of the readers ever take.
> 
> Commit 749c42b620a9 ("ipvs: reduce sync rate with time thresholds")
> already recognized this race, but only for the second element of the
> array: "As the sysctl_sync_period now can be 0 use ACCESS_ONCE to avoid
> division by zero". sysctl_sync_threshold() was left with a plain access
> even though both elements live in the same array and are written by the
> same memcpy().
> 
> A stale value is not dangerous here: the element is only compared
> against a packet counter and stored into cp->in_pkts, and an aligned int
> cannot tear on any supported architecture. What the missing annotation
> does allow is for the compiler to reload the element, so the two
> comparisons in ip_vs_sync_conn_needed() can observe different values
> for one and the same packet, and KCSAN reports the unsynchronized
> access.
> 
> Read the element once with READ_ONCE(), exactly like the adjacent
> sysctl_sync_period() has done since 749c42b620a9.
> 
> Fixes: 749c42b620a9 ("ipvs: reduce sync rate with time thresholds")
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>

Hi,

I agree that adding a READ_ONCE annotation is appropriate in
sysctl_sync_threshold().

But I wonder if it would also be appropriate to call sysctl_sync_threshold()
only once (per-packet) in ip_vs_sync_conn_needed(). Stashing the value in
a local variable if appropriate.

> ---
>  include/net/ip_vs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> index 9a83bff32569..5aa6a94ed104 100644
> --- a/include/net/ip_vs.h
> +++ b/include/net/ip_vs.h
> @@ -1353,7 +1353,7 @@ static inline int sysctl_conn_max(struct netns_ipvs *ipvs)
>  
>  static inline int sysctl_sync_threshold(struct netns_ipvs *ipvs)
>  {
> -	return ipvs->sysctl_sync_threshold[0];
> +	return READ_ONCE(ipvs->sysctl_sync_threshold[0]);
>  }
>  
>  static inline int sysctl_sync_period(struct netns_ipvs *ipvs)
> -- 
> 2.25.1
>