Skip to contentSkip to conversation

fs-verity: use kmap_local_page() instead of kmap()

PATCH In review Eric Biggers 3 participants 5 messages Aug 18, 2022 – Aug 19, 2022

Subsystems (4)
Memory management VFS fscrypt: file system level encryption support fs-verity: read-only file-based authenticity protection
lore
Share & tools
lore

Eric Biggers

author maintainer
#0 lore raw

PATCH 1/1 fs-verity: use kmap_local_page() instead of kmap()

From: Eric Biggers <[email protected]>

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 <[email protected]>
---
 fs/verity/read_metadata.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
fs/verity/read_metadata.c +3 -3
index 6ee849dc7bc183..2aefc5565152ad 100644
@@ -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

Signature · 1 line
2.37.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 11 lines · CEST Eric Biggers

From: Eric Biggers <[email protected]>

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 <[email protected]>
---
 fs/verity/read_metadata.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

It looks good to me...

Reviewed-by Fabio M. De Francesco <[email protected]>

Thanks,

Fabio

fs/verity/read_metadata.c static int fsverity_read_merkle_tree(struct inode

Quoted 5 lines
fs/verity/read_metadata.c +0 -0
index 6ee849dc7bc183..2aefc5565152ad 100644
@@ -53,14 +53,14 @@ static int fsverity_read_merkle_tree(struct inode

*inode,

fs/verity/read_metadata.c static int fsverity_read_merkle_tree(struct inode *inode,

Quoted 6 lines
fs/verity/read_metadata.c +1 -1
 			break;
 		}
 
-		virt = kmap(page);
+		virt = kmap_local_page(page);
 		if (copy_to_user(buf, virt + offs_in_page,

bytes_to_copy)) {

Quoted 18 lines
fs/verity/read_metadata.c +2 -2
-			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

Signature · 1 line
2.37.1
Reviewed-by on v1 1/1
#2 lore raw
In reply to Eric Biggers
Quoted 10 lines · On venerdì 19 agosto 2022 00:40:10 CEST Eric Biggers wrote:

From: Eric Biggers <[email protected]>

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 <[email protected]>
---
 fs/verity/read_metadata.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

It looks good to me...

Reviewed-by Fabio M. De Francesco <[email protected]>

Thanks,

Fabio

fs/verity/read_metadata.c static int fsverity_read_merkle_tree(struct inode

Quoted 5 lines
fs/verity/read_metadata.c +0 -0
index 6ee849dc7bc183..2aefc5565152ad 100644
@@ -53,14 +53,14 @@ static int fsverity_read_merkle_tree(struct inode

*inode,

fs/verity/read_metadata.c static int fsverity_read_merkle_tree(struct inode *inode,

Quoted 6 lines
fs/verity/read_metadata.c +1 -1
 			break;
 		}
 
-		virt = kmap(page);
+		virt = kmap_local_page(page);
 		if (copy_to_user(buf, virt + offs_in_page, bytes_to_copy))

{

Quoted 16 lines
fs/verity/read_metadata.c +2 -2
-			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

#3 lore raw
Quoted 18 lines · Fabio M. De Francesco
Quoted 10 lines · On venerdì 19 agosto 2022 00:40:10 CEST Eric Biggers wrote:

From: Eric Biggers <[email protected]>

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 <[email protected]>
---
 fs/verity/read_metadata.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

It looks good to me...

fs/verity/read_metadata.c static int fsverity_read_merkle_tree(struct inode *inode,

Quoted 3 lines
fs/verity/read_metadata.c +1 -1
-		virt = kmap(page);
+		virt = kmap_local_page(page);
 		if (copy_to_user(buf, virt + offs_in_page, bytes_to_copy))

{

Quoted 8 lines

fs/verity/read_metadata.c static int fsverity_read_merkle_tree(struct inode *inode, · line 63

Quoted 8 lines
fs/verity/read_metadata.c +2 -2
-			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?

#4 lore raw
Quoted 51 lines · On venerdì 19 agosto 2022 20:29:31 CEST Matthew Wilcox wrote:
Quoted 25 lines · Fabio M. De Francesco
Quoted 11 lines · On venerdì 19 agosto 2022 00:40:10 CEST Eric Biggers wrote:

From: Eric Biggers <[email protected]>

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 <[email protected]>

---

 fs/verity/read_metadata.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

It looks good to me...

fs/verity/read_metadata.c static int fsverity_read_merkle_tree(struct inode *inode,

Quoted 4 lines
fs/verity/read_metadata.c +1 -1
-		virt = kmap(page);
+		virt = kmap_local_page(page);
 
 		if (copy_to_user(buf, virt + offs_in_page, bytes_to_copy))

{

Quoted 3 lines
(quoted diff) +1 -1
-			kunmap(page);
+			kunmap_local(virt);
Quoted 8 lines

fs/verity/read_metadata.c static int fsverity_read_merkle_tree(struct inode *inode, · line 58

Quoted 8 lines
fs/verity/read_metadata.c +1 -1
 			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;
}
Quoted 1 line

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