[PATCH] drivers: base: power: Fix GFP_KERNEL in spinlock context

[PATCH] drivers: base: power: Fix GFP_KERNEL in spinlock context

[shrikant.maurya]

From: Shrikant Maurya <shrikant.maurya@techveda.org>

As reported by Jia-Ju Bai (https://lkml.org/lkml/2017/12/11/872):
API's are using GFP_KERNEL to allocate memory which may sleep.

To ensure atomicity such allocations must be avoided in critical
sections under spinlock.
Fixed by replacing GFP_KERNEL to GFP_ATOMIC.

Reported-by: Jia-Ju Bai <baijiaju1990@gmail.com>
Signed-off-by: Shrikant Maurya <shrikant.maurya@techveda.org>
Signed-off-by: Suniel Mahesh <sunil.m@techveda.org>
Signed-off-by: Raghu Bharadwaj <raghu@techveda.org>
Signed-off-by: Karthik Tummala <karthik@techveda.org>
---
Note:
- Patch was compile tested and built(ARCH=arm) on linux-next
  (latest).
- No build issues reported.
---
 drivers/base/power/wakeup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 38559f0..de56952 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -92,11 +92,11 @@ struct wakeup_source *wakeup_source_create(const char *name)
 {
 	struct wakeup_source *ws;
 
-	ws = kmalloc(sizeof(*ws), GFP_KERNEL);
+	ws = kmalloc(sizeof(*ws), GFP_ATOMIC);
 	if (!ws)
 		return NULL;
 
-	wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_KERNEL) : NULL);
+	wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_ATOMIC) : NULL);
 	return ws;
 }
 EXPORT_SYMBOL_GPL(wakeup_source_create);
-- 
1.9.1

Re: [PATCH] drivers: base: power: Fix GFP_KERNEL in spinlock context

[Geert Uytterhoeven]

Hi Shrikant,

On Tue, Dec 12, 2017 at 2:45 PM,  <shrikant.maurya@techveda.org> wrote:
> From: Shrikant Maurya <shrikant.maurya@techveda.org>
>
> As reported by Jia-Ju Bai (https://lkml.org/lkml/2017/12/11/872):
> API's are using GFP_KERNEL to allocate memory which may sleep.
>
> To ensure atomicity such allocations must be avoided in critical
> sections under spinlock.
> Fixed by replacing GFP_KERNEL to GFP_ATOMIC.
>
> Reported-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> Signed-off-by: Shrikant Maurya <shrikant.maurya@techveda.org>
> Signed-off-by: Suniel Mahesh <sunil.m@techveda.org>
> Signed-off-by: Raghu Bharadwaj <raghu@techveda.org>
> Signed-off-by: Karthik Tummala <karthik@techveda.org>

Can't the call to device_init_wakeup() in isp116x_start() just be moved
below the spinlock release?

> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -92,11 +92,11 @@ struct wakeup_source *wakeup_source_create(const char *name)
>  {
>         struct wakeup_source *ws;
>
> -       ws = kmalloc(sizeof(*ws), GFP_KERNEL);
> +       ws = kmalloc(sizeof(*ws), GFP_ATOMIC);

With GFP_ATOMIC, allocation failure is much more likely to occur.
So IMHO it's better to fix the isp116x, than to impose this burden on
every user.

>         if (!ws)
>                 return NULL;
>
> -       wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_KERNEL) : NULL);
> +       wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_ATOMIC) : NULL);
>         return ws;
>  }
>  EXPORT_SYMBOL_GPL(wakeup_source_create);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drivers: base: power: Fix GFP_KERNEL in spinlock context

[Rafael J. Wysocki]

On Tue, Dec 12, 2017 at 2:45 PM,  <shrikant.maurya@techveda.org> wrote:
> From: Shrikant Maurya <shrikant.maurya@techveda.org>
>
> As reported by Jia-Ju Bai (https://lkml.org/lkml/2017/12/11/872):
> API's are using GFP_KERNEL to allocate memory which may sleep.
>
> To ensure atomicity such allocations must be avoided in critical
> sections under spinlock.

That's right.

Which is why wakeup_source_create() should never be called under a spinlock.

Are you aware of any place that happens in the mainline kernel?

Thanks,
Rafael

Re: [PATCH] drivers: base: power: Fix GFP_KERNEL in spinlock context

[Pavel Machek]

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

On Tue 2017-12-12 15:58:00, Geert Uytterhoeven wrote:
> Hi Shrikant,
> 
> On Tue, Dec 12, 2017 at 2:45 PM,  <shrikant.maurya@techveda.org> wrote:
> > From: Shrikant Maurya <shrikant.maurya@techveda.org>
> >
> > As reported by Jia-Ju Bai (https://lkml.org/lkml/2017/12/11/872):
> > API's are using GFP_KERNEL to allocate memory which may sleep.
> >
> > To ensure atomicity such allocations must be avoided in critical
> > sections under spinlock.
> > Fixed by replacing GFP_KERNEL to GFP_ATOMIC.
> >
> > Reported-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> > Signed-off-by: Shrikant Maurya <shrikant.maurya@techveda.org>
> > Signed-off-by: Suniel Mahesh <sunil.m@techveda.org>
> > Signed-off-by: Raghu Bharadwaj <raghu@techveda.org>
> > Signed-off-by: Karthik Tummala <karthik@techveda.org>
> 
> Can't the call to device_init_wakeup() in isp116x_start() just be moved
> below the spinlock release?
> 
> > --- a/drivers/base/power/wakeup.c
> > +++ b/drivers/base/power/wakeup.c
> > @@ -92,11 +92,11 @@ struct wakeup_source *wakeup_source_create(const char *name)
> >  {
> >         struct wakeup_source *ws;
> >
> > -       ws = kmalloc(sizeof(*ws), GFP_KERNEL);
> > +       ws = kmalloc(sizeof(*ws), GFP_ATOMIC);
> 
> With GFP_ATOMIC, allocation failure is much more likely to occur.
> So IMHO it's better to fix the isp116x, than to impose this burden on
> every user.
> 
> >         if (!ws)
> >                 return NULL;
> >
> > -       wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_KERNEL) : NULL);
> > +       wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_ATOMIC) : NULL);
> >         return ws;

NAK. This will silently replace name with NULL if memory is low.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] drivers: base: power: Fix GFP_KERNEL in spinlock context

[shrikant]

On Tuesday 12 December 2017 08:28 PM, Geert Uytterhoeven wrote:
> Hi Shrikant,
> 
> On Tue, Dec 12, 2017 at 2:45 PM,  <shrikant.maurya@techveda.org> wrote:
>> From: Shrikant Maurya <shrikant.maurya@techveda.org>
>>
>> As reported by Jia-Ju Bai (https://lkml.org/lkml/2017/12/11/872):
>> API's are using GFP_KERNEL to allocate memory which may sleep.
>>
>> To ensure atomicity such allocations must be avoided in critical
>> sections under spinlock.
>> Fixed by replacing GFP_KERNEL to GFP_ATOMIC.
>>
>> Reported-by: Jia-Ju Bai <baijiaju1990@gmail.com>
>> Signed-off-by: Shrikant Maurya <shrikant.maurya@techveda.org>
>> Signed-off-by: Suniel Mahesh <sunil.m@techveda.org>
>> Signed-off-by: Raghu Bharadwaj <raghu@techveda.org>
>> Signed-off-by: Karthik Tummala <karthik@techveda.org>
> 
> Can't the call to device_init_wakeup() in isp116x_start() just be moved
> below the spinlock release?

Can't move it below the spinlock.
Value going to be written into HcRhStatus register depends on it:
isp116x_write_reg32(isp116x, HCRHSTATUS, val);

Instead we can move it before the spinlock.

> 
>> --- a/drivers/base/power/wakeup.c
>> +++ b/drivers/base/power/wakeup.c
>> @@ -92,11 +92,11 @@ struct wakeup_source *wakeup_source_create(const char *name)
>>  {
>>         struct wakeup_source *ws;
>>
>> -       ws = kmalloc(sizeof(*ws), GFP_KERNEL);
>> +       ws = kmalloc(sizeof(*ws), GFP_ATOMIC);
> 
> With GFP_ATOMIC, allocation failure is much more likely to occur.
> So IMHO it's better to fix the isp116x, than to impose this burden on
> every user.

Absolutely. Thanks for pointing it out, it's not the right solution.

> 
>>         if (!ws)
>>                 return NULL;
>>
>> -       wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_KERNEL) : NULL);
>> +       wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_ATOMIC) : NULL);
>>         return ws;
>>  }
>>  EXPORT_SYMBOL_GPL(wakeup_source_create);
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

Thank you Geert.
-- 
Shrikant
techveda.org

Re: [PATCH] drivers: base: power: Fix GFP_KERNEL in spinlock context

[shrikant]

On Friday 15 December 2017 02:15 PM, Pavel Machek wrote:
> On Tue 2017-12-12 15:58:00, Geert Uytterhoeven wrote:
>> Hi Shrikant,
>>
>> On Tue, Dec 12, 2017 at 2:45 PM,  <shrikant.maurya@techveda.org> wrote:
>>> From: Shrikant Maurya <shrikant.maurya@techveda.org>
>>>
>>> As reported by Jia-Ju Bai (https://lkml.org/lkml/2017/12/11/872):
>>> API's are using GFP_KERNEL to allocate memory which may sleep.
>>>
>>> To ensure atomicity such allocations must be avoided in critical
>>> sections under spinlock.
>>> Fixed by replacing GFP_KERNEL to GFP_ATOMIC.
>>>
>>> Reported-by: Jia-Ju Bai <baijiaju1990@gmail.com>
>>> Signed-off-by: Shrikant Maurya <shrikant.maurya@techveda.org>
>>> Signed-off-by: Suniel Mahesh <sunil.m@techveda.org>
>>> Signed-off-by: Raghu Bharadwaj <raghu@techveda.org>
>>> Signed-off-by: Karthik Tummala <karthik@techveda.org>
>>
>> Can't the call to device_init_wakeup() in isp116x_start() just be moved
>> below the spinlock release?
>>
>>> --- a/drivers/base/power/wakeup.c
>>> +++ b/drivers/base/power/wakeup.c
>>> @@ -92,11 +92,11 @@ struct wakeup_source *wakeup_source_create(const char *name)
>>>  {
>>>         struct wakeup_source *ws;
>>>
>>> -       ws = kmalloc(sizeof(*ws), GFP_KERNEL);
>>> +       ws = kmalloc(sizeof(*ws), GFP_ATOMIC);
>>
>> With GFP_ATOMIC, allocation failure is much more likely to occur.
>> So IMHO it's better to fix the isp116x, than to impose this burden on
>> every user.
>>
>>>         if (!ws)
>>>                 return NULL;
>>>
>>> -       wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_KERNEL) : NULL);
>>> +       wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_ATOMIC) : NULL);
>>>         return ws;
> 
> NAK. This will silently replace name with NULL if memory is low.

Yes, replacing GFP_KERNEL with GFP_ATOMIC in both places will cause more 
issues than it fixes.

> 
> 									Pavel
> 

Thank you Pavel.

-- 
Shrikant
techveda.org

Re: [PATCH] drivers: base: power: Fix GFP_KERNEL in spinlock context

[shrikant]

On Tuesday 12 December 2017 10:25 PM, Rafael J. Wysocki wrote:
> On Tue, Dec 12, 2017 at 2:45 PM,  <shrikant.maurya@techveda.org> wrote:
>> From: Shrikant Maurya <shrikant.maurya@techveda.org>
>>
>> As reported by Jia-Ju Bai (https://lkml.org/lkml/2017/12/11/872):
>> API's are using GFP_KERNEL to allocate memory which may sleep.
>>
>> To ensure atomicity such allocations must be avoided in critical
>> sections under spinlock.
> 
> That's right.
> 
> Which is why wakeup_source_create() should never be called under a spinlock.

Yes.

Better approach is, to move the call to device_init_wakeup() before the spinlock.

> 
> Are you aware of any place that happens in the mainline kernel?

No.

> 
> Thanks,
> Rafael
> 

Thank you Rafeal.
-- 
Shrikant
techveda.org