[PATCH nf v4 1/3] ipvs: wait the running timer cb on conn deletion

[Zihan Xi]

From: Julian Anastasov <ja@xxxxxx>

Sashiko reports for problem when deleting connections.

If connection timer expires, its callback can not be
concurrently running but if connection is deleted
the callback can be running on another CPU even
after all references are released. Before now we
continued with the connection freeing, risking the
callback to access the deleted connection after it
is freed. As ip_vs_conn_del*() run under RCU lock
there is no risk accessing a freed connection by
concurrent timer callback as Sashiko warns, may
be only if our timer expires and we try to delete
the cp->control chain.

Fix that by failing the ip_vs_conn_unlink() call after
refcnt is restored to 1 allowing the timer callback
to be scheduled for new execution which should happen
after the detected running callback finishes.

One of two things can happen when we detect the
running callback:

1. the concurrent timer callback can see refcnt 0 and
do nothing, so we will schedule new timer callback to
expire the connection after the running one finishes

2. the concurrent timer callback can see refcnt 1 and
to expire the connection as usually, in this case we
will see refcnt 0 and will do nothing

Add explicit rcu_read_lock() while deleting the cp->control
chain to protect from concurrent timer callback for ct to
expire it before us.

During such races, try to keep 0 in cp->timeout as it is
a request for deleting our cp->control chain immediately.

Link: https://sashiko.dev/#/patchset/cover.1789435989.git.zihanx%40nebusec.ai
Fixes: f9200a52eedf ("ipvs: avoid expiring many connections from timer")
Signed-off-by: Julian Anastasov <ja@xxxxxx>
---
 net/netfilter/ipvs/ip_vs_conn.c | 103 +++++++++++++++++---------------
 1 file changed, 54 insertions(+), 49 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
index 6fa3e1dc534c3..32cfc02aa2912 100644
--- a/net/netfilter/ipvs/ip_vs_conn.c
+++ b/net/netfilter/ipvs/ip_vs_conn.c
@@ -313,17 +313,34 @@ static inline int ip_vs_conn_hash(struct ip_vs_conn *cp)
 /* Try to unlink ip_vs_conn from conn_tab.
  * returns bool success.
  */
-static inline bool ip_vs_conn_unlink(struct ip_vs_conn *cp)
+static inline bool ip_vs_conn_unlink(struct ip_vs_conn *cp, bool my_cb)
 {
 	struct netns_ipvs *ipvs = cp->ipvs;
 	struct hlist_bl_head *head, *head2;
 	u32 hash_key, hash_key2;
 	struct ip_vs_rht *t;
-	bool ret = false;
 	bool use2;
 
+	if (!refcount_dec_if_one(&cp->refcnt))
+		return false;
+
 	if (cp->flags & IP_VS_CONN_F_ONE_PACKET)
-		return refcount_dec_if_one(&cp->refcnt);
+		return true;
+
+	/* Revalidate after conn is excluded from traffic:
+	 * - not controlling other conns
+	 * - no pending/running timer callback
+	 *
+	 * And the winner is ...
+	 */
+	if (atomic_read(&cp->n_control) ||
+	    (!timer_delete(&cp->timer) && !my_cb)) {
+		/* Not me? Give the timer callback another chance, even
+		 * if one is concurrently running during the conn deletion.
+		 */
+		refcount_inc(&cp->refcnt);
+		return false;
+	}
 
 	rcu_read_lock();
 	local_bh_disable();
@@ -337,15 +354,11 @@ static inline bool ip_vs_conn_unlink(struct ip_vs_conn *cp)
 		      false /* new_hash2 */, &head, &head2);
 
 	if (cp->flags & IP_VS_CONN_F_HASHED) {
-		/* Decrease refcnt and unlink conn only if we are last user */
-		if (use2 == ip_vs_conn_use_hash2(cp) &&
-		    refcount_dec_if_one(&cp->refcnt)) {
-			hlist_bl_del_rcu(&cp->hn0.node);
-			if (use2)
-				hlist_bl_del_rcu(&cp->hn1.node);
-			cp->flags &= ~IP_VS_CONN_F_HASHED;
-			ret = true;
-		}
+		/* Unlink conn as we are the last user */
+		hlist_bl_del_rcu(&cp->hn0.node);
+		if (use2)
+			hlist_bl_del_rcu(&cp->hn1.node);
+		cp->flags &= ~IP_VS_CONN_F_HASHED;
 	}
 
 	conn_tab_unlock(head, head2);
@@ -353,7 +366,7 @@ static inline bool ip_vs_conn_unlink(struct ip_vs_conn *cp)
 	local_bh_enable();
 	rcu_read_unlock();
 
-	return ret;
+	return true;
 }
 
 
@@ -1319,34 +1332,29 @@ static void ip_vs_conn_rcu_free(struct rcu_head *head)
 	kmem_cache_free(ip_vs_conn_cachep, cp);
 }
 
-/* Try to delete connection while not holding reference */
+/* Try to delete connection while not holding reference.
+ * It can be called concurrently and always under RCU lock.
+ */
 static void ip_vs_conn_del(struct ip_vs_conn *cp)
 {
-	if (timer_delete(&cp->timer)) {
-		/* Drop cp->control chain too */
-		if (cp->control)
-			cp->timeout = 0;
-		ip_vs_conn_expire(&cp->timer);
-	}
-}
+	struct timer_list *t = (void *)((unsigned long)(&cp->timer) | 1UL);
 
-/* Try to delete connection while holding reference */
-static void ip_vs_conn_del_put(struct ip_vs_conn *cp)
-{
-	if (timer_delete(&cp->timer)) {
-		/* Drop cp->control chain too */
-		if (cp->control)
-			cp->timeout = 0;
-		__ip_vs_conn_put(cp);
-		ip_vs_conn_expire(&cp->timer);
-	} else {
-		__ip_vs_conn_put(cp);
-	}
+	/* Drop cp->control chain too */
+	if (cp->control)
+		cp->timeout = 0;
+	ip_vs_conn_expire(t);
 }
 
+/* Connection is removed in the following steps:
+ * - timer expires or connection is deleted
+ * - there should be no more references (n_control>0 and refcnt>1)
+ * - there should be no pending timer or a running timer callback (on deletion)
+ */
 static void ip_vs_conn_expire(struct timer_list *t)
 {
-	struct ip_vs_conn *cp = timer_container_of(cp, t, timer);
+	bool my_cb = !((unsigned long)t & 1);
+	struct timer_list *t2 = (void *)((unsigned long)t & ~1UL);
+	struct ip_vs_conn *cp = timer_container_of(cp, t2, timer);
 	struct netns_ipvs *ipvs = cp->ipvs;
 
 	/*
@@ -1356,26 +1364,21 @@ static void ip_vs_conn_expire(struct timer_list *t)
 		goto expire_later;
 
 	/* Unlink conn if not referenced anymore */
-	if (likely(ip_vs_conn_unlink(cp))) {
+	if (likely(ip_vs_conn_unlink(cp, my_cb))) {
 		struct ip_vs_conn *ct = cp->control;
 
-		/* delete the timer if it is activated by other users */
-		timer_delete(&cp->timer);
-
 		/* does anybody control me? */
 		if (ct) {
-			bool has_ref = !cp->timeout && __ip_vs_conn_get(ct);
-
+			rcu_read_lock();
 			ip_vs_control_del(cp);
 			/* Drop CTL or non-assured TPL if not used anymore */
-			if (has_ref && !atomic_read(&ct->n_control) &&
+			if (!cp->timeout && !atomic_read(&ct->n_control) &&
 			    (!(ct->flags & IP_VS_CONN_F_TEMPLATE) ||
 			     !(ct->state & IP_VS_CTPL_S_ASSURED))) {
 				IP_VS_DBG(4, "drop controlling connection\n");
-				ip_vs_conn_del_put(ct);
-			} else if (has_ref) {
-				__ip_vs_conn_put(ct);
+				ip_vs_conn_del(ct);
 			}
+			rcu_read_unlock();
 		}
 
 		if ((cp->flags & IP_VS_CONN_F_NFCT) &&
@@ -1410,13 +1413,15 @@ static void ip_vs_conn_expire(struct timer_list *t)
 		  refcount_read(&cp->refcnt),
 		  atomic_read(&cp->n_control));
 
-	refcount_inc(&cp->refcnt);
-	cp->timeout = 60*HZ;
+	if (__ip_vs_conn_get(cp)) {
+		if (cp->timeout || atomic_read(&cp->n_control))
+			cp->timeout = 60 * HZ;
 
-	if (ipvs->sync_state & IP_VS_STATE_MASTER)
-		ip_vs_sync_conn(ipvs, cp, sysctl_sync_threshold(ipvs));
+		if (ipvs->sync_state & IP_VS_STATE_MASTER)
+			ip_vs_sync_conn(ipvs, cp, sysctl_sync_threshold(ipvs));
 
-	__ip_vs_conn_put_timer(cp);
+		__ip_vs_conn_put_timer(cp);
+	}
 }
 
 /* Modify timer, so that it expires as soon as possible.
-- 
2.43.0