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

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

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