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

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

[Linkui Xiao]

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

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

[Simon Horman]

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
> 

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

[Julian Anastasov]

	Hello,

On Mon, 26 Oct 2026, 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().

	Yes, ACCESS_ONCE was used to solve possible division-by-zero 
problem, not to solve the data race. Even if memcpy is converted
to 2 int copies on some archs (eg. x86) may be it is time to use
2 WRITE_ONCE instead of the last memcpy. Can you include this
change in v2. Post it when net-next opens again by using nf-next tag.

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

Regards

--
Julian Anastasov <ja@ssi.bg>