br_netfilter registers hooks at the bridge hook points and at ip and ip6 prerouting. It also calls NF_HOOK() itself with its own functions as okfn: br_nf_forward() and br_nf_forward_arp() run the ip, ip6 and arp forward hooks with br_nf_forward_finish(), br_nf_post_routing() runs the ip and ip6 postrouting hooks with br_nf_dev_queue_xmit(). A packet queued to userspace from one of these hook points keeps that okfn in its nf_queue_entry and nf_reinject() calls it when the verdict arrives. Until commit 72ac1c0937df ("netfilter: nf_queue: limit the hook drop flush to the unregistered hook point") unregistering any hook flushed every queued packet of the network namespace, so brnf_exit_net() dropped these entries as a side effect of removing br_nf_ops. The flush now only covers the hook points of the removed hooks, and the entries queued from forward and postrouting stay in the queue after rmmod br_netfilter. The next verdict for one of them jumps into freed module text: BUG: unable to handle page fault for address: ffffffffc0802250 #PF: supervisor instruction fetch in kernel mode #PF: error_code(0x0010) - not-present page RIP: 0010:0xffffffffc0802250 Call Trace: nfqnl_reinject+0x221/0x3d0 nfqnl_recv_verdict+0x851/0xaa0 nfnetlink_rcv_msg+0x199/0x300 netlink_rcv_skb+0x59/0x100 netlink_unicast+0x28c/0x3c0 netlink_sendmsg+0x1f2/0x420 Modules linked in: [last unloaded: br_netfilter] Flush every queued packet of the namespace when br_netfilter leaves it. Move the hook removal to a pre_exit callback and do the flush in the exit callback: the pernet core runs a grace period between the two, so no hook of this module is still running when the flush happens and no entry with an okfn in this module can be queued after it. The same split is what nf_tables_pre_exit_net() uses for base chain hooks. nf_queue_nf_hook_drop() was declared in nf_internals.h, which is private to net/netfilter; move the declaration to nf_queue.h so that br_netfilter can call it, and include that header in the two files that used the private declaration. Reproduced in virtme-ng with br_netfilter built as a module: two network namespaces connected through a bridge with bridge-nf-call-iptables=1, an ip forward chain that sends udp dport 12345 to queue 1, and the nf_queue selftest helper holding the verdicts (-H). With 10 packets queued, rmmod br_netfilter returns and /proc/net/netfilter/nfnetlink_queue still shows the 10 entries; the first verdict panics the kernel as shown above. With this patch rmmod drops the 10 entries and the verdict fails with -ENOENT. nft_queue.sh passes. The code and this commit message were written with an LLM coding assistant from the review linked below. The author reviewed the result and validated it with the reproducer and the selftest. Fixes: 72ac1c0937df ("netfilter: nf_queue: limit the hook drop flush to the unregistered hook point") Reported-by: Sashiko (nipa) Closes: https://lore.kernel.org/netfilter-devel/179237288267.3135.9555179349652175002@kernel.org/ Suggested-by: Florian Westphal Assisted-by: LLM Signed-off-by: Antonio Ojea --- include/net/netfilter/nf_queue.h | 5 +++++ net/bridge/br_netfilter_hooks.c | 21 ++++++++++++++++++--- net/netfilter/nf_conntrack_core.c | 1 + net/netfilter/nf_internals.h | 3 --- net/netfilter/nft_ct.c | 1 + 5 files changed, 25 insertions(+), 6 deletions(-) diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h index 86021c55ba3c..9e1388179097 100644 --- a/include/net/netfilter/nf_queue.h +++ b/include/net/netfilter/nf_queue.h @@ -42,6 +42,11 @@ struct nf_queue_handler { void nf_register_queue_handler(const struct nf_queue_handler *qh); void nf_unregister_queue_handler(void); +/* Drop the packets of @net queued from the hook point of @ops, or every + * queued packet of @net when @ops is NULL. + */ +void nf_queue_nf_hook_drop(struct net *net, const struct nf_hook_ops *ops); + bool nf_queue_entry_get_refs(struct nf_queue_entry *entry); void nf_queue_entry_free(struct nf_queue_entry *entry); diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c index e15355ff215a..d3fc89c3317c 100644 --- a/net/bridge/br_netfilter_hooks.c +++ b/net/bridge/br_netfilter_hooks.c @@ -37,6 +37,7 @@ #include #include #include +#include #include #include @@ -1296,14 +1297,27 @@ static int __net_init brnf_init_net(struct net *net) return br_netfilter_sysctl_init_net(net); } +static void __net_exit brnf_pre_exit_net(struct net *net) +{ + struct brnf_net *brnet = net_generic(net, brnf_net_id); + + if (brnet->enabled) + nf_unregister_net_hooks(net, br_nf_ops, ARRAY_SIZE(br_nf_ops)); +} + static void __net_exit brnf_exit_net(struct net *net) { - struct brnf_net *brnet; + struct brnf_net *brnet = net_generic(net, brnf_net_id); - brnet = net_generic(net, brnf_net_id); if (brnet->enabled) { - nf_unregister_net_hooks(net, br_nf_ops, ARRAY_SIZE(br_nf_ops)); brnet->enabled = false; + /* A grace period has passed since brnf_pre_exit_net() removed + * the hooks, so none of them is still running. Drop what they + * queued from the ip, ip6 and arp forward and postrouting hook + * points: those entries carry an okfn in this module and the + * hook removal did not flush them. + */ + nf_queue_nf_hook_drop(net, NULL); } br_netfilter_sysctl_exit_net(net, brnet); @@ -1311,6 +1325,7 @@ static void __net_exit brnf_exit_net(struct net *net) static struct pernet_operations brnf_net_ops __read_mostly = { .init = brnf_init_net, + .pre_exit = brnf_pre_exit_net, .exit = brnf_exit_net, .id = &brnf_net_id, .size = sizeof(struct brnf_net), diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c index 931b76aacd93..42a7cbdbd7d5 100644 --- a/net/netfilter/nf_conntrack_core.c +++ b/net/netfilter/nf_conntrack_core.c @@ -49,6 +49,7 @@ #include #include #include +#include #include #include diff --git a/net/netfilter/nf_internals.h b/net/netfilter/nf_internals.h index ce5aff2cae71..06a795bbbb99 100644 --- a/net/netfilter/nf_internals.h +++ b/net/netfilter/nf_internals.h @@ -23,9 +23,6 @@ #define CTA_FILTER_F_ALL (CTA_FILTER_F_MAX-1) #define CTA_FILTER_FLAG(ctattr) CTA_FILTER_F_ ## ctattr -/* nf_queue.c */ -void nf_queue_nf_hook_drop(struct net *net, const struct nf_hook_ops *ops); - /* nf_log.c */ int __init netfilter_log_init(void); diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c index 5a8db5592036..7464805651ae 100644 --- a/net/netfilter/nft_ct.c +++ b/net/netfilter/nft_ct.c @@ -23,6 +23,7 @@ #include #include #include +#include #include "nf_internals.h" struct nft_ct_helper_obj { -- 2.56.0.218.g1ec904b173-goog A packet queued to userspace keeps its conntrack reference in skb->_nfct until the verdict arrives. The conntrack hooks sit at prerouting, local in, local out and postrouting; a packet queued from the forward hook point is covered by none of them. Until commit 72ac1c0937df ("netfilter: nf_queue: limit the hook drop flush to the unregistered hook point") unregistering the conntrack hooks flushed every queued packet of the network namespace and released those references. The flush now only covers the hook points of the removed hooks. Packets queued from forward keep their nf_conn alive, and nf_conntrack_cleanup_net_list() loops on cnet->count, with pernet_ops_rwsem held for write, until userspace sends a verdict or closes the queue. Flush every queued packet of the namespaces being cleaned up in nf_conntrack_cleanup_net_list(), after the synchronize_rcu_expedited() that waits for the packets still inside the hooks, so that a packet a hook was processing while the hooks were removed is dropped as well. Reproduced in virtme-ng with nf_conntrack built as a module and loaded with enable_hooks=1: two network namespaces routed through the host namespace, an ip forward chain that sends udp dport 12345 to queue 1, and the nf_queue selftest helper holding the verdicts (-H). With 10 packets queued and 10 conntrack entries, rmmod nf_conntrack blocks until the verdicts are sent, 15 s later in the test. With this patch rmmod returns at once, the 10 entries are dropped and the verdict fails with -ENOENT. nft_queue.sh passes. The code and this commit message were written with an LLM coding assistant from the review linked below. The author reviewed the result and validated it with the reproducer and the selftest. Fixes: 72ac1c0937df ("netfilter: nf_queue: limit the hook drop flush to the unregistered hook point") Reported-by: Sashiko (nipa) Closes: https://lore.kernel.org/netfilter-devel/179237288267.3135.9555179349652175002@kernel.org/ Suggested-by: Florian Westphal Assisted-by: LLM Signed-off-by: Antonio Ojea --- net/netfilter/nf_conntrack_core.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c index 42a7cbdbd7d5..3ccee38beb1f 100644 --- a/net/netfilter/nf_conntrack_core.c +++ b/net/netfilter/nf_conntrack_core.c @@ -2490,6 +2490,12 @@ void nf_conntrack_cleanup_net_list(struct list_head *net_exit_list) * delete... */ synchronize_rcu_expedited(); + + /* Packets queued to userspace hold nf_conn references; the hook + * removal only flushed the hook points conntrack was registered at. + */ + list_for_each_entry(net, net_exit_list, exit_list) + nf_queue_nf_hook_drop(net, NULL); i_see_dead_people: busy = 0; list_for_each_entry(net, net_exit_list, exit_list) { -- 2.56.0.218.g1ec904b173-goog Tests record a failure by setting the global ret=1 and the script ends with exit $ret. test_queue() overwrites that flag with the status of a bare wait, which is always 0. test_queue_bridge() runs last and calls test_queue() twice, so a failure recorded by any earlier test is lost and the script exits 0. Drop the assignment: test_queue() checks the listener output itself and exits on a mismatch. Three connectivity checks exit with $ret after printing FAIL; ret is still 0 when nothing has failed before, so the script exits 0 there as well. Exit with 1. Replacing the test_hook_drop_scope call with ret=1 shows the difference: the script exited 0 before this patch and exits 1 with it. Reported-by: Sashiko (nipa) Closes: https://lore.kernel.org/netfilter-devel/179237288374.3135.9473018103688224106@kernel.org/ Assisted-by: LLM Signed-off-by: Antonio Ojea --- .../testing/selftests/net/netfilter/nft_queue.sh | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/tools/testing/selftests/net/netfilter/nft_queue.sh b/tools/testing/selftests/net/netfilter/nft_queue.sh index a07652813a4b..516d9012d2e4 100755 --- a/tools/testing/selftests/net/netfilter/nft_queue.sh +++ b/tools/testing/selftests/net/netfilter/nft_queue.sh @@ -240,17 +240,16 @@ test_queue() busywait "$BUSYWAIT_TIMEOUT" nf_queue_wait "$nsrouter" 1 if ! test_ping;then - echo "FAIL: netns routing/connectivity with active listener on queues 0 and 1: $ret" 1>&2 - exit $ret + echo "FAIL: netns routing/connectivity with active listener on queues 0 and 1" 1>&2 + exit 1 fi if ! test_ping_router;then - echo "FAIL: netns router unreachable listener on queue 0 and 1: $ret" 1>&2 - exit $ret + echo "FAIL: netns router unreachable listener on queue 0 and 1" 1>&2 + exit 1 fi wait - ret=$? for file in $TMPFILE0 $TMPFILE1; do last=$(tail -n1 "$file") @@ -851,7 +850,7 @@ test_queue_bridge() if ! test_ping;then echo "FAIL: netns bridge connectivity" 1>&2 - exit $ret + exit 1 fi load_ruleset "bridge" "filter" 10 @@ -979,8 +978,8 @@ if test_ping; then # queue bypass works (rules were skipped, no listener) echo "PASS: ${ns1} can reach ${ns2}" else - echo "FAIL: ${ns1} cannot reach ${ns2}: $ret" 1>&2 - exit $ret + echo "FAIL: ${ns1} cannot reach ${ns2}" 1>&2 + exit 1 fi test_queue_blackhole ip -- 2.56.0.218.g1ec904b173-goog hook_drop_case() ignored two outcomes that make its result meaningless: when the 20 packets do not all reach the queue before the timeout, the drop case still passes with an empty queue, and when the nft command under test fails, nothing is unregistered and the keep case still passes. Report both as failures. The keep case deleted an ip6 prerouting chain, which differs from the ipv4 output packets in both the family and the hook number, so a nf_hook_cmp() that compared only one of the two would still pass. Add a keep case for each: an ip6 output chain and an ip prerouting chain. Reported-by: Sashiko (nipa) Closes: https://lore.kernel.org/netfilter-devel/179237288374.3135.9473018103688224106@kernel.org/ Assisted-by: LLM Signed-off-by: Antonio Ojea --- .../selftests/net/netfilter/nft_queue.sh | 30 +++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/tools/testing/selftests/net/netfilter/nft_queue.sh b/tools/testing/selftests/net/netfilter/nft_queue.sh index 516d9012d2e4..954b7bc4d789 100755 --- a/tools/testing/selftests/net/netfilter/nft_queue.sh +++ b/tools/testing/selftests/net/netfilter/nft_queue.sh @@ -912,10 +912,18 @@ table inet nfqscope { } counter delivered {} } +table ip unrelated { + chain prerouting { + type filter hook prerouting priority 0; policy accept; + } +} table ip6 unrelated { chain prerouting { type filter hook prerouting priority 0; policy accept; } + chain output { + type filter hook output priority 0; policy accept; + } } EOF ip netns exec "$ns1" ./nf_queue -c -q 1 -H -t "$timeout" > "$TMPFILE1" 2>&1 & @@ -926,13 +934,19 @@ EOF ip netns exec "$ns1" bash -c \ "for i in \$(seq $packets); do echo x > /dev/udp/127.0.0.1/12345; done" - busywait "$BUSYWAIT_TIMEOUT" hook_drop_all_queued "$packets" - - ip netns exec "$ns1" nft "$@" - queued=$(hook_drop_queued) + if ! busywait "$BUSYWAIT_TIMEOUT" hook_drop_all_queued "$packets"; then + echo "FAIL: $desc: $(hook_drop_queued) of $packets packets queued" 1>&2 + ret=1 + elif ! ip netns exec "$ns1" nft "$@"; then + echo "FAIL: $desc: nft $* failed" 1>&2 + ret=1 + else + queued=$(hook_drop_queued) + fi kill -USR1 "$nfqpid" wait "$nfqpid" + [ -z "$queued" ] && return delivered=$(hook_drop_delivered) if [ "$queued" = "$packets" ] && [ "$delivered" = "$packets" ] && @@ -955,9 +969,15 @@ EOF test_hook_drop_scope() { - # the hook array of another hook point changes, queued packets stay + # the hook array of another hook point changes, queued packets stay. + # The packets are ipv4 at output; cover a chain that differs in both + # family and hook, in the family only and in the hook only. hook_drop_case keep "base chain removed at another hook point" \ delete chain ip6 unrelated prerouting + hook_drop_case keep "base chain removed at the same hook of another family" \ + delete chain ip6 unrelated output + hook_drop_case keep "base chain removed at another hook of the same family" \ + delete chain ip unrelated prerouting # the hook array of the queueing hook point changes, the position of # the queued packets in it is stale, they are dropped -- 2.56.0.218.g1ec904b173-goog