[PATCH bpf v2 0/2] net: xdp: fix bpf_xdp_shrink_data() page handling on generic XDP and veth

[PATCH bpf v2 0/2] net: xdp: fix bpf_xdp_shrink_data() page handling on generic XDP and veth

[Jiayuan Chen]

I'm always confused which tree(net or bpf) should XDP target.

bpf_xdp_shrink_data() frees a page_pool frag with the wrong memory type on
skb-backed XDP, hitting "Bad page state ... page_pool leak". Both the
generic XDP path and the veth path are affected.

Patch 1 fixes it by carrying the memory type in the xdp_buff itself, so it
no longer depends on rxq->mem.type (which is shared on generic XDP and gets
reset on veth). It is reported by syzbot.

Patch 2 adds a selftest that reproduces the leak on both paths.


v1 -> v2: AI found the fix was insufficient and we need a general way
          to fix them.
v1: https://lore.kernel.org/bpf/20260816031245.268898-1-jiayuan.chen@linux.dev/

Jiayuan Chen (2):
  bpf, veth: xdp: fix page_pool page leak on skb-backed XDP
  selftests/bpf: add xdp_shrink_frags

 drivers/net/veth.c                            |   5 +
 include/net/xdp.h                             |  14 ++
 net/core/dev.c                                |   5 +
 net/core/filter.c                             |   7 +
 .../bpf/prog_tests/xdp_shrink_frags.c         | 163 ++++++++++++++++++
 .../selftests/bpf/progs/xdp_shrink_frags.c    |  23 +++
 6 files changed, 217 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_shrink_frags.c
 create mode 100644 tools/testing/selftests/bpf/progs/xdp_shrink_frags.c

-- 
2.43.0

[PATCH bpf v2 1/2] bpf, veth: xdp: fix page_pool page leak on skb-backed XDP

[Jiayuan Chen]

bpf_xdp_shrink_data() frees a released frag via __xdp_return() using
xdp->rxq->mem.type, but that type is wrong for skb-backed XDP: the skb is
cow'd into page_pool memory while the rxq still says MEM_TYPE_PAGE_SHARED,
so the page_pool page is freed with page_frag_free() and we hit
"Bad page state ... page_pool leak".

Both generic XDP and veth are affected. A non-linear skb is cow'd into
page_pool memory (skb_cow_data_for_xdp() -> skb_pp_cow_data() for generic
XDP, veth_convert_skb_to_xdp_buff() for veth), so its frags become
page_pool pages while the rxq keeps MEM_TYPE_PAGE_SHARED.

We can't just fix rxq->mem.type in place:
- generic XDP: xdp->rxq is dev->_rx[queue].xdp_rxq (see
  bpf_prog_run_generic_xdp()), a shared rxq that other CPUs may access in
  parallel, so we must not write to it.
- veth: rq->xdp_rxq.mem is shared per-queue state that veth resets on XDP
  teardown, and with GRO that reset runs without stopping in-flight NAPI,
  so a type stashed there can be clobbered under a packet still in flight.

Adding a check in __xdp_return() or bpf_xdp_shrink_data() itself is not an
option either: without recording it somewhere, both can only guess the
frag's memory type, which quickly gets confusing.

So record it in the xdp_buff. Add a XDP_FLAGS_FRAGS_PAGE_POOL flag; the two
skb-cow sites set it, and bpf_xdp_shrink_data() frees the frag to the
page_pool when it is set, otherwise it keeps falling back to
xdp->rxq->mem.type unchanged. No other path changes behaviour.

Fixes: e6d5dbdd20aa ("xdp: add multi-buff support for xdp running in generic mode")
Fixes: 0ebab78cbcbf ("net: veth: add page_pool for page recycling")
Reported-by: syzbot+237bbeed8dfe0699b7f5@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=237bbeed8dfe0699b7f5
Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 drivers/net/veth.c |  5 +++++
 include/net/xdp.h  | 14 ++++++++++++++
 net/core/dev.c     |  5 +++++
 net/core/filter.c  |  7 +++++++
 4 files changed, 31 insertions(+)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 6ed3ee81153f..0afa0661ada1 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -775,6 +775,11 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 	if (skb_shinfo(skb)->nr_frags) {
 		skb_shinfo(skb)->xdp_frags_size = skb->data_len;
 		xdp_buff_set_frags_flag(xdp);
+		/* A nonlinear skb was cow'd into rq->page_pool above, so the
+		 * frags must be freed to that pool, not via the rxq's
+		 * MEM_TYPE_PAGE_SHARED.
+		 */
+		xdp_buff_set_frag_pp(xdp);
 	} else {
 		xdp_buff_clear_frags_flag(xdp);
 	}
diff --git a/include/net/xdp.h b/include/net/xdp.h
index aa742f413c35..b389dc527adc 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -81,6 +81,10 @@ enum xdp_buff_flags {
 	 * XDP program is not attached.
 	 */
 	XDP_FLAGS_FRAGS_UNREADABLE	= BIT(2),
+	/* frags are page_pool memory even though rxq->mem.type is not: a
+	 * skb-backed XDP buff (generic XDP, veth) is cow'd into a page_pool.
+	 */
+	XDP_FLAGS_FRAGS_PAGE_POOL	= BIT(3),
 };
 
 struct xdp_buff {
@@ -131,6 +135,16 @@ static __always_inline void xdp_buff_set_frag_unreadable(struct xdp_buff *xdp)
 	xdp->flags |= XDP_FLAGS_FRAGS_UNREADABLE;
 }
 
+static __always_inline void xdp_buff_set_frag_pp(struct xdp_buff *xdp)
+{
+	xdp->flags |= XDP_FLAGS_FRAGS_PAGE_POOL;
+}
+
+static __always_inline bool xdp_buff_is_frag_pp(const struct xdp_buff *xdp)
+{
+	return !!(xdp->flags & XDP_FLAGS_FRAGS_PAGE_POOL);
+}
+
 static __always_inline u32 xdp_buff_get_skb_flags(const struct xdp_buff *xdp)
 {
 	return xdp->flags;
diff --git a/net/core/dev.c b/net/core/dev.c
index 38336858c168..be36020484b6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5532,6 +5532,11 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
 	if (skb_is_nonlinear(skb)) {
 		skb_shinfo(skb)->xdp_frags_size = skb->data_len;
 		xdp_buff_set_frags_flag(xdp);
+		/* A nonlinear skb was cow'd into page_pool memory by
+		 * skb_cow_data_for_xdp() before we got here, so the frags must
+		 * be freed to that pool, not via the rxq's MEM_TYPE_PAGE_SHARED.
+		 */
+		xdp_buff_set_frag_pp(xdp);
 	} else {
 		xdp_buff_clear_frags_flag(xdp);
 	}
diff --git a/net/core/filter.c b/net/core/filter.c
index 61940e753552..d34ba56d79d8 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4378,6 +4378,13 @@ static bool bpf_xdp_shrink_data(struct xdp_buff *xdp, skb_frag_t *frag,
 	if (mem_type == MEM_TYPE_XSK_BUFF_POOL) {
 		netmem = 0;
 		zc_frag = bpf_xdp_shrink_data_zc(xdp, shrink, tail, release);
+	} else if (xdp_buff_is_frag_pp(xdp)) {
+		/*
+		 * Skb-backed XDP (generic XDP, veth) cow's the frags into a
+		 * page_pool while the rxq stays MEM_TYPE_PAGE_SHARED, so free
+		 * the frag to the pool, not via page_frag_free().
+		 */
+		mem_type = MEM_TYPE_PAGE_POOL;
 	}
 
 	if (release) {
-- 
2.43.0

[PATCH bpf v2 2/2] selftests/bpf: add xdp_shrink_frags

[Jiayuan Chen]

Add a test that attaches an xdp.frags program which shrinks a whole frag
away, so bpf_xdp_shrink_data() frees a page_pool frag.

test_tun triggers the page-type mismatch on the generic XDP path.
test_veth triggers the same mismatch on the veth path.

Both reproduce "Bad page state ... page_pool leak" on a buggy kernel.

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 .../bpf/prog_tests/xdp_shrink_frags.c         | 163 ++++++++++++++++++
 .../selftests/bpf/progs/xdp_shrink_frags.c    |  23 +++
 2 files changed, 186 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_shrink_frags.c
 create mode 100644 tools/testing/selftests/bpf/progs/xdp_shrink_frags.c

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_shrink_frags.c b/tools/testing/selftests/bpf/prog_tests/xdp_shrink_frags.c
new file mode 100644
index 000000000000..f3d8a84a6dc9
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_shrink_frags.c
@@ -0,0 +1,163 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <network_helpers.h>
+#include <linux/if_tun.h>
+#include <linux/if_ether.h>
+#include <sys/uio.h>
+#include <net/if.h>
+#include <arpa/inet.h>
+#include "xdp_shrink_frags.skel.h"
+
+/*
+ * A generic-XDP program that shrinks into the frags frees a page_pool frag.
+ * skb-backed XDP first cow's the nonlinear skb into page_pool memory
+ * (skb_cow_data_for_xdp() for generic XDP, skb_pp_cow_data() for veth), but
+ * the shared rxq is registered as MEM_TYPE_PAGE_SHARED, so a buggy kernel
+ * frees the frag with page_frag_free() -> "Bad page state ... page_pool leak".
+ */
+
+#define TAP_NAME	"xdp_shrink0"
+#define TAP_NETNS	"xdp_shrink_tap"
+
+#define VETH_LOCAL	"xdp_shrinkA"
+#define VETH_PEER	"xdp_shrinkB"
+#define VETH_NETNS	"xdp_shrink_veth"
+#define VETH_LOCAL_IP	"10.9.9.1"
+#define VETH_PEER_IP	"10.9.9.2"
+
+static int create_tap_napi_frags(const char *ifname)
+{
+	struct ifreq ifr = {
+		.ifr_flags = IFF_TAP | IFF_NO_PI | IFF_NAPI | IFF_NAPI_FRAGS,
+	};
+	int fd, err;
+
+	strscpy(ifr.ifr_name, ifname);
+
+	fd = open("/dev/net/tun", O_RDWR);
+	if (fd < 0)
+		return -1;
+
+	err = ioctl(fd, TUNSETIFF, &ifr);
+	if (err) {
+		close(fd);
+		return -1;
+	}
+
+	return fd;
+}
+
+/*
+ * Similar to flow_dissector.c: writev() an IFF_NAPI_FRAGS tap to build a
+ * nonlinear skb (sized for 4K pages, like xdp_adjust_tail.c) that tun runs
+ * through do_xdp_generic().
+ */
+static void test_tun(struct xdp_shrink_frags *skel)
+{
+	__u8 head[74], frag1[2048], frag2[2048];
+	struct ethhdr *eth = (void *)head;
+	int tap_fd = -1, ifindex, err;
+	struct netns_obj *ns = NULL;
+	struct iovec iov[3];
+	ssize_t n;
+
+	ns = netns_new(TAP_NETNS, true);
+	if (!ASSERT_OK_PTR(ns, "netns_new"))
+		return;
+
+	tap_fd = create_tap_napi_frags(TAP_NAME);
+	if (!ASSERT_GE(tap_fd, 0, "create_tap"))
+		goto out;
+
+	SYS(out, "ip link set dev " TAP_NAME " up");
+
+	ifindex = if_nametoindex(TAP_NAME);
+	if (!ASSERT_GT(ifindex, 0, "if_nametoindex"))
+		goto out;
+
+	skel->bss->shrink_ran = 0;
+
+	err = bpf_xdp_attach(ifindex, bpf_program__fd(skel->progs.xdp_shrink),
+			     0, NULL);
+	if (!ASSERT_OK(err, "bpf_xdp_attach"))
+		goto out;
+
+	memset(head, 0, sizeof(head));
+	memset(frag1, 0x41, sizeof(frag1));
+	memset(frag2, 0x42, sizeof(frag2));
+	eth->h_proto = htons(ETH_P_IP);
+
+	iov[0].iov_base = head;  iov[0].iov_len = sizeof(head);
+	iov[1].iov_base = frag1; iov[1].iov_len = sizeof(frag1);
+	iov[2].iov_base = frag2; iov[2].iov_len = sizeof(frag2);
+
+	n = writev(tap_fd, iov, ARRAY_SIZE(iov));
+	ASSERT_EQ(n, sizeof(head) + sizeof(frag1) + sizeof(frag2), "writev");
+
+	usleep(100 * 1000);
+	ASSERT_GT(skel->bss->shrink_ran, 0, "xdp_prog_ran");
+
+	bpf_xdp_detach(ifindex, 0, NULL);
+out:
+	if (tap_fd >= 0)
+		close(tap_fd);
+	netns_free(ns);
+}
+
+/*
+ * A large ping builds a nonlinear skb that veth cow's into its page_pool
+ * (sized for 4K pages, like xdp_adjust_tail.c) before running the program.
+ */
+static void test_veth(struct xdp_shrink_frags *skel)
+{
+	int ifindex, err;
+
+	SYS(out, "ip netns add " VETH_NETNS);
+	SYS(out_ns, "ip link add %s mtu 8000 type veth peer name %s mtu 8000",
+	    VETH_LOCAL, VETH_PEER);
+	SYS(out_link, "ip link set " VETH_PEER " netns " VETH_NETNS);
+	SYS(out_link, "ip addr add " VETH_LOCAL_IP "/24 dev " VETH_LOCAL);
+	SYS(out_link, "ip link set " VETH_LOCAL " up");
+	SYS(out_link, "ip -n " VETH_NETNS " addr add " VETH_PEER_IP "/24 dev " VETH_PEER);
+	SYS(out_link, "ip -n " VETH_NETNS " link set " VETH_PEER " up");
+
+	ifindex = if_nametoindex(VETH_LOCAL);
+	if (!ASSERT_GT(ifindex, 0, "if_nametoindex"))
+		goto out_link;
+
+	skel->bss->shrink_ran = 0;
+
+	err = bpf_xdp_attach(ifindex, bpf_program__fd(skel->progs.xdp_shrink),
+			     0, NULL);
+	if (!ASSERT_OK(err, "bpf_xdp_attach"))
+		goto out_link;
+
+	SYS_NOFAIL("ip netns exec " VETH_NETNS
+		   " ping -q -s 5000 -c 3 -W 1 " VETH_LOCAL_IP);
+
+	ASSERT_GT(skel->bss->shrink_ran, 0, "xdp_prog_ran");
+
+	bpf_xdp_detach(ifindex, 0, NULL);
+out_link:
+	SYS_NOFAIL("ip link del " VETH_LOCAL);
+out_ns:
+	SYS_NOFAIL("ip netns del " VETH_NETNS);
+out:
+	return;
+}
+
+void test_xdp_shrink_frags(void)
+{
+	struct xdp_shrink_frags *skel;
+
+	skel = xdp_shrink_frags__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel_open_load"))
+		return;
+
+	if (test__start_subtest("tun"))
+		test_tun(skel);
+	if (test__start_subtest("veth"))
+		test_veth(skel);
+
+	xdp_shrink_frags__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/xdp_shrink_frags.c b/tools/testing/selftests/bpf/progs/xdp_shrink_frags.c
new file mode 100644
index 000000000000..62e3791ac81d
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/xdp_shrink_frags.c
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+
+int shrink_ran;
+
+SEC("xdp.frags")
+int xdp_shrink(struct xdp_md *ctx)
+{
+	/*
+	 * The program is loaded with BPF_F_XDP_HAS_FRAGS (xdp.frags), so a
+	 * nonlinear skb entering generic XDP is cow'd into page_pool memory
+	 * before we run. Shrinking the tail far enough releases at least one
+	 * whole frag, which must be returned to its page_pool. Count only a
+	 * successful shrink so a too-small frame (e.g. ARP) does not satisfy
+	 * the test.
+	 */
+	if (bpf_xdp_adjust_tail(ctx, -3000) == 0)
+		__sync_fetch_and_add(&shrink_ran, 1);
+	return XDP_PASS;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.43.0

Re: [PATCH bpf v2 2/2] selftests/bpf: add xdp_shrink_frags

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 10216 bytes --]

> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_shrink_frags.c b/tools/testing/selftests/bpf/prog_tests/xdp_shrink_frags.c
> new file mode 100644
> index 000000000000..f3d8a84a6dc9
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_shrink_frags.c
> @@ -0,0 +1,163 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +#include <network_helpers.h>
> +#include <linux/if_tun.h>
> +#include <linux/if_ether.h>
> +#include <sys/uio.h>
> +#include <net/if.h>
> +#include <arpa/inet.h>
> +#include "xdp_shrink_frags.skel.h"
> +
> +/*
> + * A generic-XDP program that shrinks into the frags frees a page_pool frag.
> + * skb-backed XDP first cow's the nonlinear skb into page_pool memory
> + * (skb_cow_data_for_xdp() for generic XDP, skb_pp_cow_data() for veth), but
> + * the shared rxq is registered as MEM_TYPE_PAGE_SHARED, so a buggy kernel
> + * frees the frag with page_frag_free() -> "Bad page state ... page_pool leak".
> + */
> +
> +#define TAP_NAME	"xdp_shrink0"
> +#define TAP_NETNS	"xdp_shrink_tap"
> +
> +#define VETH_LOCAL	"xdp_shrinkA"
> +#define VETH_PEER	"xdp_shrinkB"
> +#define VETH_NETNS	"xdp_shrink_veth"
> +#define VETH_LOCAL_IP	"10.9.9.1"
> +#define VETH_PEER_IP	"10.9.9.2"
> +
> +static int create_tap_napi_frags(const char *ifname)
> +{
> +	struct ifreq ifr = {
> +		.ifr_flags = IFF_TAP | IFF_NO_PI | IFF_NAPI | IFF_NAPI_FRAGS,
> +	};
> +	int fd, err;
> +
> +	strscpy(ifr.ifr_name, ifname);
> +
> +	fd = open("/dev/net/tun", O_RDWR);
> +	if (fd < 0)
> +		return -1;

A subsystem pattern flags this as potentially concerning:
create_tap_napi_frags() collapses every failure into -1 and discards
errno, so the caller's ASSERT_GE(tap_fd, 0, "create_tap") becomes a hard
test failure with no diagnostic.

Two concrete cases where this would happen: /dev/net/tun missing
(CONFIG_TUN not built or the node absent) gives ENOENT/ENODEV, and
TUNSETIFF with IFF_NAPI_FRAGS rejected with EPERM when CAP_NET_ADMIN is
not held in the initial user namespace. Both of these should skip the test
with a reason rather than fail it. At minimum, would keeping errno make the
failure diagnosable?

> +
> +	err = ioctl(fd, TUNSETIFF, &ifr);
> +	if (err) {
> +		close(fd);
> +		return -1;
> +	}
> +
> +	return fd;
> +}
> +
> +/*
> + * Similar to flow_dissector.c: writev() an IFF_NAPI_FRAGS tap to build a
> + * nonlinear skb (sized for 4K pages, like xdp_adjust_tail.c) that tun runs
> + * through do_xdp_generic().
> + */
> +static void test_tun(struct xdp_shrink_frags *skel)
> +{
> +	__u8 head[74], frag1[2048], frag2[2048];
> +	struct ethhdr *eth = (void *)head;
> +	int tap_fd = -1, ifindex, err;
> +	struct netns_obj *ns = NULL;
> +	struct iovec iov[3];
> +	ssize_t n;
> +
> +	ns = netns_new(TAP_NETNS, true);
> +	if (!ASSERT_OK_PTR(ns, "netns_new"))
> +		return;
> +
> +	tap_fd = create_tap_napi_frags(TAP_NAME);
> +	if (!ASSERT_GE(tap_fd, 0, "create_tap"))
> +		goto out;
> +
> +	SYS(out, "ip link set dev " TAP_NAME " up");
> +
> +	ifindex = if_nametoindex(TAP_NAME);
> +	if (!ASSERT_GT(ifindex, 0, "if_nametoindex"))
> +		goto out;
> +
> +	skel->bss->shrink_ran = 0;
> +
> +	err = bpf_xdp_attach(ifindex, bpf_program__fd(skel->progs.xdp_shrink),
> +			     0, NULL);
> +	if (!ASSERT_OK(err, "bpf_xdp_attach"))
> +		goto out;
> +
> +	memset(head, 0, sizeof(head));
> +	memset(frag1, 0x41, sizeof(frag1));
> +	memset(frag2, 0x42, sizeof(frag2));
> +	eth->h_proto = htons(ETH_P_IP);
> +
> +	iov[0].iov_base = head;  iov[0].iov_len = sizeof(head);
> +	iov[1].iov_base = frag1; iov[1].iov_len = sizeof(frag1);
> +	iov[2].iov_base = frag2; iov[2].iov_len = sizeof(frag2);
> +
> +	n = writev(tap_fd, iov, ARRAY_SIZE(iov));
> +	ASSERT_EQ(n, sizeof(head) + sizeof(frag1) + sizeof(frag2), "writev");
> +
> +	usleep(100 * 1000);
> +	ASSERT_GT(skel->bss->shrink_ran, 0, "xdp_prog_ran");

Does this test actually exercise the page_pool frag release it is written
for? The frame sizes (74 + 2048 + 2048 = 4170 bytes) are picked for 4K
pages, but skb_pp_cow_data() puts up to SKB_WITH_OVERHEAD(PAGE_SIZE -
headroom) bytes in the linear head.

On 4K pages with XDP_PACKET_HEADROOM (256), that's ~3520 bytes in the
head, so the 4170-byte packet gets frags.

But on 64K pages, the threshold becomes ~64960 bytes, so the entire
4170-byte packet ends up linear and bpf_xdp_adjust_tail(-3000) never
reaches bpf_xdp_shrink_data() - it just moves data_end and returns 0.
shrink_ran still gets incremented, so the ASSERT_GT() passes whether or
not the fix from the preceding commit is applied.

selftests/bpf ships config.ppc64el and config.aarch64, and the existing
xdp_adjust_tail.c that the sizing comments cite explicitly branches on
getpagesize() == 65536. Would gating on getpagesize() or asserting
something that only holds when a whole frag was released (packet length or
a frag count observed from the program) make the test discriminate a fixed
kernel from a buggy one on 64K-page systems?

> +
> +	bpf_xdp_detach(ifindex, 0, NULL);

[ ... ]

> +/*
> + * A large ping builds a nonlinear skb that veth cow's into its page_pool
> + * (sized for 4K pages, like xdp_adjust_tail.c) before running the program.
> + */
> +static void test_veth(struct xdp_shrink_frags *skel)
> +{
> +	int ifindex, err;
> +
> +	SYS(out, "ip netns add " VETH_NETNS);
> +	SYS(out_ns, "ip link add %s mtu 8000 type veth peer name %s mtu 8000",
> +	    VETH_LOCAL, VETH_PEER);

A subsystem pattern flags this as potentially concerning: test_veth
hand-rolls namespace management with `ip netns add` / `ip netns del`
instead of the shared netns_new()/netns_free() helpers that test_tun uses
50 lines earlier in the same file.

It also only puts the peer into a namespace - the local end xdp_shrinkA
and its address 10.9.9.1/24 are created in whatever namespace test_progs
is running in (normally the host), so the test mutates the caller's
network configuration. The cleanup is best-effort SYS_NOFAIL, leaving the
device and address behind if the process dies between setup and teardown.
Fixed device/netns names also mean two concurrent test_progs workers
collide.

prog_tests/test_xdp_veth.c in the same directory keeps even the "local"
side inside a dedicated ns0 namespace for exactly this reason, and appends
the TID to the name. Would wrapping test_veth in netns_new() for a local
namespace and creating both ends inside it make the netns teardown remove
the veth pair, the address and the XDP program together?

> +	SYS(out_link, "ip link set " VETH_PEER " netns " VETH_NETNS);
> +	SYS(out_link, "ip addr add " VETH_LOCAL_IP "/24 dev " VETH_LOCAL);
> +	SYS(out_link, "ip link set " VETH_LOCAL " up");
> +	SYS(out_link, "ip -n " VETH_NETNS " addr add " VETH_PEER_IP "/24 dev " VETH_PEER);
> +	SYS(out_link, "ip -n " VETH_NETNS " link set " VETH_PEER " up");
> +
> +	ifindex = if_nametoindex(VETH_LOCAL);
> +	if (!ASSERT_GT(ifindex, 0, "if_nametoindex"))
> +		goto out_link;
> +
> +	skel->bss->shrink_ran = 0;
> +
> +	err = bpf_xdp_attach(ifindex, bpf_program__fd(skel->progs.xdp_shrink),
> +			     0, NULL);
> +	if (!ASSERT_OK(err, "bpf_xdp_attach"))
> +		goto out_link;
> +
> +	SYS_NOFAIL("ip netns exec " VETH_NETNS
> +		   " ping -q -s 5000 -c 3 -W 1 " VETH_LOCAL_IP);
> +
> +	ASSERT_GT(skel->bss->shrink_ran, 0, "xdp_prog_ran");

This test has the same page-size dependency as test_tun above. The `ping
-s 5000` produces ~5042 bytes on the wire, which is below the ~64960-byte
linear threshold on 64K-page kernels, so veth_convert_skb_to_xdp_buff()
sees nr_frags == 0 and bpf_xdp_adjust_tail() never enters
bpf_xdp_shrink_data().

> +
> +	bpf_xdp_detach(ifindex, 0, NULL);

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/xdp_shrink_frags.c b/tools/testing/selftests/bpf/progs/xdp_shrink_frags.c
> new file mode 100644
> index 000000000000..62e3791ac81d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/xdp_shrink_frags.c
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +
> +int shrink_ran;
> +
> +SEC("xdp.frags")
> +int xdp_shrink(struct xdp_md *ctx)
> +{
> +	/*
> +	 * The program is loaded with BPF_F_XDP_HAS_FRAGS (xdp.frags), so a
> +	 * nonlinear skb entering generic XDP is cow'd into page_pool memory
> +	 * before we run. Shrinking the tail far enough releases at least one
> +	 * whole frag, which must be returned to its page_pool. Count only a
> +	 * successful shrink so a too-small frame (e.g. ARP) does not satisfy
> +	 * the test.
> +	 */

Does this comment accurately describe what the test guarantees? "Shrinking
the tail far enough releases at least one whole frag" only holds if the
cow'd skb has frags at all.

skb_pp_cow_data() puts up to SKB_WITH_OVERHEAD(PAGE_SIZE - headroom) bytes
in the linear head:

net/core/skbuff.c:skb_pp_cow_data() {
    max_head_size = SKB_WITH_OVERHEAD(PAGE_SIZE - headroom);
    size = min_t(u32, skb->len, max_head_size);
    ...
    for (i = 0; i < MAX_SKB_FRAGS && off < skb->len; i++)
}

With PAGE_SIZE == 65536 and headroom == XDP_PACKET_HEADROOM (256),
max_head_size is ~64960 bytes. The test's frame sizes are 4170 bytes (tun)
and ~5042 bytes (veth), so on a 64K-page kernel there are zero frags and
bpf_xdp_adjust_tail() still returns 0 without calling bpf_xdp_shrink_data().

Two smaller mismatches in the same comment block and in the prog_tests
header comment:
  - This program is attached on both the generic-XDP (tun) and veth paths,
    but the comment only describes "a nonlinear skb entering generic XDP".
  - The prog_tests header says "skb_cow_data_for_xdp() for generic XDP,
    skb_pp_cow_data() for veth", which reads as two different mechanisms.
    But skb_cow_data_for_xdp() is just a wrapper that calls
    skb_pp_cow_data(), and the veth call site is
    veth_convert_skb_to_xdp_buff() - both paths end up in skb_pp_cow_data().

> +	if (bpf_xdp_adjust_tail(ctx, -3000) == 0)
> +		__sync_fetch_and_add(&shrink_ran, 1);
> +	return XDP_PASS;
> +}
> +
> +char _license[] SEC("license") = "GPL";

---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32686145995

Re: [PATCH bpf v2 2/2] selftests/bpf: add xdp_shrink_frags

[Jiayuan Chen]

On 8/24/26 11:58 AM, bot+bpf-ci@kernel.org wrote:
>> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_shrink_frags.c b/tools/testing/selftests/bpf/prog_tests/xdp_shrink_frags.c
>> new file mode 100644
>> index 000000000000..f3d8a84a6dc9
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_shrink_frags.c
>> @@ -0,0 +1,163 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#include <test_progs.h>
>> +#include <network_helpers.h>
>> +#include <linux/if_tun.h>
>> +#include <linux/if_ether.h>
>> +#include <sys/uio.h>
>> +#include <net/if.h>
>> +#include <arpa/inet.h>
>> +#include "xdp_shrink_frags.skel.h"
>> +
>> +/*
>> + * A generic-XDP program that shrinks into the frags frees a page_pool frag.
>> + * skb-backed XDP first cow's the nonlinear skb into page_pool memory
>> + * (skb_cow_data_for_xdp() for generic XDP, skb_pp_cow_data() for veth), but
>> + * the shared rxq is registered as MEM_TYPE_PAGE_SHARED, so a buggy kernel
>> + * frees the frag with page_frag_free() -> "Bad page state ... page_pool leak".
>> + */
>> +
>> +#define TAP_NAME	"xdp_shrink0"
>> +#define TAP_NETNS	"xdp_shrink_tap"
>> +
>> +#define VETH_LOCAL	"xdp_shrinkA"
>> +#define VETH_PEER	"xdp_shrinkB"
>> +#define VETH_NETNS	"xdp_shrink_veth"
>> +#define VETH_LOCAL_IP	"10.9.9.1"
>> +#define VETH_PEER_IP	"10.9.9.2"
>> +
>> +static int create_tap_napi_frags(const char *ifname)
>> +{
>> +	struct ifreq ifr = {
>> +		.ifr_flags = IFF_TAP | IFF_NO_PI | IFF_NAPI | IFF_NAPI_FRAGS,
>> +	};
>> +	int fd, err;
>> +
>> +	strscpy(ifr.ifr_name, ifname);
>> +
>> +	fd = open("/dev/net/tun", O_RDWR);
>> +	if (fd < 0)
>> +		return -1;
> A subsystem pattern flags this as potentially concerning:
> create_tap_napi_frags() collapses every failure into -1 and discards
> errno, so the caller's ASSERT_GE(tap_fd, 0, "create_tap") becomes a hard
> test failure with no diagnostic.
>
> Two concrete cases where this would happen: /dev/net/tun missing
> (CONFIG_TUN not built or the node absent) gives ENOENT/ENODEV, and
> TUNSETIFF with IFF_NAPI_FRAGS rejected with EPERM when CAP_NET_ADMIN is
> not held in the initial user namespace. Both of these should skip the test
> with a reason rather than fail it. At minimum, would keeping errno make the
> failure diagnosable?


flow_dissector.c's create_tap() (which this is modeled on) does exactly 
the same and doesn't even close the fd on the ioctl failure.

so I'd rather keep the parity than diverge here.

>
>> +
>> +	err = ioctl(fd, TUNSETIFF, &ifr);
>> +	if (err) {
>> +		close(fd);
>> +		return -1;
>> +	}
>> +
>> +	return fd;
>> +}
>> +
>> +/*
>> + * Similar to flow_dissector.c: writev() an IFF_NAPI_FRAGS tap to build a
>> + * nonlinear skb (sized for 4K pages, like xdp_adjust_tail.c) that tun runs
>> + * through do_xdp_generic().
>> + */
>> +static void test_tun(struct xdp_shrink_frags *skel)
>> +{
>> +	__u8 head[74], frag1[2048], frag2[2048];
>> +	struct ethhdr *eth = (void *)head;
>> +	int tap_fd = -1, ifindex, err;
>> +	struct netns_obj *ns = NULL;
>> +	struct iovec iov[3];
>> +	ssize_t n;
>> +
>> +	ns = netns_new(TAP_NETNS, true);
>> +	if (!ASSERT_OK_PTR(ns, "netns_new"))
>> +		return;
>> +
>> +	tap_fd = create_tap_napi_frags(TAP_NAME);
>> +	if (!ASSERT_GE(tap_fd, 0, "create_tap"))
>> +		goto out;
>> +
>> +	SYS(out, "ip link set dev " TAP_NAME " up");
>> +
>> +	ifindex = if_nametoindex(TAP_NAME);
>> +	if (!ASSERT_GT(ifindex, 0, "if_nametoindex"))
>> +		goto out;
>> +
>> +	skel->bss->shrink_ran = 0;
>> +
>> +	err = bpf_xdp_attach(ifindex, bpf_program__fd(skel->progs.xdp_shrink),
>> +			     0, NULL);
>> +	if (!ASSERT_OK(err, "bpf_xdp_attach"))
>> +		goto out;
>> +
>> +	memset(head, 0, sizeof(head));
>> +	memset(frag1, 0x41, sizeof(frag1));
>> +	memset(frag2, 0x42, sizeof(frag2));
>> +	eth->h_proto = htons(ETH_P_IP);
>> +
>> +	iov[0].iov_base = head;  iov[0].iov_len = sizeof(head);
>> +	iov[1].iov_base = frag1; iov[1].iov_len = sizeof(frag1);
>> +	iov[2].iov_base = frag2; iov[2].iov_len = sizeof(frag2);
>> +
>> +	n = writev(tap_fd, iov, ARRAY_SIZE(iov));
>> +	ASSERT_EQ(n, sizeof(head) + sizeof(frag1) + sizeof(frag2), "writev");
>> +
>> +	usleep(100 * 1000);
>> +	ASSERT_GT(skel->bss->shrink_ran, 0, "xdp_prog_ran");
> Does this test actually exercise the page_pool frag release it is written
> for? The frame sizes (74 + 2048 + 2048 = 4170 bytes) are picked for 4K
> pages, but skb_pp_cow_data() puts up to SKB_WITH_OVERHEAD(PAGE_SIZE -
> headroom) bytes in the linear head.
>
> On 4K pages with XDP_PACKET_HEADROOM (256), that's ~3520 bytes in the
> head, so the 4170-byte packet gets frags.
>
> But on 64K pages, the threshold becomes ~64960 bytes, so the entire
> 4170-byte packet ends up linear and bpf_xdp_adjust_tail(-3000) never
> reaches bpf_xdp_shrink_data() - it just moves data_end and returns 0.
> shrink_ran still gets incremented, so the ASSERT_GT() passes whether or
> not the fix from the preceding commit is applied.
>
> selftests/bpf ships config.ppc64el and config.aarch64, and the existing
> xdp_adjust_tail.c that the sizing comments cite explicitly branches on
> getpagesize() == 65536. Would gating on getpagesize() or asserting
> something that only holds when a whole frag was released (packet length or
> a frag count observed from the program) make the test discriminate a fixed
> kernel from a buggy one on 64K-page systems?


On 64K it just passes as a no-op and never false-fails, and the 
analogous in-tree test test_xdp_adjust_frags_tail_shrink() also hardcodes 4K

and runs unconditionally, so this keeps parity with it (and BPF CI runs 
4K-page kernels anyway).

>> +
>> +	bpf_xdp_detach(ifindex, 0, NULL);
> [ ... ]
>
>> +/*
>> + * A large ping builds a nonlinear skb that veth cow's into its page_pool
>> + * (sized for 4K pages, like xdp_adjust_tail.c) before running the program.
>> + */
>> +static void test_veth(struct xdp_shrink_frags *skel)
>> +{
>> +	int ifindex, err;
>> +
>> +	SYS(out, "ip netns add " VETH_NETNS);
>> +	SYS(out_ns, "ip link add %s mtu 8000 type veth peer name %s mtu 8000",
>> +	    VETH_LOCAL, VETH_PEER);
> A subsystem pattern flags this as potentially concerning: test_veth
> hand-rolls namespace management with `ip netns add` / `ip netns del`
> instead of the shared netns_new()/netns_free() helpers that test_tun uses
> 50 lines earlier in the same file.
>
> It also only puts the peer into a namespace - the local end xdp_shrinkA
> and its address 10.9.9.1/24 are created in whatever namespace test_progs
> is running in (normally the host), so the test mutates the caller's
> network configuration. The cleanup is best-effort SYS_NOFAIL, leaving the
> device and address behind if the process dies between setup and teardown.
> Fixed device/netns names also mean two concurrent test_progs workers
> collide.
>
> prog_tests/test_xdp_veth.c in the same directory keeps even the "local"
> side inside a dedicated ns0 namespace for exactly this reason, and appends
> the TID to the name. Would wrapping test_veth in netns_new() for a local
> namespace and creating both ends inside it make the netns teardown remove
> the veth pair, the address and the XDP program together?


  Sounds reasonable. I'll wrap test_veth in netns_new() and create both 
veth ends inside it, like test_tun does.


>
>> +	SYS(out_link, "ip link set " VETH_PEER " netns " VETH_NETNS);
>> +	SYS(out_link, "ip addr add " VETH_LOCAL_IP "/24 dev " VETH_LOCAL);
>> +	SYS(out_link, "ip link set " VETH_LOCAL " up");
>> +	SYS(out_link, "ip -n " VETH_NETNS " addr add " VETH_PEER_IP "/24 dev " VETH_PEER);
>> +	SYS(out_link, "ip -n " VETH_NETNS " link set " VETH_PEER " up");
>> +
>> +	ifindex = if_nametoindex(VETH_LOCAL);
>> +	if (!ASSERT_GT(ifindex, 0, "if_nametoindex"))
>> +		goto out_link;
>> +
>> +	skel->bss->shrink_ran = 0;
>> +
>> +	err = bpf_xdp_attach(ifindex, bpf_program__fd(skel->progs.xdp_shrink),
>> +			     0, NULL);
>> +	if (!ASSERT_OK(err, "bpf_xdp_attach"))
>> +		goto out_link;
>> +
>> +	SYS_NOFAIL("ip netns exec " VETH_NETNS
>> +		   " ping -q -s 5000 -c 3 -W 1 " VETH_LOCAL_IP);
>> +
>> +	ASSERT_GT(skel->bss->shrink_ran, 0, "xdp_prog_ran");
> This test has the same page-size dependency as test_tun above. The `ping
> -s 5000` produces ~5042 bytes on the wire, which is below the ~64960-byte
> linear threshold on 64K-page kernels, so veth_convert_skb_to_xdp_buff()
> sees nr_frags == 0 and bpf_xdp_adjust_tail() never enters
> bpf_xdp_shrink_data().


Same as the tun one: no false-fail on 64K, and CI is 4K.


>> +
>> +	bpf_xdp_detach(ifindex, 0, NULL);
> [ ... ]
>
>> diff --git a/tools/testing/selftests/bpf/progs/xdp_shrink_frags.c b/tools/testing/selftests/bpf/progs/xdp_shrink_frags.c
>> new file mode 100644
>> index 000000000000..62e3791ac81d
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/xdp_shrink_frags.c
>> @@ -0,0 +1,23 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#include "vmlinux.h"
>> +#include <bpf/bpf_helpers.h>
>> +
>> +int shrink_ran;
>> +
>> +SEC("xdp.frags")
>> +int xdp_shrink(struct xdp_md *ctx)
>> +{
>> +	/*
>> +	 * The program is loaded with BPF_F_XDP_HAS_FRAGS (xdp.frags), so a
>> +	 * nonlinear skb entering generic XDP is cow'd into page_pool memory
>> +	 * before we run. Shrinking the tail far enough releases at least one
>> +	 * whole frag, which must be returned to its page_pool. Count only a
>> +	 * successful shrink so a too-small frame (e.g. ARP) does not satisfy
>> +	 * the test.
>> +	 */
> Does this comment accurately describe what the test guarantees? "Shrinking
> the tail far enough releases at least one whole frag" only holds if the
> cow'd skb has frags at all.
>
> skb_pp_cow_data() puts up to SKB_WITH_OVERHEAD(PAGE_SIZE - headroom) bytes
> in the linear head:
>
> net/core/skbuff.c:skb_pp_cow_data() {
>      max_head_size = SKB_WITH_OVERHEAD(PAGE_SIZE - headroom);
>      size = min_t(u32, skb->len, max_head_size);
>      ...
>      for (i = 0; i < MAX_SKB_FRAGS && off < skb->len; i++)
> }
>
> With PAGE_SIZE == 65536 and headroom == XDP_PACKET_HEADROOM (256),
> max_head_size is ~64960 bytes. The test's frame sizes are 4170 bytes (tun)
> and ~5042 bytes (veth), so on a 64K-page kernel there are zero frags and
> bpf_xdp_adjust_tail() still returns 0 without calling bpf_xdp_shrink_data().


On the 4K pages the test targets (like the other frags tests) it does 
have frags,

so the comment holds for what the test actually runs.


> Two smaller mismatches in the same comment block and in the prog_tests
> header comment:
>    - This program is attached on both the generic-XDP (tun) and veth paths,
>      but the comment only describes "a nonlinear skb entering generic XDP".

The file header already says it covers both generic XDP and veth;

the prog comment just uses the generic path as the example and the cow 
behaviour is identical.

>    - The prog_tests header says "skb_cow_data_for_xdp() for generic XDP,
>      skb_pp_cow_data() for veth", which reads as two different mechanisms.
>      But skb_cow_data_for_xdp() is just a wrapper that calls
>      skb_pp_cow_data(), and the veth call site is
>      veth_convert_skb_to_xdp_buff() - both paths end up in skb_pp_cow_data().


Those are the actual call sites (generic goes through 
skb_cow_data_for_xdp(), veth calls skb_pp_cow_data()

directly from veth_convert_skb_to_xdp_buff()), so naming both is 
accurate rather than two different mechanisms.

>
>> +	if (bpf_xdp_adjust_tail(ctx, -3000) == 0)
>> +		__sync_fetch_and_add(&shrink_ran, 1);
>> +	return XDP_PASS;
>> +}
>> +
>> +char _license[] SEC("license") = "GPL";
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32686145995

Re: [PATCH bpf v2 1/2] bpf, veth: xdp: fix page_pool page leak on skb-backed XDP

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3896 bytes --]

> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 6ed3ee81153f..0afa0661ada1 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -775,6 +775,11 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
>  	if (skb_shinfo(skb)->nr_frags) {
>  		skb_shinfo(skb)->xdp_frags_size = skb->data_len;
>  		xdp_buff_set_frags_flag(xdp);
> +		/* A nonlinear skb was cow'd into rq->page_pool above, so the
> +		 * frags must be freed to that pool, not via the rxq's
> +		 * MEM_TYPE_PAGE_SHARED.
> +		 */
> +		xdp_buff_set_frag_pp(xdp);
>  	} else {
>  		xdp_buff_clear_frags_flag(xdp);
>  	}

[ ... ]

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 38336858c168..be36020484b6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5532,6 +5532,11 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
>  	if (skb_is_nonlinear(skb)) {
>  		skb_shinfo(skb)->xdp_frags_size = skb->data_len;
>  		xdp_buff_set_frags_flag(xdp);
> +		/* A nonlinear skb was cow'd into page_pool memory by
> +		 * skb_cow_data_for_xdp() before we got here, so the frags must
> +		 * be freed to that pool, not via the rxq's MEM_TYPE_PAGE_SHARED.
> +		 */
> +		xdp_buff_set_frag_pp(xdp);
>  	} else {
>  		xdp_buff_clear_frags_flag(xdp);
>  	}

This isn't a bug, but is the comment completely accurate?

The comment states the skb was cow'd "before we got here" by
skb_cow_data_for_xdp(). This is correct for the netif_receive_generic_xdp()
caller, where netif_skb_check_for_xdp() calls skb_cow_data_for_xdp() before
bpf_prog_run_generic_xdp().

However, bpf_prog_run_generic_xdp() has three other callers:

cpu_map_bpf_prog_run_skb():
    kernel/bpf/cpumap.c calls bpf_prog_run_generic_xdp() directly

dev_map_bpf_prog_run_skb():
    kernel/bpf/devmap.c calls bpf_prog_run_generic_xdp() directly

For these callers, skb_cow_data_for_xdp() is not called immediately before
this function. Instead, the page_pool invariant is inherited from an earlier
ingress generic-XDP pass where the skb went through
netif_receive_generic_xdp() -> netif_skb_check_for_xdp().

Should the comment acknowledge that the cow may have happened earlier in the
packet's path for some callers, rather than stating it always happens
"before we got here"?

[ ... ]

>     So record it in the xdp_buff. Add a XDP_FLAGS_FRAGS_PAGE_POOL flag; the two
>     skb-cow sites set it, and bpf_xdp_shrink_data() frees the frag to the

This isn't a bug, but is "the two skb-cow sites" the right description?

The commit message states "the two skb-cow sites set it", referring to
drivers/net/veth.c and net/core/dev.c where xdp_buff_set_frag_pp() is
called.

Looking at the code:

drivers/net/veth.c:782 is inside veth_convert_skb_to_xdp_buff(), which does
call skb_pp_cow_data() a few lines earlier to cow the nonlinear skb into
rq->page_pool. This is a cow site.

net/core/dev.c:5539 is inside bpf_prog_run_generic_xdp(), which does not
itself call skb_cow_data_for_xdp(). The cow happens one level up in
netif_receive_generic_xdp() -> netif_skb_check_for_xdp().

Would it be more precise to say "one cow site and one post-cow site" or "the
two sites where skb frags are known to be page_pool memory", since
bpf_prog_run_generic_xdp() is not itself performing the cow?

Also, bpf_prog_run_generic_xdp() has three callers that never cow at all:
cpu_map_bpf_prog_run_skb() and dev_map_bpf_prog_run_skb(). Their skbs
originate from xdp_do_generic_redirect_map() after going through
netif_receive_generic_xdp() where the cow already happened. So the flag is
set even though no cow occurs in the current call chain, which makes "two
skb-cow sites" a bit ambiguous.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32686145995

Re: [PATCH bpf v2 1/2] bpf, veth: xdp: fix page_pool page leak on skb-backed XDP

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 6335 bytes --]

> bpf_xdp_shrink_data() frees a released frag via __xdp_return() using
> xdp->rxq->mem.type, but that type is wrong for skb-backed XDP: the skb is
> cow'd into page_pool memory while the rxq still says MEM_TYPE_PAGE_SHARED,
> so the page_pool page is freed with page_frag_free() and we hit
> "Bad page state ... page_pool leak".
> 
> Both generic XDP and veth are affected. A non-linear skb is cow'd into
> page_pool memory (skb_cow_data_for_xdp() -> skb_pp_cow_data() for generic
> XDP, veth_convert_skb_to_xdp_buff() for veth), so its frags become
> page_pool pages while the rxq keeps MEM_TYPE_PAGE_SHARED.
> 
> We can't just fix rxq->mem.type in place:
> - generic XDP: xdp->rxq is dev->_rx[queue].xdp_rxq (see
>   bpf_prog_run_generic_xdp()), a shared rxq that other CPUs may access in
>   parallel, so we must not write to it.
> - veth: rq->xdp_rxq.mem is shared per-queue state that veth resets on XDP
>   teardown, and with GRO that reset runs without stopping in-flight NAPI,
>   so a type stashed there can be clobbered under a packet still in flight.
> 
> Adding a check in __xdp_return() or bpf_xdp_shrink_data() itself is not an
> option either: without recording it somewhere, both can only guess the
> frag's memory type, which quickly gets confusing.
> 
> So record it in the xdp_buff. Add a XDP_FLAGS_FRAGS_PAGE_POOL flag; the two
> skb-cow sites set it, and bpf_xdp_shrink_data() frees the frag to the
> page_pool when it is set, otherwise it keeps falling back to
> xdp->rxq->mem.type unchanged. No other path changes behaviour.
> 
> Fixes: e6d5dbdd20aa ("xdp: add multi-buff support for xdp running in generic mode")
> Fixes: 0ebab78cbcbf ("net: veth: add page_pool for page recycling")

Hi Jiayuan Chen,

thx for fixing it. Can we do something like the patch below instead?

Regards,
Lorenzo

diff --git a/net/core/xdp.c b/net/core/xdp.c
index 1d679e8fd649..4ed659b58141 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -433,16 +433,16 @@ EXPORT_SYMBOL_GPL(xdp_rxq_info_attach_page_pool);
 void __xdp_return(netmem_ref netmem, enum xdp_mem_type mem_type,
 		  bool napi_direct, struct xdp_buff *xdp)
 {
+	netmem_ref head_netmem = netmem_compound_head(netmem);
+	if (netmem_is_pp(head_netmem))
+		mem_type = MEM_TYPE_PAGE_POOL;
+
 	switch (mem_type) {
 	case MEM_TYPE_PAGE_POOL:
-		netmem = netmem_compound_head(netmem);
 		if (napi_direct && xdp_return_frame_no_direct())
 			napi_direct = false;
-		/* No need to check netmem_is_pp() as mem->type knows this a
-		 * page_pool page
-		 */
-		page_pool_put_full_netmem(netmem_get_pp(netmem), netmem,
-					  napi_direct);
+		page_pool_put_full_netmem(netmem_get_pp(head_netmem),
+					  head_netmem, napi_direct);
 		break;
 	case MEM_TYPE_PAGE_SHARED:
 		page_frag_free(__netmem_address(netmem));


> Reported-by: syzbot+237bbeed8dfe0699b7f5@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=237bbeed8dfe0699b7f5
> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
> ---
>  drivers/net/veth.c |  5 +++++
>  include/net/xdp.h  | 14 ++++++++++++++
>  net/core/dev.c     |  5 +++++
>  net/core/filter.c  |  7 +++++++
>  4 files changed, 31 insertions(+)
> 
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 6ed3ee81153f..0afa0661ada1 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -775,6 +775,11 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
>  	if (skb_shinfo(skb)->nr_frags) {
>  		skb_shinfo(skb)->xdp_frags_size = skb->data_len;
>  		xdp_buff_set_frags_flag(xdp);
> +		/* A nonlinear skb was cow'd into rq->page_pool above, so the
> +		 * frags must be freed to that pool, not via the rxq's
> +		 * MEM_TYPE_PAGE_SHARED.
> +		 */
> +		xdp_buff_set_frag_pp(xdp);
>  	} else {
>  		xdp_buff_clear_frags_flag(xdp);
>  	}
> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index aa742f413c35..b389dc527adc 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -81,6 +81,10 @@ enum xdp_buff_flags {
>  	 * XDP program is not attached.
>  	 */
>  	XDP_FLAGS_FRAGS_UNREADABLE	= BIT(2),
> +	/* frags are page_pool memory even though rxq->mem.type is not: a
> +	 * skb-backed XDP buff (generic XDP, veth) is cow'd into a page_pool.
> +	 */
> +	XDP_FLAGS_FRAGS_PAGE_POOL	= BIT(3),
>  };
>  
>  struct xdp_buff {
> @@ -131,6 +135,16 @@ static __always_inline void xdp_buff_set_frag_unreadable(struct xdp_buff *xdp)
>  	xdp->flags |= XDP_FLAGS_FRAGS_UNREADABLE;
>  }
>  
> +static __always_inline void xdp_buff_set_frag_pp(struct xdp_buff *xdp)
> +{
> +	xdp->flags |= XDP_FLAGS_FRAGS_PAGE_POOL;
> +}
> +
> +static __always_inline bool xdp_buff_is_frag_pp(const struct xdp_buff *xdp)
> +{
> +	return !!(xdp->flags & XDP_FLAGS_FRAGS_PAGE_POOL);
> +}
> +
>  static __always_inline u32 xdp_buff_get_skb_flags(const struct xdp_buff *xdp)
>  {
>  	return xdp->flags;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 38336858c168..be36020484b6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5532,6 +5532,11 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
>  	if (skb_is_nonlinear(skb)) {
>  		skb_shinfo(skb)->xdp_frags_size = skb->data_len;
>  		xdp_buff_set_frags_flag(xdp);
> +		/* A nonlinear skb was cow'd into page_pool memory by
> +		 * skb_cow_data_for_xdp() before we got here, so the frags must
> +		 * be freed to that pool, not via the rxq's MEM_TYPE_PAGE_SHARED.
> +		 */
> +		xdp_buff_set_frag_pp(xdp);
>  	} else {
>  		xdp_buff_clear_frags_flag(xdp);
>  	}
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 61940e753552..d34ba56d79d8 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4378,6 +4378,13 @@ static bool bpf_xdp_shrink_data(struct xdp_buff *xdp, skb_frag_t *frag,
>  	if (mem_type == MEM_TYPE_XSK_BUFF_POOL) {
>  		netmem = 0;
>  		zc_frag = bpf_xdp_shrink_data_zc(xdp, shrink, tail, release);
> +	} else if (xdp_buff_is_frag_pp(xdp)) {
> +		/*
> +		 * Skb-backed XDP (generic XDP, veth) cow's the frags into a
> +		 * page_pool while the rxq stays MEM_TYPE_PAGE_SHARED, so free
> +		 * the frag to the pool, not via page_frag_free().
> +		 */
> +		mem_type = MEM_TYPE_PAGE_POOL;
>  	}
>  
>  	if (release) {
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH bpf v2 1/2] bpf, veth: xdp: fix page_pool page leak on skb-backed XDP

[Jiayuan Chen]

On 8/24/26 6:31 PM, Lorenzo Bianconi wrote:
>> bpf_xdp_shrink_data() frees a released frag via __xdp_return() using
>> xdp->rxq->mem.type, but that type is wrong for skb-backed XDP: the skb is
>> cow'd into page_pool memory while the rxq still says MEM_TYPE_PAGE_SHARED,
>> so the page_pool page is freed with page_frag_free() and we hit
>> "Bad page state ... page_pool leak".
>>
>> Both generic XDP and veth are affected. A non-linear skb is cow'd into
>> page_pool memory (skb_cow_data_for_xdp() -> skb_pp_cow_data() for generic
>> XDP, veth_convert_skb_to_xdp_buff() for veth), so its frags become
>> page_pool pages while the rxq keeps MEM_TYPE_PAGE_SHARED.
>>
>> We can't just fix rxq->mem.type in place:
>> - generic XDP: xdp->rxq is dev->_rx[queue].xdp_rxq (see
>>    bpf_prog_run_generic_xdp()), a shared rxq that other CPUs may access in
>>    parallel, so we must not write to it.
>> - veth: rq->xdp_rxq.mem is shared per-queue state that veth resets on XDP
>>    teardown, and with GRO that reset runs without stopping in-flight NAPI,
>>    so a type stashed there can be clobbered under a packet still in flight.
>>
>> Adding a check in __xdp_return() or bpf_xdp_shrink_data() itself is not an
>> option either: without recording it somewhere, both can only guess the
>> frag's memory type, which quickly gets confusing.
>>
>> So record it in the xdp_buff. Add a XDP_FLAGS_FRAGS_PAGE_POOL flag; the two
>> skb-cow sites set it, and bpf_xdp_shrink_data() frees the frag to the
>> page_pool when it is set, otherwise it keeps falling back to
>> xdp->rxq->mem.type unchanged. No other path changes behaviour.
>>
>> Fixes: e6d5dbdd20aa ("xdp: add multi-buff support for xdp running in generic mode")
>> Fixes: 0ebab78cbcbf ("net: veth: add page_pool for page recycling")
> Hi Jiayuan Chen,
>
> thx for fixing it. Can we do something like the patch below instead?
>
> Regards,
> Lorenzo
>
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 1d679e8fd649..4ed659b58141 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -433,16 +433,16 @@ EXPORT_SYMBOL_GPL(xdp_rxq_info_attach_page_pool);
>   void __xdp_return(netmem_ref netmem, enum xdp_mem_type mem_type,
>   		  bool napi_direct, struct xdp_buff *xdp)
>   {
> +	netmem_ref head_netmem = netmem_compound_head(netmem);
> +	if (netmem_is_pp(head_netmem))
> +		mem_type = MEM_TYPE_PAGE_POOL;
> +
>   	switch (mem_type) {
>   	case MEM_TYPE_PAGE_POOL:
> -		netmem = netmem_compound_head(netmem);
>   		if (napi_direct && xdp_return_frame_no_direct())
>   			napi_direct = false;
> -		/* No need to check netmem_is_pp() as mem->type knows this a
> -		 * page_pool page
> -		 */
> -		page_pool_put_full_netmem(netmem_get_pp(netmem), netmem,
> -					  napi_direct);
> +		page_pool_put_full_netmem(netmem_get_pp(head_netmem),
> +					  head_netmem, napi_direct);
>   		break;
>   	case MEM_TYPE_PAGE_SHARED:
>   		page_frag_free(__netmem_address(netmem));


Hi Lorenzo,

I tried this, but it regresses the bpf selftest with a page_pool ref 
underflow (0 warns on master, 45 with the patch):

     WARNING: include/net/page_pool/helpers.h:297 at 
page_pool_alloc_frag_netmem
     skb_pp_cow_data
     veth_xdp_rcv_skb


On XDP_TX/XDP_REDIRECT veth has to take plain page refs via get_page() 
(veth_xdp_get()) and then
consume_skb(): the skb itself must be freed while the data pages stay 
alive for the frame. consume_skb()
already returns the skb's page_pool ref, so what the frame holds 
afterwards is a plain page ref, to be
dropped with page_frag_free().

netmem_is_pp() can't see that: it only says the page still belongs to a 
pool (other users may still hold pool refs on the same page),
not what kind of ref we're dropping. So __xdp_return() turns those 
plain-ref drops into a second pool
put and pp_ref goes negative.

That's why I kept the type in the xdp_buff and only override it in the 
shrink path, where we know the
frag ref is the cow'd page_pool one.

Regards,
Jiayuan

Re: [PATCH bpf v2 1/2] bpf, veth: xdp: fix page_pool page leak on skb-backed XDP

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 4729 bytes --]

> 
> On 8/24/26 6:31 PM, Lorenzo Bianconi wrote:
> > > bpf_xdp_shrink_data() frees a released frag via __xdp_return() using
> > > xdp->rxq->mem.type, but that type is wrong for skb-backed XDP: the skb is
> > > cow'd into page_pool memory while the rxq still says MEM_TYPE_PAGE_SHARED,
> > > so the page_pool page is freed with page_frag_free() and we hit
> > > "Bad page state ... page_pool leak".
> > > 
> > > Both generic XDP and veth are affected. A non-linear skb is cow'd into
> > > page_pool memory (skb_cow_data_for_xdp() -> skb_pp_cow_data() for generic
> > > XDP, veth_convert_skb_to_xdp_buff() for veth), so its frags become
> > > page_pool pages while the rxq keeps MEM_TYPE_PAGE_SHARED.
> > > 
> > > We can't just fix rxq->mem.type in place:
> > > - generic XDP: xdp->rxq is dev->_rx[queue].xdp_rxq (see
> > >    bpf_prog_run_generic_xdp()), a shared rxq that other CPUs may access in
> > >    parallel, so we must not write to it.
> > > - veth: rq->xdp_rxq.mem is shared per-queue state that veth resets on XDP
> > >    teardown, and with GRO that reset runs without stopping in-flight NAPI,
> > >    so a type stashed there can be clobbered under a packet still in flight.
> > > 
> > > Adding a check in __xdp_return() or bpf_xdp_shrink_data() itself is not an
> > > option either: without recording it somewhere, both can only guess the
> > > frag's memory type, which quickly gets confusing.
> > > 
> > > So record it in the xdp_buff. Add a XDP_FLAGS_FRAGS_PAGE_POOL flag; the two
> > > skb-cow sites set it, and bpf_xdp_shrink_data() frees the frag to the
> > > page_pool when it is set, otherwise it keeps falling back to
> > > xdp->rxq->mem.type unchanged. No other path changes behaviour.
> > > 
> > > Fixes: e6d5dbdd20aa ("xdp: add multi-buff support for xdp running in generic mode")
> > > Fixes: 0ebab78cbcbf ("net: veth: add page_pool for page recycling")
> > Hi Jiayuan Chen,
> > 
> > thx for fixing it. Can we do something like the patch below instead?
> > 
> > Regards,
> > Lorenzo
> > 
> > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > index 1d679e8fd649..4ed659b58141 100644
> > --- a/net/core/xdp.c
> > +++ b/net/core/xdp.c
> > @@ -433,16 +433,16 @@ EXPORT_SYMBOL_GPL(xdp_rxq_info_attach_page_pool);
> >   void __xdp_return(netmem_ref netmem, enum xdp_mem_type mem_type,
> >   		  bool napi_direct, struct xdp_buff *xdp)
> >   {
> > +	netmem_ref head_netmem = netmem_compound_head(netmem);
> > +	if (netmem_is_pp(head_netmem))
> > +		mem_type = MEM_TYPE_PAGE_POOL;
> > +
> >   	switch (mem_type) {
> >   	case MEM_TYPE_PAGE_POOL:
> > -		netmem = netmem_compound_head(netmem);
> >   		if (napi_direct && xdp_return_frame_no_direct())
> >   			napi_direct = false;
> > -		/* No need to check netmem_is_pp() as mem->type knows this a
> > -		 * page_pool page
> > -		 */
> > -		page_pool_put_full_netmem(netmem_get_pp(netmem), netmem,
> > -					  napi_direct);
> > +		page_pool_put_full_netmem(netmem_get_pp(head_netmem),
> > +					  head_netmem, napi_direct);
> >   		break;
> >   	case MEM_TYPE_PAGE_SHARED:
> >   		page_frag_free(__netmem_address(netmem));
> 
> 
> Hi Lorenzo,
> 
> I tried this, but it regresses the bpf selftest with a page_pool ref
> underflow (0 warns on master, 45 with the patch):
> 
>     WARNING: include/net/page_pool/helpers.h:297 at
> page_pool_alloc_frag_netmem
>     skb_pp_cow_data
>     veth_xdp_rcv_skb
> 
> 
> On XDP_TX/XDP_REDIRECT veth has to take plain page refs via get_page()
> (veth_xdp_get()) and then
> consume_skb(): the skb itself must be freed while the data pages stay alive
> for the frame. consume_skb()
> already returns the skb's page_pool ref, so what the frame holds afterwards
> is a plain page ref, to be
> dropped with page_frag_free().
> 
> netmem_is_pp() can't see that: it only says the page still belongs to a pool
> (other users may still hold pool refs on the same page),
> not what kind of ref we're dropping. So __xdp_return() turns those plain-ref
> drops into a second pool
> put and pp_ref goes negative.
> 
> That's why I kept the type in the xdp_buff and only override it in the
> shrink path, where we know the
> frag ref is the cow'd page_pool one.
> 
> Regards,
> Jiayuan
> 

Right. I can see the point now :). IIUC we are currently able to trigger
the issue just on xdp fragments running bpf_xdp_shrink_data() but the problem
theoretically occurs even for the xdp->data, right? (it is rallocated using the
page_pool in skb_pp_cow_data()). Is it better to always set this new flag when
the buffers are reallocated via skb_pp_cow_data()? (Maybe renaming it in
something like XDP_FLAGS_DATA_FROM_PP).

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH bpf v2 1/2] bpf, veth: xdp: fix page_pool page leak on skb-backed XDP

[Jiayuan Chen]

On 8/24/26 10:50 PM, Lorenzo Bianconi wrote:
>> On 8/24/26 6:31 PM, Lorenzo Bianconi wrote:
>>>> bpf_xdp_shrink_data() frees a released frag via __xdp_return() using
>>>> xdp->rxq->mem.type, but that type is wrong for skb-backed XDP: the skb is
>>>> cow'd into page_pool memory while the rxq still says MEM_TYPE_PAGE_SHARED,
>>>> so the page_pool page is freed with page_frag_free() and we hit
>>>> "Bad page state ... page_pool leak".
>>>>
>>>> Both generic XDP and veth are affected. A non-linear skb is cow'd into
>>>> page_pool memory (skb_cow_data_for_xdp() -> skb_pp_cow_data() for generic
>>>> XDP, veth_convert_skb_to_xdp_buff() for veth), so its frags become
>>>> page_pool pages while the rxq keeps MEM_TYPE_PAGE_SHARED.
>>>>
>>>> We can't just fix rxq->mem.type in place:
>>>> - generic XDP: xdp->rxq is dev->_rx[queue].xdp_rxq (see
>>>>     bpf_prog_run_generic_xdp()), a shared rxq that other CPUs may access in
>>>>     parallel, so we must not write to it.
>>>> - veth: rq->xdp_rxq.mem is shared per-queue state that veth resets on XDP
>>>>     teardown, and with GRO that reset runs without stopping in-flight NAPI,
>>>>     so a type stashed there can be clobbered under a packet still in flight.
>>>>
>>>> Adding a check in __xdp_return() or bpf_xdp_shrink_data() itself is not an
>>>> option either: without recording it somewhere, both can only guess the
>>>> frag's memory type, which quickly gets confusing.
>>>>
>>>> So record it in the xdp_buff. Add a XDP_FLAGS_FRAGS_PAGE_POOL flag; the two
>>>> skb-cow sites set it, and bpf_xdp_shrink_data() frees the frag to the
>>>> page_pool when it is set, otherwise it keeps falling back to
>>>> xdp->rxq->mem.type unchanged. No other path changes behaviour.
>>>>
>>>> Fixes: e6d5dbdd20aa ("xdp: add multi-buff support for xdp running in generic mode")
>>>> Fixes: 0ebab78cbcbf ("net: veth: add page_pool for page recycling")
>>> Hi Jiayuan Chen,
>>>
>>> thx for fixing it. Can we do something like the patch below instead?
>>>
>>> Regards,
>>> Lorenzo
>>>
>>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>>> index 1d679e8fd649..4ed659b58141 100644
>>> --- a/net/core/xdp.c
>>> +++ b/net/core/xdp.c
>>> @@ -433,16 +433,16 @@ EXPORT_SYMBOL_GPL(xdp_rxq_info_attach_page_pool);
>>>    void __xdp_return(netmem_ref netmem, enum xdp_mem_type mem_type,
>>>    		  bool napi_direct, struct xdp_buff *xdp)
>>>    {
>>> +	netmem_ref head_netmem = netmem_compound_head(netmem);
>>> +	if (netmem_is_pp(head_netmem))
>>> +		mem_type = MEM_TYPE_PAGE_POOL;
>>> +
>>>    	switch (mem_type) {
>>>    	case MEM_TYPE_PAGE_POOL:
>>> -		netmem = netmem_compound_head(netmem);
>>>    		if (napi_direct && xdp_return_frame_no_direct())
>>>    			napi_direct = false;
>>> -		/* No need to check netmem_is_pp() as mem->type knows this a
>>> -		 * page_pool page
>>> -		 */
>>> -		page_pool_put_full_netmem(netmem_get_pp(netmem), netmem,
>>> -					  napi_direct);
>>> +		page_pool_put_full_netmem(netmem_get_pp(head_netmem),
>>> +					  head_netmem, napi_direct);
>>>    		break;
>>>    	case MEM_TYPE_PAGE_SHARED:
>>>    		page_frag_free(__netmem_address(netmem));
>>
>> Hi Lorenzo,
>>
>> I tried this, but it regresses the bpf selftest with a page_pool ref
>> underflow (0 warns on master, 45 with the patch):
>>
>>      WARNING: include/net/page_pool/helpers.h:297 at
>> page_pool_alloc_frag_netmem
>>      skb_pp_cow_data
>>      veth_xdp_rcv_skb
>>
>>
>> On XDP_TX/XDP_REDIRECT veth has to take plain page refs via get_page()
>> (veth_xdp_get()) and then
>> consume_skb(): the skb itself must be freed while the data pages stay alive
>> for the frame. consume_skb()
>> already returns the skb's page_pool ref, so what the frame holds afterwards
>> is a plain page ref, to be
>> dropped with page_frag_free().
>>
>> netmem_is_pp() can't see that: it only says the page still belongs to a pool
>> (other users may still hold pool refs on the same page),
>> not what kind of ref we're dropping. So __xdp_return() turns those plain-ref
>> drops into a second pool
>> put and pp_ref goes negative.
>>
>> That's why I kept the type in the xdp_buff and only override it in the
>> shrink path, where we know the
>> frag ref is the cow'd page_pool one.
>>
>> Regards,
>> Jiayuan
>>
> Right. I can see the point now :). IIUC we are currently able to trigger
> the issue just on xdp fragments running bpf_xdp_shrink_data() but the problem
> theoretically occurs even for the xdp->data, right? (it is rallocated using the
> page_pool in skb_pp_cow_data()). Is it better to always set this new flag when
> the buffers are reallocated via skb_pp_cow_data()? (Maybe renaming it in
> something like XDP_FLAGS_DATA_FROM_PP).
>
> Regards,
> Lorenzo


Hi Lorenzo,

Theoretically you are right, the linear part has the same issue.
But in practice the head is always freed via consume_skb()/kfree_skb(), 
which are pp-aware.
If we really want to go that way, we'd probably have to touch quite a 
few function signatures.
As a fix that also needs backporting, I think the current change is the 
minimal one that covers
all the reachable cases(happy to hear a better idea.).