Re: [PATCH v4 nf-next 1/3] ipvs: add flags for per-service secure TCP state table

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Reported for policy completeness only; my verification concludes
  this is NOT a defect in the series.

Pre-existing issues:
- [Low] Reported for policy completeness only; verified as a pre-existing
  cosmetic discrepancy with no functional effect.

--

ipvs: add flags for per-service secure TCP state table

This adds the uAPI bits IP_VS_SVC_F_SECURE_TCP (0x0100) and
IP_VS_CONN_F_SECURE_TCP (1 << 17), and stamps the connection flag in
ip_vs_bind_dest() for connections bound to a destination whose service
carries the service flag, plus in the cache_bypass path of ip_vs_leave().

A couple of small observations on the new header bits below.

> diff --git a/include/uapi/linux/ip_vs.h b/include/uapi/linux/ip_vs.h
> index 2c37c6ac7525..ade170109ff4 100644
> --- a/include/uapi/linux/ip_vs.h
> +++ b/include/uapi/linux/ip_vs.h
> @@ -27,6 +27,7 @@
>  
>  #define IP_VS_SVC_F_SCHED_SH_FALLBACK	IP_VS_SVC_F_SCHED1 /* SH fallback */
>  #define IP_VS_SVC_F_SCHED_SH_PORT	IP_VS_SVC_F_SCHED2 /* SH use port */
> +#define IP_VS_SVC_F_SECURE_TCP	0x0100		/* use the hardened TCP table */

[Severity: Low]
Both new bits are documented as "use the hardened TCP table", but nothing
in this commit reads either of them, so in this tree state the bits are
accepted from user space and stamped onto connections without changing
behaviour.

Checking the rest of the series, the consumer does arrive in the following
patch, "ipvs: tcp: enable per-connection secure_tcp in state machine",
which changes set_tcp_state() in net/netfilter/ipvs/ip_vs_proto_tcp.c to:

	table = pd->tcp_state_table;
	if (cp->flags & IP_VS_CONN_F_SECURE_TCP)
		table = tcp_states_dos;
	new_state = table[state_off + state_idx].next_state[cp->state];

So by the end of the series the header comment matches the implementation
and this looks like a plain producer/consumer split rather than a defect.
Noting it only in case the intermediate state matters for bisect, since the
bit is settable by CAP_NET_ADMIN through ip_vs_ctl.c before it has any
effect.

>  
>  /*
>   *      IPVS sync daemon states
> @@ -105,6 +106,7 @@
>  
>  /* Flags that are not sent to backup server start from bit 16 */
>  #define IP_VS_CONN_F_NFCT	(1 << 16)	/* use netfilter conntrack */
> +#define IP_VS_CONN_F_SECURE_TCP	(1 << 17)	/* use the hardened TCP table */

[Severity: Low]
This isn't an issue introduced by this patch, but the new bit lands under
the comment saying these flags are not sent to the backup server, while the
v1 serializer actually puts the whole flags word on the wire.

net/netfilter/ipvs/ip_vs_sync.c:ip_vs_sync_conn()

	s->v4.flags = htonl(cp->flags & ~IP_VS_CONN_F_HASHED);

So a connection carrying IP_VS_CONN_F_SECURE_TCP does transmit bit 17.

The receiving side drops it again in ip_vs_process_message_v1():

	flags  = ntohl(s->v4.flags) & IP_VS_CONN_F_BACKUP_MASK;

and IP_VS_CONN_F_BACKUP_MASK only contains FWD_MASK, NOOUTPUT, INACTIVE,
SEQ_MASK, NO_CPORT and TEMPLATE, so no high bit survives on the backup.

IP_VS_CONN_F_NFCT at bit 16 is already transmitted the same way, so the
mismatch between the comment and the wire format predates this change; this
just adds one more flag to it. Would it be worth adjusting the comment to
say the backup masks these bits off on receive rather than that they are not
sent?

>  
>  /* Connection flags from destination that can be changed by user space */
>  #define IP_VS_CONN_F_DEST_MASK (IP_VS_CONN_F_FWD_MASK | \

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260920054708.988867-1-adrianox%40gmail.com