linux-kernel - Re: [PATCH 3/7] Documentation/mm: Don't kmap*() pages which can't come from HIGHMEM

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: <Ytny132kWjXvu1Ql@iweiny-desk3>
Date:   Thu, 21 Jul 2022 17:44:07 -0700
From:   Ira Weiny <ira.weiny@...el.com>
To:     Jonathan Corbet <corbet@....net>
CC:     "Fabio M. De Francesco" <fmdefrancesco@...il.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>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        <linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 3/7] Documentation/mm: Don't kmap*() pages which can't
 come from HIGHMEM

On Thu, Jul 21, 2022 at 03:13:00PM -0600, Jonathan Corbet wrote:
> "Fabio M. De Francesco" <fmdefrancesco@...il.com> writes:
> 
> > There is no need to kmap*() pages which are guaranteed to come from
> > ZONE_NORMAL (or lower). Linux has currently several call sites of
> > kmap{,_atomic,_local_page}() on pages allocated, for instance, with
> > alloc_page(GFP_NOFS) and other similar allocations.
> >
> > Therefore, add a paragraph to highmem.rst, to explain better that a
> > plain page_address() should be used for getting the address of pages
> > which cannot come from ZONE_HIGHMEM.
> >
> > Cc: Andrew Morton <akpm@...ux-foundation.org>
> > Cc: Matthew Wilcox (Oracle) <willy@...radead.org>
> > Cc: Mike Rapoport <rppt@...ux.ibm.com>
> > Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> > Cc: Thomas Gleixner <tglx@...utronix.de>
> > Suggested-by: Ira Weiny <ira.weiny@...el.com>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> > ---
> >  Documentation/vm/highmem.rst | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/vm/highmem.rst b/Documentation/vm/highmem.rst
> > index c9887f241c6c..f266354c82ab 100644
> > --- a/Documentation/vm/highmem.rst
> > +++ b/Documentation/vm/highmem.rst
> > @@ -71,6 +71,12 @@ list shows them in order of preference of use.
> >    kmap_local_page() always returns a valid virtual address and it is assumed
> >    that kunmap_local() will never fail.
> >  
> > +  On CONFIG_HIGHMEM=n kernels and for low memory pages this returns the
> > +  virtual address of the direct mapping. Only real highmem pages are
> > +  temporarily mapped. Therefore, users should instead call a plain
> > +  page_address() for getting the address of memory pages which, depending
> > +  on the GFP_* flags, cannot come from ZONE_HIGHMEM.
> > +
> 
> Is this good advice?  First, it requires developers to worry about
> whether their pages might be in highmem, which is kind of like worrying
> about having coins in your pocket in case you need a payphone.

This is a good point.  Perhaps this is better worded as:

	On CONFIG_HIGHMEM=n kernels and for low memory pages this returns the
	virtual address of the direct mapping. Only real highmem pages are
	temporarily mapped.  Therefore, users may call a plain page_address()
	for pages which are known to not come from ZONE_HIGHMEM.

	However, it is always safe to use kmap_local_page()/kunmap_local() and
	access through those calls will be as efficient as page_address() on
	most architectures.

> But it
> would also run afoul of other semantics for kmap*(), such as PKS, should
> that ever be merged:
> 
>   https://lwn.net/Articles/894531/

PKS is yet to be merged.  As of now, there is no good reason to force users to
use the kmap_local_page() if the page zone is known.

I believe that beyond PKS there will come a time when we need to change the
page_address() callers but currently this documentation is correct and does
allow callers to optimize for the corner case of a HIGHMEM system if they
desire.

As a reference, the use of kmap* vs page_address() was discussed recently in a
couple of places[1][2] and I can't fault the logic there at this time.

Thanks for the review,
Ira

[1] https://lore.kernel.org/linux-btrfs/20220621131521.GW20633@twin.jikos.cz/
[2] https://lore.kernel.org/lkml/CANn89iK6g+4Fy2VMV7=feUAOUDHu-J38be+oU76yp+zGH6xCJQ@mail.gmail.com/

> 
> Thanks,
> 
> jon