[PATCH v2] net: allow skb_datagram_iter to be called from any context

[PATCH v2] net: allow skb_datagram_iter to be called from any context

[Sagi Grimberg]

We only use the mapping in a single context, so kmap_local is sufficient
and cheaper. Make sure to use skb_frag_foreach_page as skb frags may
contain highmem compound pages and we need to map page by page.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
Changes from v2:
- Fix usercopy BUG() due to copy from highmem pages across page boundary
  by using skb_frag_foreach_page

 net/core/datagram.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/net/core/datagram.c b/net/core/datagram.c
index a8b625abe242..cb72923acc21 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -435,15 +435,22 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset,
 
 		end = start + skb_frag_size(frag);
 		if ((copy = end - offset) > 0) {
-			struct page *page = skb_frag_page(frag);
-			u8 *vaddr = kmap(page);
+			u32 p_off, p_len, copied;
+			struct page *p;
+			u8 *vaddr;
 
 			if (copy > len)
 				copy = len;
-			n = INDIRECT_CALL_1(cb, simple_copy_to_iter,
-					vaddr + skb_frag_off(frag) + offset - start,
-					copy, data, to);
-			kunmap(page);
+
+			skb_frag_foreach_page(frag,
+					      skb_frag_off(frag) + offset - start,
+					      copy, p, p_off, p_len, copied) {
+				vaddr = kmap_local_page(p);
+				n = INDIRECT_CALL_1(cb, simple_copy_to_iter,
+					vaddr + p_off, p_len, data, to);
+				kunmap_local(vaddr);
+			}
+
 			offset += n;
 			if (n != copy)
 				goto short_copy;
-- 
2.43.0

Re: [PATCH v2] net: allow skb_datagram_iter to be called from any context

[Sagi Grimberg]

On 23/06/2024 11:12, Sagi Grimberg wrote:
> We only use the mapping in a single context, so kmap_local is sufficient
> and cheaper. Make sure to use skb_frag_foreach_page as skb frags may
> contain highmem compound pages and we need to map page by page.
>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
> Changes from v2:

Changes from v1 actually...

> - Fix usercopy BUG() due to copy from highmem pages across page boundary
>    by using skb_frag_foreach_page
>
>   net/core/datagram.c | 19 +++++++++++++------
>   1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index a8b625abe242..cb72923acc21 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -435,15 +435,22 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset,
>   
>   		end = start + skb_frag_size(frag);
>   		if ((copy = end - offset) > 0) {
> -			struct page *page = skb_frag_page(frag);
> -			u8 *vaddr = kmap(page);
> +			u32 p_off, p_len, copied;
> +			struct page *p;
> +			u8 *vaddr;
>   
>   			if (copy > len)
>   				copy = len;
> -			n = INDIRECT_CALL_1(cb, simple_copy_to_iter,
> -					vaddr + skb_frag_off(frag) + offset - start,
> -					copy, data, to);
> -			kunmap(page);
> +
> +			skb_frag_foreach_page(frag,
> +					      skb_frag_off(frag) + offset - start,
> +					      copy, p, p_off, p_len, copied) {
> +				vaddr = kmap_local_page(p);
> +				n = INDIRECT_CALL_1(cb, simple_copy_to_iter,
> +					vaddr + p_off, p_len, data, to);
> +				kunmap_local(vaddr);
> +			}
> +
>   			offset += n;
>   			if (n != copy)
>   				goto short_copy;

Re: [PATCH v2] net: allow skb_datagram_iter to be called from any context

[Paolo Abeni]

On Sun, 2024-06-23 at 11:12 +0300, Sagi Grimberg wrote:
> We only use the mapping in a single context, so kmap_local is sufficient
> and cheaper. Make sure to use skb_frag_foreach_page as skb frags may
> contain highmem compound pages and we need to map page by page.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>

V1 is already applied to net-next, you need to either send a revert
first or share an incremental patch (that would be a fix, and will need
a fixes tag).

On next revision, please include the target tree in the subj prefix.

Thanks,

Paolo

Re: [PATCH v2] net: allow skb_datagram_iter to be called from any context

[Jakub Kicinski]

On Tue, 25 Jun 2024 15:27:41 +0200 Paolo Abeni wrote:
> On Sun, 2024-06-23 at 11:12 +0300, Sagi Grimberg wrote:
> > We only use the mapping in a single context, so kmap_local is sufficient
> > and cheaper. Make sure to use skb_frag_foreach_page as skb frags may
> > contain highmem compound pages and we need to map page by page.
> > 
> > Signed-off-by: Sagi Grimberg <sagi@grimberg.me>  
> 
> V1 is already applied to net-next, you need to either send a revert
> first or share an incremental patch (that would be a fix, and will need
> a fixes tag).
> 
> On next revision, please include the target tree in the subj prefix.

I think the bug exists in net (it just requires an arch with HIGHMEM 
to be hit). So send the fix based on net/master and we'll deal with 
the net-next conflict? Or you can send a revert for net-next at the
same time, but I think the fix should target net.

Re: [PATCH v2] net: allow skb_datagram_iter to be called from any context

[Eric Dumazet]

On Tue, Jun 25, 2024 at 4:10 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 25 Jun 2024 15:27:41 +0200 Paolo Abeni wrote:
> > On Sun, 2024-06-23 at 11:12 +0300, Sagi Grimberg wrote:
> > > We only use the mapping in a single context, so kmap_local is sufficient
> > > and cheaper. Make sure to use skb_frag_foreach_page as skb frags may
> > > contain highmem compound pages and we need to map page by page.
> > >
> > > Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> >
> > V1 is already applied to net-next, you need to either send a revert
> > first or share an incremental patch (that would be a fix, and will need
> > a fixes tag).
> >
> > On next revision, please include the target tree in the subj prefix.
>
> I think the bug exists in net (it just requires an arch with HIGHMEM
> to be hit). So send the fix based on net/master and we'll deal with
> the net-next conflict? Or you can send a revert for net-next at the
> same time, but I think the fix should target net.

I have not seen a merged patch yet.

In any case, some credits would be nice.

Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202406161539.b5ff7b20-oliver.sang@intel.com

Re: [PATCH v2] net: allow skb_datagram_iter to be called from any context

[Matthew Wilcox]

On Tue, Jun 25, 2024 at 07:10:28AM -0700, Jakub Kicinski wrote:
> On Tue, 25 Jun 2024 15:27:41 +0200 Paolo Abeni wrote:
> > On Sun, 2024-06-23 at 11:12 +0300, Sagi Grimberg wrote:
> > > We only use the mapping in a single context, so kmap_local is sufficient
> > > and cheaper. Make sure to use skb_frag_foreach_page as skb frags may
> > > contain highmem compound pages and we need to map page by page.
> > > 
> > > Signed-off-by: Sagi Grimberg <sagi@grimberg.me>  
> > 
> > V1 is already applied to net-next, you need to either send a revert
> > first or share an incremental patch (that would be a fix, and will need
> > a fixes tag).
> > 
> > On next revision, please include the target tree in the subj prefix.
> 
> I think the bug exists in net (it just requires an arch with HIGHMEM 
> to be hit). So send the fix based on net/master and we'll deal with 
> the net-next conflict? Or you can send a revert for net-next at the
> same time, but I think the fix should target net.

It does not require an arch with highmem.  I was quite clear about this
in my earlier email.