* [PATCH V2] hid: hid-core: Fix a sleep-in-atomic-context bug in __hid_request()
@ 2018-09-13 3:34 Jia-Ju Bai
2018-09-24 9:26 ` Jiri Kosina
0 siblings, 1 reply; 6+ messages in thread
From: Jia-Ju Bai @ 2018-09-13 3:34 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Jia-Ju Bai
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 <baijiaju1990@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2] hid: hid-core: Fix a sleep-in-atomic-context bug in __hid_request()
2018-09-13 3:34 [PATCH V2] hid: hid-core: Fix a sleep-in-atomic-context bug in __hid_request() Jia-Ju Bai
@ 2018-09-24 9:26 ` Jiri Kosina
2018-09-29 9:00 ` Jia-Ju Bai
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Kosina @ 2018-09-24 9:26 UTC (permalink / raw)
To: Jia-Ju Bai; +Cc: benjamin.tissoires, linux-input, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] hid: hid-core: Fix a sleep-in-atomic-context bug in __hid_request()
2018-09-24 9:26 ` Jiri Kosina
@ 2018-09-29 9:00 ` Jia-Ju Bai
2018-09-29 19:20 ` Jiri Kosina
0 siblings, 1 reply; 6+ messages in thread
From: Jia-Ju Bai @ 2018-09-29 9:00 UTC (permalink / raw)
To: Jiri Kosina; +Cc: benjamin.tissoires, linux-input, linux-kernel
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,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] hid: hid-core: Fix a sleep-in-atomic-context bug in __hid_request()
2018-09-29 9:00 ` Jia-Ju Bai
@ 2018-09-29 19:20 ` Jiri Kosina
2018-10-04 3:14 ` Jia-Ju Bai
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Kosina @ 2018-09-29 19:20 UTC (permalink / raw)
To: Jia-Ju Bai; +Cc: benjamin.tissoires, linux-input, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] hid: hid-core: Fix a sleep-in-atomic-context bug in __hid_request()
2018-09-29 19:20 ` Jiri Kosina
@ 2018-10-04 3:14 ` Jia-Ju Bai
2018-10-04 7:35 ` Jiri Kosina
0 siblings, 1 reply; 6+ messages in thread
From: Jia-Ju Bai @ 2018-10-04 3:14 UTC (permalink / raw)
To: Jiri Kosina; +Cc: benjamin.tissoires, linux-input, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] hid: hid-core: Fix a sleep-in-atomic-context bug in __hid_request()
2018-10-04 3:14 ` Jia-Ju Bai
@ 2018-10-04 7:35 ` Jiri Kosina
0 siblings, 0 replies; 6+ messages in thread
From: Jiri Kosina @ 2018-10-04 7:35 UTC (permalink / raw)
To: Jia-Ju Bai; +Cc: benjamin.tissoires, linux-input, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-04 7:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-13 3:34 [PATCH V2] hid: hid-core: Fix a sleep-in-atomic-context bug in __hid_request() Jia-Ju Bai
2018-09-24 9:26 ` Jiri Kosina
2018-09-29 9:00 ` Jia-Ju Bai
2018-09-29 19:20 ` Jiri Kosina
2018-10-04 3:14 ` Jia-Ju Bai
2018-10-04 7:35 ` Jiri Kosina
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).