[PATCH ipsec v5] xfrm: iptfs: fix pp_ref_count underflow when sharing page_pool frags

[PATCH ipsec v5] xfrm: iptfs: fix pp_ref_count underflow when sharing page_pool frags

[Antony Antony]

skb frags are either page_pool pages, tracked via pp_ref_count and
released by napi_pp_put_page(), or regular pages, tracked via _refcount
and released by put_netmem(). iptfs_consume_frags() shares a frag into the
aggregate skb with a plain reference even when pp_recycle is set, which
unbalanced the counts and caused the underflow below.

Take the page_pool reference only when the destination has pp_recycle
set and the fragment's page is page_pool owned, like skb_pp_frag_ref()
does for pages; otherwise take a plain reference.

Use the same helper in iptfs_skb_add_frags() for consistency. Its
destination skb never has pp_recycle set today, so behaviour there does
not change.

Tested on a virtio NIC. The netmem/devmem path is not addressed and
probably needs a fix in net/core.

Observed under normal traffic when a page_pool frag is shared into
two extra skbs:

[   96.908140] ------------[ cut here ]------------
[   96.908143] WARNING: ./include/net/page_pool/helpers.h:297 at page_pool_put_netmem.constprop.0+0x1f/0x40, CPU#0: swapper/0/0
[   96.908150] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Tainted: G        W           7.3.0-rc2-00486-g49bf62d4ca1d #27 PREEMPT(full)
[   96.908153] Tainted: [W]=WARN
[   96.908155] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[   96.908156] RIP: 0010:page_pool_put_netmem.constprop.0+0x1f/0x40
[   96.908159] Code: 90 90 90 90 90 90 90 90 90 90 90 48 89 f0 48 83 e0 fe 48 8b 48 28 48 ff c9 74 20 48 83 c9 ff f0 48 0f c1 48 28 48 ff c9 79 07 <0f> 0b c3 cc cc cc cc 75 13 48 c7 40 28 01 00 00 00 0f b6 ca 83 ca
[   96.908161] RSP: 0018:ffffc90000003c98 EFLAGS: 00010296
[   96.908163] RAX: ffffea00043a92c0 RBX: ffff888104124300 RCX: ffffffffffffffff
[   96.908165] RDX: 0000000000000001 RSI: ffffea00043a92c0 RDI: ffff888101c4b800
[   96.908166] RBP: 000000000000000c R08: 0000000000000042 R09: 0000000000000a00
[   96.908167] R10: 000000000000014f R11: 00000000000007e0 R12: 0000000000000000
[   96.908169] R13: ffff8881009c8800 R14: ffffea00043a92c0 R15: ffff888100b6f9c0
[   96.908172] FS:  0000000000000000(0000) GS:ffff8881f887c000(0000) knlGS:0000000000000000
[   96.908174] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   96.908176] CR2: 000056208ebdbd44 CR3: 00000001109b3001 CR4: 0000000000170eb0
[   96.908177] Call Trace:
[   96.908179]  <IRQ>
[   96.908181]  page_to_skb+0x1f3/0x210
[   96.908184]  receive_buf+0x712/0xca0
[   96.908188]  ? detach_buf_split_in_order+0x5d/0x110
[   96.908191]  virtnet_poll+0x1da/0x460
[   96.908195]  __napi_poll.constprop.0+0x2a/0x120
[   96.908197]  net_rx_action+0x11a/0x230
[   96.908199]  ? raise_softirq_irqoff+0x5/0x20
[   96.908203]  ? __napi_schedule+0x31/0x50
[   96.908206]  ? vring_interrupt+0x77/0x90
[   96.908209]  handle_softirqs+0x11e/0x270
[   96.908212]  __irq_exit_rcu+0x53/0xf0
[   96.908215]  common_interrupt+0x95/0xc0
[   96.908219]  </IRQ>
[   96.908220]  <TASK>
[   96.908221]  asm_common_interrupt+0x22/0x40
[   96.908224] RIP: 0010:default_idle+0xb/0x20
[   96.908226] Code: 00 4d 29 c8 4c 01 c7 4c 29 c2 e9 6e ff ff ff 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 eb 07 0f 00 2d 1d c5 01 00 fb f4 <fa> c3 cc cc cc cc 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 90
[   96.908228] RSP: 0018:ffffffff82a03e08 EFLAGS: 00000216
[   96.908230] RAX: 0000000000000000 RBX: ffffffff82a0b480 RCX: 00000000ffff3972
[   96.908231] RDX: 0000000000000000 RSI: ffffffff822110bf RDI: 000000000003c07c
[   96.908233] RBP: 0000000000000000 R08: 0000000000000002 R09: 0000000000000000
[   96.908234] R10: 0000000000155cc0 R11: 0000000000000000 R12: 0000000000000000
[   96.908235] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000013ab0
[   96.908238]  default_idle_call+0x3c/0x70
[   96.908240]  do_idle+0xdc/0x200
[   96.908244]  cpu_startup_entry+0x29/0x30
[   96.908247]  rest_init+0xe8/0xf0
[   96.908250]  ? __pfx_kernel_init+0x10/0x10
[   96.908252]  start_kernel+0x5fd/0x600
[   96.908257]  x86_64_start_reservations+0x20/0x20
[   96.908259]  x86_64_start_kernel+0xc9/0xd0
[   96.908262]  common_startup_64+0x129/0x148
[   96.908265]  </TASK>
[   96.908266] ---[ end trace 0000000000000000 ]---

Fixes: b96ba312e21c ("xfrm: iptfs: share page fragments of inner packets")
Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
Changes in v5:
- revert to v3; drop the net_iov/devmem handling added in v4. net_iov
 frags are not always page_pool owned (e.g. devmem TX), and ownership
 cannot be checked from xfrm. Needs a net/core change first.
 https://netdev-ai.bots.linux.dev/sashiko/#/patchset/xfrm-iptfs-pp_ref_count-underflow-v4-1-912fa72106f0%40secunet.com

- Link to v4: https://patch.msgid.link/xfrm-iptfs-pp_ref_count-underflow-v4-1-912fa72106f0@secunet.com
Changes in v4:
- add devmem/netmem ref count (Steffen)

- Link to v3: https://patch.msgid.link/xfrm-iptfs-pp_ref_count-underflow-v3-1-9266ddaf3129@secunet.com
Changes in v3:
  check page is pp before pp_ref_count bump. create helper func

- Link to v2: https://patch.msgid.link/xfrm-iptfs-pp_ref_count-underflow-v1-1-5fb363833d41@secunet.com
Changes in v2:
  rebase to latest ipsec

- Link to v1: https://lore.kernel.org/all/xfrm-iptfs-pp_ref_count-underflow-v1-1-47b319c6d2f6@secunet.com/
---
 net/xfrm/xfrm_iptfs.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index 6920940a35b4..86688c0c3db9 100644
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -14,6 +14,7 @@
 #include <net/icmp.h>
 #include <net/ip6_route.h>
 #include <net/inet_ecn.h>
+#include <net/page_pool/helpers.h>
 #include <net/xfrm.h>
 
 #include <crypto/aead.h>
@@ -449,6 +450,21 @@ static bool iptfs_skb_can_add_frags(const struct sk_buff *skb,
 	return true;
 }
 
+static void iptfs_frag_ref(skb_frag_t *frag, bool recycle)
+{
+	netmem_ref netmem = skb_frag_netmem(frag);
+	struct page *head;
+
+	if (recycle && !netmem_is_net_iov(netmem)) {
+		head = compound_head(netmem_to_page(netmem));
+		if (page_pool_page_is_pp(head)) {
+			page_pool_ref_page(head);
+			return;
+		}
+	}
+	__skb_frag_ref(frag);
+}
+
 /**
  * iptfs_skb_add_frags() - add a range of fragment references into an skb
  * @skb: skb to add references into
@@ -486,7 +502,7 @@ static int iptfs_skb_add_frags(struct sk_buff *skb,
 			tofrag->len -= offset;
 			offset = 0;
 		}
-		__skb_frag_ref(tofrag);
+		iptfs_frag_ref(tofrag, skb->pp_recycle);
 		shinfo->nr_frags++;
 		shinfo->flags |= SKBFL_SHARED_FRAG;
 
@@ -2171,7 +2187,8 @@ static void iptfs_consume_frags(struct sk_buff *to, struct sk_buff *from)
 		new_truesize = SKB_TRUESIZE(skb_end_offset(from));
 	} else {
 		iptfs_skb_head_to_frag(from, &toi->frags[toi->nr_frags]);
-		skb_frag_ref(to, toi->nr_frags++);
+		iptfs_frag_ref(&toi->frags[toi->nr_frags], to->pp_recycle);
+		toi->nr_frags++;
 		new_truesize = SKB_DATA_ALIGN(sizeof(struct sk_buff));
 	}
 

---
base-commit: 86de3a1118a16dbbbe5fabe9aa20ffb93c072ed5
change-id: xfrm-iptfs-pp_ref_count-underflow-063ee0302600

Best regards,
--  
Antony Antony <antony.antony@secunet.com>

Re: [PATCH ipsec v5] xfrm: iptfs: fix pp_ref_count underflow when sharing page_pool frags

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says the iptfs_skb_add_frags() hunk is
  cosmetic: "Its destination skb never has pp_recycle set today, so…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it sits in the
  function the patch edits.
- [Low] iptfs_frag_ref() (net/xfrm/xfrm_iptfs.c:453-466) decides whether
  a frag is page_pool owned differently from the release path.

--

xfrm: iptfs: fix pp_ref_count underflow when sharing page_pool frags

This adds iptfs_frag_ref(). When the destination skb has pp_recycle set
and the frag's head page is page_pool owned, it takes a page_pool
reference. Otherwise it takes a plain page reference. The helper is used
in iptfs_consume_frags() to fix a pp_ref_count underflow on the TX
aggregation path, and in iptfs_skb_add_frags() for consistency.

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 6920940a35b49..86688c0c3db96 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c

[ ... ]

> @@ -449,6 +450,21 @@ static bool iptfs_skb_can_add_frags(const struct sk_buff *skb,
>  	return true;
>  }
>  
> +static void iptfs_frag_ref(skb_frag_t *frag, bool recycle)
> +{
> +	netmem_ref netmem = skb_frag_netmem(frag);
> +	struct page *head;
> +
> +	if (recycle && !netmem_is_net_iov(netmem)) {

[Severity: Low]

This is a pre-existing issue, but the net_iov handling here doesn't
match the release side.

For a pp_recycle destination, every net_iov frag falls through to
__skb_frag_ref() -> get_netmem(), which never touches pp_ref_count.
The release path in napi_pp_put_page() does this:

	netmem = netmem_compound_head(netmem);

	if (unlikely(!netmem_is_pp(netmem)))
		return false;

netmem_is_pp() returns true for page_pool backed net_iov. If such a frag
is shared through iptfs_skb_add_frags(), the release would drop a
pp_ref_count that was never taken.

This doesn't look reachable today:

  - Software ESP can't pass an unreadable skb on to IPTFS input. When
    skb_copy_bits() fails, esp_remove_trailer() calls BUG().
  - mlx5 is the only unreadable-netmem driver with IPsec offload, and
    mlx5e_xfrm_add_state() rejects modes other than transport and
    tunnel.
  - On TX, iptfs_frag_ref() is only called on the head page.

The net_iov behaviour is also the same as before the patch. The commit
message already says "The netmem/devmem path is not addressed and
probably needs a fix in net/core".

Could this be handled together with that net/core change?

> +		head = compound_head(netmem_to_page(netmem));
> +		if (page_pool_page_is_pp(head)) {
> +			page_pool_ref_page(head);
> +			return;
> +		}
> +	}
> +	__skb_frag_ref(frag);
> +}
> +

[ ... ]

> @@ -486,7 +502,7 @@ static int iptfs_skb_add_frags(struct sk_buff *skb,
>  			tofrag->len -= offset;
>  			offset = 0;
>  		}
> -		__skb_frag_ref(tofrag);
> +		iptfs_frag_ref(tofrag, skb->pp_recycle);

[Severity: Medium]

The commit message says this about the hunk:

  Its destination skb never has pp_recycle set today, so behaviour
  there does not change.

Is that accurate for the RX reassembly path?

In __input_process_payload(), the "Reuse the received skb" branch keeps
the skb the driver delivered (skb_pull(skb, data)). That skb keeps its
pp_recycle bit. If the inner packet is incomplete, it becomes the
reassembly destination:

		if (skb->len < iplen) {
			/* Start reassembly */
			spin_lock(&xtfs->drop_lock);

			xtfs->ra_newskb = skb;

On the next ESP packet, iptfs_reassem_cont() uses it as newskb. The
sharing gate there only requires pp_recycle to match:

	if (!skb_has_frag_list(skb) && !skb_has_frag_list(newskb) &&
	    (skb->head_frag || skb->len == skb->data_len) &&
	    skb->pp_recycle == newskb->pp_recycle) {
	...
		iptfs_skb_add_frags(newskb, fragwalk, data, copylen);

On a page_pool NIC, both skbs have pp_recycle set. Before this patch,
the shared frags got a plain reference from __skb_frag_ref(), but they
were later released through napi_pp_put_page(), which decrements
pp_ref_count.

Doesn't that make this hunk a fix for the same underflow on the RX
reassembly path, not just a consistency change? Could the commit message
say that both the TX aggregation and RX reassembly sharing paths were
affected, since that matters for stable backports?

>  		shinfo->nr_frags++;
>  		shinfo->flags |= SKBFL_SHARED_FRAG;
>  
> @@ -2171,7 +2187,8 @@ static void iptfs_consume_frags(struct sk_buff *to, struct sk_buff *from)
>  		new_truesize = SKB_TRUESIZE(skb_end_offset(from));
>  	} else {
>  		iptfs_skb_head_to_frag(from, &toi->frags[toi->nr_frags]);
> -		skb_frag_ref(to, toi->nr_frags++);
> +		iptfs_frag_ref(&toi->frags[toi->nr_frags], to->pp_recycle);
> +		toi->nr_frags++;
>  		new_truesize = SKB_DATA_ALIGN(sizeof(struct sk_buff));
>  	}
>  

[Severity: High]

This isn't a bug introduced by this patch, but since
iptfs_consume_frags() is being changed: can it corrupt a cloned source
skb?

In iptfs_output_queued(), only the destination is checked for clone or
shared state:

		if (shi->frag_list || skb_cloned(skb) || skb_shared(skb))
			share_ok = false;

The per-source test never checks skb_cloned(skb2) or skb_shared(skb2):

			if (share_ok &&
			    (shi2->frag_list ||
			     (!skb2->head_frag && skb_headlen(skb)) ||
			     skb->pp_recycle != skb2->pp_recycle ||
			     skb_zcopy(skb2) ||
			     (shi->nr_frags + shi2->nr_frags + 1 > MAX_SKB_FRAGS)))
				share_ok = false;

iptfs_consume_frags() then memcpy()s fromi->frags into the aggregate
without taking frag references, and does:

	fromi->nr_frags = 0;

If skb2 is a clone, that skb_shared_info is shared with its sibling. The
sibling is left with data_len > 0, nr_frags == 0 and no frag_list, and
the aggregate becomes the only owner of the page references. For
comparison, skb_try_coalesce() only moves frags when !skb_cloned(from),
and takes references otherwise.

Cloned skbs could reach this point with nothing uncloning them before
dst_output() -> iptfs_output_collect():

  - netem duplicate on an xfrm interface (netem_enqueue() uses
    skb_clone())
  - a non-mmap AF_PACKET listener on the xfrmi device
    (dev_queue_xmit_nit())
  - tc mirred from xfrmi

In each case the skb can be a forwarded non-GSO, non-linear, head_frag
RX skb.

With netem duplicate, the second clone is later accounted with
skb->data_len += skb2->len (or sent as a first skb) after its frags are
gone. The ESP output path can then hit BUG_ON(len) in __skb_to_sgvec().
An AF_PACKET reader could get -EFAULT. A concurrent reader, or DMA on a
mirrored device, could access pages after the aggregate frees them.

This dates back to b96ba312e21c and needs CAP_NET_ADMIN configuration.
Should the share_ok test also reject skb_cloned(skb2) or
skb_shared(skb2)?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/xfrm-iptfs-pp_ref_count-underflow-v5-1-c25508ec9160%40secunet.com