[PATCH] uio: Fix uio driver to refcount device

[PATCH] uio: Fix uio driver to refcount device

[Brian Russell]

Protect uio driver from crashing if its owner is hot unplugged while there
are open fds.
Signed-off-by: Brian Russell <brussell@brocade.com>
---
 drivers/uio/uio.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 6276f13..70ce015 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -434,9 +434,11 @@ static int uio_open(struct inode *inode, struct file *filep)
 		goto out;
 	}
 
+	get_device(idev);
+
 	if (!try_module_get(idev->owner)) {
 		ret = -ENODEV;
-		goto out;
+		goto err_module_get;
 	}
 
 	listener = kmalloc(sizeof(*listener), GFP_KERNEL);
@@ -462,6 +464,9 @@ err_infoopen:
 err_alloc_listener:
 	module_put(idev->owner);
 
+err_module_get:
+	put_device(idev);
+
 out:
 	return ret;
 }
@@ -485,6 +490,7 @@ static int uio_release(struct inode *inode, struct file *filep)
 
 	module_put(idev->owner);
 	kfree(listener);
+	put_device(idev);
 	return ret;
 }
 
-- 
2.1.4

Re: [PATCH] uio: Fix uio driver to refcount device

[Greg Kroah-Hartman]

On Wed, Mar 11, 2015 at 03:31:42PM +0000, Brian Russell wrote:
> Protect uio driver from crashing if its owner is hot unplugged while there
> are open fds.
> Signed-off-by: Brian Russell <brussell@brocade.com>

Minor nit, you need a blank line before your s-o-b: line.



> ---
>  drivers/uio/uio.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 6276f13..70ce015 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -434,9 +434,11 @@ static int uio_open(struct inode *inode, struct file *filep)
>  		goto out;
>  	}
>  
> +	get_device(idev);

What is the real oops caused when a device is removed?  Protecting this
with a reference count seems ok, but it seems "heavy".

thanks,

greg k-h

Re: [PATCH] uio: Fix uio driver to refcount device

[Brian Russell]

On 11/03/15 15:43, Greg Kroah-Hartman wrote:
> On Wed, Mar 11, 2015 at 03:31:42PM +0000, Brian Russell wrote:
>> Protect uio driver from crashing if its owner is hot unplugged while there
>> are open fds.
>> Signed-off-by: Brian Russell <brussell@brocade.com>
> 
> Minor nit, you need a blank line before your s-o-b: line.
> 

Ack.

> 
> 
>> ---
>>  drivers/uio/uio.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>> index 6276f13..70ce015 100644
>> --- a/drivers/uio/uio.c
>> +++ b/drivers/uio/uio.c
>> @@ -434,9 +434,11 @@ static int uio_open(struct inode *inode, struct file *filep)
>>  		goto out;
>>  	}
>>  
>> +	get_device(idev);
> 
> What is the real oops caused when a device is removed?  Protecting this
> with a reference count seems ok, but it seems "heavy".
> 

I'm seeing it with PCI hotplug. The PCI subsystem calls remove and the owner module in turn calls uio_unregister_device while app stil has open fds.

Brian

> thanks,
> 
> greg k-h
> 

Re: [PATCH] uio: Fix uio driver to refcount device

[Greg Kroah-Hartman]

On Wed, Mar 11, 2015 at 03:59:36PM +0000, Brian Russell wrote:
> 
> 
> On 11/03/15 15:43, Greg Kroah-Hartman wrote:
> > On Wed, Mar 11, 2015 at 03:31:42PM +0000, Brian Russell wrote:
> >> Protect uio driver from crashing if its owner is hot unplugged while there
> >> are open fds.
> >> Signed-off-by: Brian Russell <brussell@brocade.com>
> > 
> > Minor nit, you need a blank line before your s-o-b: line.
> > 
> 
> Ack.
> 
> > 
> > 
> >> ---
> >>  drivers/uio/uio.c | 8 +++++++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> >> index 6276f13..70ce015 100644
> >> --- a/drivers/uio/uio.c
> >> +++ b/drivers/uio/uio.c
> >> @@ -434,9 +434,11 @@ static int uio_open(struct inode *inode, struct file *filep)
> >>  		goto out;
> >>  	}
> >>  
> >> +	get_device(idev);
> > 
> > What is the real oops caused when a device is removed?  Protecting this
> > with a reference count seems ok, but it seems "heavy".
> > 
> 
> I'm seeing it with PCI hotplug. The PCI subsystem calls remove and the
> owner module in turn calls uio_unregister_device while app stil has
> open fds.

Sorry, I meant, what exactly is the oops message, with the callback?
What portion of code is crashing because we have an open fd?  The pci
remove path of the UIO core should be fixed to handle this properly.
Not to say that your patch isn't correct, just want to see the crash to
know for sure.

thanks,

greg k-h

Re: [PATCH] uio: Fix uio driver to refcount device

[Brian Russell]

On 11/03/15 16:02, Greg Kroah-Hartman wrote:
> On Wed, Mar 11, 2015 at 03:59:36PM +0000, Brian Russell wrote:
>>
>>
>> On 11/03/15 15:43, Greg Kroah-Hartman wrote:
>>> On Wed, Mar 11, 2015 at 03:31:42PM +0000, Brian Russell wrote:
>>>> Protect uio driver from crashing if its owner is hot unplugged while there
>>>> are open fds.
>>>> Signed-off-by: Brian Russell <brussell@brocade.com>
>>>
>>> Minor nit, you need a blank line before your s-o-b: line.
>>>
>>
>> Ack.
>>
>>>
>>>
>>>> ---
>>>>  drivers/uio/uio.c | 8 +++++++-
>>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>>>> index 6276f13..70ce015 100644
>>>> --- a/drivers/uio/uio.c
>>>> +++ b/drivers/uio/uio.c
>>>> @@ -434,9 +434,11 @@ static int uio_open(struct inode *inode, struct file *filep)
>>>>  		goto out;
>>>>  	}
>>>>  
>>>> +	get_device(idev);
>>>
>>> What is the real oops caused when a device is removed?  Protecting this
>>> with a reference count seems ok, but it seems "heavy".
>>>
>>
>> I'm seeing it with PCI hotplug. The PCI subsystem calls remove and the
>> owner module in turn calls uio_unregister_device while app stil has
>> open fds.
> 
> Sorry, I meant, what exactly is the oops message, with the callback?
> What portion of code is crashing because we have an open fd?  The pci
> remove path of the UIO core should be fixed to handle this properly.
> Not to say that your patch isn't correct, just want to see the crash to
> know for sure.
> 

Ah, I see, sorry:

 [  168.890968] BUG: unable to handle kernel paging request at ffff8800b2fb7e70
[  168.893141] IP: [<ffffffff810b5acc>] module_put+0xc/0x20
[  168.894076] PGD 1bc8067 PUD 0 
[  168.894679] Oops: 0002 [#1] SMP 
[  168.895322] Modules linked in: igb_uio(O) xfrm_user xfrm_algo l2tp_ip6 l2tp_ip l2tp_eth l2tp_netlink l2tp_core tun uio cpufreq_userspace cpufreq_powersave cpufreq_ondemand cpufreq_conservative ipv6 crc32_pclmul microcode aesni_intel aes_x86_64 lrw gf128mul glue_helper serio_raw ablk_helper ghash_clmulni_intel intel_agp intel_gtt psmouse virtio_console processor agpgart cryptd evdev button i2c_piix4 i2c_core pcspkr thermal_sys virtio_balloon usb_storage ohci_hcd squashfs loop hid_generic usbhid hid pata_acpi ata_generic virtio_blk virtio_net floppy ata_piix virtio_pci virtio_ring virtio crc32c_intel [last unloaded: igb_uio]
[  168.900849] CPU: 0 PID: 4494 Comm: dataplane Tainted: G        W  O 3.14.33-1-amd64-vyatta #1
[  168.900849] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[  168.900849] task: ffff880036bb60b0 ti: ffff880036956000 task.ti: ffff880036956000
[  168.900849] RIP: 0010:[<ffffffff810b5acc>]  [<ffffffff810b5acc>] module_put+0xc/0x20
[  168.900849] RSP: 0018:ffff880036957ea0  EFLAGS: 00010282
[  168.900849] RAX: 00000000333b7e68 RBX: ffff880036d61ce0 RCX: 0000000000000001
[  168.900849] RDX: 0000000000000000 RSI: ffff880079c92800 RDI: ffff880078dfbb98
[  168.900849] RBP: ffff880078dfbb98 R08: 0000000000000000 R09: 0000000000000000
[  168.900849] R10: ffffffff8110da65 R11: 0000000000000001 R12: 0000000000000000
[  168.900849] R13: ffff88004be4c540 R14: ffff88007c8957a0 R15: ffff880079c92810
[  168.900849] FS:  00007fc28b9f7700(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000
[  168.900849] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  168.900849] CR2: ffff8800b2fb7e70 CR3: 00000000368f9000 CR4: 00000000000406f0
[  168.900849] Stack:
[  168.900849]  ffffffffa0191383 ffff880079c92800 0000000000000008 ffff88004fec9c30
[  168.900849]  ffffffff811431e8 ffffffff81082810 0000000000000000 ffffffff81ac0a50
[  168.900849]  ffff880036bb6ba0 ffff880036bb60b0 0000000001a8dc80 0000000000000003
[  168.900849] Call Trace:
[  168.900849]  [<ffffffffa0191383>] ? uio_release+0x43/0x70 [uio]
[  168.900849]  [<ffffffff811431e8>] ? __fput+0xc8/0x230
[  168.900849]  [<ffffffff81082810>] ? sched_clock_cpu+0x90/0xc0
[  168.900849]  [<ffffffff810707c7>] ? task_work_run+0x97/0xd0
[  168.900849]  [<ffffffff8100ceaa>] ? do_notify_resume+0x8a/0xb0
[  168.900849]  [<ffffffff814d5272>] ? int_signal+0x12/0x17
[  168.900849] Code: 48 89 de 48 c7 c7 c0 5b 70 81 31 c0 e8 8e 61 41 00 eb d9 66 66 66 2e 0f 1f 84 00 00 00 00 00 48 85 ff 74 0c 48 8b 87 28 02 00 00 <65> 48 ff 40 08 f3 c3 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 41 
[  168.900849] RIP  [<ffffffff810b5acc>] module_put+0xc/0x20
[  168.900849]  RSP <ffff880036957ea0>
[  168.900849] CR2: ffff8800b2fb7e70
[  168.900849] ---[ end trace 20f273e64b20b382 ]---
[  168.900849] Kernel panic - not syncing: Fatal exception
[  168.900849] Kernel Offset: 0x0 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffff9fffffff)
[  168.900849] Rebooting in 60 seconds..



> thanks,
> 
> greg k-h
>