linux-kernel - Re: [RESEND PATCH] ecryptfs: 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] [day] [month] [year] [list]

Message-ID: <20220925171852.GD59018@sequoia>
Date:   Sun, 25 Sep 2022 12:18:52 -0500
From:   Tyler Hicks <code@...icks.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     Christian Brauner <brauner@...nel.org>,
        Seth Forshee <sforshee@...italocean.com>,
        Amir Goldstein <amir73il@...il.com>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        Theodore Ts'o <tytso@....edu>,
        Muchun Song <songmuchun@...edance.com>,
        ecryptfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Venkataramanan, Anirudh" <anirudh.venkataramanan@...el.com>,
        Ira Weiny <ira.weiny@...el.com>
Subject: Re: [RESEND PATCH] ecryptfs: Replace kmap() with kmap_local_page()

On 2022-09-01 18:07:04, Fabio M. De Francesco wrote:
> The use of kmap() is being deprecated in favor of kmap_local_page().
> 
> There are two main problems with kmap(): (1) It comes with an overhead as
> the 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 fs/ecryptfs is safe everywhere, it should be preferred.
> 
> Therefore, replace kmap() with kmap_local_page() in fs/ecryptfs.
> 
> Cc: "Venkataramanan, Anirudh" <anirudh.venkataramanan@...el.com>
> Suggested-by: Ira Weiny <ira.weiny@...el.com>
> Reviewed-by: Ira Weiny <ira.weiny@...el.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> ---
> 
> I'm resending this patch because some recipients were missing in the
> previous submission. In the meantime I'm also adding some more information
> in the commit message. There are no changes in the code.

Thanks for the additional information, Fabio. I've tested and applied
it.

Tyler

> 
>  fs/ecryptfs/crypto.c     | 8 ++++----
>  fs/ecryptfs/read_write.c | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
> index e3f5d7f3c8a0..03263ebcccc6 100644
> --- a/fs/ecryptfs/crypto.c
> +++ b/fs/ecryptfs/crypto.c
> @@ -465,10 +465,10 @@ int ecryptfs_encrypt_page(struct page *page)
>  	}
>  
>  	lower_offset = lower_offset_for_page(crypt_stat, page);
> -	enc_extent_virt = kmap(enc_extent_page);
> +	enc_extent_virt = kmap_local_page(enc_extent_page);
>  	rc = ecryptfs_write_lower(ecryptfs_inode, enc_extent_virt, lower_offset,
>  				  PAGE_SIZE);
> -	kunmap(enc_extent_page);
> +	kunmap_local(enc_extent_virt);
>  	if (rc < 0) {
>  		ecryptfs_printk(KERN_ERR,
>  			"Error attempting to write lower page; rc = [%d]\n",
> @@ -514,10 +514,10 @@ int ecryptfs_decrypt_page(struct page *page)
>  	BUG_ON(!(crypt_stat->flags & ECRYPTFS_ENCRYPTED));
>  
>  	lower_offset = lower_offset_for_page(crypt_stat, page);
> -	page_virt = kmap(page);
> +	page_virt = kmap_local_page(page);
>  	rc = ecryptfs_read_lower(page_virt, lower_offset, PAGE_SIZE,
>  				 ecryptfs_inode);
> -	kunmap(page);
> +	kunmap_local(page_virt);
>  	if (rc < 0) {
>  		ecryptfs_printk(KERN_ERR,
>  			"Error attempting to read lower page; rc = [%d]\n",
> diff --git a/fs/ecryptfs/read_write.c b/fs/ecryptfs/read_write.c
> index 60bdcaddcbe5..5edf027c8359 100644
> --- a/fs/ecryptfs/read_write.c
> +++ b/fs/ecryptfs/read_write.c
> @@ -64,11 +64,11 @@ int ecryptfs_write_lower_page_segment(struct inode *ecryptfs_inode,
>  
>  	offset = ((((loff_t)page_for_lower->index) << PAGE_SHIFT)
>  		  + offset_in_page);
> -	virt = kmap(page_for_lower);
> +	virt = kmap_local_page(page_for_lower);
>  	rc = ecryptfs_write_lower(ecryptfs_inode, virt, offset, size);
>  	if (rc > 0)
>  		rc = 0;
> -	kunmap(page_for_lower);
> +	kunmap_local(virt);
>  	return rc;
>  }
>  
> @@ -253,11 +253,11 @@ int ecryptfs_read_lower_page_segment(struct page *page_for_ecryptfs,
>  	int rc;
>  
>  	offset = ((((loff_t)page_index) << PAGE_SHIFT) + offset_in_page);
> -	virt = kmap(page_for_ecryptfs);
> +	virt = kmap_local_page(page_for_ecryptfs);
>  	rc = ecryptfs_read_lower(virt, offset, size, ecryptfs_inode);
>  	if (rc > 0)
>  		rc = 0;
> -	kunmap(page_for_ecryptfs);
> +	kunmap_local(virt);
>  	flush_dcache_page(page_for_ecryptfs);
>  	return rc;
>  }
> -- 
> 2.37.2
>