Re: [PATCH net 07/11] ipvs: filter some flags received in the backup server

[Julian Anastasov <ja@ssi.bg>]

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


	Hello,

On Mon, 28 Sep 2026, netdev-bot+sashiko@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 0 (1)

	This patch is fine, see below. It is another patch
by another author that adds the missing checks.

> 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).
> 
> - [High] The patch does not stop forged sync messages from creating
>   templates that normal-connection lookups can hit.
> 
> Pre-existing issues:
> - [Low] This problem wasn't introduced by this patch, but
>   ip_vs_sync_validate_flags() still accepts NO_CPORT on non-template…
> 
> --
> 
> ipvs: filter some flags received in the backup server
> 
> This commit adds ip_vs_sync_validate_flags() so the backup drops sync
> entries that have both NO_CPORT and TEMPLATE set, or that have ONE_PACKET
> set. It also applies IP_VS_CONN_F_BACKUP_MASK to v0 message flags, to
> match the existing v1 handling.
> 
> > diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> > index dfa8487ec0c20..1a30817fbbafe 100644
> > --- a/net/netfilter/ipvs/ip_vs_sync.c
> > +++ b/net/netfilter/ipvs/ip_vs_sync.c
> > @@ -954,6 +954,21 @@ static void ip_vs_proc_conn(struct netns_ipvs *ipvs, struct ip_vs_conn_param *pa
> >  	ip_vs_conn_put(cp);
> >  }
> >  
> > +/* Check for incompatible flags */
> > +static bool ip_vs_sync_validate_flags(u32 flags)
> > +{
> > +	/* We do not expect NO_CPORT, especially to allow lookups
> > +	 * to hit templates
> > +	 */
> > +	if (flags & IP_VS_CONN_F_NO_CPORT) {
> > +		if (flags & IP_VS_CONN_F_TEMPLATE)
> > +			return false;
> > +	}
> 
> [Severity: High]
> Does this fully stop sync messages from creating templates that
> normal-connection lookups can hit?
> 
> Take a v0 or v1 entry with TEMPLATE set, NO_CPORT clear and a non-zero
> cport. It passes this check and survives IP_VS_CONN_F_BACKUP_MASK. Then
> ip_vs_proc_conn()->ip_vs_conn_new() creates it with the cport from the
> message: ip_vs_conn_fill_param_sync() for v1, s->cport for v0.
> 
> With no PE data, the template is hashed on the same (proto, caddr, cport,
> vaddr, vport) key a normal connection uses. __ip_vs_conn_in_get() does
> not check IP_VS_CONN_F_TEMPLATE:
> 
> net/netfilter/ipvs/ip_vs_conn.c:__ip_vs_conn_in_get() {
>     ...
> 				if (p->cport == cp->cport &&
> 				    p->vport == cp->vport && cp->af == p->af &&
> 				    ...
> 				    (!p->cport ^
> 				     (!(cp->flags & IP_VS_CONN_F_NO_CPORT))) &&
> 				    p->protocol == cp->protocol) {
>     ...
> }
> 
> When p->cport == cp->cport != 0 and NO_CPORT is clear, the XOR term is 1,
> so the lookup returns the template.
> 
> A later non-template sync for that tuple then gets the template back from
> ip_vs_conn_in_get() in ip_vs_proc_conn(). It either expires the template,
> or it toggles dest->activeconns on the template's dest and overwrites
> cp->flags, state, timeout and seq:
> 
> net/netfilter/ipvs/ip_vs_sync.c:ip_vs_proc_conn() {
>     ...
> 		if ((cp->flags ^ flags) & IP_VS_CONN_F_INACTIVE &&
> 		    !(flags & IP_VS_CONN_F_TEMPLATE) && dest) {
> 			if (flags & IP_VS_CONN_F_INACTIVE)
> 				atomic_dec(&dest->activeconns);
> 			else
> 				atomic_inc(&dest->activeconns);
> 		}
>     ...
> }
> 
> For templates, ip_vs_unbind_dest() only decrements persistconns, so the
> activeconns change is never undone. After failover, real client packets
> for that tuple would also be handled by the template in the ip_vs_in()
> path.
> 
> The master always builds templates with cport 0. Both
> ip_vs_conn_fill_param_persist() callers in ip_vs_core.c pass a literal 0.
> 
> Should the backup also reject TEMPLATE entries with a non-zero cport when
> there is no PE data?

	Above is solved with a pending patch
"ipvs: validate cport in received sync records".

> 
> [Severity: Low]
> This is a pre-existing issue, not one introduced by this patch. NO_CPORT
> without TEMPLATE is still accepted here, and nothing checks that the
> synced cport is 0. Is that intended?
> 
> Other code assumes NO_CPORT means cport == 0. ip_vs_ftp creates NO_CPORT
> connections with cport 0, and ip_vs_conn_fill_cport() uses !cp->cport to
> mean the cport has not been filled in yet:
> 
> 	if (cp->flags & IP_VS_CONN_F_NO_CPORT && (!cp->cport || by_me)) {
> 
> ip_vs_conn_new() creates a NO_CPORT entry with cport X != 0 and
> increments ipvs->no_cport_conns[af]. ip_vs_conn_in_get() can never find
> that entry. The exact lookup fails the XOR term in __ip_vs_conn_in_get(),
> and the cport_zero_p retry fails the cport comparison.
> 
> So each refresh in ip_vs_proc_conn() would create another duplicate entry,
> and each one lasts until its timeout. no_cport_conns also stays non-zero,
> which forces the second lookup on every in-direction miss. The entry can
> still be matched in the reply direction, because ip_vs_conn_out_get()
> does not check NO_CPORT.
> 
> Would it make sense to also require cport == 0 when NO_CPORT is accepted
> on a non-template entry?

	Above is solved with a pending patch
"ipvs: validate cport in received sync records".

> 
> > +	if (flags & IP_VS_CONN_F_ONE_PACKET)
> > +		return false;
> > +	return true;
> > +}
> > +
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927220816.268206-2-pablo%40netfilter.org
> 

Regards

--
Julian Anastasov <ja@ssi.bg>