Re: [Intel-wired-lan] [PATCH net-next] ice: access @pp through netmem_desc instead of page
From: Pavel Begunkov Date: Wed, 17 Dec 2025 13:11:28 + > On 12/16/25 04:20, Matthew Wilcox wrote: >> On Tue, Dec 16, 2025 at 01:07:23PM +0900, Byungchul Park wrote: >>> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c >>> @@ -1251,7 +1251,7 @@ static int ice_lbtest_receive_frames(struct >>> ice_rx_ring *rx_ring) >>> rx_buf = &rx_ring->rx_fqes[i]; >>> page = __netmem_to_page(rx_buf->netmem); >>> received_buf = page_address(page) + rx_buf->offset + >>> - page->pp->p.offset; >>> + pp_page_to_nmdesc(page)->pp->p.offset; >> >> Shouldn't we rather use: >> >> nmdesc = __netmem_to_nmdesc(rx_buf->netmem); >> received_buf = nmdesc_address(nmdesc) + rx_buf->offset + >> nmdesc->pp->p_offset; >> >> (also. i think we're missing a nmdesc_address() function in our API). > > It wouldn't make sense as net_iov backed nmdescs don't have/expose > host addresses (only dma addresses). nmdesc_address() would still > need to rely on the caller knowing that it's a page. An explicit > cast with *netmem_to_page() should be better. Sorry for the late reply. Holidays... Happy New Year everyone. I agree with Pavel. This loopback test always operates with kernel/page-backed memory. I believe it's fully valid to explicitly cast to a page in such cases and work with it. This is also more clear to the readers after all (IIRC I suggested this piece of code when Michał was working on the ice conversion). Thanks, Olek
Re: [Intel-wired-lan] [PATCH net-next] ice: access @pp through netmem_desc instead of page
On Wed, Dec 17, 2025 at 02:34:14PM +, Loktionov, Aleksandr wrote: > > -Original Message- > > From: Pavel Begunkov > > Sent: Wednesday, December 17, 2025 2:16 PM > > To: Loktionov, Aleksandr ; Byungchul > > Park ; [email protected]; [email protected] > > Cc: [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; Nguyen, Anthony L > > ; Kitszel, Przemyslaw > > ; [email protected]; > > [email protected]; [email protected]; [email protected]; intel- > > [email protected] > > Subject: Re: [Intel-wired-lan] [PATCH net-next] ice: access @pp > > through netmem_desc instead of page > > > > On 12/17/25 11:46, Loktionov, Aleksandr wrote: > > > > > > > > >> -Original Message- > > >> From: Intel-wired-lan On > > Behalf > > >> Of Byungchul Park > > >> Sent: Tuesday, December 16, 2025 5:07 AM > > >> To: [email protected]; [email protected] > > >> Cc: [email protected]; [email protected]; > > >> [email protected]; [email protected]; [email protected]; > > >> [email protected]; [email protected]; [email protected]; > > >> Nguyen, Anthony L ; Kitszel, Przemyslaw > > >> ; [email protected]; > > >> [email protected]; [email protected]; [email protected]; intel- > > >> [email protected] > > >> Subject: [Intel-wired-lan] [PATCH net-next] ice: access @pp through > > >> netmem_desc instead of page > > >> > > >> To eliminate the use of struct page in page pool, the page pool > > users > > >> should use netmem descriptor and APIs instead. > > >> > > >> Make ice driver access @pp through netmem_desc instead of page. > > >> > > > Please add test info: HW/ASIC + PF/VF/SR-IOV, kernel version/branch, > > exact repro steps, before/after results (expected vs. observed). > > > > > >> Signed-off-by: Byungchul Park > > >> --- > > >> drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c > > >> b/drivers/net/ethernet/intel/ice/ice_ethtool.c > > >> index 969d4f8f9c02..ae8a4e35cb10 100644 > > >> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c > > >> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c > > >> @@ -1251,7 +1251,7 @@ static int ice_lbtest_receive_frames(struct > > >> ice_rx_ring *rx_ring) > > >>rx_buf = &rx_ring->rx_fqes[i]; > > >>page = __netmem_to_page(rx_buf->netmem); > > >>received_buf = page_address(page) + rx_buf->offset + > > >> - page->pp->p.offset; > > >> + pp_page_to_nmdesc(page)->pp->p.offset; > > > If rx_buf->netmem is not backed by a page pool (e.g., fallback > > allocation), pp will be NULL and this dereferences NULL. > > > I think the loopback test runs in a controlled environment, but the > > code must verify pp is valid before dereferencing. > > > Isn't it? > > > > Considering "page->pp->p.offset" poking into the pool, if that can > > happen it's a pre-existing problem, which should be fixed first. > > > > -- > > Pavel Begunkov > > > Good day, Hi Byungchul, Pavel, Hi, > Thanks for pushing the driver toward netmem — I fully support removing direct > struct page accesses from the networking stack. > > Regarding this change in ice_lbtest_receive_frames(): > > received_buf = page_address(page) + rx_buf->offset + > pp_page_to_nmdesc(page)->pp->p.offset; > > Pavel, you're right that if page->pp could be NULL, it's a pre-existing bug > that should be fixed. > However, looking at the loopback test path, I'm concerned this code doesn't > handle non-page-pool allocations safely. > > The netmem model explicitly allows for buffers that aren't page-pool backed. > While the loopback test likely runs in a controlled environment, the code > should verify pp is valid before dereferencing, > or use the netmem helpers that handle this gracefully: If it's true, yeah, it definitely should be fixed but in a separate patch since it's a different issue. Let's try with a fo
Re: [Intel-wired-lan] [PATCH net-next] ice: access @pp through netmem_desc instead of page
> -Original Message- > From: Pavel Begunkov > Sent: Wednesday, December 17, 2025 2:16 PM > To: Loktionov, Aleksandr ; Byungchul > Park ; [email protected]; [email protected] > Cc: [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; Nguyen, Anthony L > ; Kitszel, Przemyslaw > ; [email protected]; > [email protected]; [email protected]; [email protected]; intel- > [email protected] > Subject: Re: [Intel-wired-lan] [PATCH net-next] ice: access @pp > through netmem_desc instead of page > > On 12/17/25 11:46, Loktionov, Aleksandr wrote: > > > > > >> -Original Message- > >> From: Intel-wired-lan On > Behalf > >> Of Byungchul Park > >> Sent: Tuesday, December 16, 2025 5:07 AM > >> To: [email protected]; [email protected] > >> Cc: [email protected]; [email protected]; > >> [email protected]; [email protected]; [email protected]; > >> [email protected]; [email protected]; [email protected]; > >> Nguyen, Anthony L ; Kitszel, Przemyslaw > >> ; [email protected]; > >> [email protected]; [email protected]; [email protected]; intel- > >> [email protected] > >> Subject: [Intel-wired-lan] [PATCH net-next] ice: access @pp through > >> netmem_desc instead of page > >> > >> To eliminate the use of struct page in page pool, the page pool > users > >> should use netmem descriptor and APIs instead. > >> > >> Make ice driver access @pp through netmem_desc instead of page. > >> > > Please add test info: HW/ASIC + PF/VF/SR-IOV, kernel version/branch, > exact repro steps, before/after results (expected vs. observed). > > > >> Signed-off-by: Byungchul Park > >> --- > >> drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c > >> b/drivers/net/ethernet/intel/ice/ice_ethtool.c > >> index 969d4f8f9c02..ae8a4e35cb10 100644 > >> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c > >> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c > >> @@ -1251,7 +1251,7 @@ static int ice_lbtest_receive_frames(struct > >> ice_rx_ring *rx_ring) > >>rx_buf = &rx_ring->rx_fqes[i]; > >>page = __netmem_to_page(rx_buf->netmem); > >>received_buf = page_address(page) + rx_buf->offset + > >> - page->pp->p.offset; > >> + pp_page_to_nmdesc(page)->pp->p.offset; > > If rx_buf->netmem is not backed by a page pool (e.g., fallback > allocation), pp will be NULL and this dereferences NULL. > > I think the loopback test runs in a controlled environment, but the > code must verify pp is valid before dereferencing. > > Isn't it? > > Considering "page->pp->p.offset" poking into the pool, if that can > happen it's a pre-existing problem, which should be fixed first. > > -- > Pavel Begunkov Good day, Hi Byungchul, Pavel, Thanks for pushing the driver toward netmem — I fully support removing direct struct page accesses from the networking stack. Regarding this change in ice_lbtest_receive_frames(): received_buf = page_address(page) + rx_buf->offset + pp_page_to_nmdesc(page)->pp->p.offset; Pavel, you're right that if page->pp could be NULL, it's a pre-existing bug that should be fixed. However, looking at the loopback test path, I'm concerned this code doesn't handle non-page-pool allocations safely. The netmem model explicitly allows for buffers that aren't page-pool backed. While the loopback test likely runs in a controlled environment, the code should verify pp is valid before dereferencing, or use the netmem helpers that handle this gracefully: struct netmem_desc *ndesc = __netmem_to_nmdesc(rx_buf->netmem); void *addr = netmem_address(rx_buf->netmem); struct page_pool *pp; if (!addr) continue; /* unreadable netmem */ pp = __netmem_get_pp(ndesc); received_buf = addr + rx_buf->offset + (pp ? pp->p.offset : 0); Alternatively, guard the existing code with page_pool_page_is_pp(page) before calling pp_page_to_nmdesc(). This would complete the netmem conversion while fixing the unsafe dereference, aligning with Matthew's earlier suggestion to use descriptor/address accessors. Also, please add test details to the commit message: HW/ASIC (e.g., E810/E823) PF vs VF, SR-IOV configuration Kernel tree/commit (net-next @ ) Repro steps: ethtool -t $dev offline Before/after behavior Happy to review v2 with these changes. Best regards, Alex
Re: [Intel-wired-lan] [PATCH net-next] ice: access @pp through netmem_desc instead of page
On 12/17/25 11:46, Loktionov, Aleksandr wrote: -Original Message- From: Intel-wired-lan On Behalf Of Byungchul Park Sent: Tuesday, December 16, 2025 5:07 AM To: [email protected]; [email protected] Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Nguyen, Anthony L ; Kitszel, Przemyslaw ; [email protected]; [email protected]; [email protected]; [email protected]; intel- [email protected] Subject: [Intel-wired-lan] [PATCH net-next] ice: access @pp through netmem_desc instead of page To eliminate the use of struct page in page pool, the page pool users should use netmem descriptor and APIs instead. Make ice driver access @pp through netmem_desc instead of page. Please add test info: HW/ASIC + PF/VF/SR-IOV, kernel version/branch, exact repro steps, before/after results (expected vs. observed). Signed-off-by: Byungchul Park --- drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c index 969d4f8f9c02..ae8a4e35cb10 100644 --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c @@ -1251,7 +1251,7 @@ static int ice_lbtest_receive_frames(struct ice_rx_ring *rx_ring) rx_buf = &rx_ring->rx_fqes[i]; page = __netmem_to_page(rx_buf->netmem); received_buf = page_address(page) + rx_buf->offset + - page->pp->p.offset; + pp_page_to_nmdesc(page)->pp->p.offset; If rx_buf->netmem is not backed by a page pool (e.g., fallback allocation), pp will be NULL and this dereferences NULL. I think the loopback test runs in a controlled environment, but the code must verify pp is valid before dereferencing. Isn't it? Considering "page->pp->p.offset" poking into the pool, if that can happen it's a pre-existing problem, which should be fixed first. -- Pavel Begunkov
Re: [Intel-wired-lan] [PATCH net-next] ice: access @pp through netmem_desc instead of page
On 12/16/25 04:20, Matthew Wilcox wrote: On Tue, Dec 16, 2025 at 01:07:23PM +0900, Byungchul Park wrote: +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c @@ -1251,7 +1251,7 @@ static int ice_lbtest_receive_frames(struct ice_rx_ring *rx_ring) rx_buf = &rx_ring->rx_fqes[i]; page = __netmem_to_page(rx_buf->netmem); received_buf = page_address(page) + rx_buf->offset + - page->pp->p.offset; + pp_page_to_nmdesc(page)->pp->p.offset; Shouldn't we rather use: nmdesc = __netmem_to_nmdesc(rx_buf->netmem); received_buf = nmdesc_address(nmdesc) + rx_buf->offset + nmdesc->pp->p_offset; (also. i think we're missing a nmdesc_address() function in our API). It wouldn't make sense as net_iov backed nmdescs don't have/expose host addresses (only dma addresses). nmdesc_address() would still need to rely on the caller knowing that it's a page. An explicit cast with *netmem_to_page() should be better. -- Pavel Begunkov
Re: [Intel-wired-lan] [PATCH net-next] ice: access @pp through netmem_desc instead of page
> -Original Message- > From: Intel-wired-lan On Behalf > Of Byungchul Park > Sent: Tuesday, December 16, 2025 5:07 AM > To: [email protected]; [email protected] > Cc: [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > Nguyen, Anthony L ; Kitszel, Przemyslaw > ; [email protected]; > [email protected]; [email protected]; [email protected]; intel- > [email protected] > Subject: [Intel-wired-lan] [PATCH net-next] ice: access @pp through > netmem_desc instead of page > > To eliminate the use of struct page in page pool, the page pool users > should use netmem descriptor and APIs instead. > > Make ice driver access @pp through netmem_desc instead of page. > Please add test info: HW/ASIC + PF/VF/SR-IOV, kernel version/branch, exact repro steps, before/after results (expected vs. observed). > Signed-off-by: Byungchul Park > --- > drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c > b/drivers/net/ethernet/intel/ice/ice_ethtool.c > index 969d4f8f9c02..ae8a4e35cb10 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c > +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c > @@ -1251,7 +1251,7 @@ static int ice_lbtest_receive_frames(struct > ice_rx_ring *rx_ring) > rx_buf = &rx_ring->rx_fqes[i]; > page = __netmem_to_page(rx_buf->netmem); > received_buf = page_address(page) + rx_buf->offset + > -page->pp->p.offset; > +pp_page_to_nmdesc(page)->pp->p.offset; If rx_buf->netmem is not backed by a page pool (e.g., fallback allocation), pp will be NULL and this dereferences NULL. I think the loopback test runs in a controlled environment, but the code must verify pp is valid before dereferencing. Isn't it? > > if (ice_lbtest_check_frame(received_buf)) > valid_frames++; > -- > 2.17.1
Re: [Intel-wired-lan] [PATCH net-next] ice: access @pp through netmem_desc instead of page
On Tue, Dec 16, 2025 at 01:07:23PM +0900, Byungchul Park wrote: > +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c > @@ -1251,7 +1251,7 @@ static int ice_lbtest_receive_frames(struct ice_rx_ring > *rx_ring) > rx_buf = &rx_ring->rx_fqes[i]; > page = __netmem_to_page(rx_buf->netmem); > received_buf = page_address(page) + rx_buf->offset + > -page->pp->p.offset; > +pp_page_to_nmdesc(page)->pp->p.offset; Shouldn't we rather use: nmdesc = __netmem_to_nmdesc(rx_buf->netmem); received_buf = nmdesc_address(nmdesc) + rx_buf->offset + nmdesc->pp->p_offset; (also. i think we're missing a nmdesc_address() function in our API).
