ipvs: fix more races around the overload flag

Sashiko warns that we still have races when the dest overload
thresholds are updated. If we narrow the range between
thresholds the lockless readers (ip_vs_bind_dest and
ip_vs_unbind_dest) may use old threshold values and
as result to skip the required flag change.

To solve the problem ip_vs_bind_dest() and ip_vs_unbind_dest()
will have the chance as before to change the overload
state not only at the exact threshold positions but
at any conn count below the lower and above the upper thresholds.
To make it work without entering the locked region for
every connection, we introduce l_threshold_val. Together with
u_threshold_val we will switch between two states:

1. not overloaded: where l_threshold_val is set to 0
to close the door for more ip_vs_unbind_dest() calls
to clear the overload state.

2. overloaded: where u_threshold_val is set to INT_MAX
to close the door for more ip_vs_bind_dest() calls
to set the overload state.

If a race occurs, multiple CPUs can repeat the state change
but this should be for short time window.

With the introduction of l_threshold_val the
IP_VS_DEST_F_OVERLOAD flag and the 'flags' field are not
needed anymore. We will use the ip_vs_dest_is_overloaded()
helper for all places that read the overload state.

Do not check IP_VS_DEST_CF_AVAILABLE in ip_vs_dh.c because
we do not hold reference to dest after it is removed and
rely on RCU to access the available dests.

To reduce the cacheline accesses while selecting destination,
keep the mostly used fields (weight and l_threshold_val)
at the first read-mostly cacheline together with the hash
nodes.

Fixes: 8f843441c4 ("ipvs: properly update the overload flag on dest edit")
Link: https://sashiko.dev/#/patchset/20260810190621.894119-1-pablo%40netfilter.org
Signed-off-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

include/net/ip_vs.h

@@ -36,9 +36,6 @@
 #define IP_VS_HDR_INVERSE	1
 #define IP_VS_HDR_ICMP		2
 
-/* Destination Server Flags */
-#define IP_VS_DEST_F_OVERLOAD	0x0002		/* server is overloaded */
-
 /* Destination Server Config Flags */
 #define IP_VS_DEST_CF_AVAILABLE	0x0001		/* server is available */
 
@@ -973,33 +970,44 @@ struct ip_vs_dest_dst {
  * and so on.
  */
 struct ip_vs_dest {
+	/* Cacheline for hash table nodes - read-mostly */
+
 	struct list_head	n_list;   /* for the dests in the service */
 	struct hlist_node	d_list;   /* for table with all the dests */
 
-	u16			af;		/* address family */
-	__be16			port;		/* port number of the server */
-	union nf_inet_addr	addr;		/* IP address of the server */
-	volatile unsigned int	flags;		/* dest status flags */
-	atomic_t		conn_flags;	/* flags to copy to conn */
 	atomic_t		weight;		/* server weight */
-	unsigned long		cflags;		/* config flags */
 	atomic_t		last_weight;	/* server latest weight */
+
+	/* connection thresholds */
+	u32			l_threshold_val;/* used lower threshold */
+	u32			u_threshold_val;/* used upper threshold */
+	/* 32/48 */
+	u32			l_threshold;	/* lower threshold */
+	u32			u_threshold;	/* upper threshold */
+
+	unsigned long		cflags;		/* config flags */
+
+	/* 44/64 */
+	atomic_t		conn_flags;	/* flags to copy to conn */
+
 	__u16			tun_type;	/* tunnel type */
 	__be16			tun_port;	/* tunnel port */
 	__u16			tun_flags;	/* tunnel flags */
 
+	u16			af;		/* address family */
+	__be16			port;		/* port number of the server */
+	/* 60/80 */
+	union nf_inet_addr	addr;		/* IP address of the server */
+
+	/* connection counters */
+	atomic_t		totalconns;	/* total connections */
+	atomic_t		activeconns;	/* active connections */
+	atomic_t		persistconns;	/* persistent connections */
+
 	refcount_t		refcnt;		/* reference counter */
 	struct ip_vs_stats      stats;          /* statistics */
 	unsigned long		idle_start;	/* start time, jiffies */
 
-	/* connection counters and thresholds */
-	atomic_t		activeconns;	/* active connections */
-	atomic_t		totalconns;	/* total connections */
-	atomic_t		persistconns;	/* persistent connections */
-	__u32			u_threshold;	/* upper threshold */
-	__u32			l_threshold;	/* lower threshold */
-	__u32			l_threshold_val;/* used lower threshold */
-
 	/* for destination cache */
 	spinlock_t		dst_lock;	/* lock of dst_cache */
 	struct ip_vs_dest_dst __rcu *dest_dst;	/* cached dst info */
@@ -1915,7 +1923,13 @@ static inline void ip_vs_dest_put_and_free(struct ip_vs_dest *dest)
 		kfree(dest);
 }
 
-void ip_vs_dest_update_overload(struct ip_vs_dest *dest, int mode);
+void ip_vs_dest_update_overload(struct ip_vs_dest *dest, bool overload);
+
+/* Check if dest is in overloaded state */
+static inline bool ip_vs_dest_is_overloaded(const struct ip_vs_dest *dest)
+{
+	return READ_ONCE(dest->l_threshold_val);
+}
 
 /* IPVS sync daemon data and function prototypes
  * (from ip_vs_sync.c)

net/netfilter/ipvs/ip_vs_conn.c

@@ -1153,8 +1153,8 @@ ip_vs_bind_dest(struct ip_vs_conn *cp, struct ip_vs_dest *dest)
 		if (!(flags & IP_VS_CONN_F_INACTIVE))
 			atomic_inc(&dest->activeconns);
 		tc = atomic_inc_return(&dest->totalconns);
-		if (tc == READ_ONCE(dest->u_threshold))
-			ip_vs_dest_update_overload(dest, 1);
+		if (tc >= READ_ONCE(dest->u_threshold_val))
+			ip_vs_dest_update_overload(dest, true);
 	} else {
 		/* It is a persistent connection/template, so increase
 		   the persistent connection counter */
@@ -1246,8 +1246,8 @@ static inline void ip_vs_unbind_dest(struct ip_vs_conn *cp)
 		if (!(cp->flags & IP_VS_CONN_F_INACTIVE))
 			atomic_dec(&dest->activeconns);
 		tc = atomic_fetch_dec(&dest->totalconns);
-		if (tc == READ_ONCE(dest->l_threshold_val))
-			ip_vs_dest_update_overload(dest, -1);
+		if (tc <= READ_ONCE(dest->l_threshold_val))
+			ip_vs_dest_update_overload(dest, false);
 	} else {
 		/* It is a persistent connection/template, so decrease
 		   the persistent connection counter */

net/netfilter/ipvs/ip_vs_ctl.c

@@ -1309,32 +1309,32 @@ void ip_vs_stats_free(struct ip_vs_stats *stats)
  * - conns reach u_threshold and exceed it: set the flag
  * - conns go below l_threshold (or 75% of u_threshold): clear the flag
  */
-static void __ip_vs_dest_update_overload(struct ip_vs_dest *dest, int mode)
+static void __ip_vs_dest_update_overload(struct ip_vs_dest *dest, bool overload)
 {
 	int conns;
 	u32 l, u;
 
 	lockdep_assert_held(&dest->dst_lock);
 	u = READ_ONCE(dest->u_threshold);
-	if (!u)
-		goto unset;
-	l = READ_ONCE(dest->l_threshold_val);
-	conns = atomic_read(&dest->totalconns);
-	if (conns >= (mode > 0 ? l : u)) {
-		dest->flags |= IP_VS_DEST_F_OVERLOAD;
-		return;
+	if (u) {
+		/* Low threshold defaults to 75% of upper threshold */
+		l = READ_ONCE(dest->l_threshold) ? : (u - (u >> 2));
+		conns = atomic_read(&dest->totalconns);
+		if (conns >= (overload ? l : u)) {
+			WRITE_ONCE(dest->u_threshold_val, INT_MAX);
+			WRITE_ONCE(dest->l_threshold_val, l);
+			return;
+		}
 	}
-	if (conns >= (mode < 0 ? u : l))
-		return;
 
-unset:
-	dest->flags &= ~IP_VS_DEST_F_OVERLOAD;
+	WRITE_ONCE(dest->u_threshold_val, u ? : INT_MAX);
+	WRITE_ONCE(dest->l_threshold_val, 0);
 }
 
-void ip_vs_dest_update_overload(struct ip_vs_dest *dest, int mode)
+void ip_vs_dest_update_overload(struct ip_vs_dest *dest, bool overload)
 {
 	spin_lock_bh(&dest->dst_lock);
-	__ip_vs_dest_update_overload(dest, mode);
+	__ip_vs_dest_update_overload(dest, overload);
 	spin_unlock_bh(&dest->dst_lock);
 }
 
@@ -1406,15 +1406,13 @@ __ip_vs_update_dest(struct ip_vs_service *svc, struct ip_vs_dest *dest,
 
 	if (READ_ONCE(dest->u_threshold) != udest->u_threshold ||
 	    READ_ONCE(dest->l_threshold) != udest->l_threshold) {
+		bool overload;
+
 		spin_lock_bh(&dest->dst_lock);
 		WRITE_ONCE(dest->u_threshold, udest->u_threshold);
 		WRITE_ONCE(dest->l_threshold, udest->l_threshold);
-		/* Low threshold defaults to 75% of upper threshold */
-		WRITE_ONCE(dest->l_threshold_val,
-			   udest->l_threshold ? :
-			   (udest->u_threshold -
-			    (udest->u_threshold >> 2)));
-		__ip_vs_dest_update_overload(dest, 0);
+		overload = ip_vs_dest_is_overloaded(dest);
+		__ip_vs_dest_update_overload(dest, overload);
 		spin_unlock_bh(&dest->dst_lock);
 	}
 

net/netfilter/ipvs/ip_vs_dh.c

@@ -195,16 +195,6 @@ static int ip_vs_dh_dest_changed(struct ip_vs_service *svc,
 }
 
 
-/*
- *      If the dest flags is set with IP_VS_DEST_F_OVERLOAD,
- *      consider that the server is overloaded here.
- */
-static inline int is_overloaded(struct ip_vs_dest *dest)
-{
-	return dest->flags & IP_VS_DEST_F_OVERLOAD;
-}
-
-
 /*
  *      Destination hashing scheduling
  */
@@ -219,10 +209,8 @@ ip_vs_dh_schedule(struct ip_vs_service *svc, const struct sk_buff *skb,
 
 	s = (struct ip_vs_dh_state *) svc->sched_data;
 	dest = ip_vs_dh_get(svc->af, s, &iph->daddr);
-	if (!dest ||
-	    !(dest->cflags & IP_VS_DEST_CF_AVAILABLE)
-	    || atomic_read(&dest->weight) <= 0
-	    || is_overloaded(dest)) {
+	if (!dest || atomic_read(&dest->weight) <= 0 ||
+	    ip_vs_dest_is_overloaded(dest)) {
 		ip_vs_scheduler_err(svc, "no destination available");
 		return NULL;
 	}

net/netfilter/ipvs/ip_vs_fo.c

@@ -29,7 +29,7 @@ ip_vs_fo_schedule(struct ip_vs_service *svc, const struct sk_buff *skb,
 	 * Find virtual server with highest weight and send it traffic
 	 */
 	list_for_each_entry_rcu(dest, &svc->destinations, n_list) {
-		if (!(dest->flags & IP_VS_DEST_F_OVERLOAD) &&
+		if (!ip_vs_dest_is_overloaded(dest) &&
 		    atomic_read(&dest->weight) > hw) {
 			hweight = dest;
 			hw = atomic_read(&dest->weight);

net/netfilter/ipvs/ip_vs_lblc.c

@@ -418,7 +418,7 @@ __ip_vs_lblc_schedule(struct ip_vs_service *svc)
 	 * new connection.
 	 */
 	list_for_each_entry_rcu(dest, &svc->destinations, n_list) {
-		if (dest->flags & IP_VS_DEST_F_OVERLOAD)
+		if (ip_vs_dest_is_overloaded(dest))
 			continue;
 		if (atomic_read(&dest->weight) > 0) {
 			least = dest;
@@ -433,7 +433,7 @@ __ip_vs_lblc_schedule(struct ip_vs_service *svc)
 	 */
   nextstage:
 	list_for_each_entry_continue_rcu(dest, &svc->destinations, n_list) {
-		if (dest->flags & IP_VS_DEST_F_OVERLOAD)
+		if (ip_vs_dest_is_overloaded(dest))
 			continue;
 
 		doh = ip_vs_dest_conn_overhead(dest);

net/netfilter/ipvs/ip_vs_lblcr.c

@@ -166,7 +166,7 @@ static inline struct ip_vs_dest *ip_vs_dest_set_min(struct ip_vs_dest_set *set)
 	/* select the first destination server, whose weight > 0 */
 	list_for_each_entry_rcu(e, &set->list, list) {
 		least = e->dest;
-		if (least->flags & IP_VS_DEST_F_OVERLOAD)
+		if (ip_vs_dest_is_overloaded(least))
 			continue;
 
 		if ((atomic_read(&least->weight) > 0) &&
@@ -181,7 +181,7 @@ static inline struct ip_vs_dest *ip_vs_dest_set_min(struct ip_vs_dest_set *set)
   nextstage:
 	list_for_each_entry_continue_rcu(e, &set->list, list) {
 		dest = e->dest;
-		if (dest->flags & IP_VS_DEST_F_OVERLOAD)
+		if (ip_vs_dest_is_overloaded(dest))
 			continue;
 
 		doh = ip_vs_dest_conn_overhead(dest);
@@ -580,7 +580,7 @@ __ip_vs_lblcr_schedule(struct ip_vs_service *svc)
 	 * new connection.
 	 */
 	list_for_each_entry_rcu(dest, &svc->destinations, n_list) {
-		if (dest->flags & IP_VS_DEST_F_OVERLOAD)
+		if (ip_vs_dest_is_overloaded(dest))
 			continue;
 
 		if (atomic_read(&dest->weight) > 0) {
@@ -596,7 +596,7 @@ __ip_vs_lblcr_schedule(struct ip_vs_service *svc)
 	 */
   nextstage:
 	list_for_each_entry_continue_rcu(dest, &svc->destinations, n_list) {
-		if (dest->flags & IP_VS_DEST_F_OVERLOAD)
+		if (ip_vs_dest_is_overloaded(dest))
 			continue;
 
 		doh = ip_vs_dest_conn_overhead(dest);

net/netfilter/ipvs/ip_vs_lc.c

@@ -38,7 +38,7 @@ ip_vs_lc_schedule(struct ip_vs_service *svc, const struct sk_buff *skb,
 	 */
 
 	list_for_each_entry_rcu(dest, &svc->destinations, n_list) {
-		if ((dest->flags & IP_VS_DEST_F_OVERLOAD) ||
+		if (ip_vs_dest_is_overloaded(dest) ||
 		    atomic_read(&dest->weight) == 0)
 			continue;
 		doh = ip_vs_dest_conn_overhead(dest);

net/netfilter/ipvs/ip_vs_mh.c

@@ -80,7 +80,7 @@ static inline void generate_hash_secret(hsiphash_key_t *hash1,
 static inline bool is_unavailable(struct ip_vs_dest *dest)
 {
 	return atomic_read(&dest->weight) <= 0 ||
-	       dest->flags & IP_VS_DEST_F_OVERLOAD;
+	       ip_vs_dest_is_overloaded(dest);
 }
 
 /* Returns hash value for IPVS MH entry */

net/netfilter/ipvs/ip_vs_nq.c

@@ -72,7 +72,7 @@ ip_vs_nq_schedule(struct ip_vs_service *svc, const struct sk_buff *skb,
 
 	list_for_each_entry_rcu(dest, &svc->destinations, n_list) {
 
-		if (dest->flags & IP_VS_DEST_F_OVERLOAD ||
+		if (ip_vs_dest_is_overloaded(dest) ||
 		    !atomic_read(&dest->weight))
 			continue;
 

net/netfilter/ipvs/ip_vs_ovf.c

@@ -33,7 +33,7 @@ ip_vs_ovf_schedule(struct ip_vs_service *svc, const struct sk_buff *skb,
 	*/
 	list_for_each_entry_rcu(dest, &svc->destinations, n_list) {
 		w = atomic_read(&dest->weight);
-		if ((dest->flags & IP_VS_DEST_F_OVERLOAD) ||
+		if (ip_vs_dest_is_overloaded(dest) ||
 		    atomic_read(&dest->activeconns) > w ||
 		    w == 0)
 			continue;

net/netfilter/ipvs/ip_vs_rr.c

@@ -66,7 +66,7 @@ ip_vs_rr_schedule(struct ip_vs_service *svc, const struct sk_buff *skb,
 		list_for_each_entry_continue_rcu(dest,
 						 &svc->destinations,
 						 n_list) {
-			if (!(dest->flags & IP_VS_DEST_F_OVERLOAD) &&
+			if (!ip_vs_dest_is_overloaded(dest) &&
 			    atomic_read(&dest->weight) > 0)
 				/* HIT */
 				goto out;

net/netfilter/ipvs/ip_vs_sed.c

@@ -75,7 +75,7 @@ ip_vs_sed_schedule(struct ip_vs_service *svc, const struct sk_buff *skb,
 	 */
 
 	list_for_each_entry_rcu(dest, &svc->destinations, n_list) {
-		if (!(dest->flags & IP_VS_DEST_F_OVERLOAD) &&
+		if (!ip_vs_dest_is_overloaded(dest) &&
 		    atomic_read(&dest->weight) > 0) {
 			least = dest;
 			loh = ip_vs_sed_dest_overhead(least);
@@ -90,7 +90,7 @@ ip_vs_sed_schedule(struct ip_vs_service *svc, const struct sk_buff *skb,
 	 */
   nextstage:
 	list_for_each_entry_continue_rcu(dest, &svc->destinations, n_list) {
-		if (dest->flags & IP_VS_DEST_F_OVERLOAD)
+		if (ip_vs_dest_is_overloaded(dest))
 			continue;
 		doh = ip_vs_sed_dest_overhead(dest);
 		if ((__s64)loh * atomic_read(&dest->weight) >

net/netfilter/ipvs/ip_vs_sh.c

@@ -73,7 +73,7 @@ struct ip_vs_sh_state {
 static inline bool is_unavailable(struct ip_vs_dest *dest)
 {
 	return atomic_read(&dest->weight) <= 0 ||
-	       dest->flags & IP_VS_DEST_F_OVERLOAD;
+	       ip_vs_dest_is_overloaded(dest);
 }
 
 /*

net/netfilter/ipvs/ip_vs_twos.c

@@ -52,7 +52,7 @@ static struct ip_vs_dest *ip_vs_twos_schedule(struct ip_vs_service *svc,
 
 	/* Generate a random weight between [0,sum of all weights) */
 	list_for_each_entry_rcu(dest, &svc->destinations, n_list) {
-		if (!(dest->flags & IP_VS_DEST_F_OVERLOAD)) {
+		if (!ip_vs_dest_is_overloaded(dest)) {
 			weight = atomic_read(&dest->weight);
 			if (weight > 0) {
 				total_weight += weight;
@@ -75,7 +75,7 @@ static struct ip_vs_dest *ip_vs_twos_schedule(struct ip_vs_service *svc,
 
 	/* Pick two weighted servers */
 	list_for_each_entry_rcu(dest, &svc->destinations, n_list) {
-		if (dest->flags & IP_VS_DEST_F_OVERLOAD)
+		if (ip_vs_dest_is_overloaded(dest))
 			continue;
 
 		weight = atomic_read(&dest->weight);

net/netfilter/ipvs/ip_vs_wlc.c

@@ -47,7 +47,7 @@ ip_vs_wlc_schedule(struct ip_vs_service *svc, const struct sk_buff *skb,
 	 */
 
 	list_for_each_entry_rcu(dest, &svc->destinations, n_list) {
-		if (!(dest->flags & IP_VS_DEST_F_OVERLOAD) &&
+		if (!ip_vs_dest_is_overloaded(dest) &&
 		    atomic_read(&dest->weight) > 0) {
 			least = dest;
 			loh = ip_vs_dest_conn_overhead(least);
@@ -62,7 +62,7 @@ ip_vs_wlc_schedule(struct ip_vs_service *svc, const struct sk_buff *skb,
 	 */
   nextstage:
 	list_for_each_entry_continue_rcu(dest, &svc->destinations, n_list) {
-		if (dest->flags & IP_VS_DEST_F_OVERLOAD)
+		if (ip_vs_dest_is_overloaded(dest))
 			continue;
 		doh = ip_vs_dest_conn_overhead(dest);
 		if ((__s64)loh * atomic_read(&dest->weight) >

net/netfilter/ipvs/ip_vs_wrr.c

@@ -176,7 +176,7 @@ ip_vs_wrr_schedule(struct ip_vs_service *svc, const struct sk_buff *skb,
 		list_for_each_entry_continue_rcu(dest,
 						 &svc->destinations,
 						 n_list) {
-			if (!(dest->flags & IP_VS_DEST_F_OVERLOAD) &&
+			if (!ip_vs_dest_is_overloaded(dest) &&
 			    atomic_read(&dest->weight) >= mark->cw)
 				goto found;
 			if (dest == stop)