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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0 (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).

- [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?

[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?

> +	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