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

[Jakub Kicinski <kuba@kernel.org>]

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: filter some flags received in the backup server

This adds ip_vs_sync_validate_flags() to the backup receive path so that
sync records carrying the NO_CPORT+TEMPLATE combination or ONE_PACKET are
dropped, and it also applies IP_VS_CONN_F_BACKUP_MASK to the flags taken
from version 0 messages for consistency with version 1.

A few questions about the new validation below.

> diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> index 5383aeafb0aed..69dc28153ec12 100644
> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c
> @@ -949,6 +949,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
> +	 */

[Severity: Low]
This isn't a bug, but does the comment describe what the code does?  The
text reads as an unconditional statement that NO_CPORT is not expected,
while the code below only rejects it when TEMPLATE is set, and both call
sites then keep the bit:

	flags &= IP_VS_CONN_F_BACKUP_MASK;

IP_VS_CONN_F_BACKUP_MASK in include/uapi/linux/ip_vs.h is documented as
"Initial bits allowed in backup server" and it lists
IP_VS_CONN_F_NO_CPORT, so bare NO_CPORT is expected here.  It is also
needed: ip_vs_conn_new() accounts NO_CPORT entries in
ipvs->no_cport_conns[] and ip_vs_conn_in_get() relies on that counter for
the cport-zero retry used by the FTP/persistence connections referenced by
the Fixes: tag.

Could the comment be reworded to say that it is the NO_CPORT+TEMPLATE
combination that is not expected?

> +	if (flags & IP_VS_CONN_F_NO_CPORT) {
> +		if (flags & IP_VS_CONN_F_TEMPLATE)
> +			return false;
> +	}

[Severity: High]
Does this close the case described in the changelog as "the NO_CPORT+TEMPLATE
combination allows lookups for normal connections to hit template"?

A record with IP_VS_CONN_F_TEMPLATE set, NO_CPORT clear and a non-zero
cport passes this test, survives flags &= IP_VS_CONN_F_BACKUP_MASK (TEMPLATE
is part of that mask) and reaches ip_vs_proc_conn() -> ip_vs_conn_new() with
the cport taken from the wire (ip_vs_conn_fill_param_sync() copies
s->v4.cport, ip_vs_process_message_v0() copies s->cport).

ip_vs_conn_hash() hashes templates into the same dir-0 key space as normal
connections, and __ip_vs_conn_in_get() has no TEMPLATE test:

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 &&
			    ip_vs_addr_equal(p->af, p->caddr, &cp->caddr) &&
			    ip_vs_addr_equal(p->af, p->vaddr, &cp->vaddr) &&
			    (!p->cport ^
			     (!(cp->flags & IP_VS_CONN_F_NO_CPORT))) &&
			    p->protocol == cp->protocol) {
	...
}

For a packet whose client port matches the injected cport, the XOR term is
(0 ^ 1), so the template is returned as if it were a normal connection.
Legitimate templates never hit this only because ip_vs_sched_persist()
always creates them with cport == 0 and without NO_CPORT, which is the
invariant this helper does not enforce.

Once that happens, ip_vs_in() has no template guard, so live traffic is
forwarded through the template's packet_xmit and daddr/dport, and
set_tcp_state() then adjusts dest->activeconns:

net/netfilter/ipvs/ip_vs_proto_tcp.c:set_tcp_state() {
	...
		if (dest) {
			if (!(cp->flags & IP_VS_CONN_F_INACTIVE) &&
			    !tcp_state_active(new_state)) {
				atomic_dec(&dest->activeconns);
	...
}

while ip_vs_bind_dest() counted that entry under persistconns and
ip_vs_unbind_dest() only decrements persistconns for templates, so
dest->activeconns stays skewed and affects lc/wlc scheduling for the real
service.  The timeout is taken from the message (bounded only by
MAX_SCHEDULE_TIMEOUT / HZ in ip_vs_proc_conn()), so the bogus template can
be long lived.

Would rejecting TEMPLATE records with a non-zero cport, or excluding
templates in __ip_vs_conn_in_get(), be needed for the stated goal to hold?

[Severity: Low]
This is a pre-existing issue and not introduced by this patch, but since
the helper is the new place where flag combinations are screened: a record
with NO_CPORT set and a non-zero cport is still accepted, because only the
flags word is inspected and never cross-checked against the record's cport.

ip_vs_conn_new() hashes such an entry under the non-zero cport and bumps
ipvs->no_cport_conns[af]:

net/netfilter/ipvs/ip_vs_conn.c:ip_vs_conn_new() {
	...
	if (unlikely(flags & IP_VS_CONN_F_NO_CPORT)) {
		int af_id = ip_vs_af_index(cp->af);

		atomic_inc(&ipvs->no_cport_conns[af_id]);
	}
	...
}

The entry can then never be matched: the direct lookup fails the
(!p->cport ^ !(cp->flags & IP_VS_CONN_F_NO_CPORT)) test, and the cport-zero
retry in ip_vs_conn_in_get() hashes a different bucket than the one the
entry sits in, so ip_vs_conn_fill_cport() never runs and the counter stays
elevated until the entry expires.  While one such entry exists, every
conn-table miss in that address family does a second full hash lookup.

Is it worth also requiring cport == 0 when NO_CPORT is set?

> +	if (flags & IP_VS_CONN_F_ONE_PACKET)
> +		return false;
> +	return true;
> +}

[Severity: Low]
The changelog says:

  "2. ONE_PACKET: it is not sent by master, so we do not expect it in
  backup."

Is that accurate for controller connections?  ip_vs_sync_conn() tests
ONE_PACKET only for the connection it is entered with, and the test sits
above the sloop label:

net/netfilter/ipvs/ip_vs_sync.c:ip_vs_sync_conn() {
	...
	/* Do not sync ONE PACKET */
	if (cp->flags & IP_VS_CONN_F_ONE_PACKET)
		goto control;
sloop:
	...
	s->v4.flags = htonl(cp->flags & ~IP_VS_CONN_F_HASHED);
	...
control:
	/* synchronize its controller if it has */
	cp = cp->control;
	if (!cp)
		return;
	...
	goto sloop;
}

The controller path re-enters below the ONE_PACKET test and the serializer
clears only IP_VS_CONN_F_HASHED, so a controller carrying ONE_PACKET is
sent on the wire with that bit.  A persistence template can carry it:
ip_vs_bind_dest() strips the dest ONE_PACKET only for non-UDP and for
SYNC-created entries, and IP_VS_CONN_F_DEST_MASK includes
IP_VS_CONN_F_ONE_PACKET, which the legacy setsockopt path copies verbatim.

For a UDP persistent service configured that way, the master emits a
TEMPLATE|ONE_PACKET record which the backup now drops with retc = 25
instead of masking ONE_PACKET off and creating or refreshing the template,
so that template is no longer replicated.  The practical effect looks small
since ip_vs_conn_hash() returns early for ONE_PACKET on the master too, and
only the single record is skipped, but could the changelog wording be
adjusted, or the controller be masked in ip_vs_sync_conn()?

> @@ -972,8 +987,7 @@ static void ip_vs_process_message_v0(struct netns_ipvs *ipvs, const char *buffer
>  			return;
>  		}
>  		s = (struct ip_vs_sync_conn_v0 *) p;
> -		flags = ntohs(s->flags) | IP_VS_CONN_F_SYNC;
> -		flags &= ~IP_VS_CONN_F_HASHED;
> +		flags = ntohs(s->flags);
>  		if (flags & IP_VS_CONN_F_SEQ_MASK) {
>  			opt = (struct ip_vs_sync_conn_options *)&s[1];
>  			p += FULL_CONN_SIZE;
> @@ -986,6 +1000,13 @@ static void ip_vs_process_message_v0(struct netns_ipvs *ipvs, const char *buffer
>  			p += SIMPLE_CONN_SIZE;
>  		}
>  
> +		if (!ip_vs_sync_validate_flags(flags)) {
> +			IP_VS_DBG(2, "BACKUP v0, Invalid flags 0x%X\n", flags);
> +			continue;
> +		}
> +		flags &= IP_VS_CONN_F_BACKUP_MASK;
> +		flags |= IP_VS_CONN_F_SYNC;
> +

[ ... ]

> @@ -1141,7 +1162,13 @@ static inline int ip_vs_proc_sync_conn(struct netns_ipvs *ipvs, __u8 *p, __u8 *m
>  	}
>  
>  	/* Get flags and Mask off unsupported */
> -	flags  = ntohl(s->v4.flags) & IP_VS_CONN_F_BACKUP_MASK;
> +	flags = ntohl(s->v4.flags);
> +	if (!ip_vs_sync_validate_flags(flags)) {
> +		IP_VS_DBG(3, "BACKUP, Invalid flags 0x%X\n", flags);
> +		retc = 25;
> +		goto out;
> +	}
> +	flags &= IP_VS_CONN_F_BACKUP_MASK;
>  	flags |= IP_VS_CONN_F_SYNC;
>  	state = ntohs(s->v4.state);