[PATCH v3] lib/test_bpf: Call page_address() on page acquired with GFP_KERNEL flag

[PATCH v3] lib/test_bpf: Call page_address() on page acquired with GFP_KERNEL flag

[Sumitra Sharma]

generate_test_data() acquires a page with alloc_page(GFP_KERNEL).
The GFP_KERNEL is typical for kernel-internal allocations.
The caller requires ZONE_NORMAL or a lower zone for direct access.

Therefore the page cannot come from ZONE_HIGHMEM. Thus there's
no need to map it with kmap().

Also, the kmap() is being deprecated in favor of
kmap_local_page() [1].

Hence, use a plain page_address() directly.

Since the page passed to the page_address() is not from the highmem
zone, the page_address() function will always return a valid kernel
virtual address and will not return NULL. Hence, remove the check
'if (!ptr)'.

Remove the unused variable 'ptr'.

[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny
@intel.com/

Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
---

- Link to v1: https://lore.kernel.org/bpf/20230613073020.GA359792@sumitra.com/T/
- Link to v2: https://lore.kernel.org/lkml/3564297.R56niFO833@suse/T/

Changes in v3:
Noted by: Fabio M. De Francesco<fmdefrancesco@gmail.com>
	- Remove the check 'if (!ptr)'.
	- Remove the unused variable 'ptr'.
	- Change the commit message.

Changes in v2:
Noted by: Fabio M. De Francesco<fmdefrancesco@gmail.com>
	- Remove the kmap() call and call page_address() instead.
	- Change the commit subject and message.


 lib/test_bpf.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index ade9ac672adb..448bc1b0b8b5 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c
@@ -14381,18 +14381,12 @@ static void *generate_test_data(struct bpf_test *test, int sub)
 		 * single fragment to the skb, filled with
 		 * test->frag_data.
 		 */
-		void *ptr;
-
 		page = alloc_page(GFP_KERNEL);
 
 		if (!page)
 			goto err_kfree_skb;
 
-		ptr = kmap(page);
-		if (!ptr)
-			goto err_free_page;
-		memcpy(ptr, test->frag_data, MAX_DATA);
-		kunmap(page);
+		memcpy(page_address(page), test->frag_data, MAX_DATA);
 		skb_add_rx_frag(skb, 0, page, 0, MAX_DATA, MAX_DATA);
 	}
 
-- 
2.25.1

[PATCH v3] lib/test_bpf: Call page_address() on page acquired with GFP_KERNEL flag

[Sumitra Sharma]

generate_test_data() acquires a page with alloc_page(GFP_KERNEL).
The GFP_KERNEL is typical for kernel-internal allocations.
The caller requires ZONE_NORMAL or a lower zone for direct access.

Therefore the page cannot come from ZONE_HIGHMEM. Thus there's
no need to map it with kmap().

Also, the kmap() is being deprecated in favor of
kmap_local_page() [1].

Hence, use a plain page_address() directly.

Since the page passed to the page_address() is not from the highmem
zone, the page_address() function will always return a valid kernel
virtual address and will not return NULL. Hence, remove the check
'if (!ptr)'.

Remove the unused variable 'ptr'.

[1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com/

Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
---

- Link to v1: https://lore.kernel.org/bpf/20230613073020.GA359792@sumitra.com/T/
- Link to v2: https://lore.kernel.org/lkml/3564297.R56niFO833@suse/T/

Changes in v3:
Noted by: Fabio M. De Francesco<fmdefrancesco@gmail.com>
	- Remove the check 'if (!ptr)'.
	- Remove the unused variable 'ptr'.
	- Change the commit message.

Changes in v2:
Noted by: Fabio M. De Francesco<fmdefrancesco@gmail.com>
    - Remove the kmap() call and call page_address() instead.
    - Change the commit subject and message.


 lib/test_bpf.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index ade9ac672adb..448bc1b0b8b5 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c
@@ -14381,18 +14381,12 @@ static void *generate_test_data(struct bpf_test *test, int sub)
 		 * single fragment to the skb, filled with
 		 * test->frag_data.
 		 */
-		void *ptr;
-
 		page = alloc_page(GFP_KERNEL);
 
 		if (!page)
 			goto err_kfree_skb;
 
-		ptr = kmap(page);
-		if (!ptr)
-			goto err_free_page;
-		memcpy(ptr, test->frag_data, MAX_DATA);
-		kunmap(page);
+		memcpy(page_address(page), test->frag_data, MAX_DATA);
 		skb_add_rx_frag(skb, 0, page, 0, MAX_DATA, MAX_DATA);
 	}
 
-- 
2.25.1

Re: [PATCH v3] lib/test_bpf: Call page_address() on page acquired with GFP_KERNEL flag

[kernel test robot]

Hi Sumitra,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]
[also build test WARNING on bpf/master linus/master v6.4-rc7 next-20230622]
[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/Sumitra-Sharma/lib-test_bpf-Call-page_address-on-page-acquired-with-GFP_KERNEL-flag/20230622-160846
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20230622080729.GA426913%40sumitra.com
patch subject: [PATCH v3] lib/test_bpf: Call page_address() on page acquired with GFP_KERNEL flag
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230623/202306230559.hU5Aonpl-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230623/202306230559.hU5Aonpl-lkp@intel.com/reproduce)

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>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306230559.hU5Aonpl-lkp@intel.com/

All warnings (new ones prefixed by >>):

   lib/test_bpf.c: In function 'generate_test_data':
>> lib/test_bpf.c:14395:1: warning: label 'err_free_page' defined but not used [-Wunused-label]
   14395 | err_free_page:
         | ^~~~~~~~~~~~~


vim +/err_free_page +14395 lib/test_bpf.c

64a8946b447e41 Alexei Starovoitov 2014-05-08  14358  
10f18e0ba1ea7e Daniel Borkmann    2014-05-23  14359  static void *generate_test_data(struct bpf_test *test, int sub)
64a8946b447e41 Alexei Starovoitov 2014-05-08  14360  {
bac142acb90e95 Nicolas Schichan   2015-08-04  14361  	struct sk_buff *skb;
bac142acb90e95 Nicolas Schichan   2015-08-04  14362  	struct page *page;
bac142acb90e95 Nicolas Schichan   2015-08-04  14363  
10f18e0ba1ea7e Daniel Borkmann    2014-05-23  14364  	if (test->aux & FLAG_NO_DATA)
10f18e0ba1ea7e Daniel Borkmann    2014-05-23  14365  		return NULL;
64a8946b447e41 Alexei Starovoitov 2014-05-08  14366  
f516420f683d14 Xu Kuohai          2022-03-21  14367  	if (test->aux & FLAG_LARGE_MEM)
f516420f683d14 Xu Kuohai          2022-03-21  14368  		return kmalloc(test->test[sub].data_size, GFP_KERNEL);
f516420f683d14 Xu Kuohai          2022-03-21  14369  
10f18e0ba1ea7e Daniel Borkmann    2014-05-23  14370  	/* Test case expects an skb, so populate one. Various
10f18e0ba1ea7e Daniel Borkmann    2014-05-23  14371  	 * subtests generate skbs of different sizes based on
10f18e0ba1ea7e Daniel Borkmann    2014-05-23  14372  	 * the same data.
10f18e0ba1ea7e Daniel Borkmann    2014-05-23  14373  	 */
bac142acb90e95 Nicolas Schichan   2015-08-04  14374  	skb = populate_skb(test->data, test->test[sub].data_size);
bac142acb90e95 Nicolas Schichan   2015-08-04  14375  	if (!skb)
bac142acb90e95 Nicolas Schichan   2015-08-04  14376  		return NULL;
bac142acb90e95 Nicolas Schichan   2015-08-04  14377  
bac142acb90e95 Nicolas Schichan   2015-08-04  14378  	if (test->aux & FLAG_SKB_FRAG) {
bac142acb90e95 Nicolas Schichan   2015-08-04  14379  		/*
bac142acb90e95 Nicolas Schichan   2015-08-04  14380  		 * when the test requires a fragmented skb, add a
bac142acb90e95 Nicolas Schichan   2015-08-04  14381  		 * single fragment to the skb, filled with
bac142acb90e95 Nicolas Schichan   2015-08-04  14382  		 * test->frag_data.
bac142acb90e95 Nicolas Schichan   2015-08-04  14383  		 */
bac142acb90e95 Nicolas Schichan   2015-08-04  14384  		page = alloc_page(GFP_KERNEL);
bac142acb90e95 Nicolas Schichan   2015-08-04  14385  
bac142acb90e95 Nicolas Schichan   2015-08-04  14386  		if (!page)
bac142acb90e95 Nicolas Schichan   2015-08-04  14387  			goto err_kfree_skb;
bac142acb90e95 Nicolas Schichan   2015-08-04  14388  
4a8b1daa0ee566 Sumitra Sharma     2023-06-22  14389  		memcpy(page_address(page), test->frag_data, MAX_DATA);
bac142acb90e95 Nicolas Schichan   2015-08-04  14390  		skb_add_rx_frag(skb, 0, page, 0, MAX_DATA, MAX_DATA);
bac142acb90e95 Nicolas Schichan   2015-08-04  14391  	}
bac142acb90e95 Nicolas Schichan   2015-08-04  14392  
bac142acb90e95 Nicolas Schichan   2015-08-04  14393  	return skb;
bac142acb90e95 Nicolas Schichan   2015-08-04  14394  
bac142acb90e95 Nicolas Schichan   2015-08-04 @14395  err_free_page:
bac142acb90e95 Nicolas Schichan   2015-08-04  14396  	__free_page(page);
bac142acb90e95 Nicolas Schichan   2015-08-04  14397  err_kfree_skb:
bac142acb90e95 Nicolas Schichan   2015-08-04  14398  	kfree_skb(skb);
bac142acb90e95 Nicolas Schichan   2015-08-04  14399  	return NULL;
10f18e0ba1ea7e Daniel Borkmann    2014-05-23  14400  }
10f18e0ba1ea7e Daniel Borkmann    2014-05-23  14401  

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

Re: [PATCH v3] lib/test_bpf: Call page_address() on page acquired with GFP_KERNEL flag

[Ira Weiny]

Sumitra Sharma wrote:
> generate_test_data() acquires a page with alloc_page(GFP_KERNEL).
> The GFP_KERNEL is typical for kernel-internal allocations.
> The caller requires ZONE_NORMAL or a lower zone for direct access.
> 
> Therefore the page cannot come from ZONE_HIGHMEM. Thus there's
> no need to map it with kmap().
> 
> Also, the kmap() is being deprecated in favor of
> kmap_local_page() [1].
> 
> Hence, use a plain page_address() directly.
> 
> Since the page passed to the page_address() is not from the highmem
> zone, the page_address() function will always return a valid kernel
> virtual address and will not return NULL. Hence, remove the check
> 'if (!ptr)'.
> 
> Remove the unused variable 'ptr'.
> 
> [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny
> @intel.com/

Odd line break here which makes looking at this reference harder.

> 
> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>

The code looks good though.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
> 
> - Link to v1: https://lore.kernel.org/bpf/20230613073020.GA359792@sumitra.com/T/
> - Link to v2: https://lore.kernel.org/lkml/3564297.R56niFO833@suse/T/
> 
> Changes in v3:
> Noted by: Fabio M. De Francesco<fmdefrancesco@gmail.com>
> 	- Remove the check 'if (!ptr)'.
> 	- Remove the unused variable 'ptr'.
> 	- Change the commit message.
> 
> Changes in v2:
> Noted by: Fabio M. De Francesco<fmdefrancesco@gmail.com>
> 	- Remove the kmap() call and call page_address() instead.
> 	- Change the commit subject and message.
> 
> 
>  lib/test_bpf.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/lib/test_bpf.c b/lib/test_bpf.c
> index ade9ac672adb..448bc1b0b8b5 100644
> --- a/lib/test_bpf.c
> +++ b/lib/test_bpf.c
> @@ -14381,18 +14381,12 @@ static void *generate_test_data(struct bpf_test *test, int sub)
>  		 * single fragment to the skb, filled with
>  		 * test->frag_data.
>  		 */
> -		void *ptr;
> -
>  		page = alloc_page(GFP_KERNEL);
>  
>  		if (!page)
>  			goto err_kfree_skb;
>  
> -		ptr = kmap(page);
> -		if (!ptr)
> -			goto err_free_page;
> -		memcpy(ptr, test->frag_data, MAX_DATA);
> -		kunmap(page);
> +		memcpy(page_address(page), test->frag_data, MAX_DATA);
>  		skb_add_rx_frag(skb, 0, page, 0, MAX_DATA, MAX_DATA);
>  	}
>  
> -- 
> 2.25.1
>