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
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
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().
Thanks,
Fabio
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]
Ira
[1]
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
[2]
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
[3]
https://lore.kernel.org/lkml/[email protected]/
>
> Thanks,
>
> Fabio
>
>
>