Re: [PATCH net 12/15] ipvs: skip cport 0 connections in ip_vs_conn_out_get()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] ip_vs_conn_out_get() (net/netfilter/ipvs/ip_vs_conn.c:593-594)
  now copies the symmetric rule from __ip_vs_conn_in_get(): `(!p->vport…

--

ipvs: skip cport 0 connections in ip_vs_conn_out_get()

This patch adds the rule from __ip_vs_conn_in_get() to
ip_vs_conn_out_get(). A lookup for client port 0 matches only
connections flagged IP_VS_CONN_F_NO_CPORT, and a lookup for a nonzero
client port matches only connections without that flag. The goal is to
stop templates and ordinary cport 0 connections from being matched on
the out path.

> diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
> index 66fcdc00ef375..83d8a9153a342 100644
> --- a/net/netfilter/ipvs/ip_vs_conn.c
> +++ b/net/netfilter/ipvs/ip_vs_conn.c
> @@ -590,6 +590,8 @@ struct ip_vs_conn *ip_vs_conn_out_get(const struct ip_vs_conn_param *p)
>  				    ip_vs_addr_equal(p->af, p->vaddr,
>  						     &cp->caddr) &&
>  				    ip_vs_addr_equal(p->af, p->caddr, saddr) &&
> +				    (!p->vport ^
> +				     (!(cp->flags & IP_VS_CONN_F_NO_CPORT))) &&

[Severity: Medium]

Can the nonzero p->vport half of this check make the out lookup miss a
MASQ connection while ip_vs_conn_fill_cport() is still filling it?

For double-hashed connections (ip_vs_conn_use_hash2(): MASQ and not a
template), ip_vs_conn_fill_cport() works in two separately locked
passes. The first pass is dir 1:

net/netfilter/ipvs/ip_vs_conn.c:ip_vs_conn_fill_cport() {
    ...
	/* No packets from inside, so we can do it in 2 steps. */
	dir = use2 ? 1 : 0;
    ...
		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;
		}
    ...
			hlist_bl_del_rcu(&hn->node);
			hlist_bl_add_head_rcu(&hn->node, head_new);
    ...
	spin_unlock_bh(&cp->lock);
	if (dir-- && by_me)
		goto next_dir;
}

After the dir 1 pass, cp->cport is set and hn1 (the node walked by
ip_vs_conn_out_get()) sits in the bucket for the new cport. But
IP_VS_CONN_F_NO_CPORT stays set until the dir 0 pass, and all locks are
dropped between the two passes.

Before this patch, an out lookup in that window matched on cport alone.
With the new check, p->vport is nonzero and the flag is still set, so
the expression is 0 ^ 0 and the entry is skipped. Nothing else in
ip_vs_conn_out_get() would find the connection.

One sequence that seems possible with an FTP PASV/EPSV data connection,
which is created with IP_VS_CONN_F_NO_CPORT:

CPU A and CPU B each handle a client packet for the data connection.
Both find it through the cport 0 fallback in ip_vs_conn_in_get().

CPU A
ip_vs_nat_xmit()
  ip_vs_conn_fill_cport()
    dir 1 pass done: cport set, hn1 rehashed, NO_CPORT still set
    cp->lock released

CPU B
ip_vs_nat_xmit()
  ip_vs_conn_fill_cport()
    sees cp->cport set and by_me false, so it does nothing
  packet forwarded to the real server

The real server's reply then reaches ip_vs_out() before CPU A runs its
dir 0 pass. ip_vs_conn_out_get() misses, so handle_response() and SNAT
are skipped. The reply is NF_ACCEPTed with the real server's address as
its source.

The window is short. It only fits a full round trip if CPU A stalls
between the passes, for example through vCPU preemption or softirq
preemption on PREEMPT_RT. TCP retransmission should recover after dir 0
completes.

The commit message's goal seems to need only the p->vport == 0 half.
For nonzero lookups, the other half only matters in this in-between
fill state. Would a one-sided check avoid opening this window? For
example:

	if (!p->vport && !(cp->flags & IP_VS_CONN_F_NO_CPORT))
		continue;

On the in side, __ip_vs_conn_in_get() uses the same symmetric rule
without this problem. There, hn0 is rehashed and the flag is cleared in
the same locked step, but hn1 has no such guarantee.

>  				    p->protocol == cp->protocol) {
>  					if (__ip_vs_conn_get(cp))
>  						goto out;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113011447.2357709-2-pablo%40netfilter.org