Hi Julian,
> On Jul 14, 2026, at 19:16, Julian Anastasov <[email protected]> wrote:
>
>
> 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.
I agree that moving AVAILABLE to a separate cfg_flags word
looks better for the scheduler cacheline footprint than my flags2
placement.
>
> 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?
I looked more closely at the proposed dst_lock retry. I think there
is still a window because the connection counters are modified
outside dst_lock in this approach.
For example, CPU A can bind a connection, take dst_lock, and in
ip_vs_dest_update_overload(dest, true) observe conns >= u. Before it
sets OVERLOAD, CPU B can expire the remaining connections. Since
OVERLOAD is still clear, B skips the unbind helper and does not take
dst_lock. CPU A can then resume and set OVERLOAD after the connection
count has reached zero.
At that point no later unbind is guaranteed to clear the bit, while
schedulers can avoid the destination because it appears overloaded.
The correct solution is to take dst_lock for every bind/unbind, however, it
seems costly. I’m sorry I didn’t find a better solution. Is there a lighter
synchronization scheme you had in mind?
>
> Regards
>
> --
> Julian Anastasov <[email protected]>
Regards,
Yizhou
