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

[netdev-bot+sashiko@kernel.org]

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