| Message ID | 20180913033432.16336-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [V2] hid: hid-core: Fix a sleep-in-atomic-context bug in __hid_request() | expand |
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,
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).
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?
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;
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(-)