[PATCH] fs-verity: use kmap_local_page() instead of kmap()

[PATCH] fs-verity: use kmap_local_page() instead of kmap()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Convert the use of kmap() to its recommended replacement
kmap_local_page().  This avoids the overhead of doing a non-local
mapping, which is unnecessary in this case.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/verity/read_metadata.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c
index 6ee849dc7bc183..2aefc5565152ad 100644
--- a/fs/verity/read_metadata.c
+++ b/fs/verity/read_metadata.c
@@ -53,14 +53,14 @@ static int fsverity_read_merkle_tree(struct inode *inode,
 			break;
 		}
 
-		virt = kmap(page);
+		virt = kmap_local_page(page);
 		if (copy_to_user(buf, virt + offs_in_page, bytes_to_copy)) {
-			kunmap(page);
+			kunmap_local(virt);
 			put_page(page);
 			err = -EFAULT;
 			break;
 		}
-		kunmap(page);
+		kunmap_local(virt);
 		put_page(page);
 
 		retval += bytes_to_copy;

base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
prerequisite-patch-id: 188e114bdf3546eb18e7984b70be8a7c773acec3
-- 
2.37.1

Re: [PATCH] fs-verity: use kmap_local_page() instead of kmap()

[Fabio M. De Francesco]

On venerdì 19 agosto 2022 00:40:10 CEST Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Convert the use of kmap() to its recommended replacement
> kmap_local_page().  This avoids the overhead of doing a non-local
> mapping, which is unnecessary in this case.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/verity/read_metadata.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

It looks good to me...

Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Thanks,

Fabio

> diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c
> index 6ee849dc7bc183..2aefc5565152ad 100644
> --- a/fs/verity/read_metadata.c
> +++ b/fs/verity/read_metadata.c
> @@ -53,14 +53,14 @@ static int fsverity_read_merkle_tree(struct inode 
*inode,
>  			break;
>  		}
> 
> -		virt = kmap(page);
> +		virt = kmap_local_page(page);
>  		if (copy_to_user(buf, virt + offs_in_page, bytes_to_copy)) 
{
> -			kunmap(page);
> +			kunmap_local(virt);
>  			put_page(page);
>  			err = -EFAULT;
>  			break;
>  		}
> -		kunmap(page);
> +		kunmap_local(virt);
>  		put_page(page);
> 
>  		retval += bytes_to_copy;
> 
> base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
> prerequisite-patch-id: 188e114bdf3546eb18e7984b70be8a7c773acec3
> --
> 2.37.1

Re: [PATCH] fs-verity: use kmap_local_page() instead of kmap()

[Matthew Wilcox]

On Fri, Aug 19, 2022 at 09:50:37AM +0200, Fabio M. De Francesco wrote:
> On venerdì 19 agosto 2022 00:40:10 CEST Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Convert the use of kmap() to its recommended replacement
> > kmap_local_page().  This avoids the overhead of doing a non-local
> > mapping, which is unnecessary in this case.
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >  fs/verity/read_metadata.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> It looks good to me...
> 
> > -		virt = kmap(page);
> > +		virt = kmap_local_page(page);
> >  		if (copy_to_user(buf, virt + offs_in_page, bytes_to_copy)) 
> {
> > -			kunmap(page);
> > +			kunmap_local(virt);
> >  			put_page(page);
> >  			err = -EFAULT;
> >  			break;
> >  		}
> > -		kunmap(page);
> > +		kunmap_local(virt);

Is this a common pattern?  eg do we want something like:

static inline int copy_user_page(void __user *dst, struct page *page,
		size_t offset, size_t len)
{
	char *src = kmap_local_page(page) + offset;
	int err = 0;

	VM_BUG_ON(offset + len > PAGE_SIZE);
	if (copy_to_user(dst, src, len))
		err = -EFAULT;

	kunmap_local(src);
	return err;
}

in highmem.h?

Re: [PATCH] fs-verity: use kmap_local_page() instead of kmap()

[Fabio M. De Francesco]

On venerdì 19 agosto 2022 20:29:31 CEST Matthew Wilcox wrote:
> On Fri, Aug 19, 2022 at 09:50:37AM +0200, Fabio M. De Francesco wrote:
> > On venerdì 19 agosto 2022 00:40:10 CEST Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > Convert the use of kmap() to its recommended replacement
> > > kmap_local_page().  This avoids the overhead of doing a non-local
> > > mapping, which is unnecessary in this case.
> > > 
> > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > ---
> > > 
> > >  fs/verity/read_metadata.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > It looks good to me...
> > 
> > > -		virt = kmap(page);
> > > +		virt = kmap_local_page(page);
> > > 
> > >  		if (copy_to_user(buf, virt + offs_in_page, bytes_to_copy))
> > 
> > {
> > 
> > > -			kunmap(page);
> > > +			kunmap_local(virt);
> > > 
> > >  			put_page(page);
> > >  			err = -EFAULT;
> > >  			break;
> > >  		
> > >  		}
> > > 
> > > -		kunmap(page);
> > > +		kunmap_local(virt);
> 
> Is this a common pattern?  eg do we want something like:
> 
> static inline int copy_user_page(void __user *dst, struct page *page,
> 		size_t offset, size_t len)
> {
> 	char *src = kmap_local_page(page) + offset;
> 	int err = 0;
> 
> 	VM_BUG_ON(offset + len > PAGE_SIZE);
> 	if (copy_to_user(dst, src, len))
> 		err = -EFAULT;
> 
> 	kunmap_local(src);
> 	return err;
> }
> 
> in highmem.h?

Not sure that it is much common...

Can the following command provide any insight?

opensuse:/usr/src/git/kernels/linux> grep -rn copy_to_user -B7 . --exclude-
dir\=Documentation | grep kmap

./drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2306-         ptr = 
kmap_local_page(p);
./drivers/gpu/drm/i915/i915_gem.c-215-  vaddr = kmap_local_page(page);
./drivers/gpu/drm/radeon/radeon_ttm.c-872-                      ptr = 
kmap(page);
./drivers/vfio/pci/mlx5/main.c-182-             from_buff = 
kmap_local_page(page);
./arch/parisc/kernel/cache.c-580-       kto = kmap_local_page(to);
./mm/memory.c-5474-                     maddr = kmap(page);
./fs/verity/read_metadata.c-56-         virt = kmap(page);
./fs/aio.c-1252-                ev = kmap_local_page(page);
./fs/exec.c-883-                char *src = kmap_local_page(bprm->page[index]) 
+ offset;

If this command is good to provide any hint, it suggests that having 
copy_to_user() in highmem.h is not probably worth.

Thanks,

Fabio 

Re: [PATCH] fs-verity: use kmap_local_page() instead of kmap()

[Fabio M. De Francesco]

On Friday, August 19, 2022 12:40:10 AM CEST Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Convert the use of kmap() to its recommended replacement
> kmap_local_page().  This avoids the overhead of doing a non-local
> mapping, which is unnecessary in this case.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/verity/read_metadata.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 

It looks good to me...

Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Thanks,

Fabio

> diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c
> index 6ee849dc7bc183..2aefc5565152ad 100644
> --- a/fs/verity/read_metadata.c
> +++ b/fs/verity/read_metadata.c
> @@ -53,14 +53,14 @@ static int fsverity_read_merkle_tree(struct inode 
*inode,
>  			break;
>  		}
>  
> -		virt = kmap(page);
> +		virt = kmap_local_page(page);
>  		if (copy_to_user(buf, virt + offs_in_page, 
bytes_to_copy)) {
> -			kunmap(page);
> +			kunmap_local(virt);
>  			put_page(page);
>  			err = -EFAULT;
>  			break;
>  		}
> -		kunmap(page);
> +		kunmap_local(virt);
>  		put_page(page);
>  
>  		retval += bytes_to_copy;
> 
> base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
> prerequisite-patch-id: 188e114bdf3546eb18e7984b70be8a7c773acec3
> -- 
> 2.37.1
> 
>