mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Andrii Pasichnyk <apasichnik9@gmail.com>
To: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: netdev@vger.kernel.org, wireguard@lists.zx2c4.com,
	Andrew Lunn <andrew+netdev@lunn.ch>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@kernel.org>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net] wireguard: peer: free packets left on the per-peer queues on removal
Date: Tue, 10 Nov 2026 20:00:34 +0200	[thread overview]
Message-ID: <20261110180034.76136-1-apasichnik9@gmail.com> (raw)
In-Reply-To: <avNIS67grg_Qc5Vy@zx2c4.com>

On Tue, Nov 10, 2026 at 05:37:31PM +0100, Jason A. Donenfeld wrote:
> I'd be interested in seeing your adjustment to netns.sh so that I can
> test this myself and validate various approaches. Do you have something
> really minimal? Also, is this a test case that could be implemented
> minimally enough that we could integrate this into netns.sh permanently?

Here is a minimal version. It relies on the created/destroyed object
check netns.sh already does at the end:

diff --git a/tools/testing/selftests/wireguard/netns.sh b/tools/testing/selftests/wireguard/netns.sh
index a8f550aec..8303802c0 100755
--- a/tools/testing/selftests/wireguard/netns.sh
+++ b/tools/testing/selftests/wireguard/netns.sh
@@ -207,6 +207,20 @@ n0 iptables -F INPUT
 ip1 link set wg0 mtu $orig_mtu
 ip2 link set wg0 mtu $orig_mtu
 
+# Remove a peer while a UDP flood keeps its receive queue busy, then re-add it;
+# packets left on the per-peer queues would keep it from being destroyed,
+# which the created/destroyed check at the end catches.
+for ((i = 0; i < 10; ++i)); do
+	n1 iperf3 -s -1 -B 192.168.241.1 &
+	pids=( $! ); waitiperf $netns1 $!
+	n2 iperf3 -Z -t 2 -b 0 -u -l 64 -P 4 -c 192.168.241.1 >/dev/null 2>&1 &
+	pids+=( $! ); sleep 1
+	n1 wg set wg0 peer "$pub2" remove
+	kill "${pids[@]}" 2>/dev/null || true; wait "${pids[@]}" || true
+	n1 wg set wg0 peer "$pub2" preshared-key <(echo "$psk") allowed-ips 192.168.241.2/32,fd00::2/128 endpoint 127.0.0.1:2
+	n1 ping -c 1 -W 1 192.168.241.2
+done
+
 # Test using IPv4 that roaming works
 ip0 -4 addr del 127.0.0.1/8 dev lo
 ip0 -4 addr add 127.212.121.99/8 dev lo
diff --git a/tools/testing/selftests/wireguard/qemu/kernel.config b/tools/testing/selftests/wireguard/qemu/kernel.config
index 368fd9d60..c69a408a5 100644
--- a/tools/testing/selftests/wireguard/qemu/kernel.config
+++ b/tools/testing/selftests/wireguard/qemu/kernel.config
@@ -67,7 +67,7 @@ CONFIG_SYSCTL=y
 CONFIG_SYSFS=y
 CONFIG_TMPFS=y
 CONFIG_CONSOLE_LOGLEVEL_DEFAULT=15
-CONFIG_LOG_BUF_SHIFT=18
+CONFIG_LOG_BUF_SHIFT=20
 CONFIG_PRINTK_TIME=y
 CONFIG_BLK_DEV_INITRD=y
 CONFIG_LEGACY_VSYSCALL_NONE=y

Two details matter:

- The ping after the re-add. Without it the re-added peer has no
  session while the other side keeps using the old keypair, so nothing
  flows until the next handshake and only the first removal happens
  under load. My original reproducer had this flaw, so the "5 in 2140"
  in the commit message undercounts: most of those removals were idle.

- The log buffer. With message_cost=0 each removal under load logs up
  to ~1000 "Packet has unallowed src IP" lines, which overflow the
  default 256 KB buffer; the object check then reports false "merely
  destroyed" errors and can miss real leaks. 1 MB is enough for 10
  iterations. Without the kernel.config change, 2 iterations fit in the
  default buffer, but caught the leak in only 2 of 6 runs.

Results on net-next 66974df65a0b, x86 KVM:

  unpatched, 1 vCPU:  failed in all 3 runs; 25-40% of loaded removals
                      leak a peer and its keypair
  unpatched, 2 vCPUs: failed (1 run)
  unpatched, 4 vCPUs: passed (1 run)
  patched, 1 and 4 vCPUs: passed; 0 leaks in 212 loaded removals,
                      including a 200-iteration run

A leak shows as "Error: wg0: Peer N: merely created" plus a matching
"Keypair M: merely created". One gotcha when switching NR_CPUS: the
qemu Makefile does not regenerate the kernel .config, so delete
build/<arch>/kernel/.config first.

So yes, I think it can go into netns.sh permanently: 10 iterations add
about 13 seconds and caught the bug on every 1-vCPU run of an unpatched
kernel. I can send it as a separate patch, or fold it into a v2 of the
fix, whichever you prefer.

Thanks,
Andrii

      reply	other threads:[~2026-11-10 18:00 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-03 20:16 Andrii Pasichnyk
2026-10-03 20:19 ` netdev-bot+sinfo
2026-10-03 20:41   ` Andrii Pasichnyk
2026-11-10 16:37 ` Jason A. Donenfeld
2026-11-10 18:00   ` Andrii Pasichnyk [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20261110180034.76136-1-apasichnik9@gmail.com \
    --to=apasichnik9@gmail.com \
    --cc=Jason@zx2c4.com \
    --cc=andrew+netdev@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=edumazet@kernel.org \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=wireguard@lists.zx2c4.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®