linux-kernel - Re: [PATCH] module: 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: <5303077.Sb9uPGUboI@opensuse>
Date:   Tue, 19 Jul 2022 11:19:24 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Matthew Wilcox <willy@...radead.org>, Song Liu <song@...nel.org>,
        Takashi Iwai <tiwai@...e.de>,
        Adam Manzanares <a.manzanares@...sung.com>,
        Davidlohr Bueso <dave@...olabs.net>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Ira Weiny <ira.weiny@...el.com>
Cc:     linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] module: Replace kmap() with kmap_local_page()

On martedì 19 luglio 2022 00:19:50 CEST Luis Chamberlain wrote:
> On Mon, Jul 18, 2022 at 02:26:45AM +0200, Fabio M. De Francesco 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.
> 
> Neat!
> 

Thanks!

> > With kmap_local_page() the mappings are per thread, CPU local, can take
> > page faults, and can be called from any context (including interrupts).
> 
> Yes but the mapping can only be accessed from within this thread and the
> thread is bound to the CPU while the mapping is active. So, what if we
> get a preemption during decompression here? What happens?

No problems at all, please read commit f3ba3c710ac5 ("mm/highmem: Provide 
kmap_local*") from Thomas Gleixner:

"[] Now that the kmap atomic index is stored in task struct provide a
preemptible variant. On context switch the maps of an outgoing task are
removed and the map of the incoming task are restored. That's obviously
slow, but highmem is slow anyway.

The kmap_local.*() functions can be invoked from both preemptible and
atomic context. kmap local sections disable migration to keep the resulting
virtual mapping address correct, but disable neither pagefaults nor
preemption.".

However, yours is a good question. It made me realize that this specific 
topic is not discussed properly in highmem.rst (which also embeds kdocs 
from highmem.h and highmem-internal.h).

Some weeks ago I reworked highmem.rst and, within a couple of days, I'll 
also send a second round of additions and clean-ups to the documentation of 
kmap_local_page(). 

I'll use this opportunity to explain more clearly what happens to the local 
mappings when a task is preempted and then is restored. It looks like 
currently this documentation lacks clarity about the fact that tasks which 
use pointers returned by kmap_local_page() _can_ be preempted and that 
these pointers are still valid when the tasks are rescheduled to run again.

In the meantime, I'll submit a v2 of this patch with a slightly extended 
commit message which informs about this specific topic.

Again thanks for pointing it out that the commit message of this patch 
should be more clear about what happens with preemption.

> > It is faster than kmap() in kernels with HIGHMEM enabled. Its use in
> > module_gzip_decompress() and in module_xz_decompress() is safe and
> > should be preferred.
> > 
> > Therefore, replace kmap() with kmap_local_page().
> 
> While this churn is going on everywhere I was wondering why not
> go ahead and adopt kmap_local_folio() instead?

I'm sorry but, due to my lack of knowledge and experience, I'm not sure to 
understand how kmap_local_folio() could help here. My fault. I'm going to 
make some research and ask for help from more experienced developers. 

>   Luis
> 
> > Tested on a QEMU/KVM x86_32 VM with 4GB RAM, booting kernels with
> > HIGHMEM64GB enabled. Modules compressed with XZ or GZIP decompress
> > properly.
> > 
> > Suggested-by: Ira Weiny <ira.weiny@...el.com>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> > ---
> >  kernel/module/decompress.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)

[snip]

Thanks,

Fabio