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 <[email protected]> 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/[email protected]?part=4
