Re: [PATCH v4 nf-next 2/3] ipvs: tcp: enable per-connection secure_tcp in state machine

[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] Documentation/networking/ipvs-sysctl.rst:283-288 documents the
  hardened ("more complicated") TCP state transition table as reachable…

--

ipvs: tcp: enable per-connection secure_tcp in state machine

This changes set_tcp_state() so that a connection carrying
IP_VS_CONN_F_SECURE_TCP is driven by tcp_states_dos instead of
pd->tcp_state_table, leaving the netns-wide table as the fallback.  The
old FIXME comment in tcp_timeout_change() is replaced with a note
describing the sysctl as the netns-wide default.

> diff --git a/net/netfilter/ipvs/ip_vs_proto_tcp.c b/net/netfilter/ipvs/ip_vs_proto_tcp.c
> index fec0e8b47b71..3b9a2c8e9a52 100644
> --- a/net/netfilter/ipvs/ip_vs_proto_tcp.c
> +++ b/net/netfilter/ipvs/ip_vs_proto_tcp.c
> @@ -451,11 +451,10 @@ static void tcp_timeout_change(struct ip_vs_proto_data *pd, int flags)
>  	int on = (flags & 1);		/* secure_tcp */
>  
>  	/*
> -	** FIXME: change secure_tcp to independent sysctl var
> -	** or make it per-service or per-app because it is valid
> -	** for most if not for all of the applications. Something
> -	** like "capabilities" (flags) for each object.
> -	*/
> +	 * This remains the netns-wide default / global floor (e.g. when
> +	 * memory pressure kicks in). Per-service hardening is now carried
> +	 * by IP_VS_CONN_F_SECURE_TCP on each connection (set_tcp_state).
> +	 */
>  	pd->tcp_state_table = (on ? tcp_states_dos : tcp_states);
>  }

[Severity: Low]
Should Documentation/networking/ipvs-sysctl.rst be updated along with this
comment change?

The only in-tree description of the hardened table currently ties it
exclusively to the sysctl:

Documentation/networking/ipvs-sysctl.rst:

	secure_tcp - INTEGER
		- 0  - disabled (default)

		The secure_tcp defense is to use a more complicated TCP state
		transition table. ...

The new comment recharacterises that knob as a netns-wide default/floor,
but the .rst still reads as if 0 means no connection uses the hardened
table.

>  
> @@ -479,6 +478,7 @@ set_tcp_state(struct ip_vs_proto_data *pd, struct ip_vs_conn *cp,
>  	int state_idx;
>  	int new_state = IP_VS_TCP_S_CLOSE;
>  	int state_off = tcp_state_off[direction];
> +	const struct tcp_states_t *table;

[ ... ]

> @@ -496,8 +496,10 @@ set_tcp_state(struct ip_vs_proto_data *pd, struct ip_vs_conn *cp,
>  		goto tcp_state_out;
>  	}
>  
> -	new_state =
> -		pd->tcp_state_table[state_off+state_idx].next_state[cp->state];
> +	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];

[Severity: Low]
With this override in set_tcp_state(), can an operator still conclude from
net.ipv4.vs.secure_tcp = 0 that no connection is using tcp_states_dos?

A service carrying the new IP_VS_SVC_F_SECURE_TCP bit
(include/uapi/linux/ip_vs.h, settable via IPVS_SVC_ATTR_FLAGS) propagates
IP_VS_CONN_F_SECURE_TCP to its connections, and then set_tcp_state() picks
tcp_states_dos regardless of the sysctl value, so for example SYN_RECV no
longer advances to ESTABLISHED on a bare SYN+ACK.

There is no entry under Documentation/ for IP_VS_SVC_F_SECURE_TCP or for
this new interaction with secure_tcp.  Could a documentation update be
added for the new user-settable flag?

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