* [PATCH] hid: hid-picolcd: fix possible sleep-in-atomic-context bug
@ 2019-12-18 8:02 Jia-Ju Bai
2019-12-18 8:41 ` Bruno Prémont
0 siblings, 1 reply; 4+ messages in thread
From: Jia-Ju Bai @ 2019-12-18 8:02 UTC (permalink / raw)
To: bonbons, jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Jia-Ju Bai
The driver may sleep while holding a read lock.
The function call path (from bottom to top) in Linux 4.19 is:
drivers/hid/hid-core.c, 1459:
hid_alloc_report_buf(GFP_KERNEL) in __hid_request
./include/linux/hid.h, 1051:
__hid_request in hid_hw_request
drivers/hid/hid-picolcd_leds.c, 56:
hid_hw_request in picolcd_leds_set
drivers/hid/hid-picolcd_leds.c, 53:
_raw_spin_lock_irqsave in picolcd_leds_set
drivers/hid/hid-core.c, 1459:
hid_alloc_report_buf(GFP_KERNEL) in __hid_request
./include/linux/hid.h, 1051:
__hid_request in hid_hw_request
drivers/hid/hid-picolcd_lcd.c, 49:
hid_hw_request in picolcd_set_contrast
drivers/hid/hid-picolcd_lcd.c, 46:
_raw_spin_lock_irqsave in picolcd_set_contrast
drivers/hid/hid-core.c, 1459:
hid_alloc_report_buf(GFP_KERNEL) in __hid_request
./include/linux/hid.h, 1051:
__hid_request in hid_hw_request
drivers/hid/hid-picolcd_core.c, 245:
hid_hw_request in picolcd_reset
drivers/hid/hid-picolcd_core.c, 235:
_raw_spin_lock_irqsave in picolcd_reset
drivers/hid/hid-core.c, 1459:
hid_alloc_report_buf(GFP_KERNEL) in __hid_request
./include/linux/hid.h, 1051:
__hid_request in hid_hw_request
drivers/hid/hid-picolcd_core.c, 111:
hid_hw_request in picolcd_send_and_wait
drivers/hid/hid-picolcd_core.c, 100:
_raw_spin_lock_irqsave in picolcd_send_and_wait
hid_alloc_report_buf(GFP_KERNEL) can sleep at runtime.
To fix these bugs, hid_hw_request() is called without holding the
spinlock.
These bugs are found by a static analysis tool STCheck written by myself.
Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
drivers/hid/hid-picolcd_core.c | 4 ++--
drivers/hid/hid-picolcd_lcd.c | 6 ++++--
drivers/hid/hid-picolcd_leds.c | 6 ++++--
3 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index 1b5c63241af0..55d1892daa15 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -99,8 +99,8 @@ struct picolcd_pending *picolcd_send_and_wait(struct hid_device *hdev,
work = NULL;
} else {
data->pending = work;
- hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
spin_unlock_irqrestore(&data->lock, flags);
+ hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
wait_for_completion_interruptible_timeout(&work->ready, HZ*2);
spin_lock_irqsave(&data->lock, flags);
data->pending = NULL;
@@ -233,8 +233,8 @@ int picolcd_reset(struct hid_device *hdev)
spin_unlock_irqrestore(&data->lock, flags);
return -ENODEV;
}
- hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
spin_unlock_irqrestore(&data->lock, flags);
+ hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
error = picolcd_check_version(hdev);
if (error)
diff --git a/drivers/hid/hid-picolcd_lcd.c b/drivers/hid/hid-picolcd_lcd.c
index 0c4b76de8ae5..1fd291674ffe 100644
--- a/drivers/hid/hid-picolcd_lcd.c
+++ b/drivers/hid/hid-picolcd_lcd.c
@@ -26,6 +26,7 @@ static int picolcd_get_contrast(struct lcd_device *ldev)
static int picolcd_set_contrast(struct lcd_device *ldev, int contrast)
{
struct picolcd_data *data = lcd_get_data(ldev);
+ int status;
struct hid_report *report = picolcd_out_report(REPORT_CONTRAST, data->hdev);
unsigned long flags;
@@ -35,9 +36,10 @@ static int picolcd_set_contrast(struct lcd_device *ldev, int contrast)
data->lcd_contrast = contrast & 0x0ff;
spin_lock_irqsave(&data->lock, flags);
hid_set_field(report->field[0], 0, data->lcd_contrast);
- if (!(data->status & PICOLCD_FAILED))
- hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
+ status = data->status;
spin_unlock_irqrestore(&data->lock, flags);
+ if (!(status & PICOLCD_FAILED))
+ hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
return 0;
}
diff --git a/drivers/hid/hid-picolcd_leds.c b/drivers/hid/hid-picolcd_leds.c
index 6b505a753511..6652aa6b98dd 100644
--- a/drivers/hid/hid-picolcd_leds.c
+++ b/drivers/hid/hid-picolcd_leds.c
@@ -32,6 +32,7 @@
void picolcd_leds_set(struct picolcd_data *data)
{
struct hid_report *report;
+ int status;
unsigned long flags;
if (!data->led[0])
@@ -42,9 +43,10 @@ void picolcd_leds_set(struct picolcd_data *data)
spin_lock_irqsave(&data->lock, flags);
hid_set_field(report->field[0], 0, data->led_state);
- if (!(data->status & PICOLCD_FAILED))
- hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
+ status = data->status;
spin_unlock_irqrestore(&data->lock, flags);
+ if (!(status & PICOLCD_FAILED))
+ hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
}
static void picolcd_led_set_brightness(struct led_classdev *led_cdev,
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] hid: hid-picolcd: fix possible sleep-in-atomic-context bug
2019-12-18 8:02 [PATCH] hid: hid-picolcd: fix possible sleep-in-atomic-context bug Jia-Ju Bai
@ 2019-12-18 8:41 ` Bruno Prémont
2019-12-18 12:11 ` Jia-Ju Bai
0 siblings, 1 reply; 4+ messages in thread
From: Bruno Prémont @ 2019-12-18 8:41 UTC (permalink / raw)
To: Jia-Ju Bai; +Cc: jikos, benjamin.tissoires, linux-input, linux-kernel
Hi Jia-Ju,
Your checker has been looking at fallback implementation for
the might-sleep hid_alloc_report_buf(GFP_KERNEL).
Did you have a look at the low-lever bus-driver implementations:
hdev->ll_driver->request
^^^^^^^
Are those all sleeping as well or maybe they don't sleep?
I will have a look over the coming days.
Best regards,
Bruno Prémont
On Wed, 18 Dec 2019 16:02:01 +0800 Jia-Ju Bai wrote:
> The driver may sleep while holding a read lock.
> The function call path (from bottom to top) in Linux 4.19 is:
>
> drivers/hid/hid-core.c, 1459:
> hid_alloc_report_buf(GFP_KERNEL) in __hid_request
> ./include/linux/hid.h, 1051:
> __hid_request in hid_hw_request
> drivers/hid/hid-picolcd_leds.c, 56:
> hid_hw_request in picolcd_leds_set
> drivers/hid/hid-picolcd_leds.c, 53:
> _raw_spin_lock_irqsave in picolcd_leds_set
>
> drivers/hid/hid-core.c, 1459:
> hid_alloc_report_buf(GFP_KERNEL) in __hid_request
> ./include/linux/hid.h, 1051:
> __hid_request in hid_hw_request
> drivers/hid/hid-picolcd_lcd.c, 49:
> hid_hw_request in picolcd_set_contrast
> drivers/hid/hid-picolcd_lcd.c, 46:
> _raw_spin_lock_irqsave in picolcd_set_contrast
>
> drivers/hid/hid-core.c, 1459:
> hid_alloc_report_buf(GFP_KERNEL) in __hid_request
> ./include/linux/hid.h, 1051:
> __hid_request in hid_hw_request
> drivers/hid/hid-picolcd_core.c, 245:
> hid_hw_request in picolcd_reset
> drivers/hid/hid-picolcd_core.c, 235:
> _raw_spin_lock_irqsave in picolcd_reset
>
> drivers/hid/hid-core.c, 1459:
> hid_alloc_report_buf(GFP_KERNEL) in __hid_request
> ./include/linux/hid.h, 1051:
> __hid_request in hid_hw_request
> drivers/hid/hid-picolcd_core.c, 111:
> hid_hw_request in picolcd_send_and_wait
> drivers/hid/hid-picolcd_core.c, 100:
> _raw_spin_lock_irqsave in picolcd_send_and_wait
>
> hid_alloc_report_buf(GFP_KERNEL) can sleep at runtime.
>
> To fix these bugs, hid_hw_request() is called without holding the
> spinlock.
>
> These bugs are found by a static analysis tool STCheck written by myself.
>
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
> drivers/hid/hid-picolcd_core.c | 4 ++--
> drivers/hid/hid-picolcd_lcd.c | 6 ++++--
> drivers/hid/hid-picolcd_leds.c | 6 ++++--
> 3 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> index 1b5c63241af0..55d1892daa15 100644
> --- a/drivers/hid/hid-picolcd_core.c
> +++ b/drivers/hid/hid-picolcd_core.c
> @@ -99,8 +99,8 @@ struct picolcd_pending *picolcd_send_and_wait(struct hid_device *hdev,
> work = NULL;
> } else {
> data->pending = work;
> - hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
> spin_unlock_irqrestore(&data->lock, flags);
> + hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
> wait_for_completion_interruptible_timeout(&work->ready, HZ*2);
> spin_lock_irqsave(&data->lock, flags);
> data->pending = NULL;
> @@ -233,8 +233,8 @@ int picolcd_reset(struct hid_device *hdev)
> spin_unlock_irqrestore(&data->lock, flags);
> return -ENODEV;
> }
> - hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> spin_unlock_irqrestore(&data->lock, flags);
> + hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
>
> error = picolcd_check_version(hdev);
> if (error)
> diff --git a/drivers/hid/hid-picolcd_lcd.c b/drivers/hid/hid-picolcd_lcd.c
> index 0c4b76de8ae5..1fd291674ffe 100644
> --- a/drivers/hid/hid-picolcd_lcd.c
> +++ b/drivers/hid/hid-picolcd_lcd.c
> @@ -26,6 +26,7 @@ static int picolcd_get_contrast(struct lcd_device *ldev)
> static int picolcd_set_contrast(struct lcd_device *ldev, int contrast)
> {
> struct picolcd_data *data = lcd_get_data(ldev);
> + int status;
> struct hid_report *report = picolcd_out_report(REPORT_CONTRAST, data->hdev);
> unsigned long flags;
>
> @@ -35,9 +36,10 @@ static int picolcd_set_contrast(struct lcd_device *ldev, int contrast)
> data->lcd_contrast = contrast & 0x0ff;
> spin_lock_irqsave(&data->lock, flags);
> hid_set_field(report->field[0], 0, data->lcd_contrast);
> - if (!(data->status & PICOLCD_FAILED))
> - hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
> + status = data->status;
> spin_unlock_irqrestore(&data->lock, flags);
> + if (!(status & PICOLCD_FAILED))
> + hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
> return 0;
> }
>
> diff --git a/drivers/hid/hid-picolcd_leds.c b/drivers/hid/hid-picolcd_leds.c
> index 6b505a753511..6652aa6b98dd 100644
> --- a/drivers/hid/hid-picolcd_leds.c
> +++ b/drivers/hid/hid-picolcd_leds.c
> @@ -32,6 +32,7 @@
> void picolcd_leds_set(struct picolcd_data *data)
> {
> struct hid_report *report;
> + int status;
> unsigned long flags;
>
> if (!data->led[0])
> @@ -42,9 +43,10 @@ void picolcd_leds_set(struct picolcd_data *data)
>
> spin_lock_irqsave(&data->lock, flags);
> hid_set_field(report->field[0], 0, data->led_state);
> - if (!(data->status & PICOLCD_FAILED))
> - hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
> + status = data->status;
> spin_unlock_irqrestore(&data->lock, flags);
> + if (!(status & PICOLCD_FAILED))
> + hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
> }
>
> static void picolcd_led_set_brightness(struct led_classdev *led_cdev,
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] hid: hid-picolcd: fix possible sleep-in-atomic-context bug
2019-12-18 8:41 ` Bruno Prémont
@ 2019-12-18 12:11 ` Jia-Ju Bai
2019-12-22 18:37 ` Bruno Prémont
0 siblings, 1 reply; 4+ messages in thread
From: Jia-Ju Bai @ 2019-12-18 12:11 UTC (permalink / raw)
To: Bruno Prémont; +Cc: jikos, benjamin.tissoires, linux-input, linux-kernel
Thanks for the reply.
On 2019/12/18 16:41, Bruno Prémont wrote:
> Hi Jia-Ju,
>
> Your checker has been looking at fallback implementation for
> the might-sleep hid_alloc_report_buf(GFP_KERNEL).
>
> Did you have a look at the low-lever bus-driver implementations:
> hdev->ll_driver->request
> ^^^^^^^
>
> Are those all sleeping as well or maybe they don't sleep?\
In fact, I find that a function possibly-related to this function
pointer can sleep:
drivers/hid/intel-ish-hid/ishtp-hid.c, 97:
kzalloc(GFP_KERNEL) in ishtp_hid_request
But I am not quite sure whether this function is really referenced by
the function pointer, so I did not report it.
Best wishes,
Jia-Ju Bai
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] hid: hid-picolcd: fix possible sleep-in-atomic-context bug
2019-12-18 12:11 ` Jia-Ju Bai
@ 2019-12-22 18:37 ` Bruno Prémont
0 siblings, 0 replies; 4+ messages in thread
From: Bruno Prémont @ 2019-12-22 18:37 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: Bruno Prémont, jikos, benjamin.tissoires, linux-input, linux-kernel
Hi Jia-Ju,
I've had a deeper look at the code (possibly also applies to hid-lg4ff).
The hdev->ll_driver->request (at least on USB bus which is the only one
that matters for hid-picolcd) points to:
usbhid_request() from drivers/hid/usbhid/hid-core.c
This one directly calls usbhid_submit_report() which then directly calls
__usbhid_submit_report() under spinlock.
Thus for USB bus there is no possible sleep left.
Just moving the hid_hw_request() calls out of the spinlock is
incorrect though as it would introduce the possibility of unexpected
concurrent initialization/submissions of reports from the distinct
sub-drivers. The report pointer used is not call-private but comes from
feature description and is filled with data on each call within the
spinlock.
The question could be whether the generic fallback in hid_hw_request()
should be adjusted to be non-sleeping.
It has been introduced rather more recently than both drivers you
detected.
Best regards,
Bruno Prémont
On Wed, 18 Dec 2019 20:11:47 Jia-Ju Bai wrote:
> Thanks for the reply.
>
> On 2019/12/18 16:41, Bruno Prémont wrote:
> > Hi Jia-Ju,
> >
> > Your checker has been looking at fallback implementation for
> > the might-sleep hid_alloc_report_buf(GFP_KERNEL).
> >
> > Did you have a look at the low-lever bus-driver implementations:
> > hdev->ll_driver->request
> > ^^^^^^^
> >
> > Are those all sleeping as well or maybe they don't sleep?\
>
> In fact, I find that a function possibly-related to this function
> pointer can sleep:
>
> drivers/hid/intel-ish-hid/ishtp-hid.c, 97:
> kzalloc(GFP_KERNEL) in ishtp_hid_request
>
> But I am not quite sure whether this function is really referenced by
> the function pointer, so I did not report it.
>
>
> Best wishes,
> Jia-Ju Bai
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-12-22 18:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-18 8:02 [PATCH] hid: hid-picolcd: fix possible sleep-in-atomic-context bug Jia-Ju Bai
2019-12-18 8:41 ` Bruno Prémont
2019-12-18 12:11 ` Jia-Ju Bai
2019-12-22 18:37 ` Bruno Prémont
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).