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

[Julian Anastasov]

	Hello,

On Wed, 23 Sep 2026, Zihan Xi wrote:

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

	Sashiko reports for refcount problem:

https://sashiko.dev/#/patchset/cover.1790146910.git.zihanx%40nebusec.ai

	Will send new version...

pw-bot: changes-requested

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

Regards

--
Julian Anastasov <ja@xxxxxx>