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

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

[Alaa Mohamed]

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().

Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
---
changes in V2:
	-edit commit subject
	-edit commit message
---
 drivers/xen/gntalloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index 4849f94372a4..55acb32842a3 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -178,9 +178,9 @@ static void __del_gref(struct gntalloc_gref *gref)
 	unsigned long addr;
 
 	if (gref->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) {
-		uint8_t *tmp = kmap(gref->page);
+		uint8_t *tmp = kmap_local_page(gref->page);
 		tmp[gref->notify.pgoff] = 0;
-		kunmap(gref->page);
+		kunmap_local(tmp);
 	}
 	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
 		notify_remote_via_evtchn(gref->notify.event);
-- 
2.35.2

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

[Julia Lawall]

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

>
> Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
> ---
> changes in V2:
> 	-edit commit subject
> 	-edit commit message
> ---
>  drivers/xen/gntalloc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
> index 4849f94372a4..55acb32842a3 100644
> --- a/drivers/xen/gntalloc.c
> +++ b/drivers/xen/gntalloc.c
> @@ -178,9 +178,9 @@ static void __del_gref(struct gntalloc_gref *gref)
>  	unsigned long addr;
>
>  	if (gref->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) {
> -		uint8_t *tmp = kmap(gref->page);
> +		uint8_t *tmp = kmap_local_page(gref->page);
>  		tmp[gref->notify.pgoff] = 0;
> -		kunmap(gref->page);
> +		kunmap_local(tmp);
>  	}
>  	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
>  		notify_remote_via_evtchn(gref->notify.event);
> --
> 2.35.2
>
>
>

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

[Fabio M. De Francesco]

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()?

Thanks,

Fabio

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

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1938 bytes --]



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.

OK, thanks for pointing that out.  That seems like a key point that should
be mentioned in log messages.  It's not even visible in the context lines
around the patch.

>
> 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()?

Sure.  I'll take a look at the precise example Alaa started with and see
if I can come up with something.

thanks,
julia

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

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1803 bytes --]



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.

julia


>
> Thanks,
>
> Fabio
>
>
>
>

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

[Fabio M. De Francesco]

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

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

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 2722 bytes --]



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

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

[Fabio M. De Francesco]

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

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

[Boris Ostrovsky]

On 4/19/22 7:43 PM, 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().
>
> Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>


Applied to for-linus-5.18. (Juergen, I kept your R-b from v1)


-boris

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

[Ira Weiny]

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/20210205232304.1670522-3-ira.weiny@intel.com/
https://lore.kernel.org/lkml/20210205232304.1670522-5-ira.weiny@intel.com/

[2]
https://lore.kernel.org/lkml/20210217024826.3466046-3-ira.weiny@intel.com/
https://lore.kernel.org/lkml/20210217024826.3466046-4-ira.weiny@intel.com/

[3]
https://lore.kernel.org/lkml/20210217024826.3466046-5-ira.weiny@intel.com/

> 
> Thanks,
> 
> Fabio
> 
> 
> 

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

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 7139 bytes --]



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@intel.com/
> https://lore.kernel.org/lkml/20210205232304.1670522-5-ira.weiny@intel.com/
>
> [2]
> https://lore.kernel.org/lkml/20210217024826.3466046-3-ira.weiny@intel.com/
> https://lore.kernel.org/lkml/20210217024826.3466046-4-ira.weiny@intel.com/
>
> [3]
> https://lore.kernel.org/lkml/20210217024826.3466046-5-ira.weiny@intel.com/
>
> >
> > Thanks,
> >
> > Fabio
> >
> >
> >
>
>