Re: [PATCH net 11/11] ipvs: fix the spin_lock usage for RT build

[Julian Anastasov <ja@ssi.bg>]

	Hello,

On Mon, 27 Apr 2026, Jakub Kicinski wrote:

> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> ipvs: fix the spin_lock usage for RT build
> 
> This commit fixes spin_lock sleeping issues on PREEMPT_RT by adjusting the
> use of cp->lock in combination with hlist_bl bit locks. It removes the
> spin_lock from ip_vs_conn_hash() and ip_vs_conn_unlink(), and changes the
> lock ordering in ip_vs_conn_fill_cport().
> 
> Link: https://sashiko.dev/#/patchset/20260415200216.79699-1-ja%40ssi.bg
> Link: https://sashiko.dev/#/patchset/20260420165539.85174-4-ja%40ssi.bg
> Link: https://sashiko.dev/#/patchset/20260422135823.50489-4-ja%40ssi.bg
> 
> > diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
> > index 84a4921a7865a9..9e23cda8482588 100644
> > --- a/net/netfilter/ipvs/ip_vs_conn.c
> > +++ b/net/netfilter/ipvs/ip_vs_conn.c
> 
> [ ... ]
> 
> > @@ -704,14 +702,23 @@ void ip_vs_conn_fill_cport(struct ip_vs_conn *cp, __be16 cport)
> >  		hlist_bl_unlock(head);
> >  		write_seqcount_end(&t->seqc[hash_key & t->seqc_mask]);
> >  		preempt_enable_nested();
> > -		spin_unlock_bh(&t->lock[hash_key & t->lock_mask].l);
> > +		spin_unlock(&t->lock[hash_key & t->lock_mask].l);
> > +		spin_unlock_bh(&cp->lock);
> >  		hash_key = hash_key_new;
> >  		goto retry;
> >  	}
> >  
> > -	spin_lock(&cp->lock);
> > -	if ((cp->flags & IP_VS_CONN_F_NO_CPORT) &&
> > -	    (cp->flags & IP_VS_CONN_F_HASHED)) {
> > +	/* Fill cport once, even if multiple packets try to do it */
> > +	if (cp->flags & IP_VS_CONN_F_NO_CPORT && (!cp->cport || by_me)) {
> 
> Does removing the IP_VS_CONN_F_HASHED check here lead to a NULL pointer
> dereference for IP_VS_CONN_F_ONE_PACKET connections?
> 
> Connections flagged with IP_VS_CONN_F_ONE_PACKET bypass hashing:
> 
> ip_vs_conn_hash() {
>     if (cp->flags & IP_VS_CONN_F_ONE_PACKET)
>         return 0;
>     ...
> }
> 
> Because they bypass hashing, their hash list nodes (hn->node) are never
> linked, leaving hn->node.pprev as NULL. If such a connection is created
> with a source port of 0, it gets the IP_VS_CONN_F_NO_CPORT flag.
> 
> When ip_vs_conn_fill_cport() is later invoked to populate the port, the
> missing IP_VS_CONN_F_HASHED check allows the execution to proceed.

	But ip_vs_conn_fill_cport() is called for connection
that is hashed and later found by lookup. If IP_VS_CONN_F_ONE_PACKET
is set the conn is not hashed and is released immediately after
creation by ip_vs_conn_put(). Both flags can not coexist, one or
the other wins.

	As for IP_VS_CONN_F_NO_CPORT, ip_vs_bind_dest() clears the
IP_VS_CONN_F_ONE_PACKET flag if not UDP and we use NO_CPORT for TCP.
So, IP_VS_CONN_F_ONE_PACKET is simply ignored for TCP. If one day
we start to use IP_VS_CONN_F_NO_CPORT for UDP and use
IP_VS_CONN_F_ONE_PACKET then it will not work because the conn will
not be hashed and as result never found to fill the cport.
This time IP_VS_CONN_F_ONE_PACKET will win the game.

> 
> > +		/* If we race with resizing make sure cport is set for dir 1 */
> > +		if (!cp->cport) {
> > +			cp->cport = cport;
> > +			by_me = true;
> > +		}
> > +		if (!dir) {
> > +			atomic_dec(&ipvs->no_cport_conns[af_id]);
> > +			cp->flags &= ~IP_VS_CONN_F_NO_CPORT;
> > +		}
> >  		/* We do not recalc hash_key_r under lock, we assume the
> >  		 * parameters in cp do not change, i.e. cport is
> >  		 * the only possible change.
> > @@ -726,21 +733,17 @@ void ip_vs_conn_fill_cport(struct ip_vs_conn *cp, __be16 cport)
> >  			hlist_bl_del_rcu(&hn->node);
> 
> If head != head2 here, hlist_bl_del_rcu(&hn->node) is called. Since
> hn->node.pprev is NULL, wouldn't this cause a panic when __hlist_bl_del()
> executes *pprev = next?
> 
> Is it possible to retain the IP_VS_CONN_F_HASHED check or otherwise
> ensure unhashed nodes aren't unlinked?

	As IP_VS_CONN_F_ONE_PACKET conns are not hashed,
ip_vs_conn_unlink() has check not to unlink the conn nodes from
tables. The conn is invisible to others except the packet
that created it. And the hash nodes are not touched.

Regards

--
Julian Anastasov <ja@ssi.bg>