Re: [PATCH nf v3 2/2] ipvs: use bitops for destination overload state

	Hi Yizhou,

On Mon, 13 Jul 2026, Yizhou Zhao wrote:

> IPVS destination schedulers read the overload state from packet processing
> paths, while connection accounting and destination updates can change it
> concurrently. IP_VS_DEST_F_OVERLOAD currently shares dest->flags with
> IP_VS_DEST_F_AVAILABLE, so plain read-modify-write operations on the two
> independent states can race and lose either update.
> 
> KCSAN reports the race with the SH scheduler and an upper connection
> threshold configured:
> 
>   BUG: KCSAN: data-race in __ip_vs_update_dest / ip_vs_sh_schedule
> 
> IP_VS_DEST_F_AVAILABLE is changed under service_mutex. Keep it in the
> existing flags word, but move the overload state to a separate unsigned
> long and access it with bitops. Use test_bit() in scheduler paths and
> set_bit()/clear_bit() in ip_vs_dest_update_overload(). This serializes the
> overload bit accesses and prevents updates to the available and overload
> states from clobbering each other.
> 
> The destination flags are not exposed by the IPVS sockopt or netlink
> interfaces, so move their definitions out of the UAPI header. Place the
> new overload word next to weight, which keeps the existing flags,
> conn_flags and weight offsets unchanged. On x86-64 this grows struct
> ip_vs_dest from 472 to 480 bytes.
> 
> test_bit() does not add reader-side ordering. Schedulers can still observe
> stale destination state, as they could before this change; this does not
> provide a fresh cross-field snapshot.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: [email protected]
> Reported-by: Yizhou Zhao <[email protected]>
> Reported-by: Yuxiang Yang <[email protected]>
> Reported-by: Ao Wang <[email protected]>
> Reported-by: Xuewei Feng <[email protected]>
> Reported-by: Qi Li <[email protected]>
> Reported-by: Ke Xu <[email protected]>
> Assisted-by: Claude-Code:GLM-5.2
> Suggested-by: Julian Anastasov <[email protected]>
> Signed-off-by: Yizhou Zhao <[email protected]>
> ---
>  include/net/ip_vs.h              | 8 ++++++++
>  include/uapi/linux/ip_vs.h       | 6 ------
>  net/netfilter/ipvs/ip_vs_conn.c  | 7 ++++---
>  net/netfilter/ipvs/ip_vs_dh.c    | 4 ++--
>  net/netfilter/ipvs/ip_vs_fo.c    | 2 +-
>  net/netfilter/ipvs/ip_vs_lblc.c  | 4 ++--
>  net/netfilter/ipvs/ip_vs_lblcr.c | 8 ++++----
>  net/netfilter/ipvs/ip_vs_lc.c    | 2 +-
>  net/netfilter/ipvs/ip_vs_mh.c    | 2 +-
>  net/netfilter/ipvs/ip_vs_nq.c    | 2 +-
>  net/netfilter/ipvs/ip_vs_ovf.c   | 2 +-
>  net/netfilter/ipvs/ip_vs_rr.c    | 2 +-
>  net/netfilter/ipvs/ip_vs_sed.c   | 4 ++--
>  net/netfilter/ipvs/ip_vs_sh.c    | 2 +-
>  net/netfilter/ipvs/ip_vs_twos.c  | 4 ++--
>  net/netfilter/ipvs/ip_vs_wlc.c   | 4 ++--
>  net/netfilter/ipvs/ip_vs_wrr.c   | 2 +-
>  17 files changed, 34 insertions(+), 31 deletions(-)
> 
> diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> index 3fc864a320fb..5e8e55f82b04 100644
> --- a/include/net/ip_vs.h
> +++ b/include/net/ip_vs.h
> @@ -36,6 +36,13 @@
>  #define IP_VS_HDR_INVERSE	1
>  #define IP_VS_HDR_ICMP		2
>  
> +/* Destination Server Flags */
> +#define IP_VS_DEST_F_AVAILABLE	0x0001		/* server is available */
> +
> +enum {
> +	IP_VS_DEST_FL_OVERLOAD,
> +};
> +
>  /* conn_tab limits (as per Kconfig) */
>  #define IP_VS_CONN_TAB_MIN_BITS	8
>  #if BITS_PER_LONG > 32
> @@ -976,6 +983,7 @@ struct ip_vs_dest {
>  	volatile unsigned int	flags;		/* dest status flags */

	Sashiko has some comments that we should fix somehow:

https://sashiko.dev/#/patchset/cover.1783931964.git.zhaoyz24%40mails.tsinghua.edu.cn

	One option is IP_VS_DEST_F_AVAILABLE to become
IP_VS_DEST_CF_AVAILABLE (CF=Config Flag)

>  	atomic_t		conn_flags;	/* flags to copy to conn */
>  	atomic_t		weight;		/* server weight */
> +	unsigned long		flags2;		/* dest status flags */

	unsigned long		cfg_flags;

	We then put IP_VS_DEST_CF_AVAILABLE in this new cache line
that most of the schedulers will not read until dest is selected.
DH even should not check the IP_VS_DEST_F_AVAILABLE flag,
only lblc/lblcr should use this flag.

	We can preserve IP_VS_DEST_F_OVERLOAD in 'flags',
even we may not need to use bitops if we start to use
spin_lock_bh(&dest->dst_lock), as this lock is already
present in the dest structure. See below...

>  	atomic_t		last_weight;	/* server latest weight */
>  	__u16			tun_type;	/* tunnel type */
>  	__be16			tun_port;	/* tunnel port */

> diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
> index fa3fbd597f3f..2591f4e143f8 100644
> --- a/net/netfilter/ipvs/ip_vs_conn.c
> +++ b/net/netfilter/ipvs/ip_vs_conn.c
> @@ -1006,7 +1006,7 @@ __always_inline void ip_vs_dest_update_overload(struct ip_vs_dest *dest)

	We can add new arg 'bool locked'. Also, we will
return false if caller should retry under lock.
It will happen when we change IP_VS_DEST_F_OVERLOAD and
require its changes to be synchronized with the
thresholds and the number of connections.

>  		goto unset;
>  	conns = ip_vs_dest_totalconns(dest);
>  	if (conns >= u) {
> -		dest->flags |= IP_VS_DEST_F_OVERLOAD;
> +		set_bit(IP_VS_DEST_FL_OVERLOAD, &dest->flags2);


	if (conns >= u) {
		if (!locked)
			return false;
		dest->flags |= IP_VS_DEST_F_OVERLOAD;
		return true;
	}

>  		return;
>  	}
>  	/* Low threshold defaults to 75% of upper threshold */
> @@ -1015,7 +1015,8 @@ __always_inline void ip_vs_dest_update_overload(struct ip_vs_dest *dest)
>  		return;
>  
>  unset:
> -	dest->flags &= ~IP_VS_DEST_F_OVERLOAD;
> +	if (test_bit(IP_VS_DEST_FL_OVERLOAD, &dest->flags2))
> +		clear_bit(IP_VS_DEST_FL_OVERLOAD, &dest->flags2);

	if (dest->flags & IP_VS_DEST_F_OVERLOAD) {
		if (!locked)
			return false;
		dest->flags &= ~IP_VS_DEST_F_OVERLOAD;
	}
	return true;

>  }
>  
>  /*
> @@ -1174,7 +1175,7 @@ static inline void ip_vs_unbind_dest(struct ip_vs_conn *cp)
>  		atomic_dec(&dest->persistconns);
>  	}
>  
> -	if (dest->flags & IP_VS_DEST_F_OVERLOAD)
> +	if (test_bit(IP_VS_DEST_FL_OVERLOAD, &dest->flags2))
>  		ip_vs_dest_update_overload(dest);

	if (dest->flags & IP_VS_DEST_F_OVERLOAD) {
		if (!ip_vs_dest_update_overload(dest, false)) {
			spin_lock_bh(&dest->dst_lock);
			ip_vs_dest_update_overload(dest, true);
			spin_unlock_bh(&dest->dst_lock);
		}
	}

	In __ip_vs_update_dest() we will always use lock:

		spin_lock_bh(&dest->dst_lock);
		WRITE_ONCE(dest->u_threshold, udest->u_threshold);
		WRITE_ONCE(dest->l_threshold, udest->l_threshold);
		ip_vs_dest_update_overload(dest, true);
		spin_unlock_bh(&dest->dst_lock);

	The goal is to avoid the lock for the common case
when flag does not change. What do you think?

Regards

--
Julian Anastasov <[email protected]>