Re: [PATCH nf 2/2] ipvs: skip cport 0 connections in ip_vs_conn_out_get()

[Julian Anastasov <ja@ssi.bg>]

[-- Attachment #1: Type: text/plain, Size: 11344 bytes --]


	Hello,

On Tue, 29 Sep 2026, netdev-bot+sashiko@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 4 · Low: 1
> 
> 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] The new `cp->cport` requirement turns the "update or create"
>   lookup in ip_vs_ftp_out() (net/netfilter/ipvs/ip_vs_ftp.c) into dead…
> - [Medium] ip_vs_new_conn_out() (net/netfilter/ipvs/ip_vs_core.c) rejects
>   only `!vport || !dport`.
> - [Medium] The commit message assumes only templates and NO_CPORT entries
>   have cport 0, but ordinary connections can too.
> - [Medium] The commit rests on the claim "Templates keep cport 0
>   throughout their lifetime", which does not hold for synced templates…
> - [Low] The patch adds a second, separate plain read of cp->cport in
>   ip_vs_conn_out_get().

	There are valid concerns, so we should send new version.

> 
> --
> 
> ipvs: skip cport 0 connections in ip_vs_conn_out_get()
> 
> This patch makes ip_vs_conn_out_get() match only connections with a
> nonzero cp->cport. The goal is to stop out-direction packets sent to
> client port 0 from matching persistence templates, or connections still
> waiting for their client port (IP_VS_CONN_F_NO_CPORT), and then updating
> their state and timeout.
> 
> > diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
> > index 6fa3e1dc534c3..24dbe34a48275 100644
> > --- a/net/netfilter/ipvs/ip_vs_conn.c
> > +++ b/net/netfilter/ipvs/ip_vs_conn.c
> > @@ -579,7 +579,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->protocol == cp->protocol) {
> > +				    p->protocol == cp->protocol &&
> > +				    cp->cport) {
> 
> [Severity: Medium]
> Does this make the "update or create" lookup in ip_vs_ftp_out() in
> net/netfilter/ipvs/ip_vs_ftp.c unreachable?
> 
> ip_vs_ftp_out() looks up with vport 0 on purpose, to find an existing
> IP_VS_CONN_F_NO_CPORT passive data connection:
> 
> net/netfilter/ipvs/ip_vs_ftp.c:ip_vs_ftp_out() {
>     ...
> 	/* Now update or create a connection entry for it */
> 	{
> 		struct ip_vs_conn_param p;
> 
> 		ip_vs_conn_fill_param(cp->ipvs, cp->af,
> 				      ipvsh->protocol, &from, port,
> 				      &cp->caddr, 0, &p);
> 		n_cp = ip_vs_conn_out_get(&p);
> 	}
> 	if (!n_cp) {
> 		...
> 		n_cp = ip_vs_conn_new(&p, cp->af, &from, port,
> 				      IP_VS_CONN_F_NO_CPORT |
> 				      IP_VS_CONN_F_NFCT,
> 				      cp->dest, skb->mark);
> 		if (!n_cp)
> 			return 0;
>     ...
> }
> 
> When p->vport is 0, the earlier check in ip_vs_conn_out_get():
> 
> 	if (p->vport != cp->cport)
> 		continue;
> 
> only lets through entries with cp->cport == 0. The new cp->cport test
> then rejects every one of them, so this lookup can never return a match.
> 
> Suppose a client sends PASV/EPSV again and the server answers with the
> same passive port, for example because it has a fixed or narrow passive
> range. Each such reply now creates a duplicate NO_CPORT entry with the
> same hash key.
> 
> Each duplicate increments no_cport_conns[], takes another n_control
> reference on the control connection and adds another conntrack
> expectation. Only one of them ever gets its cport filled. The rest stay
> until the listen timeout.
> 
> If conn_count has reached conn_max, ip_vs_conn_new() returns NULL and
> ip_vs_ftp_out() returns 0. The control reply is dropped, where before
> the existing entry would have been reused.
> 
> A retransmitted 227/229 segment does not seem to trigger this, because
> cp->app_data is set to IP_VS_FTP_ACTIVE after the first pass. A repeated
> PASV/EPSV command does trigger it.
> 
> The commit message says "FTP data connections learn their client port
> from incoming traffic before replies are processed". Doesn't that only
> cover replies on the data connection? This control-channel lookup runs
> before the port is known.

	Yep, this is a problem, we should change the
cp->cport check with !TEMPLATE check.

> 
> [Severity: Medium]
> Can this lead to a new connection being created for every packet of a
> flow started by a real server towards client port 0?
> 
> ip_vs_new_conn_out() in net/netfilter/ipvs/ip_vs_core.c only rejects a
> zero vport or dport:
> 
> 	if (!vport || !dport)
> 		return NULL;
> 
> Take a SIP-PE service with conn_out_counter > 0, where a real server
> sends a UDP packet to client port 0. This path:
> 
> ip_vs_out_hook()->__ip_vs_rs_conn_out()->ip_vs_sip_conn_out()->
>     ip_vs_new_conn_out()->ip_vs_conn_new()
> 
> creates an ordinary hashed connection with cp->cport == 0 and neither
> TEMPLATE nor NO_CPORT set.
> 
> Before this patch, the next packet of that flow matched this entry in
> ip_vs_conn_out_get(). With the new cp->cport test it misses. So
> ip_vs_out_hook() falls back to __ip_vs_rs_conn_out() again and creates
> yet another connection.
> 
> Each extra connection lives until the UDP timeout. For persistent
> services, each one also takes another n_control reference on the
> template.
> 
> __ip_vs_conn_in_get() cannot match these entries either, because a
> cport 0 lookup there requires NO_CPORT.
> 
> Linux UDP sockets refuse sendto() to port 0, so in practice this needs a
> raw socket or a non-Linux real server.
> 
> Should ip_vs_new_conn_out() also reject a zero cport? Or should the out
> lookup exclude only templates and NO_CPORT entries?

	This can be separate fix.

> 
> [Severity: Medium]
> The commit message assumes that only templates and NO_CPORT entries have
> cport 0. Does that hold for ordinary MASQ connections scheduled for a
> client that uses source port 0?
> 
> ip_vs_schedule() in net/netfilter/ipvs/ip_vs_core.c takes the client
> port directly from the packet:
> 
> 	cport = pptr[0];
> 	...
> 	ip_vs_conn_fill_param(svc->ipvs, svc->af, iph->protocol,
> 			      caddr, cport, vaddr, vport, &p);
> 	cp = ip_vs_conn_new(&p, dest->af, &dest->addr,
> 			    dest->port ? dest->port : vport,
> 			    flags, dest, skb->mark);
> 
> Nothing in the scheduling path rejects source port 0. A remote client
> sending a TCP SYN or UDP packet from port 0 to VIP:vport on a
> non-persistent MASQ service gets an ordinary connection with
> cp->cport == 0.
> 
> Before this patch, the real server's reply to client:0 matched this
> connection in ip_vs_conn_out_get() and was SNATed back to VIP:vport. Now
> the lookup rejects it.
> 
> With the default nat_icmp_send=0, ip_vs_out_hook() then returns
> NF_ACCEPT. The reply, for example a SYN-ACK or RST, leaves the director
> with the real server's private address and port as its source.
> 
> ICMP errors behave the same way. If the real server sends an error that
> quotes client:0->RS:rport, ip_vs_out_icmp() does the reversed lookup
> with vport 0:
> 
> 	cp = INDIRECT_CALL_1(pp->conn_out_get, ip_vs_conn_out_get_proto,
> 			     ipvs, AF_INET, skb, &ciph);
> 	if (!cp)
> 		return NF_ACCEPT;
> 
> This now returns NULL, so neither the outer source nor the embedded
> tuple is reverse-NATed.
> 
> __ip_vs_conn_in_get() already could not match such entries. The out
> direction, though, was NATed before this patch.
> 
> Would excluding IP_VS_CONN_F_TEMPLATE (and NO_CPORT, if intended) by
> flag avoid this?

	We should fix the above problems.

> 
> [Severity: Low]
> Can the two separate plain reads of cp->cport in ip_vs_conn_out_get()
> see different values?
> 
> The lookup holds only rcu_read_lock(). Meanwhile, ip_vs_conn_fill_cport()
> writes cp->cport under cp->lock and the bucket locks, before it updates
> hn->hash_key and moves the node:
> 
> 	if (!cp->cport) {
> 		cp->cport = cport;
> 		by_me = true;
> 	}
> 
> It can also reset cp->cport to 0 on the "Too many ht changes" path.
> 
> Suppose a MASQ NO_CPORT connection is getting its cport filled while an
> out-direction packet to client port 0 is looked up. The reader can see
> the old hash_key and read cp->cport == 0 in:
> 
> 	if (p->vport != cp->cport)
> 		continue;
> 
> and then read the new nonzero port in the added cp->cport test. The
> entry then matches, and handle_response() runs ip_vs_set_state() and the
> timeout update. That is the NO_CPORT case the commit message says this
> patch closes.
> 
> A successful match leaves the loop through goto out, so the seqcount
> retry in ip_vs_rht_for_bucket_retry() is never checked.
> 
> Neither side uses READ_ONCE()/WRITE_ONCE(), so KCSAN could also report
> this as a data race.
> 
> Would a single snapshot close this window?
> 
> 	__be16 cport = READ_ONCE(cp->cport);
> 
> 	if (!cport || p->vport != cport)
> 		continue;
> 
> [Severity: Medium]
> The commit message says:
> 
>     Templates keep cport 0 throughout their lifetime.
> 
> Is that true for synced templates that also carry IP_VS_CONN_F_NO_CPORT?
> 
> IP_VS_CONN_F_BACKUP_MASK includes both TEMPLATE and NO_CPORT. The
> template check in ip_vs_proc_conn() in net/netfilter/ipvs/ip_vs_sync.c
> only rejects a nonzero cport:
> 
> 	if (flags & IP_VS_CONN_F_TEMPLATE) {
> 		if (param->cport) {
> 			IP_VS_DBG(2, "BACKUP, template with cport dropped\n");
> 			kfree(param->pe_data);
> 			return;
> 		}
> 	}
> 
> So a forged TEMPLATE|NO_CPORT|MASQ record with cport 0 is accepted. It
> is hashed through hn0, counted in no_cport_conns[] and bound to
> ip_vs_nat_xmit().
> 
> An incoming packet C:X -> vaddr:vport then goes through:
> 
> ip_vs_in_hook()
>   ip_vs_conn_in_get()     <- the cport 0 fallback matches; TEMPLATE
>                              is not excluded
>   ip_vs_nat_xmit()
>     ip_vs_conn_fill_cport(cp, X)
> 
> ip_vs_conn_fill_cport() sets cp->cport = X and clears NO_CPORT, but
> leaves TEMPLATE set. It also rehashes hn0 to hash(C, X, vaddr, vport).
> All of this happens before the later "stopping DNAT to local address"
> check in ip_vs_nat_xmit().
> 
> Now suppose the forged record also sets daddr == vaddr and
> dport == vport. An out packet vaddr:vport -> C:X then passes the
> hash_key check, the MASQ saddr/sport comparisons and the new cp->cport
> test. handle_response() then runs ip_vs_set_state() and the timeout
> update on the template.
> 
> One way to produce that out packet: vaddr is local to the backup and
> the incoming packet has no conntrack entry. The packet is then delivered
> locally, and the local reply or RST goes through ip_vs_out_hook() at
> LOCAL_OUT.
> 
> The in-direction fallback already let such a forged template see
> packets before this patch. The out-direction match on the template is
> what reappears here.
> 
> Would it be more robust to exclude IP_VS_CONN_F_TEMPLATE explicitly in
> ip_vs_conn_out_get(), and to reject TEMPLATE|NO_CPORT records in
> ip_vs_proc_conn()?

	This is fixed by "ipvs: filter some flags received in the backup 
server"

> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925141115.16126-1-axel.mierczuk%401password.com

pw-bot: cr

Regards

--
Julian Anastasov <ja@ssi.bg>