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

[Antony Antony <antony@phenome.org>]

On Sun, Oct 11, 2026 at 10:57:57AM +0000, netdev-bot+sashiko@kernel.org wrote:
> 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…

It could be better written. If I have to send another one I woul re-write it.
Otherwise patch works. If there are no other issues it could be applied.

> 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?

Yes, I'd rather fix net_iov together with a likely net/core change seperate commit;
not addressed here, as the commit message says

> 
> > +		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?

No, it isn't accurate for RX reassembly; see below.

> 
> 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? 

Yes, you're right. The commit message could be better worded. The
iptfs_skb_add_frags() hunk also fixes RX reassembly, since the reused
driver skb keeps pp_recycle. I'll update the commit message in the next
version. I think the code is clear it fix RX side. If I send another 
vesion I would re-word it.


> 
> >  		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
>