linux-kernel - Re: [PATCH v2] kexec: 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]

Date:   Tue, 02 Aug 2022 13:02:25 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Baoquan He <bhe@...hat.com>
Cc:     Eric Biederman <ebiederm@...ssion.com>, kexec@...ts.infradead.org,
        akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
        Ira Weiny <ira.weiny@...el.com>
Subject: Re: [PATCH v2] kexec: Replace kmap() with kmap_local_page()

On martedì 2 agosto 2022 09:32:46 CEST Baoquan He wrote:
> On 08/02/22 at 08:20am, Fabio M. De Francesco wrote:
> > On martedì 2 agosto 2022 05:06:54 CEST Baoquan He wrote:
> > > On 07/08/22 at 01:15am, Fabio M. De Francesco wrote:
> > > > The use of kmap() and kmap_atomic() are being deprecated in favor 
of
> > > > kmap_local_page().
> > > > 
> > > > With kmap_local_page(), the mappings are per thread, CPU local and 
not
> > > > globally visible. Furthermore, the mappings can be acquired from 
any
> > > > context (including interrupts).
> > > > 
> > > > Therefore, use kmap_local_page() in kexec_core.c because these 
mappings 
> > are
> > > > per thread, CPU local, and not globally visible.
> > > > 
> > > > Tested on a QEMU + KVM 32-bits VM booting a kernel with HIGHMEM64GB
> > > > enabled.
> > > 
> > > Wondering what arch you tested with.
> > 
> > I'm sorry, I forgot to say that I use x86_32 with 4GB to 6GB RAM.
> > This is usually an information that I add in the commit messages of all 
the 
> > recent conversions I'm working on across the entire kernel.
> > 
> > Another important information (overlooked again this time) is that (1) 
> > kmap() 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.
> 
> Thanks a lot for update with more details, Fabio.

You're welcome.

> Maybe these can be
> added into log with v3 if you think they are worth adding, and with my
> Ack since no code change related. You decide.

For weeks I've been thinking that these details were not necessary in the 
commit message of these conversions. Later I realized that a fair number of 
maintainers weren't aware of why we should change something that had been 
working "properly" for years. Perhaps they thought that we would face too 
many risks of breaking things versus little rewards.

After discussing with Greg K-H (who was initially not fond of these changes 
to core parts of firmware loading mechanism), he suggested that a proper 
commit log with the necessary information could have helped himself and the 
other maintainers who may not yet had time to deepen this topics.

This patch to "kexec" slipped in with no such additions.

Therefore, I agree with you in full: I'll add these information and send a 
new version ASAP.

> Thanks again for the effort.

Thanks so much for the "Acked-by:" tag from you.

Fabio  

[snip]