hid_alloc_report_buf() has to be called with GFP_ATOMIC in
__hid_request(), because there are the following callchains
leading to __hid_request() being an atomic context:
picolcd_send_and_wait (acquire a spinlock)
hid_hw_request
__hid_request
hid_alloc_report_buf(GFP_KERNEL)
picolcd_reset (acquire a spinlock)
hid_hw_request
__hid_request
hid_alloc_report_buf(GFP_KERNEL)
lg4ff_play (acquire a spinlock)
hid_hw_request
__hid_request
hid_alloc_report_buf(GFP_KERNEL)
lg4ff_set_autocenter_ffex (acquire a spinlock)
hid_hw_request
__hid_request
hid_alloc_report_buf(GFP_KERNEL)
This bug is found by my static analysis tool DSAC.
Signed-off-by: Jia-Ju Bai <[email protected]>
---
v2:
* Make the description more human readable.
Thanks Jiri for good advice.
---
drivers/hid/hid-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 3942ee61bd1c..c886af00c8c9 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1442,7 +1442,7 @@ void __hid_request(struct hid_device *hid, struct hid_report *report,
int ret;
u32 len;
- buf = hid_alloc_report_buf(report, GFP_KERNEL);
+ buf = hid_alloc_report_buf(report, GFP_ATOMIC);
if (!buf)
return;
--
2.17.0
On Thu, 13 Sep 2018, Jia-Ju Bai wrote:
> hid_alloc_report_buf() has to be called with GFP_ATOMIC in
> __hid_request(), because there are the following callchains
> leading to __hid_request() being an atomic context:
>
> picolcd_send_and_wait (acquire a spinlock)
> hid_hw_request
> __hid_request
> hid_alloc_report_buf(GFP_KERNEL)
>
> picolcd_reset (acquire a spinlock)
> hid_hw_request
> __hid_request
> hid_alloc_report_buf(GFP_KERNEL)
>
> lg4ff_play (acquire a spinlock)
> hid_hw_request
> __hid_request
> hid_alloc_report_buf(GFP_KERNEL)
>
> lg4ff_set_autocenter_ffex (acquire a spinlock)
> hid_hw_request
> __hid_request
> hid_alloc_report_buf(GFP_KERNEL)
Hm, so it's always drivers calling out into core in atomic context. So
either we take this, and put our bets on being able to allocate the buffer
without sleeping, or actually fix the few drivers (it's just lg4ff and
picolcd at the end of the day) not to do that, and explicitly anotate
__hid_request() with might_sleep().
Hmm?
Thanks,
--
Jiri Kosina
SUSE Labs
On 2018/9/24 17:26, Jiri Kosina wrote:
> On Thu, 13 Sep 2018, Jia-Ju Bai wrote:
>
>> hid_alloc_report_buf() has to be called with GFP_ATOMIC in
>> __hid_request(), because there are the following callchains
>> leading to __hid_request() being an atomic context:
>>
>> picolcd_send_and_wait (acquire a spinlock)
>> hid_hw_request
>> __hid_request
>> hid_alloc_report_buf(GFP_KERNEL)
>>
>> picolcd_reset (acquire a spinlock)
>> hid_hw_request
>> __hid_request
>> hid_alloc_report_buf(GFP_KERNEL)
>>
>> lg4ff_play (acquire a spinlock)
>> hid_hw_request
>> __hid_request
>> hid_alloc_report_buf(GFP_KERNEL)
>>
>> lg4ff_set_autocenter_ffex (acquire a spinlock)
>> hid_hw_request
>> __hid_request
>> hid_alloc_report_buf(GFP_KERNEL)
> Hm, so it's always drivers calling out into core in atomic context. So
> either we take this, and put our bets on being able to allocate the buffer
> without sleeping,
In my opinion, I prefer this way.
Best wishes,
Jia-Ju Bai
> or actually fix the few drivers (it's just lg4ff and
> picolcd at the end of the day) not to do that, and explicitly anotate
> __hid_request() with might_sleep().
>
> Hmm?
>
> Thanks,
>
On Sat, 29 Sep 2018, Jia-Ju Bai wrote:
> >> picolcd_send_and_wait (acquire a spinlock)
> >> hid_hw_request
> >> __hid_request
> >> hid_alloc_report_buf(GFP_KERNEL)
> >>
> >> picolcd_reset (acquire a spinlock)
> >> hid_hw_request
> >> __hid_request
> >> hid_alloc_report_buf(GFP_KERNEL)
> >>
> >> lg4ff_play (acquire a spinlock)
> >> hid_hw_request
> >> __hid_request
> >> hid_alloc_report_buf(GFP_KERNEL)
> >>
> >> lg4ff_set_autocenter_ffex (acquire a spinlock)
> >> hid_hw_request
> >> __hid_request
> >> hid_alloc_report_buf(GFP_KERNEL)
> > Hm, so it's always drivers calling out into core in atomic context. So
> > either we take this, and put our bets on being able to allocate the buffer
> > without sleeping,
>
> In my opinion, I prefer this way.
Why? Forcing all the report buffer to be limited to be non-sleeping
allocations just because of two drivers, looks like an overkill, and
actually calls for more issues (as GFP_ATOMIC is of course in principle
less likely to succeed).
--
Jiri Kosina
SUSE Labs
On 2018/9/30 3:20, Jiri Kosina wrote:
> On Sat, 29 Sep 2018, Jia-Ju Bai wrote:
>
>>>> picolcd_send_and_wait (acquire a spinlock)
>>>> hid_hw_request
>>>> __hid_request
>>>> hid_alloc_report_buf(GFP_KERNEL)
>>>>
>>>> picolcd_reset (acquire a spinlock)
>>>> hid_hw_request
>>>> __hid_request
>>>> hid_alloc_report_buf(GFP_KERNEL)
>>>>
>>>> lg4ff_play (acquire a spinlock)
>>>> hid_hw_request
>>>> __hid_request
>>>> hid_alloc_report_buf(GFP_KERNEL)
>>>>
>>>> lg4ff_set_autocenter_ffex (acquire a spinlock)
>>>> hid_hw_request
>>>> __hid_request
>>>> hid_alloc_report_buf(GFP_KERNEL)
>>> Hm, so it's always drivers calling out into core in atomic context. So
>>> either we take this, and put our bets on being able to allocate the buffer
>>> without sleeping,
>> In my opinion, I prefer this way.
> Why? Forcing all the report buffer to be limited to be non-sleeping
> allocations just because of two drivers, looks like an overkill, and
> actually calls for more issues (as GFP_ATOMIC is of course in principle
> less likely to succeed).
>
Okay, I thought that using GFP_ATOMIC is the simplest way to fix these bugs.
But I check the Linux kernel code again, and find that hid_hw_request()
are called at many places.
So changing this function may affect many drivers.
I agree to only change the two drivers, and explicitly anotate
__hid_request() with might_sleep().
Best wishes,
Jia-Ju Bai
On Thu, 4 Oct 2018, Jia-Ju Bai wrote:
> > Why? Forcing all the report buffer to be limited to be non-sleeping
> > allocations just because of two drivers, looks like an overkill, and
> > actually calls for more issues (as GFP_ATOMIC is of course in principle
> > less likely to succeed).
>
> Okay, I thought that using GFP_ATOMIC is the simplest way to fix these bugs.
> But I check the Linux kernel code again, and find that hid_hw_request() are
> called at many places.
> So changing this function may affect many drivers.
> I agree to only change the two drivers, and explicitly anotate __hid_request()
> with might_sleep().
Thanks. Are you planning to submit a patch to do that?
--
Jiri Kosina
SUSE Labs