linux-kernel - Re: [PATCH v3 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section

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]

Date:   Thu, 28 Apr 2022 11:02:10 +0200
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     Ira Weiny <ira.weiny@...el.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Catalin Marinas <catalin.marinas@....com>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Will Deacon <will@...nel.org>,
        Peter Collingbourne <pcc@...gle.com>,
        Vlastimil Babka <vbabka@...e.cz>, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, outreachy@...ts.linux.dev,
        Jonathan Corbet <corbet@....net>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v3 4/4] Documentation/vm: Rework "Temporary Virtual
 Mappings" section

On 2022-04-27 20:38:21 [+0200], Fabio M. De Francesco wrote:
> index e05bf5524174..c8aff448612b 100644
> --- a/Documentation/vm/highmem.rst
> +++ b/Documentation/vm/highmem.rst
> @@ -50,26 +50,78 @@ space when they use mm context tags.
…
>  
> -* kmap().  This permits a short duration mapping of a single page.  It needs
> -  global synchronization, but is amortized somewhat.  It is also prone to
> -  deadlocks when using in a nested fashion, and so it is not recommended for
> -  new code.
> +  These mappings are thread-local and CPU-local (i.e., migration from one CPU
> +  to another is disabled - this is why they are called "local"), but they don't
> +  disable preemption. 

So if you replace this block with

   These mappings are thread-local and CPU-local meaning that the mapping
   can only be accessed from within this thread and the thread is bound the
   CPU while the mapping is active. Even if the thread is preempted (since
   preemption is never disabled by the function) the CPU can not be
   unplugged from the system via CPU-hotplug until the mapping is disposed.

The you could drop the latter block

>                          It's valid to take pagefaults in a local kmap region,
> +  unless the context in which the local mapping is acquired does not allow it
> +  for other reasons.

> +  kmap_local_page() always returns a valid virtual address and it is assumed
> +  that kunmap_local() will never fail.

from here

> +  If a task holding local kmaps is preempted, the maps are removed on context
> +  switch and restored when the task comes back on the CPU. The maps are
> +  strictly thread-local and CPU-local, therefore it is guaranteed that the
> +  task stays on the CPU and the CPU cannot be unplugged until the local kmaps
> +  are released.

to here since it mostly the same thing.

> +  Nesting kmap_local_page() and kmap_atomic() mappings is allowed to a certain
> +  extent (up to KMAP_TYPE_NR) but their invocations have to be strictly ordered
> +  because the map implementation is stack based. See kmap_local_page () kdocs

kmap_local_page () => kmap_local_page()

> +  (included in the "Functions" section) for details on how to manage nested
> +  mappings.
>  
>  * kmap_atomic().  This permits a very short duration mapping of a single
>    page.  Since the mapping is restricted to the CPU that issued it, it
>    performs well, but the issuing task is therefore required to stay on that
>    CPU until it has finished, lest some other task displace its mappings.
>  
> -  kmap_atomic() may also be used by interrupt contexts, since it is does not
> -  sleep and the caller may not sleep until after kunmap_atomic() is called.
> +  kmap_atomic() may also be used by interrupt contexts, since it does not
> +  sleep and the callers too may not sleep until after kunmap_atomic() is
> +  called.
> +
> +  Each call of kmap_atomic() in the kernel creates a non-preemptible section
> +  and disable pagefaults. This could be a source of unwanted latency, so it
> +  should be only used if it is absolutely required, otherwise kmap_local_page()
> +  should be used where it is feasible.

I'm not keen about the "absolutely required" wording and "feasible".
That said, the other pieces look good, thank you for the work.

> -  It may be assumed that k[un]map_atomic() won't fail.
> +  It is assumed that k[un]map_atomic() won't fail.

Sebastian