Re: [PATCH v2] xen: Convert kmap() to kmap_local_page()

[Julia Lawall <julia.lawall@xxxxxxxx>]

On Mon, 25 Apr 2022, Ira Weiny wrote:

> On Wed, Apr 20, 2022 at 04:07:36PM +0200, Fabio M. De Francesco wrote:
> > On mercoledì 20 aprile 2022 15:57:14 CEST Julia Lawall wrote:
> > >
> > > On Wed, 20 Apr 2022, Fabio M. De Francesco wrote:
> > >
> > > > 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 :)
> > >
> > > Ah, sorry for the misunderstanding.  If you have an example, I can take a
> > > look and propose something for this special case.
> > >
> > > julia
> >
> > Yes, I have the example that you are asking for. It's that commit
> > 633b0616cfe0 from Ira Weiny.
> >
> > Let me copy and paste it here for your convenience...
> >
> > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/
> > ioctl.c
> > index 90a5caf76939..2e10367ea66c 100644
> > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > @@ -604,7 +604,6 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl,
> > void __user *arg)
> >  {
> >         struct sgx_sigstruct *sigstruct;
> >         struct sgx_enclave_init init_arg;
> > -       struct page *initp_page;
> >         void *token;
> >         int ret;
> >
> > @@ -615,11 +614,15 @@ static long sgx_ioc_enclave_init(struct sgx_encl
> > *encl, void __user *arg)
> >         if (copy_from_user(&init_arg, arg, sizeof(init_arg)))
> >                 return -EFAULT;
> >
> > -       initp_page = alloc_page(GFP_KERNEL);
> > -       if (!initp_page)
> > +       /*
> > +        * 'sigstruct' must be on a page boundary and 'token' on a 512 byte
> > +        * boundary.  kmalloc() will give this alignment when allocating
> > +        * PAGE_SIZE bytes.
> > +        */
> > +       sigstruct = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > +       if (!sigstruct)
> >                 return -ENOMEM;
> >
> > -       sigstruct = kmap(initp_page);
> >         token = (void *)((unsigned long)sigstruct + PAGE_SIZE / 2);
> >         memset(token, 0, SGX_LAUNCH_TOKEN_SIZE);
> >
> > @@ -645,8 +648,7 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl,
> > void __user *arg)
> >         ret = sgx_encl_init(encl, sigstruct, token);
> >
> >  out:
> > -       kunmap(initp_page);
> > -       __free_page(initp_page);
> > +       kfree(sigstruct);
> >         return ret;
> >  }
> >
> > I think that Coccinelle might understand that "initp_page" is allocated in
> > the same function where later it is kmap()'ed. But I'm not able to write a
> > Coccinelle check to find out these kinds of special cases. In these cases
> > the correct solution is not to use kmap_local_page(). Instead delete the
> > alloc_page() and use kmalloc().
> >
>
> Sorry about missing this thread last week...
>
> I've lost the Coccinelle scripts I wrote before but the ones which helped were
> documented in patches I submitted when Coccinelle was used.
>
> I think Coccinelle can help a lot.  And probably a lot more than I know since
> I'm not an expert in the language either.
>
> However, In addition to the example Fabio shows above here are a few other
> things to look out for when writing Coccinelle scripts.
>
> 1) The addition of mem*_page() functions means sometimes the entire 
> kmap/kunmap
>    can be removed.  Check out the Coccinelle script for that.[1]
>
> 2) kunmap_local() has ordering rules which often requires some manual
>    review.[2]
>
> 3) kmap/kunmap is often wrapped in other subsystem helper functions.  I was 
> not
>    sure how to deal with that in Coccinelle.  Julia is this easy in
>    Coccinelle?[3]


Thanks for the pointers.

[3] would depend on how much risk you are willing to take.  The arguments
are the same, except for the array index, for example.  But one may prefer
to be sure that nothing complex happens between the two calls.

julia

>
>
> Ira
>
>
> [1]
> https://lore.kernel.org/lkml/20210205232304.1670522-3-ira.weiny@xxxxxxxxx/
> https://lore.kernel.org/lkml/20210205232304.1670522-5-ira.weiny@xxxxxxxxx/
>
> [2]
> https://lore.kernel.org/lkml/20210217024826.3466046-3-ira.weiny@xxxxxxxxx/
> https://lore.kernel.org/lkml/20210217024826.3466046-4-ira.weiny@xxxxxxxxx/
>
> [3]
> https://lore.kernel.org/lkml/20210217024826.3466046-5-ira.weiny@xxxxxxxxx/
>
> >
> > Thanks,
> >
> > Fabio
> >
> >
> >
>
>