linux-kernel - Re: [PATCH 2/3] hfs: Replace kmap() with kmap_local

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <001D72B9-CB6A-4025-99CA-B3C02E1D5E0A@dubeyko.com>
Date:   Tue, 9 Aug 2022 11:04:34 -0700
From:   Viacheslav Dubeyko <slava@...eyko.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
        Jeff Layton <jlayton@...nel.org>,
        Arnd Bergmann <arnd@...db.de>,
        Muchun Song <songmuchun@...edance.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Ira Weiny <ira.weiny@...el.com>
Subject: Re: [PATCH 2/3] hfs: Replace kmap() with kmap_local_page() in bnode.c



> On Aug 9, 2022, at 8:20 AM, Fabio M. De Francesco <fmdefrancesco@...il.com> wrote:
> 
> kmap() is being deprecated in favor of kmap_local_page().
> 
> Two main problems with kmap(): (1) It comes with an overhead as mapping
> space is restricted and protected by a global lock for synchronization and
> (2) it also requires global TLB invalidation when the kmap’s pool wraps
> and it might block when the mapping space is fully utilized until a slot
> becomes available.
> 
> With kmap_local_page() the mappings are per thread, CPU local, can take
> page faults, and can be called from any context (including interrupts).
> It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore,
> the tasks can be preempted and, when they are scheduled to run again, the
> kernel virtual addresses are restored and still valid.
> 
> Since its use in bnode.c is safe everywhere, it should be preferred.
> 
> Therefore, replace kmap() with kmap_local_page() in bnode.c. Where
> possible, use the suited standard helpers (memzero_page(), memcpy_page())
> instead of open coding kmap_local_page() plus memset() or memcpy().
> 
> Tested in a QEMU/KVM x86_32 VM, 6GB RAM, booting a kernel with
> HIGHMEM64GB enabled.
> 
> Suggested-by: Ira Weiny <ira.weiny@...el.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> ---

Looks good.

Reviewed-by: Viacheslav Dubeyko <slava@...eyko.com>

Thanks,
Slava.


> fs/hfs/bnode.c | 32 ++++++++++++--------------------
> 1 file changed, 12 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
> index c83fd0e8404d..2015e42e752a 100644
> --- a/fs/hfs/bnode.c
> +++ b/fs/hfs/bnode.c
> @@ -21,7 +21,6 @@ void hfs_bnode_read(struct hfs_bnode *node, void *buf, int off, int len)
> 	int pagenum;
> 	int bytes_read;
> 	int bytes_to_read;
> -	void *vaddr;
> 
> 	off += node->page_offset;
> 	pagenum = off >> PAGE_SHIFT;
> @@ -33,9 +32,7 @@ void hfs_bnode_read(struct hfs_bnode *node, void *buf, int off, int len)
> 		page = node->page[pagenum];
> 		bytes_to_read = min_t(int, len - bytes_read, PAGE_SIZE - off);
> 
> -		vaddr = kmap_atomic(page);
> -		memcpy(buf + bytes_read, vaddr + off, bytes_to_read);
> -		kunmap_atomic(vaddr);
> +		memcpy_from_page(buf + bytes_read, page, off, bytes_to_read);
> 
> 		pagenum++;
> 		off = 0; /* page offset only applies to the first page */
> @@ -80,8 +77,7 @@ void hfs_bnode_write(struct hfs_bnode *node, void *buf, int off, int len)
> 	off += node->page_offset;
> 	page = node->page[0];
> 
> -	memcpy(kmap(page) + off, buf, len);
> -	kunmap(page);
> +	memcpy_to_page(page, off, buf, len);
> 	set_page_dirty(page);
> }
> 
> @@ -105,8 +101,7 @@ void hfs_bnode_clear(struct hfs_bnode *node, int off, int len)
> 	off += node->page_offset;
> 	page = node->page[0];
> 
> -	memset(kmap(page) + off, 0, len);
> -	kunmap(page);
> +	memzero_page(page, off, len);
> 	set_page_dirty(page);
> }
> 
> @@ -123,9 +118,7 @@ void hfs_bnode_copy(struct hfs_bnode *dst_node, int dst,
> 	src_page = src_node->page[0];
> 	dst_page = dst_node->page[0];
> 
> -	memcpy(kmap(dst_page) + dst, kmap(src_page) + src, len);
> -	kunmap(src_page);
> -	kunmap(dst_page);
> +	memcpy_page(dst_page, dst, src_page, src, len);
> 	set_page_dirty(dst_page);
> }
> 
> @@ -140,9 +133,9 @@ void hfs_bnode_move(struct hfs_bnode *node, int dst, int src, int len)
> 	src += node->page_offset;
> 	dst += node->page_offset;
> 	page = node->page[0];
> -	ptr = kmap(page);
> +	ptr = kmap_local_page(page);
> 	memmove(ptr + dst, ptr + src, len);
> -	kunmap(page);
> +	kunmap_local(ptr);
> 	set_page_dirty(page);
> }
> 
> @@ -346,13 +339,14 @@ struct hfs_bnode *hfs_bnode_find(struct hfs_btree *tree, u32 num)
> 	if (!test_bit(HFS_BNODE_NEW, &node->flags))
> 		return node;
> 
> -	desc = (struct hfs_bnode_desc *)(kmap(node->page[0]) + node->page_offset);
> +	desc = (struct hfs_bnode_desc *)(kmap_local_page(node->page[0]) +
> +					 node->page_offset);
> 	node->prev = be32_to_cpu(desc->prev);
> 	node->next = be32_to_cpu(desc->next);
> 	node->num_recs = be16_to_cpu(desc->num_recs);
> 	node->type = desc->type;
> 	node->height = desc->height;
> -	kunmap(node->page[0]);
> +	kunmap_local(desc);
> 
> 	switch (node->type) {
> 	case HFS_NODE_HEADER:
> @@ -436,14 +430,12 @@ struct hfs_bnode *hfs_bnode_create(struct hfs_btree *tree, u32 num)
> 	}
> 
> 	pagep = node->page;
> -	memset(kmap(*pagep) + node->page_offset, 0,
> -	       min((int)PAGE_SIZE, (int)tree->node_size));
> +	memzero_page(*pagep, node->page_offset,
> +		     min((int)PAGE_SIZE, (int)tree->node_size));
> 	set_page_dirty(*pagep);
> -	kunmap(*pagep);
> 	for (i = 1; i < tree->pages_per_bnode; i++) {
> -		memset(kmap(*++pagep), 0, PAGE_SIZE);
> +		memzero_page(*++pagep, 0, PAGE_SIZE);
> 		set_page_dirty(*pagep);
> -		kunmap(*pagep);
> 	}
> 	clear_bit(HFS_BNODE_NEW, &node->flags);
> 	wake_up(&node->lock_wq);
> -- 
> 2.37.1
>