__might_sleep in uio_read()?

__might_sleep in uio_read()?

[Andy Grover]

Hi Hans and Greg,

Is this an issue with uio? I swear it didn't used to throw this warning...

Thanks -- Andy

[ 5174.883261] ------------[ cut here ]------------
[ 5174.883617] WARNING: CPU: 0 PID: 1532 at 
/home/agrover/git/kernel/kernel/sched/core.c:7389 __might_sleep+0x7d/0x90()
[ 5174.884407] do not call blocking ops when !TASK_RUNNING; state=1 set 
at [<ffffffffa02a5821>] uio_read+0x91/0x170 [uio]
[ 5174.885198] Modules linked in: tcm_loop target_core_user uio 
target_core_pscsi target_core_file target_core_iblock iscsi_target_mod 
target_core_mod uinput fuse nfsv3 nfs_acl nfs lockd grace fscache sunrpc 
microcode i2c_piix4 virtio_balloon i2c_core xfs libcrc32c crc32c_intel 
virtio_net virtio_blk
[ 5174.887351] CPU: 0 PID: 1532 Comm: tcmu-runner Not tainted 4.2.0-rc7+ 
#178
[ 5174.887853] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), 
BIOS 1.8.1-20150318_183358- 04/01/2014
[ 5174.888633]  ffffffff81a3b870 ffff880045393ca8 ffffffff817afaae 
0000000000000000
[ 5174.889224]  ffff880045393cf8 ffff880045393ce8 ffffffff8109a846 
ffff880045393cd8
[ 5174.889793]  ffffffffa02a7150 00000000000002dc 0000000000000000 
ffff880045008000
[ 5174.890375] Call Trace:
[ 5174.890562]  [<ffffffff817afaae>] dump_stack+0x4c/0x65
[ 5174.890938]  [<ffffffff8109a846>] warn_slowpath_common+0x86/0xc0
[ 5174.891388]  [<ffffffff8109a8c6>] warn_slowpath_fmt+0x46/0x50
[ 5174.891808]  [<ffffffffa02a5821>] ? uio_read+0x91/0x170 [uio]
[ 5174.892237]  [<ffffffffa02a5821>] ? uio_read+0x91/0x170 [uio]
[ 5174.892653]  [<ffffffff810c584d>] __might_sleep+0x7d/0x90
[ 5174.893055]  [<ffffffff811ea023>] __might_fault+0x43/0xa0
[ 5174.893448]  [<ffffffff817b31ce>] ? schedule+0x3e/0x90
[ 5174.893820]  [<ffffffffa02a58c2>] uio_read+0x132/0x170 [uio]
[ 5174.894240]  [<ffffffff810cbb80>] ? wake_up_q+0x70/0x70
[ 5174.894620]  [<ffffffff81236168>] __vfs_read+0x28/0xe0
[ 5174.894993]  [<ffffffff81353233>] ? security_file_permission+0xa3/0xc0
[ 5174.895541]  [<ffffffff8123678f>] ? rw_verify_area+0x4f/0xf0
[ 5174.896006]  [<ffffffff812368ba>] vfs_read+0x8a/0x140
[ 5174.896391]  [<ffffffff817b28f5>] ? __schedule+0x425/0xcc0
[ 5174.896788]  [<ffffffff812375d9>] SyS_read+0x49/0xb0
[ 5174.897160]  [<ffffffff817b9bee>] entry_SYSCALL_64_fastpath+0x12/0x76
[ 5174.897625] ---[ end trace 61765cb39fe48dd5 ]---

Re: __might_sleep in uio_read()?

[Greg KH]

On Wed, Sep 02, 2015 at 03:45:10PM -0700, Andy Grover wrote:
> Hi Hans and Greg,
> 
> Is this an issue with uio? I swear it didn't used to throw this warning...
> 
> Thanks -- Andy
> 
> [ 5174.883261] ------------[ cut here ]------------
> [ 5174.883617] WARNING: CPU: 0 PID: 1532 at
> /home/agrover/git/kernel/kernel/sched/core.c:7389 __might_sleep+0x7d/0x90()
> [ 5174.884407] do not call blocking ops when !TASK_RUNNING; state=1 set at
> [<ffffffffa02a5821>] uio_read+0x91/0x170 [uio]
> [ 5174.885198] Modules linked in: tcm_loop target_core_user uio
> target_core_pscsi target_core_file target_core_iblock iscsi_target_mod
> target_core_mod uinput fuse nfsv3 nfs_acl nfs lockd grace fscache sunrpc
> microcode i2c_piix4 virtio_balloon i2c_core xfs libcrc32c crc32c_intel
> virtio_net virtio_blk
> [ 5174.887351] CPU: 0 PID: 1532 Comm: tcmu-runner Not tainted 4.2.0-rc7+
> #178
> [ 5174.887853] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> 1.8.1-20150318_183358- 04/01/2014
> [ 5174.888633]  ffffffff81a3b870 ffff880045393ca8 ffffffff817afaae
> 0000000000000000
> [ 5174.889224]  ffff880045393cf8 ffff880045393ce8 ffffffff8109a846
> ffff880045393cd8
> [ 5174.889793]  ffffffffa02a7150 00000000000002dc 0000000000000000
> ffff880045008000
> [ 5174.890375] Call Trace:
> [ 5174.890562]  [<ffffffff817afaae>] dump_stack+0x4c/0x65
> [ 5174.890938]  [<ffffffff8109a846>] warn_slowpath_common+0x86/0xc0
> [ 5174.891388]  [<ffffffff8109a8c6>] warn_slowpath_fmt+0x46/0x50
> [ 5174.891808]  [<ffffffffa02a5821>] ? uio_read+0x91/0x170 [uio]
> [ 5174.892237]  [<ffffffffa02a5821>] ? uio_read+0x91/0x170 [uio]
> [ 5174.892653]  [<ffffffff810c584d>] __might_sleep+0x7d/0x90
> [ 5174.893055]  [<ffffffff811ea023>] __might_fault+0x43/0xa0
> [ 5174.893448]  [<ffffffff817b31ce>] ? schedule+0x3e/0x90
> [ 5174.893820]  [<ffffffffa02a58c2>] uio_read+0x132/0x170 [uio]
> [ 5174.894240]  [<ffffffff810cbb80>] ? wake_up_q+0x70/0x70
> [ 5174.894620]  [<ffffffff81236168>] __vfs_read+0x28/0xe0
> [ 5174.894993]  [<ffffffff81353233>] ? security_file_permission+0xa3/0xc0
> [ 5174.895541]  [<ffffffff8123678f>] ? rw_verify_area+0x4f/0xf0
> [ 5174.896006]  [<ffffffff812368ba>] vfs_read+0x8a/0x140
> [ 5174.896391]  [<ffffffff817b28f5>] ? __schedule+0x425/0xcc0
> [ 5174.896788]  [<ffffffff812375d9>] SyS_read+0x49/0xb0
> [ 5174.897160]  [<ffffffff817b9bee>] entry_SYSCALL_64_fastpath+0x12/0x76
> [ 5174.897625] ---[ end trace 61765cb39fe48dd5 ]---

I don't think anything has changed here in a while, any chance you can
bisect this?

And what uio driver are you using?

thanks,

greg k-h

Re: __might_sleep in uio_read()?

[Michal Hocko]

On Wed 02-09-15 15:45:10, Andy Grover wrote:
> Hi Hans and Greg,
> 
> Is this an issue with uio? I swear it didn't used to throw this warning...
> 
> Thanks -- Andy
> 
> [ 5174.883261] ------------[ cut here ]------------
> [ 5174.883617] WARNING: CPU: 0 PID: 1532 at
> /home/agrover/git/kernel/kernel/sched/core.c:7389 __might_sleep+0x7d/0x90()
> [ 5174.884407] do not call blocking ops when !TASK_RUNNING; state=1 set at
> [<ffffffffa02a5821>] uio_read+0x91/0x170 [uio]

The warning says that the driver is calling copy_to_user with
TASK_INTERRUPTIBLE which is wrong in general because this context can
sleep and a schedule would destroy the state. It doesn't matter here
because the code would break out from the loop regardless of the
copy_to_user return value.

I assume that TASK_INTERRUPTIBLE is necessary before the event_count
check to prevent from wake up races. If that is the case then you can
simply do:
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 3257d4220d01..7d8959e3833b 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -524,6 +524,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
 
 		event_count = atomic_read(&idev->event);
 		if (event_count != listener->event_count) {
+			__set_current_state(TASK_RUNNING);
 			if (copy_to_user(buf, &event_count, count))
 				retval = -EFAULT;
 			else {
-- 
Michal Hocko
SUSE Labs

Re: __might_sleep in uio_read()?

[Andy Grover]

On 09/03/2015 05:26 AM, Michal Hocko wrote:
> On Wed 02-09-15 15:45:10, Andy Grover wrote:
>> Hi Hans and Greg,
>>
>> Is this an issue with uio? I swear it didn't used to throw this warning...
>>
>> Thanks -- Andy
>>
>> [ 5174.883261] ------------[ cut here ]------------
>> [ 5174.883617] WARNING: CPU: 0 PID: 1532 at
>> /home/agrover/git/kernel/kernel/sched/core.c:7389 __might_sleep+0x7d/0x90()
>> [ 5174.884407] do not call blocking ops when !TASK_RUNNING; state=1 set at
>> [<ffffffffa02a5821>] uio_read+0x91/0x170 [uio]
>
> The warning says that the driver is calling copy_to_user with
> TASK_INTERRUPTIBLE which is wrong in general because this context can
> sleep and a schedule would destroy the state. It doesn't matter here
> because the code would break out from the loop regardless of the
> copy_to_user return value.
>
> I assume that TASK_INTERRUPTIBLE is necessary before the event_count
> check to prevent from wake up races. If that is the case then you can
> simply do:
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 3257d4220d01..7d8959e3833b 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -524,6 +524,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
>
>   		event_count = atomic_read(&idev->event);
>   		if (event_count != listener->event_count) {
> +			__set_current_state(TASK_RUNNING);
>   			if (copy_to_user(buf, &event_count, count))
>   				retval = -EFAULT;
>   			else {
>

This certainly makes the warning go away. If this looks good to everyone 
else can we get this change in?

Thanks -- Andy

Re: __might_sleep in uio_read()?

[Greg KH]

On Thu, Sep 03, 2015 at 11:47:34AM -0700, Andy Grover wrote:
> On 09/03/2015 05:26 AM, Michal Hocko wrote:
> >On Wed 02-09-15 15:45:10, Andy Grover wrote:
> >>Hi Hans and Greg,
> >>
> >>Is this an issue with uio? I swear it didn't used to throw this warning...
> >>
> >>Thanks -- Andy
> >>
> >>[ 5174.883261] ------------[ cut here ]------------
> >>[ 5174.883617] WARNING: CPU: 0 PID: 1532 at
> >>/home/agrover/git/kernel/kernel/sched/core.c:7389 __might_sleep+0x7d/0x90()
> >>[ 5174.884407] do not call blocking ops when !TASK_RUNNING; state=1 set at
> >>[<ffffffffa02a5821>] uio_read+0x91/0x170 [uio]
> >
> >The warning says that the driver is calling copy_to_user with
> >TASK_INTERRUPTIBLE which is wrong in general because this context can
> >sleep and a schedule would destroy the state. It doesn't matter here
> >because the code would break out from the loop regardless of the
> >copy_to_user return value.
> >
> >I assume that TASK_INTERRUPTIBLE is necessary before the event_count
> >check to prevent from wake up races. If that is the case then you can
> >simply do:
> >diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> >index 3257d4220d01..7d8959e3833b 100644
> >--- a/drivers/uio/uio.c
> >+++ b/drivers/uio/uio.c
> >@@ -524,6 +524,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
> >
> >  		event_count = atomic_read(&idev->event);
> >  		if (event_count != listener->event_count) {
> >+			__set_current_state(TASK_RUNNING);
> >  			if (copy_to_user(buf, &event_count, count))
> >  				retval = -EFAULT;
> >  			else {
> >
> 
> This certainly makes the warning go away. If this looks good to everyone
> else can we get this change in?

What changed to require this?  Why is this suddenly showing up now?

thanks,

greg k-h

Re: __might_sleep in uio_read()?

[Andy Grover]

On 09/03/2015 12:12 PM, Greg KH wrote:
> On Thu, Sep 03, 2015 at 11:47:34AM -0700, Andy Grover wrote:
>> On 09/03/2015 05:26 AM, Michal Hocko wrote:
>>> On Wed 02-09-15 15:45:10, Andy Grover wrote:
>>>> Hi Hans and Greg,
>>>>
>>>> Is this an issue with uio? I swear it didn't used to throw this warning...
>>>>
>>>> Thanks -- Andy
>>>>
>>>> [ 5174.883261] ------------[ cut here ]------------
>>>> [ 5174.883617] WARNING: CPU: 0 PID: 1532 at
>>>> /home/agrover/git/kernel/kernel/sched/core.c:7389 __might_sleep+0x7d/0x90()
>>>> [ 5174.884407] do not call blocking ops when !TASK_RUNNING; state=1 set at
>>>> [<ffffffffa02a5821>] uio_read+0x91/0x170 [uio]
>>>
>>> The warning says that the driver is calling copy_to_user with
>>> TASK_INTERRUPTIBLE which is wrong in general because this context can
>>> sleep and a schedule would destroy the state. It doesn't matter here
>>> because the code would break out from the loop regardless of the
>>> copy_to_user return value.
>>>
>>> I assume that TASK_INTERRUPTIBLE is necessary before the event_count
>>> check to prevent from wake up races. If that is the case then you can
>>> simply do:
>>> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>>> index 3257d4220d01..7d8959e3833b 100644
>>> --- a/drivers/uio/uio.c
>>> +++ b/drivers/uio/uio.c
>>> @@ -524,6 +524,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
>>>
>>>   		event_count = atomic_read(&idev->event);
>>>   		if (event_count != listener->event_count) {
>>> +			__set_current_state(TASK_RUNNING);
>>>   			if (copy_to_user(buf, &event_count, count))
>>>   				retval = -EFAULT;
>>>   			else {
>>>
>>
>> This certainly makes the warning go away. If this looks good to everyone
>> else can we get this change in?
>
> What changed to require this?  Why is this suddenly showing up now?

I'm working on drivers/target/target_core_user.c, a SCSI userspace 
passthrough that was added in 3.18, aka TCMU, which uses uio.

The checks for !TASK_RUNNING were added in 3.19 (8eb23b9f3 and 00845eb96)

...and I'm just getting back to TCMU development after a bit, so maybe 
I'm the first one to call uio_read since those checks were added in 3.19?

-- Andy

Re: __might_sleep in uio_read()?

[Greg KH]

On Thu, Sep 03, 2015 at 01:12:28PM -0700, Andy Grover wrote:
> On 09/03/2015 12:12 PM, Greg KH wrote:
> >On Thu, Sep 03, 2015 at 11:47:34AM -0700, Andy Grover wrote:
> >>On 09/03/2015 05:26 AM, Michal Hocko wrote:
> >>>On Wed 02-09-15 15:45:10, Andy Grover wrote:
> >>>>Hi Hans and Greg,
> >>>>
> >>>>Is this an issue with uio? I swear it didn't used to throw this warning...
> >>>>
> >>>>Thanks -- Andy
> >>>>
> >>>>[ 5174.883261] ------------[ cut here ]------------
> >>>>[ 5174.883617] WARNING: CPU: 0 PID: 1532 at
> >>>>/home/agrover/git/kernel/kernel/sched/core.c:7389 __might_sleep+0x7d/0x90()
> >>>>[ 5174.884407] do not call blocking ops when !TASK_RUNNING; state=1 set at
> >>>>[<ffffffffa02a5821>] uio_read+0x91/0x170 [uio]
> >>>
> >>>The warning says that the driver is calling copy_to_user with
> >>>TASK_INTERRUPTIBLE which is wrong in general because this context can
> >>>sleep and a schedule would destroy the state. It doesn't matter here
> >>>because the code would break out from the loop regardless of the
> >>>copy_to_user return value.
> >>>
> >>>I assume that TASK_INTERRUPTIBLE is necessary before the event_count
> >>>check to prevent from wake up races. If that is the case then you can
> >>>simply do:
> >>>diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> >>>index 3257d4220d01..7d8959e3833b 100644
> >>>--- a/drivers/uio/uio.c
> >>>+++ b/drivers/uio/uio.c
> >>>@@ -524,6 +524,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
> >>>
> >>>  		event_count = atomic_read(&idev->event);
> >>>  		if (event_count != listener->event_count) {
> >>>+			__set_current_state(TASK_RUNNING);
> >>>  			if (copy_to_user(buf, &event_count, count))
> >>>  				retval = -EFAULT;
> >>>  			else {
> >>>
> >>
> >>This certainly makes the warning go away. If this looks good to everyone
> >>else can we get this change in?
> >
> >What changed to require this?  Why is this suddenly showing up now?
> 
> I'm working on drivers/target/target_core_user.c, a SCSI userspace
> passthrough that was added in 3.18, aka TCMU, which uses uio.
> 
> The checks for !TASK_RUNNING were added in 3.19 (8eb23b9f3 and 00845eb96)
> 
> ...and I'm just getting back to TCMU development after a bit, so maybe I'm
> the first one to call uio_read since those checks were added in 3.19?

If so, that's sad.

greg k-h

Re: __might_sleep in uio_read()?

[Michal Hocko]

On Thu 03-09-15 11:47:34, Andy Grover wrote:
> On 09/03/2015 05:26 AM, Michal Hocko wrote:
> >On Wed 02-09-15 15:45:10, Andy Grover wrote:
> >>Hi Hans and Greg,
> >>
> >>Is this an issue with uio? I swear it didn't used to throw this warning...
> >>
> >>Thanks -- Andy
> >>
> >>[ 5174.883261] ------------[ cut here ]------------
> >>[ 5174.883617] WARNING: CPU: 0 PID: 1532 at
> >>/home/agrover/git/kernel/kernel/sched/core.c:7389 __might_sleep+0x7d/0x90()
> >>[ 5174.884407] do not call blocking ops when !TASK_RUNNING; state=1 set at
> >>[<ffffffffa02a5821>] uio_read+0x91/0x170 [uio]
> >
> >The warning says that the driver is calling copy_to_user with
> >TASK_INTERRUPTIBLE which is wrong in general because this context can
> >sleep and a schedule would destroy the state. It doesn't matter here
> >because the code would break out from the loop regardless of the
> >copy_to_user return value.
> >
> >I assume that TASK_INTERRUPTIBLE is necessary before the event_count
> >check to prevent from wake up races. If that is the case then you can
> >simply do:
> >diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> >index 3257d4220d01..7d8959e3833b 100644
> >--- a/drivers/uio/uio.c
> >+++ b/drivers/uio/uio.c
> >@@ -524,6 +524,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
> >
> >  		event_count = atomic_read(&idev->event);
> >  		if (event_count != listener->event_count) {
> >+			__set_current_state(TASK_RUNNING);
> >  			if (copy_to_user(buf, &event_count, count))
> >  				retval = -EFAULT;
> >  			else {
> >
> 
> This certainly makes the warning go away. If this looks good to everyone
> else can we get this change in?
---
>From f50005095a90d21f628d8ce6525b432f17c8138c Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Fri, 4 Sep 2015 10:08:18 +0200
Subject: [PATCH] uio: fix false positive __might_sleep warning splat

Andy has reported a __might_sleep warning
[ 5174.883617] WARNING: CPU: 0 PID: 1532 at
/home/agrover/git/kernel/kernel/sched/core.c:7389 __might_sleep+0x7d/0x90()
[ 5174.884407] do not call blocking ops when !TASK_RUNNING; state=1 set at [<ffffffffa02a5821>] uio_read+0x91/0x170 [uio]
[ 5174.885198] Modules linked in: tcm_loop target_core_user uio target_core_pscsi target_core_file target_core_iblock iscsi_target_mod  target_core_mod uinput fuse nfsv3 nfs_acl nfs lockd grace fscache sunrpc microcode i2c_piix4 virtio_balloon i2c_core xfs libcrc32c crc32c_intel  virtio_net virtio_blk
[ 5174.887351] CPU: 0 PID: 1532 Comm: tcmu-runner Not tainted 4.2.0-rc7+
[ 5174.887853] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.8.1-20150318_183358- 04/01/2014
[ 5174.888633]  ffffffff81a3b870 ffff880045393ca8 ffffffff817afaae
0000000000000000
[ 5174.889224]  ffff880045393cf8 ffff880045393ce8 ffffffff8109a846
ffff880045393cd8
[ 5174.889793]  ffffffffa02a7150 00000000000002dc 0000000000000000
ffff880045008000
[ 5174.890375] Call Trace:
[ 5174.890562]  [<ffffffff817afaae>] dump_stack+0x4c/0x65
[ 5174.890938]  [<ffffffff8109a846>] warn_slowpath_common+0x86/0xc0
[ 5174.891388]  [<ffffffff8109a8c6>] warn_slowpath_fmt+0x46/0x50
[ 5174.891808]  [<ffffffffa02a5821>] ? uio_read+0x91/0x170 [uio]
[ 5174.892237]  [<ffffffffa02a5821>] ? uio_read+0x91/0x170 [uio]
[ 5174.892653]  [<ffffffff810c584d>] __might_sleep+0x7d/0x90
[ 5174.893055]  [<ffffffff811ea023>] __might_fault+0x43/0xa0
[ 5174.893448]  [<ffffffff817b31ce>] ? schedule+0x3e/0x90
[ 5174.893820]  [<ffffffffa02a58c2>] uio_read+0x132/0x170 [uio]
[ 5174.894240]  [<ffffffff810cbb80>] ? wake_up_q+0x70/0x70
[ 5174.894620]  [<ffffffff81236168>] __vfs_read+0x28/0xe0
[ 5174.894993]  [<ffffffff81353233>] ? security_file_permission+0xa3/0xc0
[ 5174.895541]  [<ffffffff8123678f>] ? rw_verify_area+0x4f/0xf0
[ 5174.896006]  [<ffffffff812368ba>] vfs_read+0x8a/0x140
[ 5174.896391]  [<ffffffff817b28f5>] ? __schedule+0x425/0xcc0
[ 5174.896788]  [<ffffffff812375d9>] SyS_read+0x49/0xb0

The warning is a false positive because uio_read doesn't depent on
TASK_INTERRUPTIBLE after copy_to_user so it is safe to silence the
warning by an explicit setting the state to TASK_RUNNING in the path
which might call into TASK_RUNNING.

Reported-by: Andy Grover <agrover@redhat.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 drivers/uio/uio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 3257d4220d01..7d8959e3833b 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -524,6 +524,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
 
 		event_count = atomic_read(&idev->event);
 		if (event_count != listener->event_count) {
+			__set_current_state(TASK_RUNNING);
 			if (copy_to_user(buf, &event_count, count))
 				retval = -EFAULT;
 			else {
-- 
2.5.0

-- 
Michal Hocko
SUSE Labs