[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.

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

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20240623081248.170613-1-sagi@grimberg.me>
References: <20240623081248.170613-1-sagi@grimberg.me>
TO: Sagi Grimberg <sagi@grimberg.me>
TO: netdev@vger.kernel.org
TO: Jakub Kicinski <kuba@kernel.org>
CC: Eric Dumazet <edumazet@google.com>
CC: David Howells <dhowells@redhat.com>
CC: Matthew Wilcox <willy@infradead.org>
CC: Sagi Grimberg <sagi@grimberg.me>

Hi Sagi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net/main]
[also build test WARNING on net-next/main linus/master horms-ipvs/master v6.10-rc5]
[cannot apply to next-20240627]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sagi-Grimberg/net-allow-skb_datagram_iter-to-be-called-from-any-context/20240625-182355
base:   net/main
patch link:    https://lore.kernel.org/r/20240623081248.170613-1-sagi%40grimberg.me
patch subject: [PATCH v2] net: allow skb_datagram_iter to be called from any context
:::::: branch date: 3 days ago
:::::: commit date: 3 days ago
config: i386-randconfig-141-20240628 (https://download.01.org/0day-ci/archive/20240628/202406282328.e4z61dm7-lkp@intel.com/config)
compiler: gcc-8 (Ubuntu 8.4.0-3ubuntu2) 8.4.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202406282328.e4z61dm7-lkp@intel.com/

smatch warnings:
net/core/datagram.c:435 __skb_datagram_iter() error: uninitialized symbol 'n'.

vim +/n +435 net/core/datagram.c

3305b80c214c64 Herbert Xu    2005-12-13  382  
29f3490ba9d239 Eric Dumazet  2020-03-24  383  INDIRECT_CALLABLE_DECLARE(static size_t simple_copy_to_iter(const void *addr,
29f3490ba9d239 Eric Dumazet  2020-03-24  384  						size_t bytes,
29f3490ba9d239 Eric Dumazet  2020-03-24  385  						void *data __always_unused,
29f3490ba9d239 Eric Dumazet  2020-03-24  386  						struct iov_iter *i));
29f3490ba9d239 Eric Dumazet  2020-03-24  387  
56dc6d6355744b YueHaibing    2019-03-19  388  static int __skb_datagram_iter(const struct sk_buff *skb, int offset,
950fcaecd5cc6c Sagi Grimberg 2018-12-03  389  			       struct iov_iter *to, int len, bool fault_short,
56dc6d6355744b YueHaibing    2019-03-19  390  			       size_t (*cb)(const void *, size_t, void *,
56dc6d6355744b YueHaibing    2019-03-19  391  					    struct iov_iter *), void *data)
a8f820aa4066d2 Herbert Xu    2014-11-07  392  {
a8f820aa4066d2 Herbert Xu    2014-11-07  393  	int start = skb_headlen(skb);
3278682123811d Al Viro       2017-02-17  394  	int i, copy = start - offset, start_off = offset, n;
a8f820aa4066d2 Herbert Xu    2014-11-07  395  	struct sk_buff *frag_iter;
a8f820aa4066d2 Herbert Xu    2014-11-07  396  
a8f820aa4066d2 Herbert Xu    2014-11-07  397  	/* Copy header. */
a8f820aa4066d2 Herbert Xu    2014-11-07  398  	if (copy > 0) {
a8f820aa4066d2 Herbert Xu    2014-11-07  399  		if (copy > len)
a8f820aa4066d2 Herbert Xu    2014-11-07  400  			copy = len;
29f3490ba9d239 Eric Dumazet  2020-03-24  401  		n = INDIRECT_CALL_1(cb, simple_copy_to_iter,
29f3490ba9d239 Eric Dumazet  2020-03-24  402  				    skb->data + offset, copy, data, to);
3278682123811d Al Viro       2017-02-17  403  		offset += n;
3278682123811d Al Viro       2017-02-17  404  		if (n != copy)
a8f820aa4066d2 Herbert Xu    2014-11-07  405  			goto short_copy;
a8f820aa4066d2 Herbert Xu    2014-11-07  406  		if ((len -= copy) == 0)
a8f820aa4066d2 Herbert Xu    2014-11-07  407  			return 0;
a8f820aa4066d2 Herbert Xu    2014-11-07  408  	}
a8f820aa4066d2 Herbert Xu    2014-11-07  409  
a8f820aa4066d2 Herbert Xu    2014-11-07  410  	/* Copy paged appendix. Hmm... why does this look so complicated? */
a8f820aa4066d2 Herbert Xu    2014-11-07  411  	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
a8f820aa4066d2 Herbert Xu    2014-11-07  412  		int end;
a8f820aa4066d2 Herbert Xu    2014-11-07  413  		const skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
a8f820aa4066d2 Herbert Xu    2014-11-07  414  
a8f820aa4066d2 Herbert Xu    2014-11-07  415  		WARN_ON(start > offset + len);
a8f820aa4066d2 Herbert Xu    2014-11-07  416  
a8f820aa4066d2 Herbert Xu    2014-11-07  417  		end = start + skb_frag_size(frag);
a8f820aa4066d2 Herbert Xu    2014-11-07  418  		if ((copy = end - offset) > 0) {
3a8698b261c7de Sagi Grimberg 2024-06-23  419  			u32 p_off, p_len, copied;
3a8698b261c7de Sagi Grimberg 2024-06-23  420  			struct page *p;
3a8698b261c7de Sagi Grimberg 2024-06-23  421  			u8 *vaddr;
0fc07791bc7754 Sagi Grimberg 2018-12-03  422  
a8f820aa4066d2 Herbert Xu    2014-11-07  423  			if (copy > len)
a8f820aa4066d2 Herbert Xu    2014-11-07  424  				copy = len;
3a8698b261c7de Sagi Grimberg 2024-06-23  425  
3a8698b261c7de Sagi Grimberg 2024-06-23  426  			skb_frag_foreach_page(frag,
3a8698b261c7de Sagi Grimberg 2024-06-23  427  					      skb_frag_off(frag) + offset - start,
3a8698b261c7de Sagi Grimberg 2024-06-23  428  					      copy, p, p_off, p_len, copied) {
3a8698b261c7de Sagi Grimberg 2024-06-23  429  				vaddr = kmap_local_page(p);
29f3490ba9d239 Eric Dumazet  2020-03-24  430  				n = INDIRECT_CALL_1(cb, simple_copy_to_iter,
3a8698b261c7de Sagi Grimberg 2024-06-23  431  					vaddr + p_off, p_len, data, to);
3a8698b261c7de Sagi Grimberg 2024-06-23  432  				kunmap_local(vaddr);
3a8698b261c7de Sagi Grimberg 2024-06-23  433  			}
3a8698b261c7de Sagi Grimberg 2024-06-23  434  
3278682123811d Al Viro       2017-02-17 @435  			offset += n;
3278682123811d Al Viro       2017-02-17  436  			if (n != copy)
a8f820aa4066d2 Herbert Xu    2014-11-07  437  				goto short_copy;
a8f820aa4066d2 Herbert Xu    2014-11-07  438  			if (!(len -= copy))
a8f820aa4066d2 Herbert Xu    2014-11-07  439  				return 0;
a8f820aa4066d2 Herbert Xu    2014-11-07  440  		}
a8f820aa4066d2 Herbert Xu    2014-11-07  441  		start = end;
a8f820aa4066d2 Herbert Xu    2014-11-07  442  	}
a8f820aa4066d2 Herbert Xu    2014-11-07  443  
a8f820aa4066d2 Herbert Xu    2014-11-07  444  	skb_walk_frags(skb, frag_iter) {
a8f820aa4066d2 Herbert Xu    2014-11-07  445  		int end;
a8f820aa4066d2 Herbert Xu    2014-11-07  446  
a8f820aa4066d2 Herbert Xu    2014-11-07  447  		WARN_ON(start > offset + len);
a8f820aa4066d2 Herbert Xu    2014-11-07  448  
a8f820aa4066d2 Herbert Xu    2014-11-07  449  		end = start + frag_iter->len;
a8f820aa4066d2 Herbert Xu    2014-11-07  450  		if ((copy = end - offset) > 0) {
a8f820aa4066d2 Herbert Xu    2014-11-07  451  			if (copy > len)
a8f820aa4066d2 Herbert Xu    2014-11-07  452  				copy = len;
950fcaecd5cc6c Sagi Grimberg 2018-12-03  453  			if (__skb_datagram_iter(frag_iter, offset - start,
65d69e2505bb64 Sagi Grimberg 2018-12-03  454  						to, copy, fault_short, cb, data))
a8f820aa4066d2 Herbert Xu    2014-11-07  455  				goto fault;
a8f820aa4066d2 Herbert Xu    2014-11-07  456  			if ((len -= copy) == 0)
a8f820aa4066d2 Herbert Xu    2014-11-07  457  				return 0;
a8f820aa4066d2 Herbert Xu    2014-11-07  458  			offset += copy;
a8f820aa4066d2 Herbert Xu    2014-11-07  459  		}
a8f820aa4066d2 Herbert Xu    2014-11-07  460  		start = end;
a8f820aa4066d2 Herbert Xu    2014-11-07  461  	}
a8f820aa4066d2 Herbert Xu    2014-11-07  462  	if (!len)
a8f820aa4066d2 Herbert Xu    2014-11-07  463  		return 0;
a8f820aa4066d2 Herbert Xu    2014-11-07  464  
a8f820aa4066d2 Herbert Xu    2014-11-07  465  	/* This is not really a user copy fault, but rather someone
a8f820aa4066d2 Herbert Xu    2014-11-07  466  	 * gave us a bogus length on the skb.  We should probably
a8f820aa4066d2 Herbert Xu    2014-11-07  467  	 * print a warning here as it may indicate a kernel bug.
a8f820aa4066d2 Herbert Xu    2014-11-07  468  	 */
a8f820aa4066d2 Herbert Xu    2014-11-07  469  
a8f820aa4066d2 Herbert Xu    2014-11-07  470  fault:
3278682123811d Al Viro       2017-02-17  471  	iov_iter_revert(to, offset - start_off);
a8f820aa4066d2 Herbert Xu    2014-11-07  472  	return -EFAULT;
a8f820aa4066d2 Herbert Xu    2014-11-07  473  
a8f820aa4066d2 Herbert Xu    2014-11-07  474  short_copy:
950fcaecd5cc6c Sagi Grimberg 2018-12-03  475  	if (fault_short || iov_iter_count(to))
a8f820aa4066d2 Herbert Xu    2014-11-07  476  		goto fault;
a8f820aa4066d2 Herbert Xu    2014-11-07  477  
a8f820aa4066d2 Herbert Xu    2014-11-07  478  	return 0;
a8f820aa4066d2 Herbert Xu    2014-11-07  479  }
950fcaecd5cc6c Sagi Grimberg 2018-12-03  480  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki