Re: [PATCH net-next v1 4/5] net: remove skb->pp_recycle and consolidate netmem refcounting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] page_pool: TOCTOU race in page_pool_unref_netmem() non-RMW fast path
- [High] tls: page leak between netmem kTLS records and mlx5 tx_sync_info_get()
--

commit 874363fccb05e0d08d430f57bfdafc048dfc73e0
Author: Mina Almasry <almasrymina@google.com>

net: remove skb->pp_recycle and consolidate netmem refcounting

This commit removes the per-SKB pp_recycle bitfield and updates
netmem_inc_ref(), netmem_dec_ref(), and skb_pp_recycle() to check page_pool
membership directly via netmem_is_pp(). It also updates page_pool_unref_netmem()
and napi_pp_get_page() to coordinate pp_ref_count with external struct page
references, and converts TLS and wireless fragment refcounting to netmem
helpers.

> diff --git a/include/net/page_pool/helpers.h b/include/net/page_pool/helpers.h
> index 28635ce9454e8..af3cb65c30416 100644
> --- a/include/net/page_pool/helpers.h
> +++ b/include/net/page_pool/helpers.h
> @@ -242,6 +242,11 @@ page_pool_get_dma_dir(const struct page_pool *pool)
>  	return pool->p.dma_dir;
>  }
>  
> +static inline long page_pool_ref_count(netmem_ref netmem)
> +{
> +	return atomic_long_read(netmem_get_pp_ref_count_ref(netmem));
> +}
> +
>  static inline void page_pool_fragment_netmem(netmem_ref netmem, long nr)
>  {
>  	atomic_long_set(netmem_get_pp_ref_count_ref(netmem), nr);
> @@ -270,13 +275,22 @@ static inline void page_pool_fragment_page(struct page *page, long nr)
>  	page_pool_fragment_netmem(page_to_netmem(page), nr);
>  }
>  
> +static inline bool page_pool_netmem_is_exclusively_owned(netmem_ref netmem)
> +{
> +	if (netmem_is_net_iov(netmem))
> +		return true;
> +
> +	return page_ref_count(__netmem_to_page(netmem)) == 1;
> +}
> +
>  static inline long page_pool_unref_netmem(netmem_ref netmem, long nr)
>  {
>  	atomic_long_t *pp_ref_count = netmem_get_pp_ref_count_ref(netmem);
>  	long ret;
>  
> -	/* If nr == pp_ref_count then we have cleared all remaining
> -	 * references to the page:
> +	/* If nr == pp_ref_count and the page_pool holds the sole page
> +	 * reference, then we have cleared all remaining references to the
> +	 * page:
>  	 * 1. 'n == 1': no need to actually overwrite it.
>  	 * 2. 'n != 1': overwrite it with one, which is the rare case
>  	 *              for pp_ref_count draining.
> @@ -288,8 +302,15 @@ static inline long page_pool_unref_netmem(netmem_ref netmem, long nr)
>  	 * handling by ensuring all pages have partitioned into only 1 piece
>  	 * initially, and only overwrite it when the page is partitioned into
>  	 * more than one piece.
> +	 *
> +	 * If an external page reference exists (page_ref_count > 1), a
> +	 * concurrent napi_pp_get_page() caller holding only a page reference
> +	 * may attempt atomic_long_inc_not_zero(pp_ref_count), so we must
> +	 * perform an atomic RMW subtraction to 0 and leave pp_ref_count at 0
> +	 * while the page is released from the pool.
>  	 */
> -	if (atomic_long_read(pp_ref_count) == nr) {
> +	if (atomic_long_read(pp_ref_count) == nr &&
> +	    likely(page_pool_netmem_is_exclusively_owned(netmem))) {
>  		/* As we have ensured nr is always one for constant case using
>  		 * the BUILD_BUG_ON(), only need to handle the non-constant case
>  		 * here for pp_ref_count draining, which is a rare case.

[Severity: Critical]
Can the non-RMW fast path in page_pool_unref_netmem() race with a caller
that converts a struct page reference into a pp_ref_count reference, causing
an in-use page to be recycled into the pool?

With netmem_inc_ref() now calling napi_pp_get_page(), callers holding a
struct page reference such as zerocopy_fill_skb_from_iter() convert their
page_ref_count into a pp_ref_count:

net/core/datagram.c:zerocopy_fill_skb_from_iter() {
	...
			skb_fill_page_desc_noacc(skb, frag++, head, start, size);
			netmem_inc_ref(page_to_netmem(head));
			last_head = head;
			refs++;
		}
		if (refs)
			page_ref_sub(last_head, refs);
}

Consider a page_pool page with pp_ref_count == 1 (held by an SKB being freed
on CPU 0) and page_ref_count == 2 (1 held by the page_pool, 1 held by GUP in
zerocopy_fill_skb_from_iter() on CPU 1):

CPU 0                                   CPU 1
napi_pp_put_page()
  page_pool_put_full_netmem()
    page_pool_unref_netmem(netmem, 1)
      atomic_long_read(pp_ref_count) == 1
      (true; pp_ref_count stays 1)
                                        zerocopy_fill_skb_from_iter()
                                          netmem_inc_ref()
                                            napi_pp_get_page()
                                              atomic_long_inc_not_zero(pp_ref_count)
                                              (pp_ref_count: 1 -> 2)
                                          page_ref_sub(last_head, 1)
                                            (page_ref_count: 2 -> 1)
      page_pool_netmem_is_exclusively_owned()
        page_ref_count(page) == 1 (true!)
      returns 0 (leaving pp_ref_count == 2)
    __page_pool_put_page()
      __page_pool_page_can_be_recycled() (true)
      recycles page into pool while CPU 1's TX SKB still references it!

Similarly, on weakly ordered architectures, because atomic_long_read() and
page_ref_count() are both relaxed loads with no barrier between them, when
pp_ref_count == 2 and page_ref_count == 1, CPU 0 can speculatively load
page_ref_count == 1 before CPU 1 maps the page via tcp_zerocopy_receive() ->
vm_insert_pages() (page_ref_count: 1 -> 2) and drops its pp_ref_count
(2 -> 1). CPU 0 then reads pp_ref_count == 1 and returns 0 with pp_ref_count
left at 1 instead of 0 while __page_pool_put_page() ->
page_pool_return_netmem() releases the page from the pool.

[ ... ]

> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index 4cdfbb57ecff0..670fdeeeae567 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c

[ ... ]

> @@ -269,7 +271,7 @@ static void tls_append_frag(struct tls_record_info *record,
>  		skb_frag_fill_page_desc(frag, pfrag->page, pfrag->offset,
>  					size);
>  		++record->num_frags;
> -		get_page(pfrag->page);
> +		netmem_inc_ref(page_to_netmem(pfrag->page));
>  	}
>  
>  	pfrag->offset += size;
> @@ -302,7 +304,7 @@ static int tls_push_record(struct sock *sk,
>  		sg_set_page(&offload_ctx->sg_tx_data[i], skb_frag_page(frag),
>  			    skb_frag_size(frag), skb_frag_off(frag));
>  		sk_mem_charge(sk, skb_frag_size(frag));
> -		get_page(skb_frag_page(frag));
> +		netmem_inc_ref(skb_frag_netmem(frag));
>  	}
>  	sg_mark_end(&offload_ctx->sg_tx_data[record->num_frags - 1]);

[Severity: High]
Does switching tls_append_frag() and tls_push_record() to netmem_inc_ref()
cause a page leak during mlx5 kTLS TX resync when page_pool pages are
spliced into a hardware-offloaded TLS socket?

When a page_pool page is appended to a kTLS record (for example via
MSG_SPLICE_PAGES in tls_device_sendmsg() -> tls_push_data()),
tls_append_frag() and tls_push_record() now increment pp_ref_count via
netmem_inc_ref() -> napi_pp_get_page() instead of incrementing struct page
_refcount, leaving page_ref_count(page) == 1.

However, the mlx5 kTLS TX resync path in
drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c still manipulates
raw struct page _refcount on record->frags[i]:

tx_sync_info_get() {
	...
	while (remaining > 0) {
		skb_frag_t *frag = &record->frags[i];

		page_ref_inc(skb_frag_page(frag));
		remaining -= skb_frag_size(frag);
		info->frags[i++] = *frag;
	}
}

and in mlx5e_ktls_tx_handle_ooo() and mlx5e_ktls_tx_handle_resync_dump_comp():

void mlx5e_ktls_tx_handle_resync_dump_comp(struct mlx5e_txqsq *sq,
					   struct mlx5e_tx_wqe_info *wi,
					   u32 *dma_fifo_cc)
{
	...
	mlx5e_tx_dma_unmap(sq->pdev, dma);
	page_ref_dec(wi->resync_dump_frag_page);
	stats->tls_dump_packets++;
	stats->tls_dump_bytes += wi->num_bytes;
}

If the TCP ACK for the record arrives while the resync DUMP WQE is in
flight, tls_tcp_clean_acked() -> destroy_record() -> netmem_dec_ref() ->
napi_pp_put_page() runs while page_ref_count(page) > 1 (elevated by
page_ref_inc() / page_ref_add() in mlx5). Because page_ref_count(page) > 1,
__page_pool_page_can_be_recycled() returns false and __page_pool_put_page()
calls page_pool_return_netmem(), which unmaps the page from the pool, clears
pp_magic, and drops the page_pool's _refcount via put_page().

When the DUMP WQE later completes in mlx5e_ktls_tx_handle_resync_dump_comp()
(or unwinds at err_out in mlx5e_ktls_tx_handle_ooo()), page_ref_dec()
decrements page->_refcount to 0 without freeing the page to the page
allocator, leaking the page.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103211424.127627-1-almasrymina@google.com?part=4