linux-kernel - Re: [PATCH v2] xen: Convert kmap() to 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: <3990312.6PsWsQAL7t@leap>
Date:   Wed, 20 Apr 2022 15:55:34 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Julia Lawall <julia.lawall@...ia.fr>, ira.weiny@...el.com
Cc:     Alaa Mohamed <eng.alaamohamedsoliman.am@...il.com>,
        outreachy@...ts.linux.dev, boris.ostrovsky@...cle.com,
        jgross@...e.com, sstabellini@...nel.org,
        xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] xen:  Convert kmap() to kmap_local_page()

On mercoledì 20 aprile 2022 15:40:10 CEST Julia Lawall wrote:
> 
> On Wed, 20 Apr 2022, Fabio M. De Francesco wrote:
> 
> > On mercoledì 20 aprile 2022 08:03:05 CEST Julia Lawall wrote:
> > >
> > > On Wed, 20 Apr 2022, Alaa Mohamed wrote:
> > >
> > > > kmap() is being deprecated and these usages are all local to the 
thread
> > > > so there is no reason kmap_local_page() can't be used.
> > > >
> > > > Replace kmap() calls with kmap_local_page().
> > >
> > > OK, so from a Coccinelle point of view, could we do
> > >
> > > @@
> > > expression e1,e2,x,f;
> > > @@
> > >
> > > e1 =
> > > - kmap
> > > + kmap_local_page
> > >     (e2)
> > > ... when != x = e1 // not stored in any location and not passed to
> > another function
> > >     when != f(...,e1,...)
> > >     when != x = e2
> > >     when != f(...,e2,...)
> > > -kunmap(e2)
> > > +kunmap_local(e1)
> > >
> > > julia
> > >
> >
> > I've never spent sufficient time to understand properly the syntax and
> > semantics of expressions of Coccinelle. However, thanks Julia, this 
code
> > looks good and can be very helpful.
> >
> > Only a minor objection... it doesn't tell when 'e2' has been allocated
> > within the same function where the kmap() call is.
> >
> > In the particular case that I cite above, I'd prefer to remove the
> > allocation of the page (say with alloc_page()) and convert kmap() /
kunmap()
> > to use kmalloc() / kfree().
> >
> > Fox example, this is done in the following patch:
> >
> > commit 633b0616cfe0 ("x86/sgx: Remove unnecessary kmap() from
> > sgx_ioc_enclave_init()") from Ira Weiny.
> >
> > Can Coccinelle catch also those special cases where a page that is 
passed
> > to kmap() is allocated within that same function (vs. being passed as
> > argument to this function) and, if so, propose a replacement with
> > kmalloc()?
> 
> It looks complex in this case, because the allocation is in another
> function, and it is passed to another function.

This is not the special case I was talking about. In this case your code 
for Coccinelle tells the right proposal and it is exactly what Alaa did in 
her patch (which is good!).

I'm talking about other special cases like the one I pointed to with the 
link I provided. I'm sorry if my bad English made you think that Alaa's 
patch was one of those cases where the page is allocated within the same 
function where kmap() is.

I hope that now I've been clearer :)

Thanks,

Fabio