linux-kernel - Re: [RESEND PATCH] fs/aio: Replace kmap{,_atomic}() 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]

Date:   Wed, 19 Oct 2022 20:52:04 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Jeff Moyer <jmoyer@...hat.com>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Benjamin LaHaise <bcrl@...ck.org>,
        linux-fsdevel@...r.kernel.org, linux-aio@...ck.org,
        linux-kernel@...r.kernel.org,
        "Venkataramanan, Anirudh" <anirudh.venkataramanan@...el.com>,
        Ira Weiny <ira.weiny@...el.com>
Subject: Re: [RESEND PATCH] fs/aio: Replace kmap{,_atomic}() with kmap_local_page()

On Wednesday, October 19, 2022 5:41:21 PM CEST Jeff Moyer wrote:
> "Fabio M. De Francesco" <fmdefrancesco@...il.com> writes:
> 
> > The use of kmap() and kmap_atomic() are being deprecated in favor of
> > kmap_local_page().
> >
> > There are two main problems with kmap(): (1) It comes with an overhead as
> > the 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.
> >
> > With kmap_local_page() the mappings are per thread, CPU local, can take
> > page faults, and can be called from any context (including interrupts).
> > It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore,
> > the tasks can be preempted and, when they are scheduled to run again, the
> > kernel virtual addresses are restored and still valid.
> >
> > Since its use in fs/aio.c is safe everywhere, it should be preferred.
> 
> That sentence is very ambiguous.  I don't know what "its" refers to, and
> I'm not sure what "safe" means in this context.

I'm sorry for not being clearer.

"its use" means "the use of kmap_local_page()". Few lines above you may also 
see "It is faster", meaning "kmap_local_page() is faster".

The "safety" is a very concise way to assert that I've checked, by code 
inspection and by testing (as it is better detailed some lines below) that 
these conversions (1) don't break any of the rules of use of local mapping 
when converting kmap() (please read highmem.rst about these) and (2) the call 
sites of kmap_atomic() didn't rely on its side effects (pagefaults disable and 
potential preemption disables). 

Therefore, you may read it as it was: "The use of kmap_local_page() in fs/
aio.c has been carefully checked to assure that the conversions won't break 
the code, therefore the newer API is preferred".

I hope it makes my argument clearer.

> 
> The patch looks okay to me.
> 
> Reviewed-by: Jeff Moyer <jmoyer@...hat.com>
> 

Thank you so much for the  "Reviewed-by" tag.

Regards,

Fabio