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 patch fixes PREEMPT_RT issues with spin_lock usage under bit
> locks in the IPVS resizable hash tables, and reworks ip_vs_conn_hash(),
> ip_vs_conn_unlink(), and ip_vs_conn_fill_cport() accordingly.
> 
> 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 84a4921a7865..9e23cda84825 100644
> > --- a/net/netfilter/ipvs/ip_vs_conn.c
> > +++ b/net/netfilter/ipvs/ip_vs_conn.c
> 
> [ ... ]
> 
> > @@ -637,6 +628,7 @@ void ip_vs_conn_fill_cport(struct ip_vs_conn *cp, __be16 cport)
> >  	struct ip_vs_conn_hnode *hn;
> >  	u32 hash_key, hash_key_new;
> >  	struct ip_vs_conn_param p;
> > +	bool by_me = false;
> >  	int ntbl;
> >  	int dir;
> >  
> > @@ -664,8 +656,11 @@ void ip_vs_conn_fill_cport(struct ip_vs_conn *cp, __be16 cport)
> >  		t = rcu_dereference(t->new_tbl);
> >  		ntbl++;
> >  		/* We are lost? */
> > -		if (ntbl >= 2)
> > +		if (ntbl >= 2) {
> > +			IP_VS_ERR_RL("%s(): Too many ht changes for dir %d\n",
> > +				     __func__, dir);
> >  			return;
> > +		}
> >  	}
> 
> Can the ntbl >= 2 early return strand cp in an unrecoverable state
> when it is reached on the dir=0 iteration after dir=1 has already
> committed partial state?

	The right word here is "unreachable" but still recovarable
on expiration.

> In the pre-patch code, cp->cport, the clearing of
> IP_VS_CONN_F_NO_CPORT, and the atomic_dec() of no_cport_conns[] were
> all performed together inside a single if (!dir) critical section, so
> an abort via ntbl >= 2 simply left cp->cport == 0 and a later packet
> could retry from scratch.

	We saw this concern in previous review, and now it
reappeared. We explained in the commit that this ntbl >= 2 is a
situation that should not occur and added error to catch it. It is
a code that avoids dead loop. For connection with double hashing,
filling cport is a two-step operation, we fill cport early in
dir 1 (dir 0 follows in second step) to allow the resizing work (if 
running concurrently) to see valid hash key (cp->cport) all the time.
It is not fatal if we attempt second rehashing after this resizing,
ip_vs_conn_fill_cport() detects this with a 'if (head != head2)'
check, i.e. we are already on the desired bucket. Here is what
happens on resizing running concurrently with filling cport:

conn_resize_work_handler                ip_vs_conn_fill_cport
-------------------------------------------------------------
Still keep old ipvs->conn_tab but
rcu_assign_pointer(t->new_tbl, t_new)
to attach new table to the current
one

                                        Any RCU read-side sections here
                                        complete before the resizing.
                                        They may see the new table and
                                        move the nodes there. ntbl will
                                        not change above 0.
                                        
first synchronize_rcu() call

                                        rcu_read_lock() - after the
                                        synchronize_rcu() in resize_work

                                        - we see the new table in t->new_tbl
start to move conn nodes to             - we first process dir=1 concurrently
t->new_tbl:                             with resizing, successfully
- change hn->hash_key under             
locks, now hn->hash_key points to       We now process dir=0 (hn0):
t->new_tbl and it is not changed        We may see the old hn->hash_key or
until new resize/fill_cport             the new one. The old matches the
                                        current table. When we reach the locks
                                        we will see the new value if it 
                                        is already assigned.

                                        When we see the early change of
                                        hn->hash_key which matches
                                        t->new_tbl (not t) => ntbl++ (ntbl
                                        becomes 1) and t jumps to t->new_tbl,
                                        i.e. we see that entry is linked
                                        already on the new table.

                                        In all cases we should see ntbl
                                        to reach 1, not more. Because we
                                        do the resizing in steps which
                                        avoid one RCU section to see many
                                        new tables.
                                        If ntbl becomes 2 we have some 
                                        kind of corruption,
                                        so we just exit the loop.

Finally, install t_new as
ipvs->conn_tab
                                        Alternatively, for dir=0 we can see
                                        the new ipvs->conn_tab, so here
                                        hn->hash_key will match both t and
                                        t->new_tbl (they are same) and
                                        dir=0 will never goto retry,
                                        ntbl remains 0.

> 
> [ ... ]
> 
> > -	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)) {
> > +		/* 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;
> > +		}
> 
> With the new structure, the dir=1 iteration (which runs first when
> use2 is true) sets cp->cport = cport and by_me = true, but leaves
> IP_VS_CONN_F_NO_CPORT set and does not touch no_cport_conns[].  The
> flag clear and the atomic_dec happen only in the subsequent dir=0
> iteration.
> 
> If the dir=0 iteration then returns early via the ntbl >= 2 path,
> either directly on entry or after an accumulated ntbl across retry:
> restarts (ntbl is reset per next_dir, not per retry), cp ends up
> with cp->cport set to the new value but IP_VS_CONN_F_NO_CPORT still
> set.
> 
> Can any subsequent ip_vs_conn_fill_cport() call complete the
> transition?  The gate is:
> 
> 	if (cp->flags & IP_VS_CONN_F_NO_CPORT && (!cp->cport || by_me)) {
> 
> For a fresh caller, by_me is false and cp->cport is now non-zero, so
> the gate is structurally unsatisfiable and the flag clear /
> atomic_dec never runs.
> 
> In __ip_vs_conn_in_get() the lookup
> 
> 	!p->cport ^ !(cp->flags & IP_VS_CONN_F_NO_CPORT)
> 
> evaluates false for this cp (flag still set, p->cport non-zero), and
> the cport=0 fallback also fails since p->cport != cp->cport (cp->cport
> is now non-zero).  Does this leave cp unreachable via forward
> direction lookup until the timer expires?

	Yes, we suspect corruption. If it is broken data in
the connection we isolate it (not reachable via lookup) and
then let it expire and later restore the no_cport_conns
counter. If we try to restore the cp->cport to 0 on ntbl >= 2
then we should go and rehash hn1 (dir=1) but it is not fatal
if we do not rehash it because next packet will come again in 
ip_vs_conn_fill_cport() and rehash it properly. This can be
done if we want retransmitted packet to continue with the
connection instead of expiring it:

diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
index 9e23cda84825..9ea6b4fa78bf 100644
--- a/net/netfilter/ipvs/ip_vs_conn.c
+++ b/net/netfilter/ipvs/ip_vs_conn.c
@@ -657,6 +657,11 @@ void ip_vs_conn_fill_cport(struct ip_vs_conn *cp, __be16 cport)
 		ntbl++;
 		/* We are lost? */
 		if (ntbl >= 2) {
+			spin_lock_bh(&cp->lock);
+			if (cp->flags & IP_VS_CONN_F_NO_CPORT && by_me)
+				cp->cport = 0;
+			/* hn1 will be rehashed on next packet */
+			spin_unlock_bh(&cp->lock);
 			IP_VS_ERR_RL("%s(): Too many ht changes for dir %d\n",
 				     __func__, dir);
 			return;

	I.e. this is a change we can do but in all cases we
are not sure why ntbl reaches 2.

> As a side effect, does no_cport_conns[af_id] stay elevated for the
> remainder of cp's lifetime, forcing ip_vs_conn_in_get() into the
> slower cport=0 fallback path for that af/netns while the stuck entry
> lives?

	For FTP service we are often using double lookup
when PASV connections are in negotiation phase (each up
to 120 secs).

Regards

--
Julian Anastasov <ja@ssi.bg>