linux-kernel - Re: [PATCH v2 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h

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: <YmwLXvtSgl2BA2mC@iweiny-desk3>
Date:   Fri, 29 Apr 2022 08:59:26 -0700
From:   Ira Weiny <ira.weiny@...el.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
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>, linux-kernel@...r.kernel.org,
        Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
        outreachy@...ts.linux.dev,
        "Acked-by : Mike Rapoport" <rppt@...ux.ibm.com>
Subject: Re: [PATCH v2 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h

On Tue, Apr 26, 2022 at 09:01:32AM +0200, Sebastian Andrzej Siewior wrote:
> On 2022-04-25 18:23:57 [+0200], Fabio M. De Francesco wrote:
> > index a77be5630209..aa22daeed617 100644
> > --- a/include/linux/highmem-internal.h
> > +++ b/include/linux/highmem-internal.h
> > @@ -236,9 +236,17 @@ static inline unsigned long totalhigh_pages(void) { return 0UL; }
> >  
> >  #endif /* CONFIG_HIGHMEM */
> >  
> > -/*
> > - * Prevent people trying to call kunmap_atomic() as if it were kunmap()
> > - * kunmap_atomic() should get the return value of kmap_atomic, not the page.
> > +/**
> > + * kunmap_atomic - Unmap the virtual address mapped by kmap_atomic()
> > + * @__addr:       Virtual address to be unmapped
> > + *
> > + * Unmaps an address previously mapped by kmap_atomic() and re-enables
> > + * pagefaults and preemption. Mappings should be unmapped in the reverse
> 
> You mind adding "Deprecated!" like kmap_atomic() has? The part about
> disabling/ enabling preemption is true for !PREEMPT_RT. The part that
> worries me is that people use it and rely on disabled preemption like
> some did in the past. 
> I've been told this API is about to be removed (or so I have been told)
> so I hope that it will be gone soon ;)

I think some discussion needs to happen around this API.

Highmem has little use.  I don't think anyone disagrees with Linus there.
(Although I think there are still a few users out there.)

kmap may be a poor name for an API without the highmem functionality.  But
perhaps not.  One could interpret it to mean simply getting the kernel mapping
of the page rather than creating one.  After all that is what 64bit has done
all along.

This interpretation helps when you consider features which attempt to layer the
direct map with additional protections like PKS.[1]  Those protections mean
that a simple page_address() is insufficient to access the direct map.

As far as calling kmap() and kmap_atomic() deprecated I'm ok with that if the
community is.

The current kmap() call sites need work and Fabio's work on auditing them is
extremely helpful.  That said, if we officially deprecate kmap_atomic() then
those sites could be added to the list for rework.

Ira

[1] https://lore.kernel.org/lkml/20220419170649.1022246-1-ira.weiny@intel.com/

> 
> > + * order that they were mapped. See kmap_local_page() for details.
> > + * @__addr can be any address within the mapped page, so there is no need
> > + * to subtract any offset that has been added. In contrast to kunmap(),
> > + * this function takes the address returned from kmap_atomic(), not the
> > + * page passed to it. The compiler will warn you if you pass the page.
> >   */
> >  #define kunmap_atomic(__addr)					\
> >  do {								\
> 
> Sebastian