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

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

[Linkui Xiao]

Hi Julian, Simon,

Thanks for the reviews.

On Wed, Oct 28, 2026 at 07:15:34PM +0300, Julian Anastasov wrote:
 > 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.

Agreed. v2 replaces the last memcpy() in proc_do_sync_threshold() with
two WRITE_ONCE() stores, one per element. The if/else branches gained
braces to keep checkpatch --strict happy about the now multi-statement
else.

net-next is currently closed and is scheduled to reopen on November 9th
(https://netdev.bots.linux.dev/net-next.html). I will post it as
[PATCH nf-next v2] then.

On Tue, Oct 28, 2026 at 12:49:35PM +0000, Simon Horman wrote:
 > 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.

Agreed, and thank you. v2 reads the value once into a local variable,
sync_threshold, before the branch and uses it in either branch.

One precision while re-checking the code: the two call sites are on
mutually exclusive branches of the same if/else, so a single packet
never reads the value twice at runtime. My v1 commit message overstated
this ("the two comparisons ... can observe different values for one and
the same packet"); v2 rewords that paragraph and notes the rewording in
Changes in v2. Stashing a single snapshot is still the cleaner shape, so
the change is worthwhile regardless.

Thanks,
Linkui Xiao

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

[Julian Anastasov]

	Hello,

On Thu, 29 Oct 2026, Linkui Xiao wrote:

> Hi Julian, Simon,
> 
> Thanks for the reviews.
> 
> On Wed, Oct 28, 2026 at 07:15:34PM +0300, Julian Anastasov wrote:
> > 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.
> 
> Agreed. v2 replaces the last memcpy() in proc_do_sync_threshold() with
> two WRITE_ONCE() stores, one per element. The if/else branches gained
> braces to keep checkpatch --strict happy about the now multi-statement
> else.
> 
> net-next is currently closed and is scheduled to reopen on November 9th
> (https://netdev.bots.linux.dev/net-next.html). I will post it as
> [PATCH nf-next v2] then.
> 
> On Tue, Oct 28, 2026 at 12:49:35PM +0000, Simon Horman wrote:
> > 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.
> 
> Agreed, and thank you. v2 reads the value once into a local variable,
> sync_threshold, before the branch and uses it in either branch.
> 
> One precision while re-checking the code: the two call sites are on
> mutually exclusive branches of the same if/else, so a single packet
> never reads the value twice at runtime. My v1 commit message overstated
> this ("the two comparisons ... can observe different values for one and
> the same packet"); v2 rewords that paragraph and notes the rewording in
> Changes in v2. Stashing a single snapshot is still the cleaner shape, so
> the change is worthwhile regardless.

	IMHO, ip_vs_sync_conn_needed() should not be changed, 
sysctl_sync_threshold() is not called twice in same flow. OTOH,
sysctl_sync_period() is properly stored in sync_period because
it is used twice. 

	But there is still little gap where proc_do_sync_threshold()
changes the two values one after another while ip_vs_sync_conn_needed()
can not take snapshot without using some common lock, eg. some
seqcount should be more appropriate for this. Writers are already
synchronized with the mutex, so ip_vs_sync_conn_needed() can read the two
values into temp vars under read_seqcount_begin()+read_seqcount_retry().
And later to use the temp vars. proc_do_sync_threshold() can use 
write_seqcount_begin() and write_seqcount_end():

	local_bh_disable();
	preempt_disable_nested();
	write_seqcount_begin();
	2 WRITE_ONCE
	write_seqcount_end();
	preempt_enable_nested();
	local_bh_enable();

Regards

--
Julian Anastasov <ja@ssi.bg>