[PATCH V2] hid: hid-core: Fix a sleep-in-atomic-context bug in __hid_request()

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 <redacted>
---
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:

quoted

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:

quoted

quoted

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:

quoted

quoted

quoted

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:

quoted

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