[PATCH v4 nf-next 0/3] ipvs: add per-service secure_tcp

[PATCH v4 nf-next 0/3] ipvs: add per-service secure_tcp

[Adriano Cordova]

IPVS currently exposes secure_tcp as a per-netns sysctl. It switches the
TCP state machine to the hardened tcp_states_dos table.

These patches make it per-service: a virtual service can set
IP_VS_SVC_F_SECURE_TCP which is passed into IP_VS_CONN_F_SECURE_TCP at
connection creation. set_tcp_state() then selects tcp_states_dos for those
connections with IP_VS_CONN_F_SECURE_TCP set and keeps pd->tcp_state_table
(the netns default, including the nomem floor) otherwise.

  1. uapi: define the per-service secure_tcp flags and stamp the flag in
     ip_vs_bind_dest() (cache-bypass has no dest)
  2. honor it in the TCP state machine, resolving the stale FIXME
  3. kselftest contrasting a secure vs. a plain service

Changes in v2:
- Let IP_VS_SVC_F_SECURE_TCP be 0x0100, leaving 0x0040/0x0080 for the
  scheduler flags. Let IP_VS_CONN_F_SECURE_TCP be (1 << 17) and no longer
  in IP_VS_CONN_F_BACKUP_MASK.
- Set flag in ip_vs_bind_dest() instead of at every ip_vs_conn_new()
  call site.
- selftest: send probes with TTL=1 instead of an nft drop, and fix
  Sashiko comments.

Changes in v3:
- Merge patch 1/4 and 2/4
- Re-send patches (v2 dropped patches 3/4 and 4/4)

Changes in v4:
- selftest: check the return values of setsockopt(IP_HDRINCL), sendto()
  and mnl_socket_bind(), and fail do_add() when IPVS does not reply.
- selftest: fix the remaining Sashiko comments: check inet_pton(),
  initialize fam and the parsed addresses, and verify the flags attribute
  length before copying it.

Adriano Cordova (3):
  ipvs: add flags for per-service secure TCP state table
  ipvs: tcp: enable per-connection secure_tcp in state machine
  selftests: netfilter: ipvs: add per-service secure_tcp test

 include/uapi/linux/ip_vs.h                    |   2 +
 net/netfilter/ipvs/ip_vs_conn.c               |   4 +
 net/netfilter/ipvs/ip_vs_core.c               |   3 +
 net/netfilter/ipvs/ip_vs_proto_tcp.c          |  16 +-
 .../testing/selftests/net/netfilter/Makefile  |   6 +
 .../selftests/net/netfilter/gen_tcp_probe.c   | 158 ++++++++
 .../net/netfilter/ipvs_secure_tcp.sh          | 153 ++++++++
 .../net/netfilter/ipvs_secure_tcp_mln.c       | 337 ++++++++++++++++++
 8 files changed, 672 insertions(+), 7 deletions(-)
 create mode 100644 tools/testing/selftests/net/netfilter/gen_tcp_probe.c
 create mode 100755 tools/testing/selftests/net/netfilter/ipvs_secure_tcp.sh
 create mode 100644 tools/testing/selftests/net/netfilter/ipvs_secure_tcp_mln.c


base-commit: 87b80c2f6b05cad9f0ff9136709c62a0f59923e3
-- 
2.51.0

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

[Adriano Cordova]

Add IP_VS_SVC_F_SECURE_TCP to mark a virtual service for the
DoS-hardened TCP connection state table, and
IP_VS_CONN_F_SECURE_TCP to carry that capability on a connection
so the TCP state machine can select the hardened table for it.

The service flag is 0x0100: bits 0x0008/0x0010/0x0020 are the
scheduler bits, so leave 0x0040/0x0080 free for schedulers.

Set IP_VS_CONN_F_SECURE_TCP on every connection bound to a
destination whose service carries IP_VS_SVC_F_SECURE_TCP.

Signed-off-by: Adriano Cordova <adrianox@gmail.com>
---
Changes in v2:
- Let IP_VS_SVC_F_SECURE_TCP be 0x0100, leaving 0x0040/0x0080 free for
  scheduler flags.
- Let IP_VS_CONN_F_SECURE_TCP be (1 << 17) and drop it from
  IP_VS_CONN_F_BACKUP_MASK.
- Set the flag in ip_vs_bind_dest() instead of at every ip_vs_conn_new()
  call site.

Changes in v3:
- Merge the uapi flag definition and the ip_vs_bind_dest() stamping into
  a single patch.

(no changes since v3)

 include/uapi/linux/ip_vs.h      | 2 ++
 net/netfilter/ipvs/ip_vs_conn.c | 4 ++++
 net/netfilter/ipvs/ip_vs_core.c | 3 +++
 3 files changed, 9 insertions(+)

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 */
 
 /*
  *      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 */
 
 /* Connection flags from destination that can be changed by user space */
 #define IP_VS_CONN_F_DEST_MASK (IP_VS_CONN_F_FWD_MASK | \
diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
index 6fa3e1dc534c..0b465298b64b 100644
--- a/net/netfilter/ipvs/ip_vs_conn.c
+++ b/net/netfilter/ipvs/ip_vs_conn.c
@@ -1090,6 +1090,7 @@ ip_vs_bind_dest(struct ip_vs_conn *cp, struct ip_vs_dest *dest)
 {
 	unsigned int conn_flags;
 	__u32 flags;
+	struct ip_vs_service *svc;
 
 	/* if dest is NULL, then return directly */
 	if (!dest)
@@ -1102,6 +1103,9 @@ ip_vs_bind_dest(struct ip_vs_conn *cp, struct ip_vs_dest *dest)
 	if (cp->protocol != IPPROTO_UDP)
 		conn_flags &= ~IP_VS_CONN_F_ONE_PACKET;
 	flags = cp->flags;
+	svc = rcu_dereference(dest->svc);
+	if (svc && (svc->flags & IP_VS_SVC_F_SECURE_TCP))
+		flags |= IP_VS_CONN_F_SECURE_TCP;
 	/* Bind with the destination and its corresponding transmitter */
 	if (flags & IP_VS_CONN_F_SYNC) {
 		/* Synced conns are hashed, so they can not get this flag */
diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index ba0957798bad..aa8a3a964ffd 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -784,6 +784,9 @@ int ip_vs_leave(struct ip_vs_service *svc, struct sk_buff *skb,
 				      IP_VS_CONN_F_ONE_PACKET : 0;
 		union nf_inet_addr daddr = { .all = { 0, 0, 0, 0 } };
 
+		if (svc->flags & IP_VS_SVC_F_SECURE_TCP)
+			flags |= IP_VS_CONN_F_SECURE_TCP;
+
 		/* create a new connection entry */
 		IP_VS_DBG(6, "%s(): create a cache_bypass entry\n", __func__);
 		{
-- 
2.51.0

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

[Adriano Cordova]

Let set_tcp_state select tcp_states_dos when the connection
carries IP_VS_CONN_F_SECURE_TCP, otherwise keep using the
global pd->tcp_state_table (the netns default / or if nomem).

Signed-off-by: Adriano Cordova <adrianox@gmail.com>
---
(no changes since v1)

 net/netfilter/ipvs/ip_vs_proto_tcp.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

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);
 }
 
@@ -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;
 
 	/*
 	 *    Update state offset to INPUT_ONLY if necessary
@@ -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];
 
   tcp_state_out:
 	if (new_state != cp->state) {
-- 
2.51.0

[PATCH v4 nf-next 3/3] selftests: netfilter: ipvs: add per-service secure_tcp test

[Adriano Cordova]

Two services share a VIP, one carrying IP_VS_SVC_F_SECURE_TCP.
A bare SYN+ACK suffices to test the state machine: the normal
service reaches ESTABLISHED, but the secure one stays in SYN_RECV.

Assisted-by: opencode:deepseek-flash
Signed-off-by: Adriano Cordova <adrianox@gmail.com>
---
Changes in v2:
- send probes with TTL=1 instead of an nft drop, and fix
  Sashiko comments.

Changes in v4:
- Check the return value of setsockopt(IP_HDRINCL), sendto() and
  mnl_socket_bind(), and fail do_add() when IPVS does not reply.
- Check inet_pton(), initialize fam and the parsed addresses, and verify
  the flags attribute length before copying it (Sashiko).

 .../testing/selftests/net/netfilter/Makefile  |   6 +
 .../selftests/net/netfilter/gen_tcp_probe.c   | 158 ++++++++
 .../net/netfilter/ipvs_secure_tcp.sh          | 153 ++++++++
 .../net/netfilter/ipvs_secure_tcp_mln.c       | 337 ++++++++++++++++++
 4 files changed, 654 insertions(+)
 create mode 100644 tools/testing/selftests/net/netfilter/gen_tcp_probe.c
 create mode 100755 tools/testing/selftests/net/netfilter/ipvs_secure_tcp.sh
 create mode 100644 tools/testing/selftests/net/netfilter/ipvs_secure_tcp_mln.c

diff --git a/tools/testing/selftests/net/netfilter/Makefile b/tools/testing/selftests/net/netfilter/Makefile
index df3c20c90f5d..f88bf69cc874 100644
--- a/tools/testing/selftests/net/netfilter/Makefile
+++ b/tools/testing/selftests/net/netfilter/Makefile
@@ -22,6 +22,7 @@ TEST_PROGS := \
 	conntrack_tcp_unreplied.sh \
 	conntrack_vrf.sh \
 	ipvs.sh \
+	ipvs_secure_tcp.sh \
 	nf_conntrack_packetdrill.sh \
 	nf_nat_edemux.sh \
 	nft_audit.sh \
@@ -52,6 +53,8 @@ TEST_GEN_FILES = \
 	connect_close \
 	conntrack_dump_flush \
 	conntrack_reverse_clash \
+	gen_tcp_probe \
+	ipvs_secure_tcp_mln \
 	nf_queue \
 	sctp_collision \
 	udpclash \
@@ -62,6 +65,9 @@ include ../../lib.mk
 $(OUTPUT)/nf_queue: CFLAGS += $(MNL_CFLAGS)
 $(OUTPUT)/nf_queue: LDLIBS += $(MNL_LDLIBS)
 
+$(OUTPUT)/ipvs_secure_tcp_mln: CFLAGS += $(MNL_CFLAGS)
+$(OUTPUT)/ipvs_secure_tcp_mln: LDLIBS += $(MNL_LDLIBS)
+
 $(OUTPUT)/conntrack_dump_flush: CFLAGS += $(MNL_CFLAGS)
 $(OUTPUT)/conntrack_dump_flush: LDLIBS += $(MNL_LDLIBS)
 $(OUTPUT)/udpclash: LDLIBS += -lpthread
diff --git a/tools/testing/selftests/net/netfilter/gen_tcp_probe.c b/tools/testing/selftests/net/netfilter/gen_tcp_probe.c
new file mode 100644
index 000000000000..185c3b1146dc
--- /dev/null
+++ b/tools/testing/selftests/net/netfilter/gen_tcp_probe.c
@@ -0,0 +1,158 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Send a TCP SYN then a TCP ACK (no SYN-ACK, no data) to the VIP.
+ * IPVS's TCP state machine only inspects SYN/FIN/ACK/RST bits, so this
+ * exercises the INPUT-direction state transition:
+ *
+ *   SYN:  NONE -> SYN_RECV
+ *   ACK:  SYN_RECV -> ESTABLISHED   (tcp_states, normal)
+ *         SYN_RECV -> SYN_RECV      (tcp_states_dos, secure_tcp)
+ *
+ * A TTL of 1 keeps the packets from reaching the real server: IPVS updates
+ * the connection state before forwarding, then the packet expires and an
+ * ICMP time-exceeded is sent back to us.
+ *
+ * Requires CAP_NET_RAW.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <stdint.h>
+#include <arpa/inet.h>
+#include <sys/socket.h>
+#include <netinet/ip.h>
+#include <netinet/tcp.h>
+#include <linux/if_ether.h>
+
+static inline uint16_t csump(const void *data, size_t len)
+{
+	const uint8_t *p = data;
+	uint32_t sum = 0;
+
+	while (len > 1) {
+		uint16_t w;
+
+		memcpy(&w, p, sizeof(w));
+		sum += w;
+		p += 2;
+		len -= 2;
+	}
+	if (len) {
+		uint16_t w = 0;
+
+		memcpy(&w, p, 1);
+		sum += w;
+	}
+	while (sum >> 16)
+		sum = (sum & 0xffff) + (sum >> 16);
+	return ~sum;
+}
+
+static int send_seg(int fd, const struct in_addr *sip, uint16_t sport,
+		    const struct in_addr *dip, uint16_t dport,
+		    uint32_t seq, int syn, int ack)
+{
+	struct {
+		struct iphdr ip;
+		struct tcphdr tcp;
+	} pkt = { };
+	struct iphdr *ip = &pkt.ip;
+	struct tcphdr *tcp = &pkt.tcp;
+	struct sockaddr_in dst;
+
+	ip->version = 4;
+	ip->ihl = 5;
+	ip->tot_len = htons(sizeof(pkt));
+	ip->id = htons((uint16_t)(seq & 0xffff));
+	ip->ttl = 1;
+	ip->protocol = IPPROTO_TCP;
+	ip->saddr = sip->s_addr;
+	ip->daddr = dip->s_addr;
+
+	tcp->source = sport;
+	tcp->dest = dport;
+	tcp->seq = htonl(seq);
+	tcp->ack_seq = htonl(seq + 1);
+	tcp->doff = 5;
+	if (syn)
+		tcp->syn = 1;
+	if (ack)
+		tcp->ack = 1;
+	tcp->window = htons(1024);
+
+	ip->check = csump(ip, sizeof(struct iphdr));
+	/* pseudo header for TCP checksum */
+	{
+		uint8_t ph[12];
+		uint8_t tcpbuf[12 + sizeof(struct tcphdr)];
+
+		memcpy(ph, &ip->saddr, 4);
+		memcpy(ph + 4, &ip->daddr, 4);
+		ph[8] = 0;
+		ph[9] = IPPROTO_TCP;
+		ph[10] = (sizeof(struct tcphdr) >> 8) & 0xff;
+		ph[11] = sizeof(struct tcphdr) & 0xff;
+
+		memcpy(tcpbuf, ph, 12);
+		memcpy(tcpbuf + 12, tcp, sizeof(struct tcphdr));
+		tcp->check = csump(tcpbuf, sizeof(tcpbuf));
+	}
+
+	memset(&dst, 0, sizeof(dst));
+	dst.sin_family = AF_INET;
+	dst.sin_addr = *dip;
+	dst.sin_port = dport;
+	if (sendto(fd, &pkt, sizeof(pkt), 0, (struct sockaddr *)&dst,
+		   sizeof(dst)) < 0) {
+		perror("sendto");
+		return -1;
+	}
+	return 0;
+}
+
+int main(int argc, char *argv[])
+{
+	struct in_addr sip = { }, dip = { };
+	uint16_t sport, dport;
+	int fd, one = 1;
+	uint32_t seq = 0x12345678;
+
+	if (argc != 5) {
+		fprintf(stderr, "usage: %s <src_ip> <src_port> <dst_ip> <dst_port>\n",
+			argv[0]);
+		return 2;
+	}
+	if (inet_pton(AF_INET, argv[1], &sip) != 1 ||
+	    inet_pton(AF_INET, argv[3], &dip) != 1) {
+		fprintf(stderr, "bad address\n");
+		return 2;
+	}
+	sport = htons((uint16_t)atoi(argv[2]));
+	dport = htons((uint16_t)atoi(argv[4]));
+
+	fd = socket(AF_INET, SOCK_RAW, IPPROTO_RAW);
+	if (fd < 0) {
+		perror("raw socket");
+		return 1;
+	}
+	if (setsockopt(fd, IPPROTO_IP, IP_HDRINCL, &one, sizeof(one)) < 0) {
+		perror("setsockopt");
+		close(fd);
+		return 1;
+	}
+
+	if (send_seg(fd, &sip, sport, &dip, dport, seq, 1, 0) < 0) {
+		close(fd);
+		return 1;
+	}
+	usleep(100000);
+	if (send_seg(fd, &sip, sport, &dip, dport, seq + 1, 0, 1) < 0) {
+		close(fd);
+		return 1;
+	}
+
+	close(fd);
+	return 0;
+}
diff --git a/tools/testing/selftests/net/netfilter/ipvs_secure_tcp.sh b/tools/testing/selftests/net/netfilter/ipvs_secure_tcp.sh
new file mode 100755
index 000000000000..4d250bcbb2cf
--- /dev/null
+++ b/tools/testing/selftests/net/netfilter/ipvs_secure_tcp.sh
@@ -0,0 +1,153 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Runtime test for per-service secure_tcp (IP_VS_SVC_F_SECURE_TCP).
+#
+# Sets up the same 3-namespace topology as ipvs.sh
+# but checks the TCP state machine, not data forwarding.  Two
+# identical TCP services are added on the same VIP on different ports,
+# one is marked secure_tcp, the other is not. For each a bare SYN is
+# followed by a bare ACK (no SYN-ACK / no data).  IPVS classifies the
+# connection from the flag bits:
+#   * normal service:  SYN -> SYN_RECV, ACK -> ESTABLISHED
+#   * secure_tcp service:  SYN -> SYN_RECV, ACK -> SYN_RECV
+# This test checks that this is the case via `ipvsadm -Lnc`.
+#
+# Requires root, netns, ipvsadm, and the built helpers
+# ipvs_secure_tcp_mln and gen_tcp_probe.
+
+source lib.sh
+
+ret=0
+readonly vip="207.175.44.110"
+readonly gip="10.0.0.1"
+readonly dip="172.16.0.1"
+readonly rip="172.16.0.2"
+readonly cip="10.0.0.2"
+readonly sip="10.0.0.3"
+readonly port_secure=8081
+readonly port_plain=8080
+
+GREEN='\033[0;92m'
+RED='\033[0;31m'
+NC='\033[0m'
+
+checktool "ipvsadm -v" "run test without ipvsadm"
+
+setup() {
+	setup_ns ns0 ns1 ns2
+
+	ip link add veth01 netns "${ns0}" type veth peer name veth10 netns "${ns1}"
+	ip link add veth02 netns "${ns0}" type veth peer name veth20 netns "${ns2}"
+	ip link add veth12 netns "${ns1}" type veth peer name veth21 netns "${ns2}"
+
+	ip netns exec "${ns0}" ip link set veth01 up
+	ip netns exec "${ns0}" ip link set veth02 up
+	ip netns exec "${ns0}" ip link add br0 type bridge
+	ip netns exec "${ns0}" ip link set veth01 master br0
+	ip netns exec "${ns0}" ip link set veth02 master br0
+	ip netns exec "${ns0}" ip link set br0 up
+	ip netns exec "${ns0}" ip addr add "${cip}/24" dev br0
+
+	ip netns exec "${ns1}" ip link set veth10 up
+	ip netns exec "${ns1}" ip addr add "${gip}/24" dev veth10
+	ip netns exec "${ns1}" ip link set veth12 up
+	ip netns exec "${ns1}" ip addr add "${dip}/24" dev veth12
+	ip netns exec "${ns1}" ip link set lo up
+	ip netns exec "${ns1}" ip addr add "${vip}/32" dev lo:1
+	ip netns exec "${ns1}" sysctl -qw net.ipv4.ip_forward=1
+
+	ip netns exec "${ns2}" ip link set veth20 up
+	ip netns exec "${ns2}" ip addr add "${sip}/24" dev veth20
+	ip netns exec "${ns2}" ip link set veth21 up
+	ip netns exec "${ns2}" ip addr add "${rip}/24" dev veth21
+
+	ip netns exec "${ns2}" ip addr add "${vip}/32" dev lo:1
+
+	ip netns exec "${ns0}" ip route add "${vip}/32" via "${gip}" dev br0
+
+	# load ipvs, then the rr scheduler (separate calls: modprobe treats
+	# the second name as a module parameter, not a second module)
+	ip netns exec "${ns1}" modprobe ip_vs
+	ip netns exec "${ns1}" modprobe ip_vs_rr
+
+	sleep 1
+}
+
+cleanup() {
+	cleanup_all_ns
+}
+
+# State of the connection to the VIP:port, from `ipvsadm -Lnc`.
+# Fields: pro  expire  state  source  virtual  destination
+conn_state() {
+	local vport=$1
+	ip netns exec "${ns1}" ipvsadm -Lnc 2>/dev/null |
+		awk -v vt="${vip}:${vport}" '$5==vt { print $3; exit }'
+}
+
+assert_state() {
+	local port=$1 want=$2
+	local got
+	got="$(conn_state "$port")"
+	echo "  vip ${vip}:${port}: state=${got:-?}"
+	if [ "${got:-}" != "$want" ]; then
+		echo -e "${RED}FAIL${NC}: vip ${vip}:${port} expected state" \
+			"${want}, got ${got:-none}"
+		ret=1
+	fi
+}
+
+test_secure() {
+	local bin probe
+
+	# Register the two services (secure_tcp on the secure port)
+	bin="$(pwd)/ipvs_secure_tcp_mln"
+	probe="$(pwd)/gen_tcp_probe"
+	ip netns exec "${ns1}" "$bin" add "${vip}" "${port_secure}" secure
+	ip netns exec "${ns1}" "$bin" add "${vip}" "${port_plain}" plain
+
+	# Add a real server to both services.  Use NAT (-m): in DR the conn gets
+	# IP_VS_CONN_F_NOOUTPUT, which makes the client ACK an INPUT_ONLY event
+	# and even tcp_states_dos promotes to ESTABLISHED, hiding the difference.
+	ip netns exec "${ns1}" ipvsadm -a -m -t "${vip}:${port_secure}" -r "${rip}:${port_secure}"
+	ip netns exec "${ns1}" ipvsadm -a -m -t "${vip}:${port_plain}" -r "${rip}:${port_plain}"
+
+	# verify the flag was actually set
+	local got
+	got="$(ip netns exec "${ns1}" "$bin" get "${vip}" "${port_secure}")"
+	echo "  secured service reports: ${got}"
+	echo "${got}" | grep -q "secure_tcp=1" ||
+		{ echo -e "${RED}FAIL${NC}: flag not set"; ret=1; }
+	got="$(ip netns exec "${ns1}" "$bin" get "${vip}" "${port_plain}")"
+	echo "${got}" | grep -q "secure_tcp=0" ||
+		{ echo -e "${RED}FAIL${NC}: flag unexpectedly set"; ret=1; }
+
+	# The probes carry TTL=1, so IPVS updates the connection state and
+	# then the packet expires before reaching the real server (which
+	# stays silent, no RST to interfere with the observation).
+
+	# Push SYN then ACK to each service from the client
+	ip netns exec "${ns0}" "$probe" "${cip}" 40000 "${vip}" "${port_secure}"
+	ip netns exec "${ns0}" "$probe" "${cip}" 40001 "${vip}" "${port_plain}"
+	sleep 1
+
+	echo "Testing per-service secure_tcp..."
+	echo "  --- connection table (ipvsadm -Lnc) ---"
+	ip netns exec "${ns1}" ipvsadm -Lnc 2>/dev/null
+	echo "  --- end connection table ---"
+	assert_state "${port_plain}" ESTABLISHED
+	assert_state "${port_secure}" SYN_RECV
+}
+
+trap cleanup EXIT
+
+setup
+test_secure
+
+if [ "$ret" -ne 0 ]; then
+	echo -e "$(basename $0): ${RED}FAIL${NC}"
+	exit 1
+fi
+echo -e "$(basename $0): ${GREEN}PASS${NC}"
+exit 0
diff --git a/tools/testing/selftests/net/netfilter/ipvs_secure_tcp_mln.c b/tools/testing/selftests/net/netfilter/ipvs_secure_tcp_mln.c
new file mode 100644
index 000000000000..54c6d860addf
--- /dev/null
+++ b/tools/testing/selftests/net/netfilter/ipvs_secure_tcp_mln.c
@@ -0,0 +1,337 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * libmnl helper to set/query the per-service secure_tcp flag
+ * (IP_VS_SVC_F_SECURE_TCP), which ipvsadm does not expose.
+ *
+ * Usage:
+ *   ipvs_secure_tcp_mln add <vip> <port> <secure|plain>
+ *       Create a TCP virtual service (scheduler "rr") with the flag either
+ *       set or not.  Add real servers afterwards with:
+ *           ipvsadm -a -t <vip>:<port> -r <rs>:<port>
+ *   ipvs_secure_tcp_mln get <vip> <port>
+ *       Print "secure_tcp=<0|1>" for the service.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <arpa/inet.h>
+
+#include <linux/netlink.h>
+#include <linux/genetlink.h>
+#include <linux/ip_vs.h>
+
+#include <libmnl/libmnl.h>
+
+/* Fallback in case the kernel's installed uapi header is older */
+#ifndef IP_VS_SVC_F_SECURE_TCP
+#define IP_VS_SVC_F_SECURE_TCP	0x0100
+#endif
+
+/* 16-byte address storage, matching union nf_inet_addr for AF_INET */
+struct inet_addr16 {
+	uint8_t all[16];
+};
+
+/* ---------------- family resolver ---------------- */
+static int ctrl_attr_cb(const struct nlattr *attr, void *data)
+{
+	const struct nlattr **tb = data;
+	int type = mnl_attr_get_type(attr);
+
+	if (mnl_attr_type_valid(attr, CTRL_ATTR_MAX) < 0)
+		return MNL_CB_ERROR;
+	if (type == CTRL_ATTR_FAMILY_ID) {
+		if (mnl_attr_validate(attr, MNL_TYPE_U16) < 0)
+			return MNL_CB_ERROR;
+		tb[CTRL_ATTR_FAMILY_ID] = attr;
+	}
+	return MNL_CB_OK;
+}
+
+static int ctrl_data_cb(const struct nlmsghdr *nlh, void *data)
+{
+	const struct nlattr *tb[CTRL_ATTR_MAX + 1] = { 0 };
+	uint16_t *fam = data;
+
+	if (nlh->nlmsg_type != GENL_ID_CTRL)
+		return MNL_CB_OK;
+	mnl_attr_parse(nlh, sizeof(struct genlmsghdr),
+		       (mnl_attr_cb_t)ctrl_attr_cb, tb);
+	if (tb[CTRL_ATTR_FAMILY_ID]) {
+		*fam = mnl_attr_get_u16(tb[CTRL_ATTR_FAMILY_ID]);
+		return MNL_CB_STOP;
+	}
+	return MNL_CB_OK;
+}
+
+static int resolve_family(const char *name, uint16_t *fam)
+{
+	struct mnl_socket *nl;
+	char buf[MNL_SOCKET_BUFFER_SIZE];
+	struct nlmsghdr *nlh;
+	struct genlmsghdr *genl;
+	int ret;
+
+	*fam = 0;
+	nl = mnl_socket_open(NETLINK_GENERIC);
+	if (!nl)
+		return -errno;
+	if (mnl_socket_bind(nl, 0, 0) < 0) {
+		mnl_socket_close(nl);
+		return -errno;
+	}
+
+	nlh = mnl_nlmsg_put_header(buf);
+	genl = mnl_nlmsg_put_extra_header(nlh, sizeof(struct genlmsghdr));
+	genl->cmd = CTRL_CMD_GETFAMILY;
+	genl->version = 1;
+	nlh->nlmsg_type = GENL_ID_CTRL;
+	nlh->nlmsg_flags = NLM_F_REQUEST;
+	mnl_attr_put_strz(nlh, CTRL_ATTR_FAMILY_NAME, name);
+
+	if (mnl_socket_sendto(nl, nlh, nlh->nlmsg_len) < 0) {
+		mnl_socket_close(nl);
+		return -errno;
+	}
+	do {
+		ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
+		if (ret < 0) {
+			if (errno == EAGAIN)
+				continue;
+			mnl_socket_close(nl);
+			return -errno;
+		}
+		ret = mnl_cb_run(buf, ret, 0, mnl_socket_get_portid(nl),
+				 (mnl_cb_t)ctrl_data_cb, fam);
+	} while (ret > 0 && *fam == 0);
+
+	mnl_socket_close(nl);
+	return *fam ? 0 : -ENOENT;
+}
+
+/* ---------------- fill service identifying attrs ---------------- */
+static int fill_service(struct nlmsghdr *nlh, const char *vip,
+			uint16_t port, int full, int secure)
+{
+	struct inet_addr16 vaddr = { 0 };
+	struct nlattr *nest;
+	struct ip_vs_flags fl;
+	int af = AF_INET;
+
+	if (inet_pton(af, vip, vaddr.all) != 1) {
+		fprintf(stderr, "bad VIP %s\n", vip);
+		return -EINVAL;
+	}
+
+	nest = mnl_attr_nest_start(nlh, IPVS_CMD_ATTR_SERVICE);
+	mnl_attr_put_u16(nlh, IPVS_SVC_ATTR_AF, af);
+	mnl_attr_put_u16(nlh, IPVS_SVC_ATTR_PROTOCOL, IPPROTO_TCP);
+	mnl_attr_put(nlh, IPVS_SVC_ATTR_ADDR, sizeof(vaddr), &vaddr);
+	/* port/be16: port is passed in network order from main() */
+	mnl_attr_put_u16(nlh, IPVS_SVC_ATTR_PORT, port);
+
+	if (full) {
+		mnl_attr_put_strz(nlh, IPVS_SVC_ATTR_SCHED_NAME, "rr");
+		memset(&fl, 0, sizeof(fl));
+		fl.mask = IP_VS_SVC_F_SECURE_TCP;
+		if (secure)
+			fl.flags = IP_VS_SVC_F_SECURE_TCP;
+		mnl_attr_put(nlh, IPVS_SVC_ATTR_FLAGS, sizeof(fl), &fl);
+		mnl_attr_put_u32(nlh, IPVS_SVC_ATTR_TIMEOUT, 0);
+		mnl_attr_put_u32(nlh, IPVS_SVC_ATTR_NETMASK, 0xffffffff);
+	}
+	mnl_attr_nest_end(nlh, nest);
+	return 0;
+}
+
+static int send_cmd(struct mnl_socket *nl, struct nlmsghdr *nlh)
+{
+	if (mnl_socket_sendto(nl, nlh, nlh->nlmsg_len) < 0) {
+		perror("sendto");
+		return -1;
+	}
+	return 0;
+}
+
+/* ---------------- get secure flag ---------------- */
+static int svc_attr_cb(const struct nlattr *attr, void *data)
+{
+	const struct nlattr **tb = data;
+	int type = mnl_attr_get_type(attr);
+
+	if (mnl_attr_type_valid(attr, IPVS_SVC_ATTR_MAX) < 0)
+		return MNL_CB_ERROR;
+	tb[type] = attr;
+	return MNL_CB_OK;
+}
+
+static int get_cb(const struct nlmsghdr *nlh, void *data)
+{
+	const struct nlattr *tb[IPVS_SVC_ATTR_MAX + 1] = { 0 };
+	struct ip_vs_flags fl;
+	int *secure = data;
+	struct nlattr *nest;
+
+	mnl_attr_for_each(nest, nlh, sizeof(struct genlmsghdr)) {
+		if (mnl_attr_get_type(nest) == IPVS_CMD_ATTR_SERVICE)
+			mnl_attr_parse_nested(nest, (mnl_attr_cb_t)svc_attr_cb, tb);
+	}
+	if (tb[IPVS_SVC_ATTR_FLAGS] &&
+	    mnl_attr_get_payload_len(tb[IPVS_SVC_ATTR_FLAGS]) >= sizeof(fl)) {
+		memcpy(&fl, mnl_attr_get_payload(tb[IPVS_SVC_ATTR_FLAGS]),
+		       sizeof(fl));
+		*secure = !!(fl.flags & IP_VS_SVC_F_SECURE_TCP);
+	}
+	return MNL_CB_STOP;
+}
+
+static int do_get(uint16_t fam, const char *vip, uint16_t port)
+{
+	struct mnl_socket *nl;
+	char buf[MNL_SOCKET_BUFFER_SIZE];
+	struct nlmsghdr *nlh;
+	struct genlmsghdr *genl;
+	int ret, secure = -1;
+
+	nl = mnl_socket_open(NETLINK_GENERIC);
+	if (!nl)
+		return -errno;
+	if (mnl_socket_bind(nl, 0, 0) < 0) {
+		mnl_socket_close(nl);
+		return -errno;
+	}
+	nlh = mnl_nlmsg_put_header(buf);
+	genl = mnl_nlmsg_put_extra_header(nlh, sizeof(struct genlmsghdr));
+	genl->cmd = IPVS_CMD_GET_SERVICE;
+	genl->version = IPVS_GENL_VERSION;
+	nlh->nlmsg_type = fam;
+	nlh->nlmsg_flags = NLM_F_REQUEST;
+	ret = fill_service(nlh, vip, port, 0, 0);
+	if (ret < 0) {
+		mnl_socket_close(nl);
+		return ret;
+	}
+	if (send_cmd(nl, nlh) < 0) {
+		mnl_socket_close(nl);
+		return -1;
+	}
+
+	ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
+	while (ret >= 0) {
+		ret = mnl_cb_run(buf, ret, 0, mnl_socket_get_portid(nl),
+				 (mnl_cb_t)get_cb, &secure);
+		if (ret <= MNL_CB_STOP || secure >= 0)
+			break;
+		ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
+	}
+	mnl_socket_close(nl);
+	if (secure < 0)
+		return -ENOENT;
+	printf("secure_tcp=%d\n", secure);
+	return 0;
+}
+
+/* ---------------- add service with flag ---------------- */
+static int do_add(uint16_t fam, const char *vip, uint16_t port, int secure)
+{
+	struct mnl_socket *nl;
+	char buf[MNL_SOCKET_BUFFER_SIZE];
+	struct nlmsghdr *nlh;
+	struct genlmsghdr *genl;
+	int ret;
+
+	/* NLM_F_EXCL: fail if the service already exists */
+	nlh = mnl_nlmsg_put_header(buf);
+	genl = mnl_nlmsg_put_extra_header(nlh, sizeof(struct genlmsghdr));
+	genl->cmd = IPVS_CMD_NEW_SERVICE;
+	genl->version = IPVS_GENL_VERSION;
+	nlh->nlmsg_type = fam;
+	nlh->nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK | NLM_F_CREATE | NLM_F_EXCL;
+	ret = fill_service(nlh, vip, port, 1, secure);
+	if (ret < 0)
+		return 1;
+
+	nl = mnl_socket_open(NETLINK_GENERIC);
+	if (!nl)
+		return 1;
+	if (mnl_socket_bind(nl, 0, 0) < 0) {
+		mnl_socket_close(nl);
+		return 1;
+	}
+	if (send_cmd(nl, nlh) < 0) {
+		mnl_socket_close(nl);
+		return 1;
+	}
+
+	/* Read the reply so we can report why a command may have failed */
+	for (;;) {
+		ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
+		if (ret <= 0) {
+			fprintf(stderr, "no reply from IPVS\n");
+			mnl_socket_close(nl);
+			return 1;
+		}
+		ret = mnl_cb_run(buf, ret, 0, mnl_socket_get_portid(nl),
+				 NULL, NULL);
+		if (ret < 0) {
+			int e = errno;
+
+			fprintf(stderr, "IPVS netlink error: ret=%d errno=%d (%s)\n",
+				ret, e, strerror(e));
+			mnl_socket_close(nl);
+			return 1;
+		}
+		if (ret <= MNL_CB_STOP)
+			break;
+	}
+	mnl_socket_close(nl);
+	return 0;
+}
+
+int main(int argc, char *argv[])
+{
+	const char *cmd, *vip;
+	uint16_t fam = 0;
+	uint16_t port;
+	int ret, secure = 0;
+
+	if (argc < 4) {
+		fprintf(stderr,
+			"usage: %s add <vip> <port> <secure|plain>\n"
+			"       %s get <vip> <port>\n", argv[0], argv[0]);
+		return 2;
+	}
+	cmd = argv[1];
+	vip = argv[2];
+	port = (uint16_t)atoi(argv[3]);
+	port = htons(port);
+
+	ret = resolve_family(IPVS_GENL_NAME, &fam);
+	if (ret) {
+		fprintf(stderr, "cannot resolve IPVS genl family: %s\n",
+			strerror(-ret));
+		return 1;
+	}
+
+	if (strcmp(cmd, "add") == 0) {
+		if (argc < 5) {
+			fprintf(stderr, "usage: %s add ... <secure|plain>\n",
+				argv[0]);
+			return 2;
+		}
+		if (strcmp(argv[4], "secure") == 0) {
+			secure = 1;
+		} else if (strcmp(argv[4], "plain") != 0) {
+			fprintf(stderr, "unknown mode %s\n", argv[4]);
+			return 2;
+		}
+		return do_add(fam, vip, port, secure);
+	} else if (strcmp(cmd, "get") == 0) {
+		return do_get(fam, vip, port);
+	}
+
+	fprintf(stderr, "unknown command %s\n", cmd);
+	return 2;
+}
-- 
2.51.0

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

[netdev-bot+sashiko]

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

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

[netdev-bot+sashiko]

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

Re: [PATCH v4 nf-next 3/3] selftests: netfilter: ipvs: add per-service secure_tcp test

[netdev-bot+sashiko]

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

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

- [Medium] tools/testing/selftests/net/netfilter/ipvs_secure_tcp.sh has
  no kernel-side prerequisite check and no $ksft_skip path.
- [Medium] The secure-service assertion cannot distinguish 'per-service
  secure_tcp worked' from 'the ACK probe never arrived'.
- [Low] The commit message states "A bare SYN+ACK suffices to test the
  state machine: the normal service reaches ESTABLISHED, but the secure…

--

selftests: netfilter: ipvs: add per-service secure_tcp test

This adds a new netfilter selftest that creates two IPVS TCP services on
the same VIP, one with IP_VS_SVC_F_SECURE_TCP, and checks the resulting
connection states with ipvsadm -Lnc.  Two helpers are added: a libmnl
program to set/query the per-service flag and a raw-socket probe that
sends a bare SYN followed by a bare ACK.

>     Two services share a VIP, one carrying IP_VS_SVC_F_SECURE_TCP.
>     A bare SYN+ACK suffices to test the state machine: the normal
>     service reaches ESTABLISHED, but the secure one stays in SYN_RECV.

[Severity: Low]
Does the changelog wording match what gen_tcp_probe.c actually sends?

main() transmits two separate bare segments, a SYN and then a pure ACK:

	if (send_seg(fd, &sip, sport, &dip, dport, seq, 1, 0) < 0) {
	...
	usleep(100000);
	if (send_seg(fd, &sip, sport, &dip, dport, seq + 1, 0, 1) < 0) {

A single segment with both bits set would not show anything, since
tcp_state_idx() in net/netfilter/ipvs/ip_vs_proto_tcp.c tests rst, then
syn, then fin, then ack:

	if (th->rst)
		return 3;
	if (th->syn)
		return 0;
	if (th->fin)
		return 1;
	if (th->ack)
		return 2;

so SYN|ACK is classified as syn, and the INPUT syn row of both tcp_states
and tcp_states_dos maps sNO -> sSR, i.e. no difference between the secure
and the plain service.

Would "a bare SYN followed by a bare ACK" (the wording already used in the
gen_tcp_probe.c header comment) be more accurate here?

> diff --git a/tools/testing/selftests/net/netfilter/ipvs_secure_tcp.sh b/tools/testing/selftests/net/netfilter/ipvs_secure_tcp.sh
> new file mode 100755
> index 000000000000..4d250bcbb2cf
> --- /dev/null
> +++ b/tools/testing/selftests/net/netfilter/ipvs_secure_tcp.sh
> @@ -0,0 +1,153 @@

[ ... ]

> +source lib.sh
> +
> +ret=0

[ ... ]

> +checktool "ipvsadm -v" "run test without ipvsadm"
> +
> +setup() {
> +	setup_ns ns0 ns1 ns2

[ ... ]

> +	# load ipvs, then the rr scheduler (separate calls: modprobe treats
> +	# the second name as a module parameter, not a second module)
> +	ip netns exec "${ns1}" modprobe ip_vs
> +	ip netns exec "${ns1}" modprobe ip_vs_rr

[Severity: Medium]
What happens on a kernel built without CONFIG_IP_VS or without the rr
scheduler?

The only pre-flight gate is checktool "ipvsadm -v", which validates the
userspace binary only.  Both modprobe calls in setup() have their exit
status discarded (no set -e, no ||), so a missing ip_vs goes unnoticed:
ipvs_secure_tcp_mln add fails, the connection table stays empty, both
assert_state() calls set ret=1, and the script exits 1.

$ksft_skip is never used anywhere in this script even though lib.sh is
sourced.  The pre-existing ipvs.sh in the same directory guards this:

	if [ ! -d $sysipvsnet ]; then
		if ! modprobe -q ip_vs; then
			echo "skip: could not run test without ipvs module"
			exit $ksft_skip
		fi
	fi

Could the same skip be added here, so unsupported configurations are
reported as skipped rather than failed?

Note that the helper's get readback cannot be used for feature detection
either: ip_vs_genl_parse_service() stores the requested bits without
rejecting unknown ones,

		usvc->flags = (usvc->flags & ~flags.mask) |
			      (flags.flags & flags.mask);

so a kernel lacking per-service secure_tcp still echoes secure_tcp=1.

> +
> +	sleep 1
> +}

[ ... ]

> +assert_state() {
> +	local port=$1 want=$2
> +	local got
> +	got="$(conn_state "$port")"
> +	echo "  vip ${vip}:${port}: state=${got:-?}"
> +	if [ "${got:-}" != "$want" ]; then
> +		echo -e "${RED}FAIL${NC}: vip ${vip}:${port} expected state" \
> +			"${want}, got ${got:-none}"
> +		ret=1
> +	fi
> +}

[ ... ]

> +	# Push SYN then ACK to each service from the client
> +	ip netns exec "${ns0}" "$probe" "${cip}" 40000 "${vip}" "${port_secure}"
> +	ip netns exec "${ns0}" "$probe" "${cip}" 40001 "${vip}" "${port_plain}"
> +	sleep 1

[ ... ]

> +	assert_state "${port_plain}" ESTABLISHED
> +	assert_state "${port_secure}" SYN_RECV
> +}

[Severity: Medium]
Can assert_state "${port_secure}" SYN_RECV pass without the ACK ever
being processed?

SYN_RECV is exactly the state the first SYN alone produces.  In
net/netfilter/ipvs/ip_vs_proto_tcp.c the INPUT syn row is sNO -> sSR in
both tables, and only the following input ACK differs:

/*syn*/ {{sSR, sES, sES, sSR, sSR, sSR, sSR, sSR, sSR, sSR, sSR }}, /* tcp_states */
/*syn*/ {{sSR, sES, sES, sSR, sSR, sSR, sSR, sSR, sSR, sSR, sSA }}, /* tcp_states_dos */

The probe exit status is discarded here:

	ip netns exec "${ns0}" "$probe" "${cip}" 40000 "${vip}" "${port_secure}"

There is no || and no set -e, while gen_tcp_probe.c main() returns 1 when
the second send_seg() fails:

	usleep(100000);
	if (send_seg(fd, &sip, sport, &dip, dport, seq + 1, 0, 1) < 0) {
		close(fd);
		return 1;
	}

So a probe run that dies after the SYN (sendto failure, or the helper
being killed during the usleep) leaves the connection in SYN_RECV and the
secure-side assertion still succeeds.  The plain-service assertion is only
a partial control, since it is a separate invocation with a different
source port.

Would checking the probe exit status, or asserting something that only the
processed ACK can produce, make this assertion distinguish the two cases?

> +
> +trap cleanup EXIT
> +
> +setup
> +test_secure
> +
> +if [ "$ret" -ne 0 ]; then
> +	echo -e "$(basename $0): ${RED}FAIL${NC}"
> +	exit 1
> +fi
> +echo -e "$(basename $0): ${GREEN}PASS${NC}"
> +exit 0

[ ... ]

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