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