[PATCH net] xsk: respect the offsets when copying frags

[PATCH net] xsk: respect the offsets when copying frags

[Bui Quang Minh]

Add the missing offsets when copying frags in xdp_copy_frags_from_zc().

Fixes: 560d958c6c68 ("xsk: add generic XSk &xdp_buff -> skb conversion")
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 net/core/xdp.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/core/xdp.c b/net/core/xdp.c
index f86eedad586a..a723dc301f94 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -697,7 +697,8 @@ static noinline bool xdp_copy_frags_from_zc(struct sk_buff *skb,
 	nr_frags = xinfo->nr_frags;
 
 	for (u32 i = 0; i < nr_frags; i++) {
-		u32 len = skb_frag_size(&xinfo->frags[i]);
+		const skb_frag_t *frag = &xinfo->frags[i];
+		u32 len = skb_frag_size(frag);
 		u32 offset, truesize = len;
 		netmem_ref netmem;
 
@@ -707,8 +708,8 @@ static noinline bool xdp_copy_frags_from_zc(struct sk_buff *skb,
 			return false;
 		}
 
-		memcpy(__netmem_address(netmem),
-		       __netmem_address(xinfo->frags[i].netmem),
+		memcpy(__netmem_address(netmem) + offset,
+		       __netmem_address(frag->netmem) + skb_frag_off(frag),
 		       LARGEST_ALIGN(len));
 		__skb_fill_netmem_desc_noacc(sinfo, i, netmem, offset, len);
 
-- 
2.43.0

Re: [PATCH net] xsk: respect the offsets when copying frags

[Stanislav Fomichev]

On 04/23, Bui Quang Minh wrote:
> Add the missing offsets when copying frags in xdp_copy_frags_from_zc().

Can you please share more about how you've hit this problem?
I don't see the caller of this function (xdp_build_skb_from_zc)
being used at all.

Alexander, do you have plans to use it? Or should we remove it for now?

Re: [PATCH net] xsk: respect the offsets when copying frags

[Bui Quang Minh]

On 4/23/25 21:41, Stanislav Fomichev wrote:
> On 04/23, Bui Quang Minh wrote:
>> Add the missing offsets when copying frags in xdp_copy_frags_from_zc().
> Can you please share more about how you've hit this problem?
> I don't see the caller of this function (xdp_build_skb_from_zc)
> being used at all.
>
> Alexander, do you have plans to use it? Or should we remove it for now?
Hi,

I've been playing around to add support for zerocopy XDP socket with 
multi buffer mergeable buffer in virtio-net (this TODO: 
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/drivers/net/virtio_net.c#n1312). 
In that case, I'll have a XDP buff with frags. When we have XDP_PASS 
return, I need to convert the XDP buff with frags to skb with frags, so 
I think the helper is quite helpful. I used it and got packet dropped 
due to checksum error. Debugging the problem, I've found out this issue 
which makes the skb's frag data incorrect.

Thanks,
Quang Minh.

Re: [PATCH net] xsk: respect the offsets when copying frags

[Maciej Fijalkowski]

On Wed, Apr 23, 2025 at 09:58:16PM +0700, Bui Quang Minh wrote:
> On 4/23/25 21:41, Stanislav Fomichev wrote:
> > On 04/23, Bui Quang Minh wrote:
> > > Add the missing offsets when copying frags in xdp_copy_frags_from_zc().
> > Can you please share more about how you've hit this problem?
> > I don't see the caller of this function (xdp_build_skb_from_zc)
> > being used at all.
> > 
> > Alexander, do you have plans to use it? Or should we remove it for now?

libeth xdp support uses this:
https://lore.kernel.org/netdev/20250415172825.3731091-15-aleksander.lobakin@intel.com/

> Hi,
> 
> I've been playing around to add support for zerocopy XDP socket with multi
> buffer mergeable buffer in virtio-net (this TODO: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/drivers/net/virtio_net.c#n1312).
> In that case, I'll have a XDP buff with frags. When we have XDP_PASS return,
> I need to convert the XDP buff with frags to skb with frags, so I think the
> helper is quite helpful. I used it and got packet dropped due to checksum
> error. Debugging the problem, I've found out this issue which makes the
> skb's frag data incorrect.

Nice! I'll take a look at patch content tomorrow and probably ack it.

> 
> Thanks,
> Quang Minh.

Re: [PATCH net] xsk: respect the offsets when copying frags

[Alexander Lobakin]

From: Bui Quang Minh <minhquangbui99@gmail.com>
Date: Wed, 23 Apr 2025 17:10:47 +0700

> Add the missing offsets when copying frags in xdp_copy_frags_from_zc().
> 
> Fixes: 560d958c6c68 ("xsk: add generic XSk &xdp_buff -> skb conversion")
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
>  net/core/xdp.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index f86eedad586a..a723dc301f94 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -697,7 +697,8 @@ static noinline bool xdp_copy_frags_from_zc(struct sk_buff *skb,
>  	nr_frags = xinfo->nr_frags;
>  
>  	for (u32 i = 0; i < nr_frags; i++) {
> -		u32 len = skb_frag_size(&xinfo->frags[i]);
> +		const skb_frag_t *frag = &xinfo->frags[i];
> +		u32 len = skb_frag_size(frag);
>  		u32 offset, truesize = len;
>  		netmem_ref netmem;
>  
> @@ -707,8 +708,8 @@ static noinline bool xdp_copy_frags_from_zc(struct sk_buff *skb,
>  			return false;
>  		}
>  
> -		memcpy(__netmem_address(netmem),
> -		       __netmem_address(xinfo->frags[i].netmem),
> +		memcpy(__netmem_address(netmem) + offset,
> +		       __netmem_address(frag->netmem) + skb_frag_off(frag),
>  		       LARGEST_ALIGN(len));
>  		__skb_fill_netmem_desc_noacc(sinfo, i, netmem, offset, len);

Incorrect fix.

page_pool_dev_alloc_netmem() allocates a buffer of skb_frag_size() len,
but then you pass offset when copying, which may lead to access beyond
the end of the buffer.

I know that my code here is incorrect as well, but the idea was to
allocate only skb_frag_size() and copy the actual payload without any
offset to the new buffer. So, you need to pass the offset only to the
second argument of memcpy() and then pass 0 as @offset to
__skb_fill_netmem_desc_noacc().

Thanks,
Olek

Re: [PATCH net] xsk: respect the offsets when copying frags

[Bui Quang Minh]

On 4/24/25 21:02, Alexander Lobakin wrote:
> From: Bui Quang Minh <minhquangbui99@gmail.com>
> Date: Wed, 23 Apr 2025 17:10:47 +0700
>
>> Add the missing offsets when copying frags in xdp_copy_frags_from_zc().
>>
>> Fixes: 560d958c6c68 ("xsk: add generic XSk &xdp_buff -> skb conversion")
>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>> ---
>>   net/core/xdp.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index f86eedad586a..a723dc301f94 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -697,7 +697,8 @@ static noinline bool xdp_copy_frags_from_zc(struct sk_buff *skb,
>>   	nr_frags = xinfo->nr_frags;
>>   
>>   	for (u32 i = 0; i < nr_frags; i++) {
>> -		u32 len = skb_frag_size(&xinfo->frags[i]);
>> +		const skb_frag_t *frag = &xinfo->frags[i];
>> +		u32 len = skb_frag_size(frag);
>>   		u32 offset, truesize = len;
>>   		netmem_ref netmem;
>>   
>> @@ -707,8 +708,8 @@ static noinline bool xdp_copy_frags_from_zc(struct sk_buff *skb,
>>   			return false;
>>   		}
>>   
>> -		memcpy(__netmem_address(netmem),
>> -		       __netmem_address(xinfo->frags[i].netmem),
>> +		memcpy(__netmem_address(netmem) + offset,
>> +		       __netmem_address(frag->netmem) + skb_frag_off(frag),
>>   		       LARGEST_ALIGN(len));
>>   		__skb_fill_netmem_desc_noacc(sinfo, i, netmem, offset, len);
> Incorrect fix.
>
> page_pool_dev_alloc_netmem() allocates a buffer of skb_frag_size() len,
> but then you pass offset when copying, which may lead to access beyond
> the end of the buffer.
>
> I know that my code here is incorrect as well, but the idea was to
> allocate only skb_frag_size() and copy the actual payload without any
> offset to the new buffer. So, you need to pass the offset only to the
> second argument of memcpy() and then pass 0 as @offset to
> __skb_fill_netmem_desc_noacc().

I'm not quite familiar with the page_pool API so I might be wrong. 
AFAICS, the netmem_ref is just a wrapper around struct page now. 
page_pool_dev_alloc_netmem(pp, &offset, &truesize) returns the allocated 
page for our request but we must use the returned offset to access our 
allocated space. The start of page may be currently used by other user.

That's my understanding when looking at this code path

page_pool_dev_alloc_netmem
-> page_pool_alloc_netmem
     -> page_pool_alloc_frag_netmem <- specifically this function

Thanks,
Quang Minh.

Re: [PATCH net] xsk: respect the offsets when copying frags

[Jakub Kicinski]

On Wed, 23 Apr 2025 17:10:47 +0700 Bui Quang Minh wrote:
> Add the missing offsets when copying frags in xdp_copy_frags_from_zc().
> 
> Fixes: 560d958c6c68 ("xsk: add generic XSk &xdp_buff -> skb conversion")
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>

I think the fix is right but I dislike the use of netmem here :(
Could we switch back to page_pool_dev_alloc() ?
Allocating a netmem to immediately call __netmem_address() is strange.
At least to me. Because netmem is supposed to be potentially unreadable.
And using normal page allocation will avoid the confusion and bug we're
dealing with now.

As Stanislav pointed out this function is not used anywhere today,
so let's target the rewrite to net-next and explain in the commit 
message where the bug comes from and why it doesn't need to be
backported (and drop the Fixes tag)?

Re: [PATCH net] xsk: respect the offsets when copying frags

[Bui Quang Minh]

On 4/23/25 17:10, Bui Quang Minh wrote:
> Add the missing offsets when copying frags in xdp_copy_frags_from_zc().
>
> Fixes: 560d958c6c68 ("xsk: add generic XSk &xdp_buff -> skb conversion")
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
>   net/core/xdp.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index f86eedad586a..a723dc301f94 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -697,7 +697,8 @@ static noinline bool xdp_copy_frags_from_zc(struct sk_buff *skb,
>   	nr_frags = xinfo->nr_frags;
>   
>   	for (u32 i = 0; i < nr_frags; i++) {
> -		u32 len = skb_frag_size(&xinfo->frags[i]);
> +		const skb_frag_t *frag = &xinfo->frags[i];
> +		u32 len = skb_frag_size(frag);
>   		u32 offset, truesize = len;
>   		netmem_ref netmem;
>   
> @@ -707,8 +708,8 @@ static noinline bool xdp_copy_frags_from_zc(struct sk_buff *skb,
>   			return false;
>   		}
>   
> -		memcpy(__netmem_address(netmem),
> -		       __netmem_address(xinfo->frags[i].netmem),
> +		memcpy(__netmem_address(netmem) + offset,
> +		       __netmem_address(frag->netmem) + skb_frag_off(frag),
>   		       LARGEST_ALIGN(len));
>   		__skb_fill_netmem_desc_noacc(sinfo, i, netmem, offset, len);
>   

I know it's very unlikely but do we need to 
kmap_local_page(skb_frag_page(frag) before using 
__netmem_address(frag->netmem) to make sure the frag's page is mapped? 
Or it is impossible that the frag's page to be highmem and unmapped? Thanks,
Quang Minh.

Re: [PATCH net] xsk: respect the offsets when copying frags

[Jakub Kicinski]

On Fri, 25 Apr 2025 22:46:35 +0700 Bui Quang Minh wrote:
> On 4/23/25 17:10, Bui Quang Minh wrote:
> > Add the missing offsets when copying frags in xdp_copy_frags_from_zc().
> >
> > Fixes: 560d958c6c68 ("xsk: add generic XSk &xdp_buff -> skb conversion")
> > Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> > ---
> >   net/core/xdp.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > index f86eedad586a..a723dc301f94 100644
> > --- a/net/core/xdp.c
> > +++ b/net/core/xdp.c
> > @@ -697,7 +697,8 @@ static noinline bool xdp_copy_frags_from_zc(struct sk_buff *skb,
> >   	nr_frags = xinfo->nr_frags;
> >   
> >   	for (u32 i = 0; i < nr_frags; i++) {
> > -		u32 len = skb_frag_size(&xinfo->frags[i]);
> > +		const skb_frag_t *frag = &xinfo->frags[i];
> > +		u32 len = skb_frag_size(frag);
> >   		u32 offset, truesize = len;
> >   		netmem_ref netmem;
> >   
> > @@ -707,8 +708,8 @@ static noinline bool xdp_copy_frags_from_zc(struct sk_buff *skb,
> >   			return false;
> >   		}
> >   
> > -		memcpy(__netmem_address(netmem),
> > -		       __netmem_address(xinfo->frags[i].netmem),
> > +		memcpy(__netmem_address(netmem) + offset,
> > +		       __netmem_address(frag->netmem) + skb_frag_off(frag),
> >   		       LARGEST_ALIGN(len));
> >   		__skb_fill_netmem_desc_noacc(sinfo, i, netmem, offset, len);
> >     
> 
> I know it's very unlikely but do we need to 
> kmap_local_page(skb_frag_page(frag) before using 
> __netmem_address(frag->netmem) to make sure the frag's page is mapped? 
> Or it is impossible that the frag's page to be highmem and unmapped?

AFAIU these frags come from a AF_XDP umem so they should be mapped
already.