| 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
| ||
|
Message-ID: <20220822092625.ag2naoeuhhrq6pqz@quack3>
Date: Mon, 22 Aug 2022 11:26:25 +0200
From: Jan Kara <jack@...e.cz>
To: "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc: "Matthew Wilcox (Oracle)" <willy@...radead.org>,
Jan Kara <jack@...e.cz>,
Muchun Song <songmuchun@...edance.com>,
Jens Axboe <axboe@...nel.dk>,
Bart Van Assche <bvanassche@....org>,
linux-kernel@...r.kernel.org,
Andrew Morton <akpm@...ux-foundation.org>,
Ira Weiny <ira.weiny@...el.com>,
kernel test robot <lkp@...el.com>
Subject: Re: [RESEND PATCH v2] fs/isofs: Replace kmap() with kmap_local_page()
On Sun 21-08-22 19:50:12, 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
> 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).
> Tasks can be preempted and, when scheduled to run again, the kernel
> virtual addresses are restored and still valid. It is faster than kmap()
> in kernels with HIGHMEM enabled.
>
> Since kmap_local_page() can be safely used in compress.c, it should be
> called everywhere instead of kmap().
>
> Therefore, replace kmap() with kmap_local_page() in compress.c. Where it
> is needed, use memzero_page() instead of open coding kmap_local_page()
> plus memset() to fill the pages with zeros. Delete the redundant
> flush_dcache_page() in the two call sites of memzero_page().
>
> Tested with mkisofs on a QEMU/KVM x86_32 VM, 6GB RAM, booting a kernel
> with HIGHMEM64GB enabled.
>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Jan Kara <jack@...e.cz>
> Suggested-by: Ira Weiny <ira.weiny@...el.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
Thanks for the patch! It looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@...e.cz>
Honza
> ---
>
> Some days ago Andrew requested a resend of this patch.[1]
>
> v1->v2: Cast zisofs_sink_page to pointer to unsigned char.
> Reported-by: kernel test robot <lkp@...el.com>
>
> Many thanks to Jan Kara for the comments and suggestions provided with
> replying to my previous RFC.[2] Furthermore, I want to thank Ira Weiny for
> the advices he provided, especially about how to use mkisofs to test that
> this patch is working properly.
>
> [1] https://lore.kernel.org/all/20220801122709.8164-1-fmdefrancesco@gmail.com/
> [2] https://lore.kernel.org/lkml/20220726145024.rryvw7ot7j2c6tqv@quack3/
>
> fs/isofs/compress.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/fs/isofs/compress.c b/fs/isofs/compress.c
> index 95a19f25d61c..107007c38d08 100644
> --- a/fs/isofs/compress.c
> +++ b/fs/isofs/compress.c
> @@ -67,8 +67,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, loff_t block_start,
> for ( i = 0 ; i < pcount ; i++ ) {
> if (!pages[i])
> continue;
> - memset(page_address(pages[i]), 0, PAGE_SIZE);
> - flush_dcache_page(pages[i]);
> + memzero_page(pages[i], 0, PAGE_SIZE);
> SetPageUptodate(pages[i]);
> }
> return ((loff_t)pcount) << PAGE_SHIFT;
> @@ -120,7 +119,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, loff_t block_start,
> zerr != Z_STREAM_END) {
> if (!stream.avail_out) {
> if (pages[curpage]) {
> - stream.next_out = page_address(pages[curpage])
> + stream.next_out = kmap_local_page(pages[curpage])
> + poffset;
> stream.avail_out = PAGE_SIZE - poffset;
> poffset = 0;
> @@ -176,6 +175,10 @@ static loff_t zisofs_uncompress_block(struct inode *inode, loff_t block_start,
> flush_dcache_page(pages[curpage]);
> SetPageUptodate(pages[curpage]);
> }
> + if (stream.next_out != (unsigned char *)zisofs_sink_page) {
> + kunmap_local(stream.next_out);
> + stream.next_out = NULL;
> + }
> curpage++;
> }
> if (!stream.avail_in)
> @@ -183,6 +186,8 @@ static loff_t zisofs_uncompress_block(struct inode *inode, loff_t block_start,
> }
> inflate_out:
> zlib_inflateEnd(&stream);
> + if (stream.next_out && stream.next_out != (unsigned char *)zisofs_sink_page)
> + kunmap_local(stream.next_out);
>
> z_eio:
> mutex_unlock(&zisofs_zlib_lock);
> @@ -283,9 +288,7 @@ static int zisofs_fill_pages(struct inode *inode, int full_page, int pcount,
> }
>
> if (poffset && *pages) {
> - memset(page_address(*pages) + poffset, 0,
> - PAGE_SIZE - poffset);
> - flush_dcache_page(*pages);
> + memzero_page(*pages, poffset, PAGE_SIZE - poffset);
> SetPageUptodate(*pages);
> }
> return 0;
> @@ -343,10 +346,8 @@ static int zisofs_read_folio(struct file *file, struct folio *folio)
> for (i = 0; i < pcount; i++, index++) {
> if (i != full_page)
> pages[i] = grab_cache_page_nowait(mapping, index);
> - if (pages[i]) {
> + if (pages[i])
> ClearPageError(pages[i]);
> - kmap(pages[i]);
> - }
> }
>
> err = zisofs_fill_pages(inode, full_page, pcount, pages);
> @@ -357,7 +358,6 @@ static int zisofs_read_folio(struct file *file, struct folio *folio)
> flush_dcache_page(pages[i]);
> if (i == full_page && err)
> SetPageError(pages[i]);
> - kunmap(pages[i]);
> unlock_page(pages[i]);
> if (i != full_page)
> put_page(pages[i]);
> --
> 2.37.1
>
--
Jan Kara <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists