[PATCH] efi: Use GFP_ATOMIC instead of GFP_KERNEL

[PATCH] efi: Use GFP_ATOMIC instead of GFP_KERNEL

[Vaishali Thakkar]

Function dup_variable_bug is called inside the spinlock.
This may lead to issues when kzalloc sleeps, so it is
better to use GFP_ATOMIC in this spinlocked context.

Problem found using Coccinelle.

Signed-off-by: Vaishali Thakkar <vaishali.thakkar@oracle.com>
---
 drivers/firmware/efi/vars.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 0ac594c..d5e2f28 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -411,7 +411,7 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
 	 */
 	efivar_wq_enabled = false;
 
-	str8 = kzalloc(len8, GFP_KERNEL);
+	str8 = kzalloc(len8, GFP_ATOMIC);
 	if (!str8)
 		return;
 
-- 
2.1.4

Re: [PATCH] efi: Use GFP_ATOMIC instead of GFP_KERNEL

[Matt Fleming]

On Mon, 11 Apr, at 04:23:29PM, Vaishali Thakkar wrote:
> Function dup_variable_bug is called inside the spinlock.
> This may lead to issues when kzalloc sleeps, so it is
> better to use GFP_ATOMIC in this spinlocked context.
> 
> Problem found using Coccinelle.
 
Dang it, I broke coccinelle ;)

> Signed-off-by: Vaishali Thakkar <vaishali.thakkar@oracle.com>
> ---
>  drivers/firmware/efi/vars.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index 0ac594c..d5e2f28 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -411,7 +411,7 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
>  	 */
>  	efivar_wq_enabled = false;
>  
> -	str8 = kzalloc(len8, GFP_KERNEL);
> +	str8 = kzalloc(len8, GFP_ATOMIC);
>  	if (!str8)
>  		return;
>  

This was brought up by Saurabh last year,

  https://lkml.kernel.org/r/1446003747-3760-1-git-send-email-saurabh.truth@gmail.com

dup_variable_bug() is never called while holding the spinlock in
practice, and I'm guessing Coccinelle cannot understand that because
it'd need to look at program control flow, across multiple compilation
units.

If anyone wants to send a patch to clean up the EFI code so that it's
easier for coccinelle to check it, I'd be happy to review it.

Re: [PATCH] efi: Use GFP_ATOMIC instead of GFP_KERNEL

[Julia Lawall]

On Thu, 14 Apr 2016, Matt Fleming wrote:

> On Mon, 11 Apr, at 04:23:29PM, Vaishali Thakkar wrote:
> > Function dup_variable_bug is called inside the spinlock.
> > This may lead to issues when kzalloc sleeps, so it is
> > better to use GFP_ATOMIC in this spinlocked context.
> > 
> > Problem found using Coccinelle.
>  
> Dang it, I broke coccinelle ;)
> 
> > Signed-off-by: Vaishali Thakkar <vaishali.thakkar@oracle.com>
> > ---
> >  drivers/firmware/efi/vars.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > index 0ac594c..d5e2f28 100644
> > --- a/drivers/firmware/efi/vars.c
> > +++ b/drivers/firmware/efi/vars.c
> > @@ -411,7 +411,7 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
> >  	 */
> >  	efivar_wq_enabled = false;
> >  
> > -	str8 = kzalloc(len8, GFP_KERNEL);
> > +	str8 = kzalloc(len8, GFP_ATOMIC);
> >  	if (!str8)
> >  		return;
> >  
> 
> This was brought up by Saurabh last year,
> 
>   https://lkml.kernel.org/r/1446003747-3760-1-git-send-email-saurabh.truth@gmail.com
> 
> dup_variable_bug() is never called while holding the spinlock in
> practice, and I'm guessing Coccinelle cannot understand that because
> it'd need to look at program control flow, across multiple compilation
> units.
> 
> If anyone wants to send a patch to clean up the EFI code so that it's
> easier for coccinelle to check it, I'd be happy to review it.

I looked at it a bit with Vaishali.  I wonder if it would be possible at 
least to have only one flag?  Then one wouldn't have to maintain the 
subtle relationship between atomic and duplicates.  I'm not sure that it 
would help Coccinelle, but at least one could see more quickly that 
Coccinelle is giving a false positive.

julia

Re: [PATCH] efi: Use GFP_ATOMIC instead of GFP_KERNEL

[Matt Fleming]

On Fri, 15 Apr, at 08:38:37AM, Julia Lawall wrote:
> 
> I looked at it a bit with Vaishali.  I wonder if it would be possible at 
> least to have only one flag? Then one wouldn't have to maintain the 
> subtle relationship between atomic and duplicates.  I'm not sure that it 
> would help Coccinelle, but at least one could see more quickly that 
> Coccinelle is giving a false positive.

Yeah, that would be a good idea.

How about we drop the @atomic parameter and simply use @duplicates to
figure out whether to perform duplicate detection, which we should
note in the comment of efivar_init() cannot be performed atomically.
Bonus points if someone can clean up the code flow too.

Otherwise, efivar_init() is done while holding a spinlock.

Re: [PATCH] efi: Use GFP_ATOMIC instead of GFP_KERNEL

[Vaishali Thakkar]

On Monday 18 April 2016 02:48 AM, Matt Fleming wrote:
> On Fri, 15 Apr, at 08:38:37AM, Julia Lawall wrote:
>> I looked at it a bit with Vaishali.  I wonder if it would be possible at 
>> least to have only one flag? Then one wouldn't have to maintain the 
>> subtle relationship between atomic and duplicates.  I'm not sure that it 
>> would help Coccinelle, but at least one could see more quickly that 
>> Coccinelle is giving a false positive.
> Yeah, that would be a good idea.
>
> How about we drop the @atomic parameter and simply use @duplicates to
> figure out whether to perform duplicate detection, which we should
> note in the comment of efivar_init() cannot be performed atomically.
> Bonus points if someone can clean up the code flow too.
I think using only @duplicates would be helpful to make code
more clear. I can actually clean up the code flow but it may take
some time for me to send the patches as I  already have some other
work with me.

Do you want to go for it? Or for now we can just go for dropping @atomic
parameter. 

> Otherwise, efivar_init() is done while holding a spinlock.

-- 
Vaishali