* [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference
@ 2016-11-06 15:40 Jörg Otte
2016-11-08 18:42 ` Linus Torvalds
0 siblings, 1 reply; 14+ messages in thread
From: Jörg Otte @ 2016-11-06 15:40 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linus Torvalds, Linux Kernel Mailing List, Linux Media Mailing List
Since v4.9-rc4 I get following crash in dvb-usb-cinergyT2 module.
dvb-usb: found a 'TerraTec/qanu USB2.0 Highspeed DVB-T Receiver' in warm state.
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100
PGD 0
Oops: 0002 [#1] SMP
Modules linked in: dvb_usb_cinergyT2(+) dvb_usb
CPU: 0 PID: 2029 Comm: modprobe Not tainted 4.9.0-rc4-dvbmod #24
Hardware name: FUJITSU LIFEBOOK A544/FJNBB35 , BIOS Version 1.17 05/09/2014
task: ffff88020e943840 task.stack: ffff8801f36ec000
RIP: 0010:[<ffffffff846617af>] [<ffffffff846617af>]
__mutex_lock_slowpath+0x6f/0x100
RSP: 0018:ffff8801f36efb10 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffff88021509bdc8 RCX: 00000000c0000100
RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff88021509bdcc
RBP: ffff8801f36efb58 R08: ffff88021f216320 R09: 0000000000100000
R10: ffff88021f216320 R11: 00000023fee6c5a1 R12: ffff88020e943840
R13: ffff88021509bdcc R14: 00000000ffffffff R15: ffff88021509bdd0
FS: 00007f21adb86740(0000) GS:ffff88021f200000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000215bce000 CR4: 00000000001406f0
Stack:
ffff88021509bdd0 0000000000000000 0000000000000000 ffffffffc0137c80
ffff88021509bdc8 ffff8801f5944000 0000000000000001 ffffffffc0136b00
ffff880213e52000 ffff88021509bdc8 ffffffff84661856 ffff88021509bd80
Call Trace:
[<ffffffff84661856>] ? mutex_lock+0x16/0x25
[<ffffffffc013616f>] ? cinergyt2_power_ctrl+0x1f/0x60 [dvb_usb_cinergyT2]
[<ffffffffc012e67e>] ? dvb_usb_device_init+0x21e/0x5d0 [dvb_usb]
[<ffffffffc0136021>] ? cinergyt2_usb_probe+0x21/0x50 [dvb_usb_cinergyT2]
[<ffffffff844326f3>] ? usb_probe_interface+0xf3/0x2a0
[<ffffffff8438e348>] ? driver_probe_device+0x208/0x2b0
[<ffffffff8438e477>] ? __driver_attach+0x87/0x90
[<ffffffff8438e3f0>] ? driver_probe_device+0x2b0/0x2b0
[<ffffffff8438c612>] ? bus_for_each_dev+0x52/0x80
[<ffffffff8438d983>] ? bus_add_driver+0x1a3/0x220
[<ffffffff8438ec06>] ? driver_register+0x56/0xd0
[<ffffffff84431527>] ? usb_register_driver+0x77/0x130
[<ffffffffc013a000>] ? 0xffffffffc013a000
[<ffffffff84000426>] ? do_one_initcall+0x46/0x180
[<ffffffff840eb2c8>] ? free_vmap_area_noflush+0x38/0x70
[<ffffffff840f3844>] ? kmem_cache_alloc+0x84/0xc0
[<ffffffff840b802c>] ? do_init_module+0x50/0x1be
[<ffffffff84095adb>] ? load_module+0x1d8b/0x2100
[<ffffffff84093020>] ? find_symbol_in_section+0xa0/0xa0
[<ffffffff84095fe9>] ? SyS_finit_module+0x89/0x90
[<ffffffff846637a0>] ? entry_SYSCALL_64_fastpath+0x13/0x94
Code: e8 a7 1d 00 00 8b 03 83 f8 01 0f 84 97 00 00 00 48 8b 43 10 4c
8d 7b 08 48 89 63 10 4c 89 3c 24 41 be ff ff ff ff 48 89 44 24 08 <48>
89 20 4c 89 64 24 10 eb 1a 49 c7 44 24 08 02 00 00 00 c6 43
RIP [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100
RSP <ffff8801f36efb10>
CR2: 0000000000000000
---[ end trace 648d79474da94e34 ]---
Thanks, Jörg
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference
2016-11-06 15:40 [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference Jörg Otte
@ 2016-11-08 18:42 ` Linus Torvalds
2016-11-08 20:22 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 14+ messages in thread
From: Linus Torvalds @ 2016-11-08 18:42 UTC (permalink / raw)
To: Jörg Otte, Patrick Boettcher, Mauro Carvalho Chehab
Cc: Mauro Carvalho Chehab, Linux Kernel Mailing List,
Linux Media Mailing List
On Sun, Nov 6, 2016 at 7:40 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
> Since v4.9-rc4 I get following crash in dvb-usb-cinergyT2 module.
Looks like it's commit 5ef8ed0e5608f ("[media] cinergyT2-core: don't
do DMA on stack"), which movced the DMA data array from the stack to
the "private" pointer. In the process it also added serialization in
the form of "data_mutex", but and now it oopses on that mutex because
the private pointer is NULL.
It looks like the "->private" pointer is allocated in dvb_usb_adapter_init()
cinergyt2_usb_probe ->
dvb_usb_device_init ->
dvb_usb_init() ->
dvb_usb_adapter_init()
but the dvb_usb_init() function calls dvb_usb_device_power_ctrl()
(which calls the "power_ctrl" function, which is
cinergyt2_power_ctrl() for that drive) *before* it initializes the
private field.
Mauro, Patrick, could dvb_usb_adapter_init() be called earlier, perhaps?
Linus
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference
2016-11-08 18:42 ` Linus Torvalds
@ 2016-11-08 20:22 ` Mauro Carvalho Chehab
2016-11-08 21:15 ` Benjamin Larsson
2016-11-09 11:09 ` Jörg Otte
0 siblings, 2 replies; 14+ messages in thread
From: Mauro Carvalho Chehab @ 2016-11-08 20:22 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jörg Otte, Patrick Boettcher, Mauro Carvalho Chehab,
Linux Kernel Mailing List, Linux Media Mailing List
Em Tue, 8 Nov 2016 10:42:03 -0800
Linus Torvalds <torvalds@linux-foundation.org> escreveu:
> On Sun, Nov 6, 2016 at 7:40 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
> > Since v4.9-rc4 I get following crash in dvb-usb-cinergyT2 module.
>
> Looks like it's commit 5ef8ed0e5608f ("[media] cinergyT2-core: don't
> do DMA on stack"), which movced the DMA data array from the stack to
> the "private" pointer. In the process it also added serialization in
> the form of "data_mutex", but and now it oopses on that mutex because
> the private pointer is NULL.
>
> It looks like the "->private" pointer is allocated in dvb_usb_adapter_init()
>
> cinergyt2_usb_probe ->
> dvb_usb_device_init ->
> dvb_usb_init() ->
> dvb_usb_adapter_init()
>
> but the dvb_usb_init() function calls dvb_usb_device_power_ctrl()
> (which calls the "power_ctrl" function, which is
> cinergyt2_power_ctrl() for that drive) *before* it initializes the
> private field.
>
> Mauro, Patrick, could dvb_usb_adapter_init() be called earlier, perhaps?
Calling it earlier won't work, as we need to load the firmware before
sending the power control commands on some devices.
Probably the best here is to pass an extra optional function parameter
that will initialize the mutex before calling any functions.
Btw, if it broke here, the DMA fixes will likely break on other drivers.
So, after Jörg tests this patch, I'll work on a patch series addressing
this issue on the other drivers I touched.
Regards,
Mauro
-
[PATCH RFC] cinergyT2-core: initialize the mutex early
NOTE: don't merge this patch as-is... I actually folded two patches
together here, in order to make easier to test, but the best is to
place the changes at the core first, and then the changes at the
drivers that would need an early init.
The mutex used to protect the URB data buffer needs to be
inialized early, as otherwise it will cause an OOPS:
dvb-usb: found a 'TerraTec/qanu USB2.0 Highspeed DVB-T Receiver' in warm state.
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100 PGD 0
Oops: 0002 [#1] SMP
Modules linked in: dvb_usb_cinergyT2(+) dvb_usb
CPU: 0 PID: 2029 Comm: modprobe Not tainted 4.9.0-rc4-dvbmod #24
Hardware name: FUJITSU LIFEBOOK A544/FJNBB35 , BIOS Version 1.17 05/09/2014
task: ffff88020e943840 task.stack: ffff8801f36ec000
RIP: 0010:[<ffffffff846617af>] [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100
RSP: 0018:ffff8801f36efb10 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffff88021509bdc8 RCX: 00000000c0000100
RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff88021509bdcc
RBP: ffff8801f36efb58 R08: ffff88021f216320 R09: 0000000000100000
R10: ffff88021f216320 R11: 00000023fee6c5a1 R12: ffff88020e943840
R13: ffff88021509bdcc R14: 00000000ffffffff R15: ffff88021509bdd0
FS: 00007f21adb86740(0000) GS:ffff88021f200000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000215bce000 CR4: 00000000001406f0
Stack:
ffff88021509bdd0 0000000000000000 0000000000000000 ffffffffc0137c80
ffff88021509bdc8 ffff8801f5944000 0000000000000001 ffffffffc0136b00
ffff880213e52000 ffff88021509bdc8 ffffffff84661856 ffff88021509bd80
Call Trace:
[<ffffffff84661856>] ? mutex_lock+0x16/0x25
[<ffffffffc013616f>] ? cinergyt2_power_ctrl+0x1f/0x60 [dvb_usb_cinergyT2]
[<ffffffffc012e67e>] ? dvb_usb_device_init+0x21e/0x5d0 [dvb_usb]
[<ffffffffc0136021>] ? cinergyt2_usb_probe+0x21/0x50 [dvb_usb_cinergyT2]
[<ffffffff844326f3>] ? usb_probe_interface+0xf3/0x2a0
[<ffffffff8438e348>] ? driver_probe_device+0x208/0x2b0
[<ffffffff8438e477>] ? __driver_attach+0x87/0x90
[<ffffffff8438e3f0>] ? driver_probe_device+0x2b0/0x2b0
[<ffffffff8438c612>] ? bus_for_each_dev+0x52/0x80
[<ffffffff8438d983>] ? bus_add_driver+0x1a3/0x220
[<ffffffff8438ec06>] ? driver_register+0x56/0xd0
[<ffffffff84431527>] ? usb_register_driver+0x77/0x130
[<ffffffffc013a000>] ? 0xffffffffc013a000
[<ffffffff84000426>] ? do_one_initcall+0x46/0x180
[<ffffffff840eb2c8>] ? free_vmap_area_noflush+0x38/0x70
[<ffffffff840f3844>] ? kmem_cache_alloc+0x84/0xc0
[<ffffffff840b802c>] ? do_init_module+0x50/0x1be
[<ffffffff84095adb>] ? load_module+0x1d8b/0x2100
[<ffffffff84093020>] ? find_symbol_in_section+0xa0/0xa0
[<ffffffff84095fe9>] ? SyS_finit_module+0x89/0x90
[<ffffffff846637a0>] ? entry_SYSCALL_64_fastpath+0x13/0x94
Code: e8 a7 1d 00 00 8b 03 83 f8 01 0f 84 97 00 00 00 48 8b 43 10 4c 8d 7b 08 48 89 63 10 4c 89 3c 24 41 be ff ff ff ff 48 89 44 24 08 <48> 89 20 4c 89 64 24 10 eb 1a 49 c7 44 24 08 02 00 00 00 c6 43 RIP [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100 RSP <ffff8801f36efb10>
CR2: 0000000000000000
Reported-by: Jörg Otte <jrg.otte@gmail.com>
Fixes: 6679a901c380 ("[media] cinergyT2-core: don't do DMA on stack")
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>From cbc7e48a86e8ffd16e49b10061120dfc417397f8 Mon Sep 17 00:00:00 2001
From: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Date: Tue, 8 Nov 2016 18:04:24 -0200
Subject: [PATCH] [media] dvb-usb: allow early initialization of usb device
priv data
On some drivers, we need to initialize a mutex before calling
power control or firmware download routines. Add an extra
parameter to allow it.
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
diff --git a/drivers/media/usb/dvb-usb/a800.c b/drivers/media/usb/dvb-usb/a800.c
index 7ba975bea96a..1e14f79aa57a 100644
--- a/drivers/media/usb/dvb-usb/a800.c
+++ b/drivers/media/usb/dvb-usb/a800.c
@@ -107,7 +107,7 @@ static int a800_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
return dvb_usb_device_init(intf, &a800_properties,
- THIS_MODULE, NULL, adapter_nr);
+ THIS_MODULE, NULL, adapter_nr, NULL);
}
/* do not change the order of the ID table */
diff --git a/drivers/media/usb/dvb-usb/af9005.c b/drivers/media/usb/dvb-usb/af9005.c
index b257780fb380..1625b4714d83 100644
--- a/drivers/media/usb/dvb-usb/af9005.c
+++ b/drivers/media/usb/dvb-usb/af9005.c
@@ -1009,7 +1009,7 @@ static int af9005_usb_probe(struct usb_interface *intf,
int ret;
ret = dvb_usb_device_init(intf, &af9005_properties,
- THIS_MODULE, &d, adapter_nr);
+ THIS_MODULE, &d, adapter_nr, NULL);
if (ret < 0)
return ret;
diff --git a/drivers/media/usb/dvb-usb/az6027.c b/drivers/media/usb/dvb-usb/az6027.c
index 2e711362847e..e8f73a96efd9 100644
--- a/drivers/media/usb/dvb-usb/az6027.c
+++ b/drivers/media/usb/dvb-usb/az6027.c
@@ -946,7 +946,7 @@ static int az6027_usb_probe(struct usb_interface *intf,
&az6027_properties,
THIS_MODULE,
NULL,
- adapter_nr);
+ adapter_nr, NULL);
}
/* I2C */
diff --git a/drivers/media/usb/dvb-usb/cinergyT2-core.c b/drivers/media/usb/dvb-usb/cinergyT2-core.c
index 8ac825413d5a..2aa99c52e39d 100644
--- a/drivers/media/usb/dvb-usb/cinergyT2-core.c
+++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c
@@ -206,24 +206,21 @@ static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
return ret;
}
-static int cinergyt2_usb_probe(struct usb_interface *intf,
- const struct usb_device_id *id)
+static int cinergyT2_init_mutex(struct dvb_usb_device *d)
{
- struct dvb_usb_device *d;
- struct cinergyt2_state *st;
- int ret;
-
- ret = dvb_usb_device_init(intf, &cinergyt2_properties,
- THIS_MODULE, &d, adapter_nr);
- if (ret < 0)
- return ret;
+ struct cinergyt2_state *st = d->priv;
- st = d->priv;
mutex_init(&st->data_mutex);
-
return 0;
}
+static int cinergyt2_usb_probe(struct usb_interface *intf,
+ const struct usb_device_id *id)
+{
+ return dvb_usb_device_init(intf, &cinergyt2_properties,
+ THIS_MODULE, NULL, adapter_nr,
+ cinergyT2_init_mutex);
+}
static struct usb_device_id cinergyt2_usb_table[] = {
{ USB_DEVICE(USB_VID_TERRATEC, 0x0038) },
diff --git a/drivers/media/usb/dvb-usb/cxusb.c b/drivers/media/usb/dvb-usb/cxusb.c
index 39772812269d..73c1a8568b55 100644
--- a/drivers/media/usb/dvb-usb/cxusb.c
+++ b/drivers/media/usb/dvb-usb/cxusb.c
@@ -1465,33 +1465,33 @@ static int cxusb_probe(struct usb_interface *intf,
struct cxusb_state *st;
if (0 == dvb_usb_device_init(intf, &cxusb_medion_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &cxusb_bluebird_lgh064f_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &cxusb_bluebird_dee1601_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &cxusb_bluebird_lgz201_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &cxusb_bluebird_dtt7579_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &cxusb_bluebird_dualdig4_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &cxusb_bluebird_nano2_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf,
&cxusb_bluebird_nano2_needsfirmware_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &cxusb_aver_a868r_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf,
&cxusb_bluebird_dualdig4_rev2_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &cxusb_d680_dmb_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &cxusb_mygica_d689_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &cxusb_mygica_t230_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0) {
st = d->priv;
mutex_init(&st->data_mutex);
diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c b/drivers/media/usb/dvb-usb/dib0700_core.c
index 92d5408684ac..3423b56c92b3 100644
--- a/drivers/media/usb/dvb-usb/dib0700_core.c
+++ b/drivers/media/usb/dvb-usb/dib0700_core.c
@@ -871,7 +871,7 @@ static int dib0700_probe(struct usb_interface *intf,
for (i = 0; i < dib0700_device_count; i++)
if (dvb_usb_device_init(intf, &dib0700_devices[i], THIS_MODULE,
- &dev, adapter_nr) == 0) {
+ &dev, adapter_nr, NULL) == 0) {
struct dib0700_state *st = dev->priv;
u32 hwversion, romversion, fw_version, fwtype;
diff --git a/drivers/media/usb/dvb-usb/dibusb-mb.c b/drivers/media/usb/dvb-usb/dibusb-mb.c
index a0057641cc86..de4ffe81e8d7 100644
--- a/drivers/media/usb/dvb-usb/dibusb-mb.c
+++ b/drivers/media/usb/dvb-usb/dibusb-mb.c
@@ -111,13 +111,13 @@ static int dibusb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
if (0 == dvb_usb_device_init(intf, &dibusb1_1_properties,
- THIS_MODULE, NULL, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &dibusb1_1_an2235_properties,
- THIS_MODULE, NULL, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &dibusb2_0b_properties,
- THIS_MODULE, NULL, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &artec_t1_usb2_properties,
- THIS_MODULE, NULL, adapter_nr))
+ THIS_MODULE, NULL, adapter_nr, NULL))
return 0;
return -EINVAL;
diff --git a/drivers/media/usb/dvb-usb/dibusb-mc.c b/drivers/media/usb/dvb-usb/dibusb-mc.c
index 08fb8a3f6e0c..d50731bb5372 100644
--- a/drivers/media/usb/dvb-usb/dibusb-mc.c
+++ b/drivers/media/usb/dvb-usb/dibusb-mc.c
@@ -23,7 +23,7 @@ static int dibusb_mc_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
return dvb_usb_device_init(intf, &dibusb_mc_properties, THIS_MODULE,
- NULL, adapter_nr);
+ NULL, adapter_nr, NULL);
}
/* do not change the order of the ID table */
diff --git a/drivers/media/usb/dvb-usb/digitv.c b/drivers/media/usb/dvb-usb/digitv.c
index 4284f6984dc1..3dad2293e598 100644
--- a/drivers/media/usb/dvb-usb/digitv.c
+++ b/drivers/media/usb/dvb-usb/digitv.c
@@ -269,7 +269,7 @@ static int digitv_probe(struct usb_interface *intf,
{
struct dvb_usb_device *d;
int ret = dvb_usb_device_init(intf, &digitv_properties, THIS_MODULE, &d,
- adapter_nr);
+ adapter_nr, NULL);
if (ret == 0) {
u8 b[4] = { 0 };
diff --git a/drivers/media/usb/dvb-usb/dtt200u.c b/drivers/media/usb/dvb-usb/dtt200u.c
index f88572c7ae7c..3255fee31433 100644
--- a/drivers/media/usb/dvb-usb/dtt200u.c
+++ b/drivers/media/usb/dvb-usb/dtt200u.c
@@ -149,15 +149,15 @@ static int dtt200u_usb_probe(struct usb_interface *intf,
struct dtt200u_state *st;
if (0 == dvb_usb_device_init(intf, &dtt200u_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &wt220u_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &wt220u_fc_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &wt220u_zl0353_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, &d, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &wt220u_miglia_properties,
- THIS_MODULE, &d, adapter_nr)) {
+ THIS_MODULE, &d, adapter_nr, NULL)) {
st = d->priv;
mutex_init(&st->data_mutex);
diff --git a/drivers/media/usb/dvb-usb/dtv5100.c b/drivers/media/usb/dvb-usb/dtv5100.c
index c60fb54f445f..36b553cb3133 100644
--- a/drivers/media/usb/dvb-usb/dtv5100.c
+++ b/drivers/media/usb/dvb-usb/dtv5100.c
@@ -165,7 +165,7 @@ static int dtv5100_probe(struct usb_interface *intf,
}
ret = dvb_usb_device_init(intf, &dtv5100_properties,
- THIS_MODULE, NULL, adapter_nr);
+ THIS_MODULE, NULL, adapter_nr, NULL);
if (ret)
return ret;
diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c
index 3896ba9a4179..66493fb25645 100644
--- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
+++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
@@ -138,23 +138,14 @@ static int dvb_usb_exit(struct dvb_usb_device *d)
return 0;
}
-static int dvb_usb_init(struct dvb_usb_device *d, short *adapter_nums)
+static int dvb_usb_init(struct dvb_usb_device *d, short *adapter_nums,
+ int (init_device)(struct dvb_usb_device *d))
{
int ret = 0;
mutex_init(&d->usb_mutex);
mutex_init(&d->i2c_mutex);
- d->state = DVB_USB_STATE_INIT;
-
- if (d->props.size_of_priv > 0) {
- d->priv = kzalloc(d->props.size_of_priv, GFP_KERNEL);
- if (d->priv == NULL) {
- err("no memory for priv in 'struct dvb_usb_device'");
- return -ENOMEM;
- }
- }
-
/* check the capabilities and set appropriate variables */
dvb_usb_device_power_ctrl(d, 1);
@@ -233,7 +224,8 @@ int dvb_usb_device_power_ctrl(struct dvb_usb_device *d, int onoff)
int dvb_usb_device_init(struct usb_interface *intf,
struct dvb_usb_device_properties *props,
struct module *owner, struct dvb_usb_device **du,
- short *adapter_nums)
+ short *adapter_nums,
+ int (init_device)(struct dvb_usb_device *d))
{
struct usb_device *udev = interface_to_usbdev(intf);
struct dvb_usb_device *d = NULL;
@@ -249,19 +241,42 @@ int dvb_usb_device_init(struct usb_interface *intf,
return -ENODEV;
}
- if (cold) {
- info("found a '%s' in cold state, will try to load a firmware", desc->name);
- ret = dvb_usb_download_firmware(udev, props);
- if (!props->no_reconnect || ret != 0)
- return ret;
- }
-
- info("found a '%s' in warm state.", desc->name);
d = kzalloc(sizeof(struct dvb_usb_device), GFP_KERNEL);
if (d == NULL) {
err("no memory for 'struct dvb_usb_device'");
return -ENOMEM;
}
+ d->state = DVB_USB_STATE_INIT;
+
+ if (d->props.size_of_priv > 0) {
+ d->priv = kzalloc(d->props.size_of_priv, GFP_KERNEL);
+ if (d->priv == NULL) {
+ err("no memory for priv in 'struct dvb_usb_device'");
+ ret = -ENOMEM;
+ goto err;
+ }
+ }
+
+ /*
+ * Some drivers may need to early initialize the device private data,
+ * for example, when a mutex is serializing URB reads/writes,
+ * in order for dvb_usb_device_power_ctrl() or firmware load to work.
+ */
+ if (init_device) {
+ ret = init_device(d);
+ if (ret < 0)
+ goto err;
+ }
+
+ if (cold) {
+ info("found a '%s' in cold state, will try to load a firmware",
+ desc->name);
+ ret = dvb_usb_download_firmware(udev, props);
+ if (!props->no_reconnect || ret != 0)
+ goto err;
+ } else {
+ info("found a '%s' in warm state.", desc->name);
+ }
d->udev = udev;
memcpy(&d->props, props, sizeof(struct dvb_usb_device_properties));
@@ -273,12 +288,17 @@ int dvb_usb_device_init(struct usb_interface *intf,
if (du != NULL)
*du = d;
- ret = dvb_usb_init(d, adapter_nums);
+ ret = dvb_usb_init(d, adapter_nums, init_device);
- if (ret == 0)
+ if (!ret) {
info("%s successfully initialized and connected.", desc->name);
- else
- info("%s error while loading driver (%d)", desc->name, ret);
+ return 0;
+ }
+err:
+ info("%s error while loading driver (%d)", desc->name, ret);
+
+ kfree(d->priv);
+ kfree(d);
return ret;
}
EXPORT_SYMBOL(dvb_usb_device_init);
diff --git a/drivers/media/usb/dvb-usb/dvb-usb.h b/drivers/media/usb/dvb-usb/dvb-usb.h
index 1448c3d27ea2..02c4dd3c206a 100644
--- a/drivers/media/usb/dvb-usb/dvb-usb.h
+++ b/drivers/media/usb/dvb-usb/dvb-usb.h
@@ -458,7 +458,9 @@ struct dvb_usb_device {
extern int dvb_usb_device_init(struct usb_interface *,
struct dvb_usb_device_properties *,
struct module *, struct dvb_usb_device **,
- short *adapter_nums);
+ short *adapter_nums,
+ int (init_device)(struct dvb_usb_device *d));
+
extern void dvb_usb_device_exit(struct usb_interface *);
/* the generic read/write method for device control */
diff --git a/drivers/media/usb/dvb-usb/dw2102.c b/drivers/media/usb/dvb-usb/dw2102.c
index 2c720cb2fb00..1cb8d96f485e 100644
--- a/drivers/media/usb/dvb-usb/dw2102.c
+++ b/drivers/media/usb/dvb-usb/dw2102.c
@@ -2284,27 +2284,27 @@ static int dw2102_probe(struct usb_interface *intf,
s421->adapter->fe[0].frontend_attach = m88rs2000_frontend_attach;
if (0 == dvb_usb_device_init(intf, &dw2102_properties,
- THIS_MODULE, NULL, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &dw2104_properties,
- THIS_MODULE, NULL, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &dw3101_properties,
- THIS_MODULE, NULL, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &s6x0_properties,
- THIS_MODULE, NULL, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, p1100,
- THIS_MODULE, NULL, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, s660,
- THIS_MODULE, NULL, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, p7500,
- THIS_MODULE, NULL, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, s421,
- THIS_MODULE, NULL, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &su3000_properties,
- THIS_MODULE, NULL, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &t220_properties,
- THIS_MODULE, NULL, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &tt_s2_4600_properties,
- THIS_MODULE, NULL, adapter_nr))
+ THIS_MODULE, NULL, adapter_nr, NULL))
return 0;
return -ENODEV;
diff --git a/drivers/media/usb/dvb-usb/friio.c b/drivers/media/usb/dvb-usb/friio.c
index 474a17e4db0c..3854ac7434ad 100644
--- a/drivers/media/usb/dvb-usb/friio.c
+++ b/drivers/media/usb/dvb-usb/friio.c
@@ -437,7 +437,7 @@ static int friio_probe(struct usb_interface *intf,
}
ret = dvb_usb_device_init(intf, &friio_properties,
- THIS_MODULE, &d, adapter_nr);
+ THIS_MODULE, &d, adapter_nr, NULL);
if (ret == 0)
friio_streaming_ctrl(&d->adapter[0], 1);
diff --git a/drivers/media/usb/dvb-usb/gp8psk.c b/drivers/media/usb/dvb-usb/gp8psk.c
index 2829e3082d15..cede0d8b0f8a 100644
--- a/drivers/media/usb/dvb-usb/gp8psk.c
+++ b/drivers/media/usb/dvb-usb/gp8psk.c
@@ -262,7 +262,7 @@ static int gp8psk_usb_probe(struct usb_interface *intf,
int ret;
struct usb_device *udev = interface_to_usbdev(intf);
ret = dvb_usb_device_init(intf, &gp8psk_properties,
- THIS_MODULE, NULL, adapter_nr);
+ THIS_MODULE, NULL, adapter_nr, NULL);
if (ret == 0) {
info("found Genpix USB device pID = %x (hex)",
le16_to_cpu(udev->descriptor.idProduct));
diff --git a/drivers/media/usb/dvb-usb/m920x.c b/drivers/media/usb/dvb-usb/m920x.c
index eafc5c82467f..b4c83f36abee 100644
--- a/drivers/media/usb/dvb-usb/m920x.c
+++ b/drivers/media/usb/dvb-usb/m920x.c
@@ -835,14 +835,14 @@ static int m920x_probe(struct usb_interface *intf,
*/
ret = dvb_usb_device_init(intf, &megasky_properties,
- THIS_MODULE, &d, adapter_nr);
+ THIS_MODULE, &d, adapter_nr, NULL);
if (ret == 0) {
rc_init_seq = megasky_rc_init;
goto found;
}
ret = dvb_usb_device_init(intf, &digivox_mini_ii_properties,
- THIS_MODULE, &d, adapter_nr);
+ THIS_MODULE, &d, adapter_nr, NULL);
if (ret == 0) {
/* No remote control, so no rc_init_seq */
goto found;
@@ -850,28 +850,28 @@ static int m920x_probe(struct usb_interface *intf,
/* This configures both tuners on the TV Walker Twin */
ret = dvb_usb_device_init(intf, &tvwalkertwin_properties,
- THIS_MODULE, &d, adapter_nr);
+ THIS_MODULE, &d, adapter_nr, NULL);
if (ret == 0) {
rc_init_seq = tvwalkertwin_rc_init;
goto found;
}
ret = dvb_usb_device_init(intf, &dposh_properties,
- THIS_MODULE, &d, adapter_nr);
+ THIS_MODULE, &d, adapter_nr, NULL);
if (ret == 0) {
/* Remote controller not supported yet. */
goto found;
}
ret = dvb_usb_device_init(intf, &pinnacle_pctv310e_properties,
- THIS_MODULE, &d, adapter_nr);
+ THIS_MODULE, &d, adapter_nr, NULL);
if (ret == 0) {
rc_init_seq = pinnacle310e_init;
goto found;
}
ret = dvb_usb_device_init(intf, &vp7049_properties,
- THIS_MODULE, &d, adapter_nr);
+ THIS_MODULE, &d, adapter_nr, NULL);
if (ret == 0) {
rc_init_seq = vp7049_rc_init;
goto found;
diff --git a/drivers/media/usb/dvb-usb/nova-t-usb2.c b/drivers/media/usb/dvb-usb/nova-t-usb2.c
index 1babd3341910..0d8f06430ca5 100644
--- a/drivers/media/usb/dvb-usb/nova-t-usb2.c
+++ b/drivers/media/usb/dvb-usb/nova-t-usb2.c
@@ -157,7 +157,7 @@ static int nova_t_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
return dvb_usb_device_init(intf, &nova_t_properties,
- THIS_MODULE, NULL, adapter_nr);
+ THIS_MODULE, NULL, adapter_nr, NULL);
}
/* do not change the order of the ID table */
diff --git a/drivers/media/usb/dvb-usb/opera1.c b/drivers/media/usb/dvb-usb/opera1.c
index 2566d2f1c2ad..9f2f156e2939 100644
--- a/drivers/media/usb/dvb-usb/opera1.c
+++ b/drivers/media/usb/dvb-usb/opera1.c
@@ -563,7 +563,7 @@ static int opera1_probe(struct usb_interface *intf,
}
if (0 != dvb_usb_device_init(intf, &opera1_properties,
- THIS_MODULE, NULL, adapter_nr))
+ THIS_MODULE, NULL, adapter_nr, NULL))
return -EINVAL;
return 0;
}
diff --git a/drivers/media/usb/dvb-usb/pctv452e.c b/drivers/media/usb/dvb-usb/pctv452e.c
index 07fa08be9e99..2f844ff39840 100644
--- a/drivers/media/usb/dvb-usb/pctv452e.c
+++ b/drivers/media/usb/dvb-usb/pctv452e.c
@@ -1059,9 +1059,9 @@ static int pctv452e_usb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
if (0 == dvb_usb_device_init(intf, &pctv452e_properties,
- THIS_MODULE, NULL, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &tt_connect_s2_3600_properties,
- THIS_MODULE, NULL, adapter_nr))
+ THIS_MODULE, NULL, adapter_nr, NULL))
return 0;
return -ENODEV;
diff --git a/drivers/media/usb/dvb-usb/technisat-usb2.c b/drivers/media/usb/dvb-usb/technisat-usb2.c
index 4706628a3ed5..80fc5043e4cf 100644
--- a/drivers/media/usb/dvb-usb/technisat-usb2.c
+++ b/drivers/media/usb/dvb-usb/technisat-usb2.c
@@ -765,7 +765,7 @@ static int technisat_usb2_probe(struct usb_interface *intf,
struct dvb_usb_device *dev;
if (dvb_usb_device_init(intf, &technisat_usb2_devices, THIS_MODULE,
- &dev, adapter_nr) != 0)
+ &dev, adapter_nr, NULL) != 0)
return -ENODEV;
if (dev) {
diff --git a/drivers/media/usb/dvb-usb/ttusb2.c b/drivers/media/usb/dvb-usb/ttusb2.c
index ecc207fbaf3c..192057ea58c0 100644
--- a/drivers/media/usb/dvb-usb/ttusb2.c
+++ b/drivers/media/usb/dvb-usb/ttusb2.c
@@ -604,11 +604,11 @@ static int ttusb2_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
if (0 == dvb_usb_device_init(intf, &ttusb2_properties,
- THIS_MODULE, NULL, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &ttusb2_properties_s2400,
- THIS_MODULE, NULL, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr, NULL) ||
0 == dvb_usb_device_init(intf, &ttusb2_properties_ct3650,
- THIS_MODULE, NULL, adapter_nr))
+ THIS_MODULE, NULL, adapter_nr, NULL))
return 0;
return -ENODEV;
}
diff --git a/drivers/media/usb/dvb-usb/umt-010.c b/drivers/media/usb/dvb-usb/umt-010.c
index 58ad5b4f856c..9af8cca5f2d2 100644
--- a/drivers/media/usb/dvb-usb/umt-010.c
+++ b/drivers/media/usb/dvb-usb/umt-010.c
@@ -78,7 +78,7 @@ static int umt_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
if (0 == dvb_usb_device_init(intf, &umt_properties,
- THIS_MODULE, NULL, adapter_nr))
+ THIS_MODULE, NULL, adapter_nr, NULL))
return 0;
return -EINVAL;
}
diff --git a/drivers/media/usb/dvb-usb/vp702x.c b/drivers/media/usb/dvb-usb/vp702x.c
index 40de33de90a7..e95a84e3f9de 100644
--- a/drivers/media/usb/dvb-usb/vp702x.c
+++ b/drivers/media/usb/dvb-usb/vp702x.c
@@ -337,7 +337,7 @@ static int vp702x_usb_probe(struct usb_interface *intf,
int ret;
ret = dvb_usb_device_init(intf, &vp702x_properties,
- THIS_MODULE, &d, adapter_nr);
+ THIS_MODULE, &d, adapter_nr, NULL);
if (ret)
goto out;
diff --git a/drivers/media/usb/dvb-usb/vp7045.c b/drivers/media/usb/dvb-usb/vp7045.c
index 13340af0d39c..350603bbe3da 100644
--- a/drivers/media/usb/dvb-usb/vp7045.c
+++ b/drivers/media/usb/dvb-usb/vp7045.c
@@ -225,7 +225,7 @@ static int vp7045_usb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
return dvb_usb_device_init(intf, &vp7045_properties,
- THIS_MODULE, NULL, adapter_nr);
+ THIS_MODULE, NULL, adapter_nr, NULL);
}
static struct usb_device_id vp7045_usb_table [] = {
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference
2016-11-08 20:22 ` Mauro Carvalho Chehab
@ 2016-11-08 21:15 ` Benjamin Larsson
2016-11-08 21:38 ` Mauro Carvalho Chehab
2016-11-09 11:09 ` Jörg Otte
1 sibling, 1 reply; 14+ messages in thread
From: Benjamin Larsson @ 2016-11-08 21:15 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Linus Torvalds
Cc: Jörg Otte, Patrick Boettcher, Mauro Carvalho Chehab,
Linux Kernel Mailing List, Linux Media Mailing List
On 11/08/2016 09:22 PM, Mauro Carvalho Chehab wrote:
> Em Tue, 8 Nov 2016 10:42:03 -0800
> Linus Torvalds <torvalds@linux-foundation.org> escreveu:
>
>> On Sun, Nov 6, 2016 at 7:40 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
>>> Since v4.9-rc4 I get following crash in dvb-usb-cinergyT2 module.
>>
>> Looks like it's commit 5ef8ed0e5608f ("[media] cinergyT2-core: don't
>> do DMA on stack"), which movced the DMA data array from the stack to
>> the "private" pointer. In the process it also added serialization in
>> the form of "data_mutex", but and now it oopses on that mutex because
>> the private pointer is NULL.
>>
>> It looks like the "->private" pointer is allocated in dvb_usb_adapter_init()
>>
>> cinergyt2_usb_probe ->
>> dvb_usb_device_init ->
>> dvb_usb_init() ->
>> dvb_usb_adapter_init()
>>
>> but the dvb_usb_init() function calls dvb_usb_device_power_ctrl()
>> (which calls the "power_ctrl" function, which is
>> cinergyt2_power_ctrl() for that drive) *before* it initializes the
>> private field.
>>
>> Mauro, Patrick, could dvb_usb_adapter_init() be called earlier, perhaps?
>
> Calling it earlier won't work, as we need to load the firmware before
> sending the power control commands on some devices.
>
> Probably the best here is to pass an extra optional function parameter
> that will initialize the mutex before calling any functions.
>
> Btw, if it broke here, the DMA fixes will likely break on other drivers.
> So, after Jörg tests this patch, I'll work on a patch series addressing
> this issue on the other drivers I touched.
>
> Regards,
> Mauro
Just for reference I got the following call trace a week ago. I looks
like this confirms that other drivers are affected also.
MvH
Benjamin Larsson
dvb-usb: found a 'Mygica T230 DVB-T/T2/C' in warm state.
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffff8182f688>] __mutex_lock_slowpath+0x98/0x130
PGD 0
Oops: 0002 [#1] SMP
Modules linked in: dvb_usb_cxusb(OE+) dib0070(OE) dvb_usb_rtl28xxu(OE+)
dvb_usb_dw2102(OE+) dvb_usb(OE) dvb_usb_v2(OE) dvb_core(OE) rc_core(OE)
ipmi_ssif media(OE) lpc_sch joydev input_leds i2c_ismt ipmi_si
8250_fintek ipmi_msghandler shpchp mac_hid kvm_intel kvm irqbypass
ib_iser rdma_cm iw_cm ib_cm ib_sa ib_mad ib_core ib_addr iscsi_tcp
libiscsi_tcp libiscsi scsi_transport_iscsi autofs4 btrfs raid10
multipath linear raid456 async_raid6_recov async_memcpy async_pq
async_xor async_tx xor raid6_pq libcrc32c raid0 raid1 hid_generic igb
dca usbhid ptp pps_core i2c_algo_bit hid ahci libahci fjes
CPU: 3 PID: 571 Comm: systemd-udevd Tainted: G OE
4.4.0-45-generic #66-Ubuntu
Hardware name: Supermicro X9SBAA/X9SBAA, BIOS 1.00 04/29/2014
task: ffff880231678000 ti: ffff880232048000 task.ti: ffff880232048000
RIP: 0010:[<ffffffff8182f688>] [<ffffffff8182f688>]
__mutex_lock_slowpath+0x98/0x130
RSP: 0018:ffff88023204b920 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffff88023152fea8 RCX: 0000000000000000
RDX: 0000000000000001 RSI: ffff880231678000 RDI: ffff88023152feac
RBP: ffff88023204b970 R08: ffff880232048000 R09: 0000000000000000
R10: 0000000000000325 R11: 0000000000000000 R12: ffff88023152feac
R13: ffff880231678000 R14: 00000000ffffffff R15: ffff88023152feb0
FS: 00007fc5390d68c0(0000) GS:ffff88023fd80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000023262f000 CR4: 00000000000006e0
Stack:
ffff88023152feb0 0000000000000000 ffff88023204b978 0000000027226a0c
01ff880200000000 ffff88023152fea8 ffff88023152fe40 ffff88023152fea8
ffff8800ba9e4000 0000000000000000 ffff88023204b988 ffffffff8182f73f
Call Trace:
[<ffffffff8182f73f>] mutex_lock+0x1f/0x30
[<ffffffffc04793da>] cxusb_ctrl_msg+0x5a/0xe0 [dvb_usb_cxusb]
[<ffffffffc0479ac8>] cxusb_power_ctrl+0x58/0x60 [dvb_usb_cxusb]
[<ffffffffc047b0db>] cxusb_d680_dmb_power_ctrl+0x2b/0x90 [dvb_usb_cxusb]
[<ffffffffc050e5a1>] dvb_usb_device_power_ctrl.part.4+0x31/0x50 [dvb_usb]
[<ffffffffc050e5ea>] dvb_usb_device_power_ctrl+0x2a/0x40 [dvb_usb]
[<ffffffffc050e844>] dvb_usb_device_init+0x244/0x750 [dvb_usb]
[<ffffffff8128baca>] ? kernfs_activate+0x7a/0xe0
[<ffffffffc0479360>] cxusb_probe+0x260/0x280 [dvb_usb_cxusb]
[<ffffffff8161e858>] usb_probe_interface+0x118/0x2f0
[<ffffffff815550d2>] driver_probe_device+0x222/0x4a0
[<ffffffff815553d4>] __driver_attach+0x84/0x90
[<ffffffff81555350>] ? driver_probe_device+0x4a0/0x4a0
[<ffffffff81552cfc>] bus_for_each_dev+0x6c/0xc0
[<ffffffff8155488e>] driver_attach+0x1e/0x20
[<ffffffff815543cb>] bus_add_driver+0x1eb/0x280
[<ffffffff81555ce0>] driver_register+0x60/0xe0
[<ffffffff8161d224>] usb_register_driver+0x84/0x140
[<ffffffffc03d8000>] ? 0xffffffffc03d8000
[<ffffffffc03d801e>] cxusb_driver_init+0x1e/0x1000 [dvb_usb_cxusb]
[<ffffffff81002123>] do_one_initcall+0xb3/0x200
[<ffffffff811cfa51>] ? __vunmap+0x91/0xe0
[<ffffffff811ebc03>] ? kmem_cache_alloc_trace+0x183/0x1f0
[<ffffffff811ec9fa>] ? kfree+0x13a/0x150
[<ffffffff8118caa3>] do_init_module+0x5f/0x1cf
[<ffffffff8110a2ff>] load_module+0x166f/0x1c10
[<ffffffff811068a0>] ? __symbol_put+0x60/0x60
[<ffffffff81213760>] ? kernel_read+0x50/0x80
[<ffffffff8110aae4>] SYSC_finit_module+0xb4/0xe0
[<ffffffff8110ab2e>] SyS_finit_module+0xe/0x10
[<ffffffff818318b2>] entry_SYSCALL_64_fastpath+0x16/0x71
Code: e8 ae 1f 00 00 8b 03 83 f8 01 0f 84 94 00 00 00 48 8b 43 10 4c 8d
7b 08 48 89 63 10 41 be ff ff ff ff 4c 89 3c 24 48 89 44 24 08 <48> 89
20 4c 89 6c 24 10 eb 1f 49 c7 45 00 02 00 00 00 4c 89 e7
RIP [<ffffffff8182f688>] __mutex_lock_slowpath+0x98/0x130
RSP <ffff88023204b920>
CR2: 0000000000000000
---[ end trace b28b9190ee8e4917 ]---
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference
2016-11-08 21:15 ` Benjamin Larsson
@ 2016-11-08 21:38 ` Mauro Carvalho Chehab
2016-11-09 19:57 ` Malcolm Priestley
0 siblings, 1 reply; 14+ messages in thread
From: Mauro Carvalho Chehab @ 2016-11-08 21:38 UTC (permalink / raw)
To: Benjamin Larsson
Cc: Linus Torvalds, Jörg Otte, Patrick Boettcher,
Mauro Carvalho Chehab, Linux Kernel Mailing List,
Linux Media Mailing List
Em Tue, 8 Nov 2016 22:15:24 +0100
Benjamin Larsson <benjamin@southpole.se> escreveu:
> On 11/08/2016 09:22 PM, Mauro Carvalho Chehab wrote:
> > Em Tue, 8 Nov 2016 10:42:03 -0800
> > Linus Torvalds <torvalds@linux-foundation.org> escreveu:
> >
> >> On Sun, Nov 6, 2016 at 7:40 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
> >>> Since v4.9-rc4 I get following crash in dvb-usb-cinergyT2 module.
> >>
> >> Looks like it's commit 5ef8ed0e5608f ("[media] cinergyT2-core: don't
> >> do DMA on stack"), which movced the DMA data array from the stack to
> >> the "private" pointer. In the process it also added serialization in
> >> the form of "data_mutex", but and now it oopses on that mutex because
> >> the private pointer is NULL.
> >>
> >> It looks like the "->private" pointer is allocated in dvb_usb_adapter_init()
> >>
> >> cinergyt2_usb_probe ->
> >> dvb_usb_device_init ->
> >> dvb_usb_init() ->
> >> dvb_usb_adapter_init()
> >>
> >> but the dvb_usb_init() function calls dvb_usb_device_power_ctrl()
> >> (which calls the "power_ctrl" function, which is
> >> cinergyt2_power_ctrl() for that drive) *before* it initializes the
> >> private field.
> >>
> >> Mauro, Patrick, could dvb_usb_adapter_init() be called earlier, perhaps?
> >
> > Calling it earlier won't work, as we need to load the firmware before
> > sending the power control commands on some devices.
> >
> > Probably the best here is to pass an extra optional function parameter
> > that will initialize the mutex before calling any functions.
> >
> > Btw, if it broke here, the DMA fixes will likely break on other drivers.
> > So, after Jörg tests this patch, I'll work on a patch series addressing
> > this issue on the other drivers I touched.
> >
> > Regards,
> > Mauro
>
> Just for reference I got the following call trace a week ago. I looks
> like this confirms that other drivers are affected also.
Yeah, I avoided serializing the logic that detects if the firmware is
loaded, but forgot that the power control had the same issue. The
newer dvb usb drivers use the dvb-usb-v2, so I didn't touch this
code for a while.
I'll need to review all touched drivers to be sure that they'll all
do the right thing. The good news is that it will likely simplify
the drivers' logic a little bit.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference
2016-11-08 20:22 ` Mauro Carvalho Chehab
2016-11-08 21:15 ` Benjamin Larsson
@ 2016-11-09 11:09 ` Jörg Otte
2016-11-09 19:07 ` Linus Torvalds
1 sibling, 1 reply; 14+ messages in thread
From: Jörg Otte @ 2016-11-09 11:09 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linus Torvalds, Patrick Boettcher, Mauro Carvalho Chehab,
Linux Kernel Mailing List, Linux Media Mailing List
2016-11-08 21:22 GMT+01:00 Mauro Carvalho Chehab <mchehab@s-opensource.com>:
> Em Tue, 8 Nov 2016 10:42:03 -0800
> Linus Torvalds <torvalds@linux-foundation.org> escreveu:
>
>> On Sun, Nov 6, 2016 at 7:40 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
>> > Since v4.9-rc4 I get following crash in dvb-usb-cinergyT2 module.
>>
>> Looks like it's commit 5ef8ed0e5608f ("[media] cinergyT2-core: don't
>> do DMA on stack"), which movced the DMA data array from the stack to
>> the "private" pointer. In the process it also added serialization in
>> the form of "data_mutex", but and now it oopses on that mutex because
>> the private pointer is NULL.
>>
>> It looks like the "->private" pointer is allocated in dvb_usb_adapter_init()
>>
>> cinergyt2_usb_probe ->
>> dvb_usb_device_init ->
>> dvb_usb_init() ->
>> dvb_usb_adapter_init()
>>
>> but the dvb_usb_init() function calls dvb_usb_device_power_ctrl()
>> (which calls the "power_ctrl" function, which is
>> cinergyt2_power_ctrl() for that drive) *before* it initializes the
>> private field.
>>
>> Mauro, Patrick, could dvb_usb_adapter_init() be called earlier, perhaps?
>
> Calling it earlier won't work, as we need to load the firmware before
> sending the power control commands on some devices.
>
> Probably the best here is to pass an extra optional function parameter
> that will initialize the mutex before calling any functions.
>
> Btw, if it broke here, the DMA fixes will likely break on other drivers.
> So, after Jörg tests this patch, I'll work on a patch series addressing
> this issue on the other drivers I touched.
>
> Regards,
> Mauro
>
> -
>
> [PATCH RFC] cinergyT2-core: initialize the mutex early
>
> NOTE: don't merge this patch as-is... I actually folded two patches
> together here, in order to make easier to test, but the best is to
> place the changes at the core first, and then the changes at the
> drivers that would need an early init.
>
> The mutex used to protect the URB data buffer needs to be
> inialized early, as otherwise it will cause an OOPS:
>
> dvb-usb: found a 'TerraTec/qanu USB2.0 Highspeed DVB-T Receiver' in warm state.
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100 PGD 0
> Oops: 0002 [#1] SMP
> Modules linked in: dvb_usb_cinergyT2(+) dvb_usb
> CPU: 0 PID: 2029 Comm: modprobe Not tainted 4.9.0-rc4-dvbmod #24
> Hardware name: FUJITSU LIFEBOOK A544/FJNBB35 , BIOS Version 1.17 05/09/2014
> task: ffff88020e943840 task.stack: ffff8801f36ec000
> RIP: 0010:[<ffffffff846617af>] [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100
> RSP: 0018:ffff8801f36efb10 EFLAGS: 00010282
> RAX: 0000000000000000 RBX: ffff88021509bdc8 RCX: 00000000c0000100
> RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff88021509bdcc
> RBP: ffff8801f36efb58 R08: ffff88021f216320 R09: 0000000000100000
> R10: ffff88021f216320 R11: 00000023fee6c5a1 R12: ffff88020e943840
> R13: ffff88021509bdcc R14: 00000000ffffffff R15: ffff88021509bdd0
> FS: 00007f21adb86740(0000) GS:ffff88021f200000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 0000000215bce000 CR4: 00000000001406f0
> Stack:
> ffff88021509bdd0 0000000000000000 0000000000000000 ffffffffc0137c80
> ffff88021509bdc8 ffff8801f5944000 0000000000000001 ffffffffc0136b00
> ffff880213e52000 ffff88021509bdc8 ffffffff84661856 ffff88021509bd80
> Call Trace:
> [<ffffffff84661856>] ? mutex_lock+0x16/0x25
> [<ffffffffc013616f>] ? cinergyt2_power_ctrl+0x1f/0x60 [dvb_usb_cinergyT2]
> [<ffffffffc012e67e>] ? dvb_usb_device_init+0x21e/0x5d0 [dvb_usb]
> [<ffffffffc0136021>] ? cinergyt2_usb_probe+0x21/0x50 [dvb_usb_cinergyT2]
> [<ffffffff844326f3>] ? usb_probe_interface+0xf3/0x2a0
> [<ffffffff8438e348>] ? driver_probe_device+0x208/0x2b0
> [<ffffffff8438e477>] ? __driver_attach+0x87/0x90
> [<ffffffff8438e3f0>] ? driver_probe_device+0x2b0/0x2b0
> [<ffffffff8438c612>] ? bus_for_each_dev+0x52/0x80
> [<ffffffff8438d983>] ? bus_add_driver+0x1a3/0x220
> [<ffffffff8438ec06>] ? driver_register+0x56/0xd0
> [<ffffffff84431527>] ? usb_register_driver+0x77/0x130
> [<ffffffffc013a000>] ? 0xffffffffc013a000
> [<ffffffff84000426>] ? do_one_initcall+0x46/0x180
> [<ffffffff840eb2c8>] ? free_vmap_area_noflush+0x38/0x70
> [<ffffffff840f3844>] ? kmem_cache_alloc+0x84/0xc0
> [<ffffffff840b802c>] ? do_init_module+0x50/0x1be
> [<ffffffff84095adb>] ? load_module+0x1d8b/0x2100
> [<ffffffff84093020>] ? find_symbol_in_section+0xa0/0xa0
> [<ffffffff84095fe9>] ? SyS_finit_module+0x89/0x90
> [<ffffffff846637a0>] ? entry_SYSCALL_64_fastpath+0x13/0x94
> Code: e8 a7 1d 00 00 8b 03 83 f8 01 0f 84 97 00 00 00 48 8b 43 10 4c 8d 7b 08 48 89 63 10 4c 89 3c 24 41 be ff ff ff ff 48 89 44 24 08 <48> 89 20 4c 89 64 24 10 eb 1a 49 c7 44 24 08 02 00 00 00 c6 43 RIP [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100 RSP <ffff8801f36efb10>
> CR2: 0000000000000000
>
> Reported-by: Jörg Otte <jrg.otte@gmail.com>
> Fixes: 6679a901c380 ("[media] cinergyT2-core: don't do DMA on stack")
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>
> From cbc7e48a86e8ffd16e49b10061120dfc417397f8 Mon Sep 17 00:00:00 2001
> From: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Date: Tue, 8 Nov 2016 18:04:24 -0200
> Subject: [PATCH] [media] dvb-usb: allow early initialization of usb device
> priv data
>
> On some drivers, we need to initialize a mutex before calling
> power control or firmware download routines. Add an extra
> parameter to allow it.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>
> diff --git a/drivers/media/usb/dvb-usb/a800.c b/drivers/media/usb/dvb-usb/a800.c
> index 7ba975bea96a..1e14f79aa57a 100644
> --- a/drivers/media/usb/dvb-usb/a800.c
> +++ b/drivers/media/usb/dvb-usb/a800.c
> @@ -107,7 +107,7 @@ static int a800_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> return dvb_usb_device_init(intf, &a800_properties,
> - THIS_MODULE, NULL, adapter_nr);
> + THIS_MODULE, NULL, adapter_nr, NULL);
> }
>
> /* do not change the order of the ID table */
> diff --git a/drivers/media/usb/dvb-usb/af9005.c b/drivers/media/usb/dvb-usb/af9005.c
> index b257780fb380..1625b4714d83 100644
> --- a/drivers/media/usb/dvb-usb/af9005.c
> +++ b/drivers/media/usb/dvb-usb/af9005.c
> @@ -1009,7 +1009,7 @@ static int af9005_usb_probe(struct usb_interface *intf,
> int ret;
>
> ret = dvb_usb_device_init(intf, &af9005_properties,
> - THIS_MODULE, &d, adapter_nr);
> + THIS_MODULE, &d, adapter_nr, NULL);
>
> if (ret < 0)
> return ret;
> diff --git a/drivers/media/usb/dvb-usb/az6027.c b/drivers/media/usb/dvb-usb/az6027.c
> index 2e711362847e..e8f73a96efd9 100644
> --- a/drivers/media/usb/dvb-usb/az6027.c
> +++ b/drivers/media/usb/dvb-usb/az6027.c
> @@ -946,7 +946,7 @@ static int az6027_usb_probe(struct usb_interface *intf,
> &az6027_properties,
> THIS_MODULE,
> NULL,
> - adapter_nr);
> + adapter_nr, NULL);
> }
>
> /* I2C */
> diff --git a/drivers/media/usb/dvb-usb/cinergyT2-core.c b/drivers/media/usb/dvb-usb/cinergyT2-core.c
> index 8ac825413d5a..2aa99c52e39d 100644
> --- a/drivers/media/usb/dvb-usb/cinergyT2-core.c
> +++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c
> @@ -206,24 +206,21 @@ static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
> return ret;
> }
>
> -static int cinergyt2_usb_probe(struct usb_interface *intf,
> - const struct usb_device_id *id)
> +static int cinergyT2_init_mutex(struct dvb_usb_device *d)
> {
> - struct dvb_usb_device *d;
> - struct cinergyt2_state *st;
> - int ret;
> -
> - ret = dvb_usb_device_init(intf, &cinergyt2_properties,
> - THIS_MODULE, &d, adapter_nr);
> - if (ret < 0)
> - return ret;
> + struct cinergyt2_state *st = d->priv;
>
> - st = d->priv;
> mutex_init(&st->data_mutex);
> -
> return 0;
> }
>
> +static int cinergyt2_usb_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + return dvb_usb_device_init(intf, &cinergyt2_properties,
> + THIS_MODULE, NULL, adapter_nr,
> + cinergyT2_init_mutex);
> +}
>
> static struct usb_device_id cinergyt2_usb_table[] = {
> { USB_DEVICE(USB_VID_TERRATEC, 0x0038) },
> diff --git a/drivers/media/usb/dvb-usb/cxusb.c b/drivers/media/usb/dvb-usb/cxusb.c
> index 39772812269d..73c1a8568b55 100644
> --- a/drivers/media/usb/dvb-usb/cxusb.c
> +++ b/drivers/media/usb/dvb-usb/cxusb.c
> @@ -1465,33 +1465,33 @@ static int cxusb_probe(struct usb_interface *intf,
> struct cxusb_state *st;
>
> if (0 == dvb_usb_device_init(intf, &cxusb_medion_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &cxusb_bluebird_lgh064f_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &cxusb_bluebird_dee1601_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &cxusb_bluebird_lgz201_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &cxusb_bluebird_dtt7579_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &cxusb_bluebird_dualdig4_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &cxusb_bluebird_nano2_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf,
> &cxusb_bluebird_nano2_needsfirmware_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &cxusb_aver_a868r_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf,
> &cxusb_bluebird_dualdig4_rev2_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &cxusb_d680_dmb_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &cxusb_mygica_d689_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &cxusb_mygica_t230_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0) {
> st = d->priv;
> mutex_init(&st->data_mutex);
> diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c b/drivers/media/usb/dvb-usb/dib0700_core.c
> index 92d5408684ac..3423b56c92b3 100644
> --- a/drivers/media/usb/dvb-usb/dib0700_core.c
> +++ b/drivers/media/usb/dvb-usb/dib0700_core.c
> @@ -871,7 +871,7 @@ static int dib0700_probe(struct usb_interface *intf,
>
> for (i = 0; i < dib0700_device_count; i++)
> if (dvb_usb_device_init(intf, &dib0700_devices[i], THIS_MODULE,
> - &dev, adapter_nr) == 0) {
> + &dev, adapter_nr, NULL) == 0) {
> struct dib0700_state *st = dev->priv;
> u32 hwversion, romversion, fw_version, fwtype;
>
> diff --git a/drivers/media/usb/dvb-usb/dibusb-mb.c b/drivers/media/usb/dvb-usb/dibusb-mb.c
> index a0057641cc86..de4ffe81e8d7 100644
> --- a/drivers/media/usb/dvb-usb/dibusb-mb.c
> +++ b/drivers/media/usb/dvb-usb/dibusb-mb.c
> @@ -111,13 +111,13 @@ static int dibusb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> if (0 == dvb_usb_device_init(intf, &dibusb1_1_properties,
> - THIS_MODULE, NULL, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &dibusb1_1_an2235_properties,
> - THIS_MODULE, NULL, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &dibusb2_0b_properties,
> - THIS_MODULE, NULL, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &artec_t1_usb2_properties,
> - THIS_MODULE, NULL, adapter_nr))
> + THIS_MODULE, NULL, adapter_nr, NULL))
> return 0;
>
> return -EINVAL;
> diff --git a/drivers/media/usb/dvb-usb/dibusb-mc.c b/drivers/media/usb/dvb-usb/dibusb-mc.c
> index 08fb8a3f6e0c..d50731bb5372 100644
> --- a/drivers/media/usb/dvb-usb/dibusb-mc.c
> +++ b/drivers/media/usb/dvb-usb/dibusb-mc.c
> @@ -23,7 +23,7 @@ static int dibusb_mc_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> return dvb_usb_device_init(intf, &dibusb_mc_properties, THIS_MODULE,
> - NULL, adapter_nr);
> + NULL, adapter_nr, NULL);
> }
>
> /* do not change the order of the ID table */
> diff --git a/drivers/media/usb/dvb-usb/digitv.c b/drivers/media/usb/dvb-usb/digitv.c
> index 4284f6984dc1..3dad2293e598 100644
> --- a/drivers/media/usb/dvb-usb/digitv.c
> +++ b/drivers/media/usb/dvb-usb/digitv.c
> @@ -269,7 +269,7 @@ static int digitv_probe(struct usb_interface *intf,
> {
> struct dvb_usb_device *d;
> int ret = dvb_usb_device_init(intf, &digitv_properties, THIS_MODULE, &d,
> - adapter_nr);
> + adapter_nr, NULL);
> if (ret == 0) {
> u8 b[4] = { 0 };
>
> diff --git a/drivers/media/usb/dvb-usb/dtt200u.c b/drivers/media/usb/dvb-usb/dtt200u.c
> index f88572c7ae7c..3255fee31433 100644
> --- a/drivers/media/usb/dvb-usb/dtt200u.c
> +++ b/drivers/media/usb/dvb-usb/dtt200u.c
> @@ -149,15 +149,15 @@ static int dtt200u_usb_probe(struct usb_interface *intf,
> struct dtt200u_state *st;
>
> if (0 == dvb_usb_device_init(intf, &dtt200u_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &wt220u_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &wt220u_fc_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &wt220u_zl0353_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, &d, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &wt220u_miglia_properties,
> - THIS_MODULE, &d, adapter_nr)) {
> + THIS_MODULE, &d, adapter_nr, NULL)) {
> st = d->priv;
> mutex_init(&st->data_mutex);
>
> diff --git a/drivers/media/usb/dvb-usb/dtv5100.c b/drivers/media/usb/dvb-usb/dtv5100.c
> index c60fb54f445f..36b553cb3133 100644
> --- a/drivers/media/usb/dvb-usb/dtv5100.c
> +++ b/drivers/media/usb/dvb-usb/dtv5100.c
> @@ -165,7 +165,7 @@ static int dtv5100_probe(struct usb_interface *intf,
> }
>
> ret = dvb_usb_device_init(intf, &dtv5100_properties,
> - THIS_MODULE, NULL, adapter_nr);
> + THIS_MODULE, NULL, adapter_nr, NULL);
> if (ret)
> return ret;
>
> diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> index 3896ba9a4179..66493fb25645 100644
> --- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
> +++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> @@ -138,23 +138,14 @@ static int dvb_usb_exit(struct dvb_usb_device *d)
> return 0;
> }
>
> -static int dvb_usb_init(struct dvb_usb_device *d, short *adapter_nums)
> +static int dvb_usb_init(struct dvb_usb_device *d, short *adapter_nums,
> + int (init_device)(struct dvb_usb_device *d))
> {
> int ret = 0;
>
> mutex_init(&d->usb_mutex);
> mutex_init(&d->i2c_mutex);
>
> - d->state = DVB_USB_STATE_INIT;
> -
> - if (d->props.size_of_priv > 0) {
> - d->priv = kzalloc(d->props.size_of_priv, GFP_KERNEL);
> - if (d->priv == NULL) {
> - err("no memory for priv in 'struct dvb_usb_device'");
> - return -ENOMEM;
> - }
> - }
> -
> /* check the capabilities and set appropriate variables */
> dvb_usb_device_power_ctrl(d, 1);
>
> @@ -233,7 +224,8 @@ int dvb_usb_device_power_ctrl(struct dvb_usb_device *d, int onoff)
> int dvb_usb_device_init(struct usb_interface *intf,
> struct dvb_usb_device_properties *props,
> struct module *owner, struct dvb_usb_device **du,
> - short *adapter_nums)
> + short *adapter_nums,
> + int (init_device)(struct dvb_usb_device *d))
> {
> struct usb_device *udev = interface_to_usbdev(intf);
> struct dvb_usb_device *d = NULL;
> @@ -249,19 +241,42 @@ int dvb_usb_device_init(struct usb_interface *intf,
> return -ENODEV;
> }
>
> - if (cold) {
> - info("found a '%s' in cold state, will try to load a firmware", desc->name);
> - ret = dvb_usb_download_firmware(udev, props);
> - if (!props->no_reconnect || ret != 0)
> - return ret;
> - }
> -
> - info("found a '%s' in warm state.", desc->name);
> d = kzalloc(sizeof(struct dvb_usb_device), GFP_KERNEL);
> if (d == NULL) {
> err("no memory for 'struct dvb_usb_device'");
> return -ENOMEM;
> }
> + d->state = DVB_USB_STATE_INIT;
> +
> + if (d->props.size_of_priv > 0) {
> + d->priv = kzalloc(d->props.size_of_priv, GFP_KERNEL);
> + if (d->priv == NULL) {
> + err("no memory for priv in 'struct dvb_usb_device'");
> + ret = -ENOMEM;
> + goto err;
> + }
> + }
> +
> + /*
> + * Some drivers may need to early initialize the device private data,
> + * for example, when a mutex is serializing URB reads/writes,
> + * in order for dvb_usb_device_power_ctrl() or firmware load to work.
> + */
> + if (init_device) {
> + ret = init_device(d);
> + if (ret < 0)
> + goto err;
> + }
> +
> + if (cold) {
> + info("found a '%s' in cold state, will try to load a firmware",
> + desc->name);
> + ret = dvb_usb_download_firmware(udev, props);
> + if (!props->no_reconnect || ret != 0)
> + goto err;
> + } else {
> + info("found a '%s' in warm state.", desc->name);
> + }
>
> d->udev = udev;
> memcpy(&d->props, props, sizeof(struct dvb_usb_device_properties));
> @@ -273,12 +288,17 @@ int dvb_usb_device_init(struct usb_interface *intf,
> if (du != NULL)
> *du = d;
>
> - ret = dvb_usb_init(d, adapter_nums);
> + ret = dvb_usb_init(d, adapter_nums, init_device);
>
> - if (ret == 0)
> + if (!ret) {
> info("%s successfully initialized and connected.", desc->name);
> - else
> - info("%s error while loading driver (%d)", desc->name, ret);
> + return 0;
> + }
> +err:
> + info("%s error while loading driver (%d)", desc->name, ret);
> +
> + kfree(d->priv);
> + kfree(d);
> return ret;
> }
> EXPORT_SYMBOL(dvb_usb_device_init);
> diff --git a/drivers/media/usb/dvb-usb/dvb-usb.h b/drivers/media/usb/dvb-usb/dvb-usb.h
> index 1448c3d27ea2..02c4dd3c206a 100644
> --- a/drivers/media/usb/dvb-usb/dvb-usb.h
> +++ b/drivers/media/usb/dvb-usb/dvb-usb.h
> @@ -458,7 +458,9 @@ struct dvb_usb_device {
> extern int dvb_usb_device_init(struct usb_interface *,
> struct dvb_usb_device_properties *,
> struct module *, struct dvb_usb_device **,
> - short *adapter_nums);
> + short *adapter_nums,
> + int (init_device)(struct dvb_usb_device *d));
> +
> extern void dvb_usb_device_exit(struct usb_interface *);
>
> /* the generic read/write method for device control */
> diff --git a/drivers/media/usb/dvb-usb/dw2102.c b/drivers/media/usb/dvb-usb/dw2102.c
> index 2c720cb2fb00..1cb8d96f485e 100644
> --- a/drivers/media/usb/dvb-usb/dw2102.c
> +++ b/drivers/media/usb/dvb-usb/dw2102.c
> @@ -2284,27 +2284,27 @@ static int dw2102_probe(struct usb_interface *intf,
> s421->adapter->fe[0].frontend_attach = m88rs2000_frontend_attach;
>
> if (0 == dvb_usb_device_init(intf, &dw2102_properties,
> - THIS_MODULE, NULL, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &dw2104_properties,
> - THIS_MODULE, NULL, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &dw3101_properties,
> - THIS_MODULE, NULL, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &s6x0_properties,
> - THIS_MODULE, NULL, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, p1100,
> - THIS_MODULE, NULL, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, s660,
> - THIS_MODULE, NULL, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, p7500,
> - THIS_MODULE, NULL, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, s421,
> - THIS_MODULE, NULL, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &su3000_properties,
> - THIS_MODULE, NULL, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &t220_properties,
> - THIS_MODULE, NULL, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &tt_s2_4600_properties,
> - THIS_MODULE, NULL, adapter_nr))
> + THIS_MODULE, NULL, adapter_nr, NULL))
> return 0;
>
> return -ENODEV;
> diff --git a/drivers/media/usb/dvb-usb/friio.c b/drivers/media/usb/dvb-usb/friio.c
> index 474a17e4db0c..3854ac7434ad 100644
> --- a/drivers/media/usb/dvb-usb/friio.c
> +++ b/drivers/media/usb/dvb-usb/friio.c
> @@ -437,7 +437,7 @@ static int friio_probe(struct usb_interface *intf,
> }
>
> ret = dvb_usb_device_init(intf, &friio_properties,
> - THIS_MODULE, &d, adapter_nr);
> + THIS_MODULE, &d, adapter_nr, NULL);
> if (ret == 0)
> friio_streaming_ctrl(&d->adapter[0], 1);
>
> diff --git a/drivers/media/usb/dvb-usb/gp8psk.c b/drivers/media/usb/dvb-usb/gp8psk.c
> index 2829e3082d15..cede0d8b0f8a 100644
> --- a/drivers/media/usb/dvb-usb/gp8psk.c
> +++ b/drivers/media/usb/dvb-usb/gp8psk.c
> @@ -262,7 +262,7 @@ static int gp8psk_usb_probe(struct usb_interface *intf,
> int ret;
> struct usb_device *udev = interface_to_usbdev(intf);
> ret = dvb_usb_device_init(intf, &gp8psk_properties,
> - THIS_MODULE, NULL, adapter_nr);
> + THIS_MODULE, NULL, adapter_nr, NULL);
> if (ret == 0) {
> info("found Genpix USB device pID = %x (hex)",
> le16_to_cpu(udev->descriptor.idProduct));
> diff --git a/drivers/media/usb/dvb-usb/m920x.c b/drivers/media/usb/dvb-usb/m920x.c
> index eafc5c82467f..b4c83f36abee 100644
> --- a/drivers/media/usb/dvb-usb/m920x.c
> +++ b/drivers/media/usb/dvb-usb/m920x.c
> @@ -835,14 +835,14 @@ static int m920x_probe(struct usb_interface *intf,
> */
>
> ret = dvb_usb_device_init(intf, &megasky_properties,
> - THIS_MODULE, &d, adapter_nr);
> + THIS_MODULE, &d, adapter_nr, NULL);
> if (ret == 0) {
> rc_init_seq = megasky_rc_init;
> goto found;
> }
>
> ret = dvb_usb_device_init(intf, &digivox_mini_ii_properties,
> - THIS_MODULE, &d, adapter_nr);
> + THIS_MODULE, &d, adapter_nr, NULL);
> if (ret == 0) {
> /* No remote control, so no rc_init_seq */
> goto found;
> @@ -850,28 +850,28 @@ static int m920x_probe(struct usb_interface *intf,
>
> /* This configures both tuners on the TV Walker Twin */
> ret = dvb_usb_device_init(intf, &tvwalkertwin_properties,
> - THIS_MODULE, &d, adapter_nr);
> + THIS_MODULE, &d, adapter_nr, NULL);
> if (ret == 0) {
> rc_init_seq = tvwalkertwin_rc_init;
> goto found;
> }
>
> ret = dvb_usb_device_init(intf, &dposh_properties,
> - THIS_MODULE, &d, adapter_nr);
> + THIS_MODULE, &d, adapter_nr, NULL);
> if (ret == 0) {
> /* Remote controller not supported yet. */
> goto found;
> }
>
> ret = dvb_usb_device_init(intf, &pinnacle_pctv310e_properties,
> - THIS_MODULE, &d, adapter_nr);
> + THIS_MODULE, &d, adapter_nr, NULL);
> if (ret == 0) {
> rc_init_seq = pinnacle310e_init;
> goto found;
> }
>
> ret = dvb_usb_device_init(intf, &vp7049_properties,
> - THIS_MODULE, &d, adapter_nr);
> + THIS_MODULE, &d, adapter_nr, NULL);
> if (ret == 0) {
> rc_init_seq = vp7049_rc_init;
> goto found;
> diff --git a/drivers/media/usb/dvb-usb/nova-t-usb2.c b/drivers/media/usb/dvb-usb/nova-t-usb2.c
> index 1babd3341910..0d8f06430ca5 100644
> --- a/drivers/media/usb/dvb-usb/nova-t-usb2.c
> +++ b/drivers/media/usb/dvb-usb/nova-t-usb2.c
> @@ -157,7 +157,7 @@ static int nova_t_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> return dvb_usb_device_init(intf, &nova_t_properties,
> - THIS_MODULE, NULL, adapter_nr);
> + THIS_MODULE, NULL, adapter_nr, NULL);
> }
>
> /* do not change the order of the ID table */
> diff --git a/drivers/media/usb/dvb-usb/opera1.c b/drivers/media/usb/dvb-usb/opera1.c
> index 2566d2f1c2ad..9f2f156e2939 100644
> --- a/drivers/media/usb/dvb-usb/opera1.c
> +++ b/drivers/media/usb/dvb-usb/opera1.c
> @@ -563,7 +563,7 @@ static int opera1_probe(struct usb_interface *intf,
> }
>
> if (0 != dvb_usb_device_init(intf, &opera1_properties,
> - THIS_MODULE, NULL, adapter_nr))
> + THIS_MODULE, NULL, adapter_nr, NULL))
> return -EINVAL;
> return 0;
> }
> diff --git a/drivers/media/usb/dvb-usb/pctv452e.c b/drivers/media/usb/dvb-usb/pctv452e.c
> index 07fa08be9e99..2f844ff39840 100644
> --- a/drivers/media/usb/dvb-usb/pctv452e.c
> +++ b/drivers/media/usb/dvb-usb/pctv452e.c
> @@ -1059,9 +1059,9 @@ static int pctv452e_usb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> if (0 == dvb_usb_device_init(intf, &pctv452e_properties,
> - THIS_MODULE, NULL, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &tt_connect_s2_3600_properties,
> - THIS_MODULE, NULL, adapter_nr))
> + THIS_MODULE, NULL, adapter_nr, NULL))
> return 0;
>
> return -ENODEV;
> diff --git a/drivers/media/usb/dvb-usb/technisat-usb2.c b/drivers/media/usb/dvb-usb/technisat-usb2.c
> index 4706628a3ed5..80fc5043e4cf 100644
> --- a/drivers/media/usb/dvb-usb/technisat-usb2.c
> +++ b/drivers/media/usb/dvb-usb/technisat-usb2.c
> @@ -765,7 +765,7 @@ static int technisat_usb2_probe(struct usb_interface *intf,
> struct dvb_usb_device *dev;
>
> if (dvb_usb_device_init(intf, &technisat_usb2_devices, THIS_MODULE,
> - &dev, adapter_nr) != 0)
> + &dev, adapter_nr, NULL) != 0)
> return -ENODEV;
>
> if (dev) {
> diff --git a/drivers/media/usb/dvb-usb/ttusb2.c b/drivers/media/usb/dvb-usb/ttusb2.c
> index ecc207fbaf3c..192057ea58c0 100644
> --- a/drivers/media/usb/dvb-usb/ttusb2.c
> +++ b/drivers/media/usb/dvb-usb/ttusb2.c
> @@ -604,11 +604,11 @@ static int ttusb2_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> if (0 == dvb_usb_device_init(intf, &ttusb2_properties,
> - THIS_MODULE, NULL, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &ttusb2_properties_s2400,
> - THIS_MODULE, NULL, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr, NULL) ||
> 0 == dvb_usb_device_init(intf, &ttusb2_properties_ct3650,
> - THIS_MODULE, NULL, adapter_nr))
> + THIS_MODULE, NULL, adapter_nr, NULL))
> return 0;
> return -ENODEV;
> }
> diff --git a/drivers/media/usb/dvb-usb/umt-010.c b/drivers/media/usb/dvb-usb/umt-010.c
> index 58ad5b4f856c..9af8cca5f2d2 100644
> --- a/drivers/media/usb/dvb-usb/umt-010.c
> +++ b/drivers/media/usb/dvb-usb/umt-010.c
> @@ -78,7 +78,7 @@ static int umt_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> if (0 == dvb_usb_device_init(intf, &umt_properties,
> - THIS_MODULE, NULL, adapter_nr))
> + THIS_MODULE, NULL, adapter_nr, NULL))
> return 0;
> return -EINVAL;
> }
> diff --git a/drivers/media/usb/dvb-usb/vp702x.c b/drivers/media/usb/dvb-usb/vp702x.c
> index 40de33de90a7..e95a84e3f9de 100644
> --- a/drivers/media/usb/dvb-usb/vp702x.c
> +++ b/drivers/media/usb/dvb-usb/vp702x.c
> @@ -337,7 +337,7 @@ static int vp702x_usb_probe(struct usb_interface *intf,
> int ret;
>
> ret = dvb_usb_device_init(intf, &vp702x_properties,
> - THIS_MODULE, &d, adapter_nr);
> + THIS_MODULE, &d, adapter_nr, NULL);
> if (ret)
> goto out;
>
> diff --git a/drivers/media/usb/dvb-usb/vp7045.c b/drivers/media/usb/dvb-usb/vp7045.c
> index 13340af0d39c..350603bbe3da 100644
> --- a/drivers/media/usb/dvb-usb/vp7045.c
> +++ b/drivers/media/usb/dvb-usb/vp7045.c
> @@ -225,7 +225,7 @@ static int vp7045_usb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> return dvb_usb_device_init(intf, &vp7045_properties,
> - THIS_MODULE, NULL, adapter_nr);
> + THIS_MODULE, NULL, adapter_nr, NULL);
> }
>
> static struct usb_device_id vp7045_usb_table [] = {
Tried patch with no success. Again a NULL ptr dereferece.
Thanks Jörg
Hardware name: FUJITSU LIFEBOOK A544/FJNBB35 , BIOS Version 1.17 05/09/2014
task: ffff8802134ac380 task.stack: ffff8801ed878000
RIP: 0010:[<ffffffffa406b444>] [<ffffffffa406b444>] __mutex_init+0x4/0x30
RSP: 0018:ffff8801ed87bb98 EFLAGS: 00010206
RAX: 0000000000000050 RBX: ffff8801ef23c000 RCX: ffffea0007bc8f01
RDX: ffffffffc035ef84 RSI: ffffffffc035d772 RDI: 0000000000000048
RBP: ffffffffc035db00 R08: ffffffffffffffe1 R09: ffffffffa4c5fa90
R10: 0000000000000004 R11: 0000000000000000 R12: ffffffffc035d030
R13: ffff880214c93000 R14: 0000000000000000 R15: ffff88020d942400
FS: 00007fb1359ae880(0000) GS:ffff88021f380000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000048 CR3: 00000001f53a2000 CR4: 00000000001406e0
Stack:
ffffffffc035d04e ffffffffc03555f0 ffffffffc035e970 ffffffffc035ddd0
ffffffffc035ec80 00000000a4397a2f ffffffffc035ddd0 ffff880214c93098
ffff880214c93000 ffffffffc035d8e8 ffff88020d942400 ffffffffc035d980
Call Trace:
[<ffffffffc035d04e>] ? cinergyT2_init_mutex+0x1e/0x30 [dvb_usb_cinergyT2]
[<ffffffffc03555f0>] ? dvb_usb_device_init+0x190/0x640 [dvb_usb]
[<ffffffffa44326f3>] ? usb_probe_interface+0xf3/0x2a0
[<ffffffffa438e348>] ? driver_probe_device+0x208/0x2b0
[<ffffffffa438e477>] ? __driver_attach+0x87/0x90
[<ffffffffa438e3f0>] ? driver_probe_device+0x2b0/0x2b0
[<ffffffffa438c612>] ? bus_for_each_dev+0x52/0x80
[<ffffffffa438d983>] ? bus_add_driver+0x1a3/0x220
[<ffffffffa438ec06>] ? driver_register+0x56/0xd0
[<ffffffffa4431527>] ? usb_register_driver+0x77/0x130
[<ffffffffc0361000>] ? 0xffffffffc0361000
[<ffffffffa4000426>] ? do_one_initcall+0x46/0x180
[<ffffffffa40eb2c8>] ? free_vmap_area_noflush+0x38/0x70
[<ffffffffa40f3844>] ? kmem_cache_alloc+0x84/0xc0
[<ffffffffa40b802c>] ? do_init_module+0x50/0x1be
[<ffffffffa4095adb>] ? load_module+0x1d8b/0x2100
[<ffffffffa4093020>] ? find_symbol_in_section+0xa0/0xa0
[<ffffffffa4095fe9>] ? SyS_finit_module+0x89/0x90
[<ffffffffa46637a0>] ? entry_SYSCALL_64_fastpath+0x13/0x94
Code: 5b 48 89 ee 89 c2 5d e9 bb f9 ff ff 41 b8 00 04 00 00 eb b1 c6
83 a1 00 00 00 00 c7 43 38 ff ff ff ff e9 2f ff ff ff 48 8d 47 08 <c7>
07 01 00 00 00 c7 47 04 00 00 00 00 48 89 47 08 48 89 47 10
RIP [<ffffffffa406b444>] __mutex_init+0x4/0x30
RSP <ffff8801ed87bb98>
CR2: 0000000000000048
---[ end trace 03576741447cea2c ]---
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference
2016-11-09 11:09 ` Jörg Otte
@ 2016-11-09 19:07 ` Linus Torvalds
2016-11-09 20:21 ` Mauro Carvalho Chehab
2016-11-10 8:40 ` Mauro Carvalho Chehab
0 siblings, 2 replies; 14+ messages in thread
From: Linus Torvalds @ 2016-11-09 19:07 UTC (permalink / raw)
To: Jörg Otte
Cc: Mauro Carvalho Chehab, Patrick Boettcher, Mauro Carvalho Chehab,
Linux Kernel Mailing List, Linux Media Mailing List
On Wed, Nov 9, 2016 at 3:09 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
>
> Tried patch with no success. Again a NULL ptr dereferece.
That patch was pure garbage, I think. Pretty much all the other
drivers that use the same approach will have the same issue. Adding
that init function just for the semaphore is crazy.
I suspect a much simpler approach is to just miove the "data_mutex"
away from the priv area and into "struct dvb_usb_device" and
"dvb_usb_adapter". Sure, that grows those structures a tiny bit, and
not every driver may need that mutex, but it simplifies things
enormously. Mauro?
Linus
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference
2016-11-08 21:38 ` Mauro Carvalho Chehab
@ 2016-11-09 19:57 ` Malcolm Priestley
2016-11-09 20:25 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 14+ messages in thread
From: Malcolm Priestley @ 2016-11-09 19:57 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Benjamin Larsson
Cc: Linus Torvalds, Jörg Otte, Patrick Boettcher,
Mauro Carvalho Chehab, Linux Kernel Mailing List,
Linux Media Mailing List
On 08/11/16 21:38, Mauro Carvalho Chehab wrote:
> Em Tue, 8 Nov 2016 22:15:24 +0100
> Benjamin Larsson <benjamin@southpole.se> escreveu:
>
>> On 11/08/2016 09:22 PM, Mauro Carvalho Chehab wrote:
>>> Em Tue, 8 Nov 2016 10:42:03 -0800
>>> Linus Torvalds <torvalds@linux-foundation.org> escreveu:
>>>
>>>> On Sun, Nov 6, 2016 at 7:40 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
>>>>> Since v4.9-rc4 I get following crash in dvb-usb-cinergyT2 module.
>>>>
>>>> Looks like it's commit 5ef8ed0e5608f ("[media] cinergyT2-core: don't
>>>> do DMA on stack"), which movced the DMA data array from the stack to
>>>> the "private" pointer. In the process it also added serialization in
>>>> the form of "data_mutex", but and now it oopses on that mutex because
>>>> the private pointer is NULL.
>>>>
>>>> It looks like the "->private" pointer is allocated in dvb_usb_adapter_init()
>>>>
>>>> cinergyt2_usb_probe ->
>>>> dvb_usb_device_init ->
>>>> dvb_usb_init() ->
>>>> dvb_usb_adapter_init()
>>>>
>>>> but the dvb_usb_init() function calls dvb_usb_device_power_ctrl()
>>>> (which calls the "power_ctrl" function, which is
>>>> cinergyt2_power_ctrl() for that drive) *before* it initializes the
>>>> private field.
>>>>
>>>> Mauro, Patrick, could dvb_usb_adapter_init() be called earlier, perhaps?
>>>
>>> Calling it earlier won't work, as we need to load the firmware before
>>> sending the power control commands on some devices.
>>>
>>> Probably the best here is to pass an extra optional function parameter
>>> that will initialize the mutex before calling any functions.
>>>
>>> Btw, if it broke here, the DMA fixes will likely break on other drivers.
>>> So, after Jörg tests this patch, I'll work on a patch series addressing
>>> this issue on the other drivers I touched.
>>>
>>> Regards,
>>> Mauro
>>
>> Just for reference I got the following call trace a week ago. I looks
>> like this confirms that other drivers are affected also.
>
> Yeah, I avoided serializing the logic that detects if the firmware is
> loaded, but forgot that the power control had the same issue. The
> newer dvb usb drivers use the dvb-usb-v2, so I didn't touch this
> code for a while.
I think the problem is that the usb buffer has been put in struct
cinergyt2_state private area which has not been initialized for initial
usb probing.
That was one of the main reasons for porting drivers to dvb-usb-v2.
Regards
Malcolm
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference
2016-11-09 19:07 ` Linus Torvalds
@ 2016-11-09 20:21 ` Mauro Carvalho Chehab
2016-11-10 8:40 ` Mauro Carvalho Chehab
1 sibling, 0 replies; 14+ messages in thread
From: Mauro Carvalho Chehab @ 2016-11-09 20:21 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jörg Otte, Patrick Boettcher, Mauro Carvalho Chehab,
Linux Kernel Mailing List, Linux Media Mailing List
Em Wed, 9 Nov 2016 11:07:35 -0800
Linus Torvalds <torvalds@linux-foundation.org> escreveu:
> On Wed, Nov 9, 2016 at 3:09 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
> >
> > Tried patch with no success. Again a NULL ptr dereferece.
>
> That patch was pure garbage, I think. Pretty much all the other
> drivers that use the same approach will have the same issue. Adding
> that init function just for the semaphore is crazy.
>
> I suspect a much simpler approach is to just miove the "data_mutex"
> away from the priv area and into "struct dvb_usb_device" and
> "dvb_usb_adapter". Sure, that grows those structures a tiny bit, and
> not every driver may need that mutex, but it simplifies things
> enormously. Mauro?
Yeah, makes sense.
I'll work on moving data_mutex to struct dvb_usb_device.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference
2016-11-09 19:57 ` Malcolm Priestley
@ 2016-11-09 20:25 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 14+ messages in thread
From: Mauro Carvalho Chehab @ 2016-11-09 20:25 UTC (permalink / raw)
To: Malcolm Priestley
Cc: Benjamin Larsson, Linus Torvalds, Jörg Otte,
Patrick Boettcher, Mauro Carvalho Chehab,
Linux Kernel Mailing List, Linux Media Mailing List
Em Wed, 9 Nov 2016 19:57:58 +0000
Malcolm Priestley <tvboxspy@gmail.com> escreveu:
> > Yeah, I avoided serializing the logic that detects if the firmware is
> > loaded, but forgot that the power control had the same issue. The
> > newer dvb usb drivers use the dvb-usb-v2, so I didn't touch this
> > code for a while.
>
> I think the problem is that the usb buffer has been put in struct
> cinergyt2_state private area which has not been initialized for initial
> usb probing.
>
> That was one of the main reasons for porting drivers to dvb-usb-v2.
True, but converting to dvb-usb-v2, is more complex. In particular,
dib0700 and dib3000 drivers rely on some things that may not be ported
to dvb-usb-v2.
So, I don't think we should do such change during the -rc cycle.
Also, such change requires testing. So, one with those hardware should
help with it, or the developer willing to do the conversion would
need to get those old hardware in hands.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference
2016-11-09 19:07 ` Linus Torvalds
2016-11-09 20:21 ` Mauro Carvalho Chehab
@ 2016-11-10 8:40 ` Mauro Carvalho Chehab
2016-11-10 11:15 ` Jörg Otte
1 sibling, 1 reply; 14+ messages in thread
From: Mauro Carvalho Chehab @ 2016-11-10 8:40 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jörg Otte, Patrick Boettcher, Mauro Carvalho Chehab,
Linux Kernel Mailing List, Linux Media Mailing List
Em Wed, 9 Nov 2016 11:07:35 -0800
Linus Torvalds <torvalds@linux-foundation.org> escreveu:
> On Wed, Nov 9, 2016 at 3:09 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
> >
> > Tried patch with no success. Again a NULL ptr dereferece.
>
> That patch was pure garbage, I think. Pretty much all the other
> drivers that use the same approach will have the same issue. Adding
> that init function just for the semaphore is crazy.
>
> I suspect a much simpler approach is to just miove the "data_mutex"
> away from the priv area and into "struct dvb_usb_device" and
> "dvb_usb_adapter". Sure, that grows those structures a tiny bit, and
> not every driver may need that mutex, but it simplifies things
> enormously. Mauro?
>
> Linus
[PATCH] cinergyT2-core: move data_mutex to struct dvb_usb_device
The data_mutex is initialized too late, as it is needed for
the device's power control, causing an OOPS:
dvb-usb: found a 'TerraTec/qanu USB2.0 Highspeed DVB-T Receiver' in warm state.
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100 PGD 0
Oops: 0002 [#1] SMP
Modules linked in: dvb_usb_cinergyT2(+) dvb_usb
CPU: 0 PID: 2029 Comm: modprobe Not tainted 4.9.0-rc4-dvbmod #24
Hardware name: FUJITSU LIFEBOOK A544/FJNBB35 , BIOS Version 1.17 05/09/2014
task: ffff88020e943840 task.stack: ffff8801f36ec000
RIP: 0010:[<ffffffff846617af>] [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100
RSP: 0018:ffff8801f36efb10 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffff88021509bdc8 RCX: 00000000c0000100
RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff88021509bdcc
RBP: ffff8801f36efb58 R08: ffff88021f216320 R09: 0000000000100000
R10: ffff88021f216320 R11: 00000023fee6c5a1 R12: ffff88020e943840
R13: ffff88021509bdcc R14: 00000000ffffffff R15: ffff88021509bdd0
FS: 00007f21adb86740(0000) GS:ffff88021f200000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000215bce000 CR4: 00000000001406f0
Stack:
ffff88021509bdd0 0000000000000000 0000000000000000 ffffffffc0137c80
ffff88021509bdc8 ffff8801f5944000 0000000000000001 ffffffffc0136b00
ffff880213e52000 ffff88021509bdc8 ffffffff84661856 ffff88021509bd80
Call Trace:
[<ffffffff84661856>] ? mutex_lock+0x16/0x25
[<ffffffffc013616f>] ? cinergyt2_power_ctrl+0x1f/0x60 [dvb_usb_cinergyT2]
[<ffffffffc012e67e>] ? dvb_usb_device_init+0x21e/0x5d0 [dvb_usb]
[<ffffffffc0136021>] ? cinergyt2_usb_probe+0x21/0x50 [dvb_usb_cinergyT2]
[<ffffffff844326f3>] ? usb_probe_interface+0xf3/0x2a0
[<ffffffff8438e348>] ? driver_probe_device+0x208/0x2b0
[<ffffffff8438e477>] ? __driver_attach+0x87/0x90
[<ffffffff8438e3f0>] ? driver_probe_device+0x2b0/0x2b0
[<ffffffff8438c612>] ? bus_for_each_dev+0x52/0x80
[<ffffffff8438d983>] ? bus_add_driver+0x1a3/0x220
[<ffffffff8438ec06>] ? driver_register+0x56/0xd0
[<ffffffff84431527>] ? usb_register_driver+0x77/0x130
[<ffffffffc013a000>] ? 0xffffffffc013a000
[<ffffffff84000426>] ? do_one_initcall+0x46/0x180
[<ffffffff840eb2c8>] ? free_vmap_area_noflush+0x38/0x70
[<ffffffff840f3844>] ? kmem_cache_alloc+0x84/0xc0
[<ffffffff840b802c>] ? do_init_module+0x50/0x1be
[<ffffffff84095adb>] ? load_module+0x1d8b/0x2100
[<ffffffff84093020>] ? find_symbol_in_section+0xa0/0xa0
[<ffffffff84095fe9>] ? SyS_finit_module+0x89/0x90
[<ffffffff846637a0>] ? entry_SYSCALL_64_fastpath+0x13/0x94
Code: e8 a7 1d 00 00 8b 03 83 f8 01 0f 84 97 00 00 00 48 8b 43 10 4c 8d 7b 08 48 89 63 10 4c 89 3c 24 41 be ff ff ff ff 48 89 44 24 08 <48> 89 20 4c 89 64 24 10 eb 1a 49 c7 44 24 08 02 00 00 00 c6 43 RIP [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100 RSP <ffff8801f36efb10>
CR2: 0000000000000000
So, move it to the struct dvb_usb_device and initialize it
before calling the driver's callbacks.
Reported-by: Jörg Otte <jrg.otte@gmail.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
diff --git a/drivers/media/usb/dvb-usb/cinergyT2-core.c b/drivers/media/usb/dvb-usb/cinergyT2-core.c
index 8ac825413d5a..87e3bd33900d 100644
--- a/drivers/media/usb/dvb-usb/cinergyT2-core.c
+++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c
@@ -42,7 +42,6 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
struct cinergyt2_state {
u8 rc_counter;
unsigned char data[64];
- struct mutex data_mutex;
};
/* We are missing a release hook with usb_device data */
@@ -56,12 +55,12 @@ static int cinergyt2_streaming_ctrl(struct dvb_usb_adapter *adap, int enable)
struct cinergyt2_state *st = d->priv;
int ret;
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
st->data[0] = CINERGYT2_EP1_CONTROL_STREAM_TRANSFER;
st->data[1] = enable ? 1 : 0;
ret = dvb_usb_generic_rw(d, st->data, 2, st->data, 64, 0);
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
return ret;
}
@@ -71,12 +70,12 @@ static int cinergyt2_power_ctrl(struct dvb_usb_device *d, int enable)
struct cinergyt2_state *st = d->priv;
int ret;
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
st->data[0] = CINERGYT2_EP1_SLEEP_MODE;
st->data[1] = enable ? 0 : 1;
ret = dvb_usb_generic_rw(d, st->data, 2, st->data, 3, 0);
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
return ret;
}
@@ -89,7 +88,7 @@ static int cinergyt2_frontend_attach(struct dvb_usb_adapter *adap)
adap->fe_adap[0].fe = cinergyt2_fe_attach(adap->dev);
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
st->data[0] = CINERGYT2_EP1_GET_FIRMWARE_VERSION;
ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 3, 0);
@@ -97,7 +96,7 @@ static int cinergyt2_frontend_attach(struct dvb_usb_adapter *adap)
deb_rc("cinergyt2_power_ctrl() Failed to retrieve sleep "
"state info\n");
}
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
/* Copy this pointer as we are gonna need it in the release phase */
cinergyt2_usb_device = adap->dev;
@@ -166,7 +165,7 @@ static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
*state = REMOTE_NO_KEY_PRESSED;
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS;
ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0);
@@ -202,7 +201,7 @@ static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
}
ret:
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
return ret;
}
@@ -210,7 +209,6 @@ static int cinergyt2_usb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
struct dvb_usb_device *d;
- struct cinergyt2_state *st;
int ret;
ret = dvb_usb_device_init(intf, &cinergyt2_properties,
@@ -218,9 +216,6 @@ static int cinergyt2_usb_probe(struct usb_interface *intf,
if (ret < 0)
return ret;
- st = d->priv;
- mutex_init(&st->data_mutex);
-
return 0;
}
diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c
index 3896ba9a4179..84308569e7dc 100644
--- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
+++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
@@ -142,6 +142,7 @@ static int dvb_usb_init(struct dvb_usb_device *d, short *adapter_nums)
{
int ret = 0;
+ mutex_init(&d->data_mutex);
mutex_init(&d->usb_mutex);
mutex_init(&d->i2c_mutex);
diff --git a/drivers/media/usb/dvb-usb/dvb-usb.h b/drivers/media/usb/dvb-usb/dvb-usb.h
index 1448c3d27ea2..12b71acee550 100644
--- a/drivers/media/usb/dvb-usb/dvb-usb.h
+++ b/drivers/media/usb/dvb-usb/dvb-usb.h
@@ -404,6 +404,7 @@ struct dvb_usb_adapter {
* Powered is in/decremented for each call to modify the state.
* @udev: pointer to the device's struct usb_device.
*
+ * @data_mutex: mutex to protect the data structure used to store URB data
* @usb_mutex: semaphore of USB control messages (reading needs two messages)
* @i2c_mutex: semaphore for i2c-transfers
*
@@ -433,6 +434,7 @@ struct dvb_usb_device {
int powered;
/* locking */
+ struct mutex data_mutex;
struct mutex usb_mutex;
/* i2c */
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference
2016-11-10 8:40 ` Mauro Carvalho Chehab
@ 2016-11-10 11:15 ` Jörg Otte
2016-11-11 13:55 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 14+ messages in thread
From: Jörg Otte @ 2016-11-10 11:15 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linus Torvalds, Patrick Boettcher, Mauro Carvalho Chehab,
Linux Kernel Mailing List, Linux Media Mailing List
2016-11-10 9:40 GMT+01:00 Mauro Carvalho Chehab <mchehab@s-opensource.com>:
> Em Wed, 9 Nov 2016 11:07:35 -0800
> Linus Torvalds <torvalds@linux-foundation.org> escreveu:
>
>> On Wed, Nov 9, 2016 at 3:09 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
>> >
>> > Tried patch with no success. Again a NULL ptr dereferece.
>>
>> That patch was pure garbage, I think. Pretty much all the other
>> drivers that use the same approach will have the same issue. Adding
>> that init function just for the semaphore is crazy.
>>
>> I suspect a much simpler approach is to just miove the "data_mutex"
>> away from the priv area and into "struct dvb_usb_device" and
>> "dvb_usb_adapter". Sure, that grows those structures a tiny bit, and
>> not every driver may need that mutex, but it simplifies things
>> enormously. Mauro?
>>
>> Linus
>
>
> [PATCH] cinergyT2-core: move data_mutex to struct dvb_usb_device
>
> The data_mutex is initialized too late, as it is needed for
> the device's power control, causing an OOPS:
>
> dvb-usb: found a 'TerraTec/qanu USB2.0 Highspeed DVB-T Receiver' in warm state.
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100 PGD 0
> Oops: 0002 [#1] SMP
> Modules linked in: dvb_usb_cinergyT2(+) dvb_usb
> CPU: 0 PID: 2029 Comm: modprobe Not tainted 4.9.0-rc4-dvbmod #24
> Hardware name: FUJITSU LIFEBOOK A544/FJNBB35 , BIOS Version 1.17 05/09/2014
> task: ffff88020e943840 task.stack: ffff8801f36ec000
> RIP: 0010:[<ffffffff846617af>] [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100
> RSP: 0018:ffff8801f36efb10 EFLAGS: 00010282
> RAX: 0000000000000000 RBX: ffff88021509bdc8 RCX: 00000000c0000100
> RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff88021509bdcc
> RBP: ffff8801f36efb58 R08: ffff88021f216320 R09: 0000000000100000
> R10: ffff88021f216320 R11: 00000023fee6c5a1 R12: ffff88020e943840
> R13: ffff88021509bdcc R14: 00000000ffffffff R15: ffff88021509bdd0
> FS: 00007f21adb86740(0000) GS:ffff88021f200000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 0000000215bce000 CR4: 00000000001406f0
> Stack:
> ffff88021509bdd0 0000000000000000 0000000000000000 ffffffffc0137c80
> ffff88021509bdc8 ffff8801f5944000 0000000000000001 ffffffffc0136b00
> ffff880213e52000 ffff88021509bdc8 ffffffff84661856 ffff88021509bd80
> Call Trace:
> [<ffffffff84661856>] ? mutex_lock+0x16/0x25
> [<ffffffffc013616f>] ? cinergyt2_power_ctrl+0x1f/0x60 [dvb_usb_cinergyT2]
> [<ffffffffc012e67e>] ? dvb_usb_device_init+0x21e/0x5d0 [dvb_usb]
> [<ffffffffc0136021>] ? cinergyt2_usb_probe+0x21/0x50 [dvb_usb_cinergyT2]
> [<ffffffff844326f3>] ? usb_probe_interface+0xf3/0x2a0
> [<ffffffff8438e348>] ? driver_probe_device+0x208/0x2b0
> [<ffffffff8438e477>] ? __driver_attach+0x87/0x90
> [<ffffffff8438e3f0>] ? driver_probe_device+0x2b0/0x2b0
> [<ffffffff8438c612>] ? bus_for_each_dev+0x52/0x80
> [<ffffffff8438d983>] ? bus_add_driver+0x1a3/0x220
> [<ffffffff8438ec06>] ? driver_register+0x56/0xd0
> [<ffffffff84431527>] ? usb_register_driver+0x77/0x130
> [<ffffffffc013a000>] ? 0xffffffffc013a000
> [<ffffffff84000426>] ? do_one_initcall+0x46/0x180
> [<ffffffff840eb2c8>] ? free_vmap_area_noflush+0x38/0x70
> [<ffffffff840f3844>] ? kmem_cache_alloc+0x84/0xc0
> [<ffffffff840b802c>] ? do_init_module+0x50/0x1be
> [<ffffffff84095adb>] ? load_module+0x1d8b/0x2100
> [<ffffffff84093020>] ? find_symbol_in_section+0xa0/0xa0
> [<ffffffff84095fe9>] ? SyS_finit_module+0x89/0x90
> [<ffffffff846637a0>] ? entry_SYSCALL_64_fastpath+0x13/0x94
> Code: e8 a7 1d 00 00 8b 03 83 f8 01 0f 84 97 00 00 00 48 8b 43 10 4c 8d 7b 08 48 89 63 10 4c 89 3c 24 41 be ff ff ff ff 48 89 44 24 08 <48> 89 20 4c 89 64 24 10 eb 1a 49 c7 44 24 08 02 00 00 00 c6 43 RIP [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100 RSP <ffff8801f36efb10>
> CR2: 0000000000000000
>
> So, move it to the struct dvb_usb_device and initialize it
> before calling the driver's callbacks.
>
> Reported-by: Jörg Otte <jrg.otte@gmail.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>
> diff --git a/drivers/media/usb/dvb-usb/cinergyT2-core.c b/drivers/media/usb/dvb-usb/cinergyT2-core.c
> index 8ac825413d5a..87e3bd33900d 100644
> --- a/drivers/media/usb/dvb-usb/cinergyT2-core.c
> +++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c
> @@ -42,7 +42,6 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
> struct cinergyt2_state {
> u8 rc_counter;
> unsigned char data[64];
> - struct mutex data_mutex;
> };
>
> /* We are missing a release hook with usb_device data */
> @@ -56,12 +55,12 @@ static int cinergyt2_streaming_ctrl(struct dvb_usb_adapter *adap, int enable)
> struct cinergyt2_state *st = d->priv;
> int ret;
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
> st->data[0] = CINERGYT2_EP1_CONTROL_STREAM_TRANSFER;
> st->data[1] = enable ? 1 : 0;
>
> ret = dvb_usb_generic_rw(d, st->data, 2, st->data, 64, 0);
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
>
> return ret;
> }
> @@ -71,12 +70,12 @@ static int cinergyt2_power_ctrl(struct dvb_usb_device *d, int enable)
> struct cinergyt2_state *st = d->priv;
> int ret;
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
> st->data[0] = CINERGYT2_EP1_SLEEP_MODE;
> st->data[1] = enable ? 0 : 1;
>
> ret = dvb_usb_generic_rw(d, st->data, 2, st->data, 3, 0);
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
>
> return ret;
> }
> @@ -89,7 +88,7 @@ static int cinergyt2_frontend_attach(struct dvb_usb_adapter *adap)
>
> adap->fe_adap[0].fe = cinergyt2_fe_attach(adap->dev);
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
> st->data[0] = CINERGYT2_EP1_GET_FIRMWARE_VERSION;
>
> ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 3, 0);
> @@ -97,7 +96,7 @@ static int cinergyt2_frontend_attach(struct dvb_usb_adapter *adap)
> deb_rc("cinergyt2_power_ctrl() Failed to retrieve sleep "
> "state info\n");
> }
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
>
> /* Copy this pointer as we are gonna need it in the release phase */
> cinergyt2_usb_device = adap->dev;
> @@ -166,7 +165,7 @@ static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
>
> *state = REMOTE_NO_KEY_PRESSED;
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
> st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS;
>
> ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0);
> @@ -202,7 +201,7 @@ static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
> }
>
> ret:
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
> return ret;
> }
>
> @@ -210,7 +209,6 @@ static int cinergyt2_usb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> struct dvb_usb_device *d;
> - struct cinergyt2_state *st;
> int ret;
>
> ret = dvb_usb_device_init(intf, &cinergyt2_properties,
> @@ -218,9 +216,6 @@ static int cinergyt2_usb_probe(struct usb_interface *intf,
> if (ret < 0)
> return ret;
>
> - st = d->priv;
> - mutex_init(&st->data_mutex);
> -
> return 0;
> }
>
> diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> index 3896ba9a4179..84308569e7dc 100644
> --- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
> +++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> @@ -142,6 +142,7 @@ static int dvb_usb_init(struct dvb_usb_device *d, short *adapter_nums)
> {
> int ret = 0;
>
> + mutex_init(&d->data_mutex);
> mutex_init(&d->usb_mutex);
> mutex_init(&d->i2c_mutex);
>
> diff --git a/drivers/media/usb/dvb-usb/dvb-usb.h b/drivers/media/usb/dvb-usb/dvb-usb.h
> index 1448c3d27ea2..12b71acee550 100644
> --- a/drivers/media/usb/dvb-usb/dvb-usb.h
> +++ b/drivers/media/usb/dvb-usb/dvb-usb.h
> @@ -404,6 +404,7 @@ struct dvb_usb_adapter {
> * Powered is in/decremented for each call to modify the state.
> * @udev: pointer to the device's struct usb_device.
> *
> + * @data_mutex: mutex to protect the data structure used to store URB data
> * @usb_mutex: semaphore of USB control messages (reading needs two messages)
> * @i2c_mutex: semaphore for i2c-transfers
> *
> @@ -433,6 +434,7 @@ struct dvb_usb_device {
> int powered;
>
> /* locking */
> + struct mutex data_mutex;
> struct mutex usb_mutex;
>
> /* i2c */
>
>
The patch works for me.
Thanks, Jörg
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference
2016-11-10 11:15 ` Jörg Otte
@ 2016-11-11 13:55 ` Mauro Carvalho Chehab
2016-11-11 16:43 ` Jörg Otte
0 siblings, 1 reply; 14+ messages in thread
From: Mauro Carvalho Chehab @ 2016-11-11 13:55 UTC (permalink / raw)
To: Jörg Otte, Benjamin Larsson
Cc: Mauro Carvalho Chehab, Linus Torvalds, Patrick Boettcher,
Mauro Carvalho Chehab, Linux Kernel Mailing List,
Linux Media Mailing List
Em Thu, 10 Nov 2016 12:15:39 +0100
Jörg Otte <jrg.otte@gmail.com> escreveu:
> 2016-11-10 9:40 GMT+01:00 Mauro Carvalho Chehab <mchehab@s-opensource.com>:
> > Em Wed, 9 Nov 2016 11:07:35 -0800
> > Linus Torvalds <torvalds@linux-foundation.org> escreveu:
> >
> >> On Wed, Nov 9, 2016 at 3:09 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
> >> >
> >> > Tried patch with no success. Again a NULL ptr dereferece.
> >>
> >> I suspect a much simpler approach is to just miove the "data_mutex"
> >> away from the priv area and into "struct dvb_usb_device" and
> >> "dvb_usb_adapter". Sure, that grows those structures a tiny bit, and
> >> not every driver may need that mutex, but it simplifies things
> >> enormously. Mauro?
> >>
> >> Linus
> The patch works for me.
Thanks for testing! That's the (hopefully) final version of it,
with the fix applied to the other dvb-usb drivers that use
data_mutex (except for the frontend ones, with uses a different
private structure, and where the mutex is initialized at attach).
Benjamin,
Could you please test it?
Thanks!
Mauro
-
[PATCH] dvb-usb: move data_mutex to struct dvb_usb_device
The data_mutex is initialized too late, as it is needed for
each device driver's power control, causing an OOPS:
dvb-usb: found a 'TerraTec/qanu USB2.0 Highspeed DVB-T Receiver' in warm state.
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100 PGD 0
Oops: 0002 [#1] SMP
Modules linked in: dvb_usb_cinergyT2(+) dvb_usb
CPU: 0 PID: 2029 Comm: modprobe Not tainted 4.9.0-rc4-dvbmod #24
Hardware name: FUJITSU LIFEBOOK A544/FJNBB35 , BIOS Version 1.17 05/09/2014
task: ffff88020e943840 task.stack: ffff8801f36ec000
RIP: 0010:[<ffffffff846617af>] [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100
RSP: 0018:ffff8801f36efb10 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffff88021509bdc8 RCX: 00000000c0000100
RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff88021509bdcc
RBP: ffff8801f36efb58 R08: ffff88021f216320 R09: 0000000000100000
R10: ffff88021f216320 R11: 00000023fee6c5a1 R12: ffff88020e943840
R13: ffff88021509bdcc R14: 00000000ffffffff R15: ffff88021509bdd0
FS: 00007f21adb86740(0000) GS:ffff88021f200000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000215bce000 CR4: 00000000001406f0
Stack:
ffff88021509bdd0 0000000000000000 0000000000000000 ffffffffc0137c80
ffff88021509bdc8 ffff8801f5944000 0000000000000001 ffffffffc0136b00
ffff880213e52000 ffff88021509bdc8 ffffffff84661856 ffff88021509bd80
Call Trace:
[<ffffffff84661856>] ? mutex_lock+0x16/0x25
[<ffffffffc013616f>] ? cinergyt2_power_ctrl+0x1f/0x60 [dvb_usb_cinergyT2]
[<ffffffffc012e67e>] ? dvb_usb_device_init+0x21e/0x5d0 [dvb_usb]
[<ffffffffc0136021>] ? cinergyt2_usb_probe+0x21/0x50 [dvb_usb_cinergyT2]
[<ffffffff844326f3>] ? usb_probe_interface+0xf3/0x2a0
[<ffffffff8438e348>] ? driver_probe_device+0x208/0x2b0
[<ffffffff8438e477>] ? __driver_attach+0x87/0x90
[<ffffffff8438e3f0>] ? driver_probe_device+0x2b0/0x2b0
[<ffffffff8438c612>] ? bus_for_each_dev+0x52/0x80
[<ffffffff8438d983>] ? bus_add_driver+0x1a3/0x220
[<ffffffff8438ec06>] ? driver_register+0x56/0xd0
[<ffffffff84431527>] ? usb_register_driver+0x77/0x130
[<ffffffffc013a000>] ? 0xffffffffc013a000
[<ffffffff84000426>] ? do_one_initcall+0x46/0x180
[<ffffffff840eb2c8>] ? free_vmap_area_noflush+0x38/0x70
[<ffffffff840f3844>] ? kmem_cache_alloc+0x84/0xc0
[<ffffffff840b802c>] ? do_init_module+0x50/0x1be
[<ffffffff84095adb>] ? load_module+0x1d8b/0x2100
[<ffffffff84093020>] ? find_symbol_in_section+0xa0/0xa0
[<ffffffff84095fe9>] ? SyS_finit_module+0x89/0x90
[<ffffffff846637a0>] ? entry_SYSCALL_64_fastpath+0x13/0x94
Code: e8 a7 1d 00 00 8b 03 83 f8 01 0f 84 97 00 00 00 48 8b 43 10 4c 8d 7b 08 48 89 63 10 4c 89 3c 24 41 be ff ff ff ff 48 89 44 24 08 <48> 89 20 4c 89 64 24 10 eb 1a 49 c7 44 24 08 02 00 00 00 c6 43 RIP [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100 RSP <ffff8801f36efb10>
CR2: 0000000000000000
So, move it to the struct dvb_usb_device and initialize it
before calling the driver's callbacks.
Reported-by: Jörg Otte <jrg.otte@gmail.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
diff --git a/drivers/media/usb/dvb-usb/af9005.c b/drivers/media/usb/dvb-usb/af9005.c
index b257780fb380..7853261906b1 100644
--- a/drivers/media/usb/dvb-usb/af9005.c
+++ b/drivers/media/usb/dvb-usb/af9005.c
@@ -53,7 +53,6 @@ struct af9005_device_state {
u8 sequence;
int led_state;
unsigned char data[256];
- struct mutex data_mutex;
};
static int af9005_generic_read_write(struct dvb_usb_device *d, u16 reg,
@@ -72,7 +71,7 @@ static int af9005_generic_read_write(struct dvb_usb_device *d, u16 reg,
return -EINVAL;
}
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
st->data[0] = 14; /* rest of buffer length low */
st->data[1] = 0; /* rest of buffer length high */
@@ -140,7 +139,7 @@ static int af9005_generic_read_write(struct dvb_usb_device *d, u16 reg,
values[i] = st->data[8 + i];
ret:
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
return ret;
}
@@ -481,7 +480,7 @@ int af9005_send_command(struct dvb_usb_device *d, u8 command, u8 * wbuf,
}
packet_len = wlen + 5;
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
st->data[0] = (u8) (packet_len & 0xff);
st->data[1] = (u8) ((packet_len & 0xff00) >> 8);
@@ -512,7 +511,7 @@ int af9005_send_command(struct dvb_usb_device *d, u8 command, u8 * wbuf,
rbuf[i] = st->data[i + 7];
}
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
return ret;
}
@@ -523,7 +522,7 @@ int af9005_read_eeprom(struct dvb_usb_device *d, u8 address, u8 * values,
u8 seq;
int ret, i;
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
memset(st->data, 0, sizeof(st->data));
@@ -559,7 +558,7 @@ int af9005_read_eeprom(struct dvb_usb_device *d, u8 address, u8 * values,
for (i = 0; i < len; i++)
values[i] = st->data[6 + i];
}
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
return ret;
}
@@ -847,7 +846,7 @@ static int af9005_rc_query(struct dvb_usb_device *d, u32 * event, int *state)
return 0;
}
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
/* deb_info("rc_query\n"); */
st->data[0] = 3; /* rest of packet length low */
@@ -890,7 +889,7 @@ static int af9005_rc_query(struct dvb_usb_device *d, u32 * event, int *state)
}
ret:
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
return ret;
}
@@ -1004,20 +1003,8 @@ static struct dvb_usb_device_properties af9005_properties;
static int af9005_usb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
- struct dvb_usb_device *d;
- struct af9005_device_state *st;
- int ret;
-
- ret = dvb_usb_device_init(intf, &af9005_properties,
- THIS_MODULE, &d, adapter_nr);
-
- if (ret < 0)
- return ret;
-
- st = d->priv;
- mutex_init(&st->data_mutex);
-
- return 0;
+ return dvb_usb_device_init(intf, &af9005_properties,
+ THIS_MODULE, NULL, adapter_nr);
}
enum af9005_usb_table_entry {
diff --git a/drivers/media/usb/dvb-usb/cinergyT2-core.c b/drivers/media/usb/dvb-usb/cinergyT2-core.c
index 8ac825413d5a..290275bc7fde 100644
--- a/drivers/media/usb/dvb-usb/cinergyT2-core.c
+++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c
@@ -42,7 +42,6 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
struct cinergyt2_state {
u8 rc_counter;
unsigned char data[64];
- struct mutex data_mutex;
};
/* We are missing a release hook with usb_device data */
@@ -56,12 +55,12 @@ static int cinergyt2_streaming_ctrl(struct dvb_usb_adapter *adap, int enable)
struct cinergyt2_state *st = d->priv;
int ret;
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
st->data[0] = CINERGYT2_EP1_CONTROL_STREAM_TRANSFER;
st->data[1] = enable ? 1 : 0;
ret = dvb_usb_generic_rw(d, st->data, 2, st->data, 64, 0);
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
return ret;
}
@@ -71,12 +70,12 @@ static int cinergyt2_power_ctrl(struct dvb_usb_device *d, int enable)
struct cinergyt2_state *st = d->priv;
int ret;
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
st->data[0] = CINERGYT2_EP1_SLEEP_MODE;
st->data[1] = enable ? 0 : 1;
ret = dvb_usb_generic_rw(d, st->data, 2, st->data, 3, 0);
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
return ret;
}
@@ -89,7 +88,7 @@ static int cinergyt2_frontend_attach(struct dvb_usb_adapter *adap)
adap->fe_adap[0].fe = cinergyt2_fe_attach(adap->dev);
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
st->data[0] = CINERGYT2_EP1_GET_FIRMWARE_VERSION;
ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 3, 0);
@@ -97,7 +96,7 @@ static int cinergyt2_frontend_attach(struct dvb_usb_adapter *adap)
deb_rc("cinergyt2_power_ctrl() Failed to retrieve sleep "
"state info\n");
}
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
/* Copy this pointer as we are gonna need it in the release phase */
cinergyt2_usb_device = adap->dev;
@@ -166,7 +165,7 @@ static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
*state = REMOTE_NO_KEY_PRESSED;
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS;
ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0);
@@ -202,29 +201,17 @@ static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
}
ret:
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
return ret;
}
static int cinergyt2_usb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
- struct dvb_usb_device *d;
- struct cinergyt2_state *st;
- int ret;
-
- ret = dvb_usb_device_init(intf, &cinergyt2_properties,
- THIS_MODULE, &d, adapter_nr);
- if (ret < 0)
- return ret;
-
- st = d->priv;
- mutex_init(&st->data_mutex);
-
- return 0;
+ return dvb_usb_device_init(intf, &cinergyt2_properties,
+ THIS_MODULE, NULL, adapter_nr);
}
-
static struct usb_device_id cinergyt2_usb_table[] = {
{ USB_DEVICE(USB_VID_TERRATEC, 0x0038) },
{ 0 }
diff --git a/drivers/media/usb/dvb-usb/cxusb.c b/drivers/media/usb/dvb-usb/cxusb.c
index 39772812269d..243403081fa5 100644
--- a/drivers/media/usb/dvb-usb/cxusb.c
+++ b/drivers/media/usb/dvb-usb/cxusb.c
@@ -68,7 +68,7 @@ static int cxusb_ctrl_msg(struct dvb_usb_device *d,
wo = (rbuf == NULL || rlen == 0); /* write-only */
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
st->data[0] = cmd;
memcpy(&st->data[1], wbuf, wlen);
if (wo)
@@ -77,7 +77,7 @@ static int cxusb_ctrl_msg(struct dvb_usb_device *d,
ret = dvb_usb_generic_rw(d, st->data, 1 + wlen,
rbuf, rlen, 0);
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
return ret;
}
@@ -1461,43 +1461,36 @@ static struct dvb_usb_device_properties cxusb_mygica_t230_properties;
static int cxusb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
- struct dvb_usb_device *d;
- struct cxusb_state *st;
-
if (0 == dvb_usb_device_init(intf, &cxusb_medion_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, &cxusb_bluebird_lgh064f_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, &cxusb_bluebird_dee1601_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, &cxusb_bluebird_lgz201_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, &cxusb_bluebird_dtt7579_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, &cxusb_bluebird_dualdig4_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, &cxusb_bluebird_nano2_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf,
&cxusb_bluebird_nano2_needsfirmware_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, &cxusb_aver_a868r_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf,
&cxusb_bluebird_dualdig4_rev2_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, &cxusb_d680_dmb_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, &cxusb_mygica_d689_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, &cxusb_mygica_t230_properties,
- THIS_MODULE, &d, adapter_nr) ||
- 0) {
- st = d->priv;
- mutex_init(&st->data_mutex);
-
+ THIS_MODULE, NULL, adapter_nr) ||
+ 0)
return 0;
- }
return -EINVAL;
}
diff --git a/drivers/media/usb/dvb-usb/cxusb.h b/drivers/media/usb/dvb-usb/cxusb.h
index 9f3ee0e47d5c..18acda19527a 100644
--- a/drivers/media/usb/dvb-usb/cxusb.h
+++ b/drivers/media/usb/dvb-usb/cxusb.h
@@ -37,7 +37,6 @@ struct cxusb_state {
struct i2c_client *i2c_client_tuner;
unsigned char data[MAX_XFER_SIZE];
- struct mutex data_mutex;
};
#endif
diff --git a/drivers/media/usb/dvb-usb/dtt200u.c b/drivers/media/usb/dvb-usb/dtt200u.c
index f88572c7ae7c..fcbff7fb0c4e 100644
--- a/drivers/media/usb/dvb-usb/dtt200u.c
+++ b/drivers/media/usb/dvb-usb/dtt200u.c
@@ -22,7 +22,6 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
struct dtt200u_state {
unsigned char data[80];
- struct mutex data_mutex;
};
static int dtt200u_power_ctrl(struct dvb_usb_device *d, int onoff)
@@ -30,23 +29,24 @@ static int dtt200u_power_ctrl(struct dvb_usb_device *d, int onoff)
struct dtt200u_state *st = d->priv;
int ret = 0;
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
st->data[0] = SET_INIT;
if (onoff)
ret = dvb_usb_generic_write(d, st->data, 2);
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
return ret;
}
static int dtt200u_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
{
- struct dtt200u_state *st = adap->dev->priv;
+ struct dvb_usb_device *d = adap->dev;
+ struct dtt200u_state *st = d->priv;
int ret;
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
st->data[0] = SET_STREAMING;
st->data[1] = onoff;
@@ -61,26 +61,27 @@ static int dtt200u_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
ret = dvb_usb_generic_write(adap->dev, st->data, 1);
ret:
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
return ret;
}
static int dtt200u_pid_filter(struct dvb_usb_adapter *adap, int index, u16 pid, int onoff)
{
- struct dtt200u_state *st = adap->dev->priv;
+ struct dvb_usb_device *d = adap->dev;
+ struct dtt200u_state *st = d->priv;
int ret;
pid = onoff ? pid : 0;
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
st->data[0] = SET_PID_FILTER;
st->data[1] = index;
st->data[2] = pid & 0xff;
st->data[3] = (pid >> 8) & 0x1f;
ret = dvb_usb_generic_write(adap->dev, st->data, 4);
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
return ret;
}
@@ -91,7 +92,7 @@ static int dtt200u_rc_query(struct dvb_usb_device *d)
u32 scancode;
int ret;
- mutex_lock(&st->data_mutex);
+ mutex_lock(&d->data_mutex);
st->data[0] = GET_RC_CODE;
ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0);
@@ -126,7 +127,7 @@ static int dtt200u_rc_query(struct dvb_usb_device *d)
deb_info("st->data: %*ph\n", 5, st->data);
ret:
- mutex_unlock(&st->data_mutex);
+ mutex_unlock(&d->data_mutex);
return ret;
}
@@ -145,24 +146,17 @@ static struct dvb_usb_device_properties wt220u_miglia_properties;
static int dtt200u_usb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
- struct dvb_usb_device *d;
- struct dtt200u_state *st;
-
if (0 == dvb_usb_device_init(intf, &dtt200u_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, &wt220u_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, &wt220u_fc_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, &wt220u_zl0353_properties,
- THIS_MODULE, &d, adapter_nr) ||
+ THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, &wt220u_miglia_properties,
- THIS_MODULE, &d, adapter_nr)) {
- st = d->priv;
- mutex_init(&st->data_mutex);
-
+ THIS_MODULE, NULL, adapter_nr))
return 0;
- }
return -ENODEV;
}
diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c
index 3896ba9a4179..84308569e7dc 100644
--- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
+++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
@@ -142,6 +142,7 @@ static int dvb_usb_init(struct dvb_usb_device *d, short *adapter_nums)
{
int ret = 0;
+ mutex_init(&d->data_mutex);
mutex_init(&d->usb_mutex);
mutex_init(&d->i2c_mutex);
diff --git a/drivers/media/usb/dvb-usb/dvb-usb.h b/drivers/media/usb/dvb-usb/dvb-usb.h
index 1448c3d27ea2..67f898b6f6d0 100644
--- a/drivers/media/usb/dvb-usb/dvb-usb.h
+++ b/drivers/media/usb/dvb-usb/dvb-usb.h
@@ -404,8 +404,12 @@ struct dvb_usb_adapter {
* Powered is in/decremented for each call to modify the state.
* @udev: pointer to the device's struct usb_device.
*
- * @usb_mutex: semaphore of USB control messages (reading needs two messages)
- * @i2c_mutex: semaphore for i2c-transfers
+ * @data_mutex: mutex to protect the data structure used to store URB data
+ * @usb_mutex: mutex of USB control messages (reading needs two messages).
+ * Please notice that this mutex is used internally at the generic
+ * URB control functions. So, drivers using dvb_usb_generic_rw() and
+ * derivated functions should not lock it internally.
+ * @i2c_mutex: mutex for i2c-transfers
*
* @i2c_adap: device's i2c_adapter if it uses I2CoverUSB
*
@@ -433,6 +437,7 @@ struct dvb_usb_device {
int powered;
/* locking */
+ struct mutex data_mutex;
struct mutex usb_mutex;
/* i2c */
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference
2016-11-11 13:55 ` Mauro Carvalho Chehab
@ 2016-11-11 16:43 ` Jörg Otte
0 siblings, 0 replies; 14+ messages in thread
From: Jörg Otte @ 2016-11-11 16:43 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Benjamin Larsson, Mauro Carvalho Chehab, Linus Torvalds,
Patrick Boettcher, Mauro Carvalho Chehab,
Linux Kernel Mailing List, Linux Media Mailing List
2016-11-11 14:55 GMT+01:00 Mauro Carvalho Chehab <mchehab@osg.samsung.com>:
> Em Thu, 10 Nov 2016 12:15:39 +0100
> Jörg Otte <jrg.otte@gmail.com> escreveu:
>
>> 2016-11-10 9:40 GMT+01:00 Mauro Carvalho Chehab <mchehab@s-opensource.com>:
>> > Em Wed, 9 Nov 2016 11:07:35 -0800
>> > Linus Torvalds <torvalds@linux-foundation.org> escreveu:
>> >
>> >> On Wed, Nov 9, 2016 at 3:09 AM, Jörg Otte <jrg.otte@gmail.com> wrote:
>> >> >
>> >> > Tried patch with no success. Again a NULL ptr dereferece.
>> >>
>> >> I suspect a much simpler approach is to just miove the "data_mutex"
>> >> away from the priv area and into "struct dvb_usb_device" and
>> >> "dvb_usb_adapter". Sure, that grows those structures a tiny bit, and
>> >> not every driver may need that mutex, but it simplifies things
>> >> enormously. Mauro?
>> >>
>> >> Linus
>
>> The patch works for me.
>
> Thanks for testing! That's the (hopefully) final version of it,
> with the fix applied to the other dvb-usb drivers that use
> data_mutex (except for the frontend ones, with uses a different
> private structure, and where the mutex is initialized at attach).
>
> Benjamin,
>
> Could you please test it?
>
> Thanks!
> Mauro
>
> -
>
> [PATCH] dvb-usb: move data_mutex to struct dvb_usb_device
>
> The data_mutex is initialized too late, as it is needed for
> each device driver's power control, causing an OOPS:
>
> dvb-usb: found a 'TerraTec/qanu USB2.0 Highspeed DVB-T Receiver' in warm state.
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100 PGD 0
> Oops: 0002 [#1] SMP
> Modules linked in: dvb_usb_cinergyT2(+) dvb_usb
> CPU: 0 PID: 2029 Comm: modprobe Not tainted 4.9.0-rc4-dvbmod #24
> Hardware name: FUJITSU LIFEBOOK A544/FJNBB35 , BIOS Version 1.17 05/09/2014
> task: ffff88020e943840 task.stack: ffff8801f36ec000
> RIP: 0010:[<ffffffff846617af>] [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100
> RSP: 0018:ffff8801f36efb10 EFLAGS: 00010282
> RAX: 0000000000000000 RBX: ffff88021509bdc8 RCX: 00000000c0000100
> RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff88021509bdcc
> RBP: ffff8801f36efb58 R08: ffff88021f216320 R09: 0000000000100000
> R10: ffff88021f216320 R11: 00000023fee6c5a1 R12: ffff88020e943840
> R13: ffff88021509bdcc R14: 00000000ffffffff R15: ffff88021509bdd0
> FS: 00007f21adb86740(0000) GS:ffff88021f200000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 0000000215bce000 CR4: 00000000001406f0
> Stack:
> ffff88021509bdd0 0000000000000000 0000000000000000 ffffffffc0137c80
> ffff88021509bdc8 ffff8801f5944000 0000000000000001 ffffffffc0136b00
> ffff880213e52000 ffff88021509bdc8 ffffffff84661856 ffff88021509bd80
> Call Trace:
> [<ffffffff84661856>] ? mutex_lock+0x16/0x25
> [<ffffffffc013616f>] ? cinergyt2_power_ctrl+0x1f/0x60 [dvb_usb_cinergyT2]
> [<ffffffffc012e67e>] ? dvb_usb_device_init+0x21e/0x5d0 [dvb_usb]
> [<ffffffffc0136021>] ? cinergyt2_usb_probe+0x21/0x50 [dvb_usb_cinergyT2]
> [<ffffffff844326f3>] ? usb_probe_interface+0xf3/0x2a0
> [<ffffffff8438e348>] ? driver_probe_device+0x208/0x2b0
> [<ffffffff8438e477>] ? __driver_attach+0x87/0x90
> [<ffffffff8438e3f0>] ? driver_probe_device+0x2b0/0x2b0
> [<ffffffff8438c612>] ? bus_for_each_dev+0x52/0x80
> [<ffffffff8438d983>] ? bus_add_driver+0x1a3/0x220
> [<ffffffff8438ec06>] ? driver_register+0x56/0xd0
> [<ffffffff84431527>] ? usb_register_driver+0x77/0x130
> [<ffffffffc013a000>] ? 0xffffffffc013a000
> [<ffffffff84000426>] ? do_one_initcall+0x46/0x180
> [<ffffffff840eb2c8>] ? free_vmap_area_noflush+0x38/0x70
> [<ffffffff840f3844>] ? kmem_cache_alloc+0x84/0xc0
> [<ffffffff840b802c>] ? do_init_module+0x50/0x1be
> [<ffffffff84095adb>] ? load_module+0x1d8b/0x2100
> [<ffffffff84093020>] ? find_symbol_in_section+0xa0/0xa0
> [<ffffffff84095fe9>] ? SyS_finit_module+0x89/0x90
> [<ffffffff846637a0>] ? entry_SYSCALL_64_fastpath+0x13/0x94
> Code: e8 a7 1d 00 00 8b 03 83 f8 01 0f 84 97 00 00 00 48 8b 43 10 4c 8d 7b 08 48 89 63 10 4c 89 3c 24 41 be ff ff ff ff 48 89 44 24 08 <48> 89 20 4c 89 64 24 10 eb 1a 49 c7 44 24 08 02 00 00 00 c6 43 RIP [<ffffffff846617af>] __mutex_lock_slowpath+0x6f/0x100 RSP <ffff8801f36efb10>
> CR2: 0000000000000000
>
> So, move it to the struct dvb_usb_device and initialize it
> before calling the driver's callbacks.
>
> Reported-by: Jörg Otte <jrg.otte@gmail.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>
> diff --git a/drivers/media/usb/dvb-usb/af9005.c b/drivers/media/usb/dvb-usb/af9005.c
> index b257780fb380..7853261906b1 100644
> --- a/drivers/media/usb/dvb-usb/af9005.c
> +++ b/drivers/media/usb/dvb-usb/af9005.c
> @@ -53,7 +53,6 @@ struct af9005_device_state {
> u8 sequence;
> int led_state;
> unsigned char data[256];
> - struct mutex data_mutex;
> };
>
> static int af9005_generic_read_write(struct dvb_usb_device *d, u16 reg,
> @@ -72,7 +71,7 @@ static int af9005_generic_read_write(struct dvb_usb_device *d, u16 reg,
> return -EINVAL;
> }
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
> st->data[0] = 14; /* rest of buffer length low */
> st->data[1] = 0; /* rest of buffer length high */
>
> @@ -140,7 +139,7 @@ static int af9005_generic_read_write(struct dvb_usb_device *d, u16 reg,
> values[i] = st->data[8 + i];
>
> ret:
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
> return ret;
>
> }
> @@ -481,7 +480,7 @@ int af9005_send_command(struct dvb_usb_device *d, u8 command, u8 * wbuf,
> }
> packet_len = wlen + 5;
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
>
> st->data[0] = (u8) (packet_len & 0xff);
> st->data[1] = (u8) ((packet_len & 0xff00) >> 8);
> @@ -512,7 +511,7 @@ int af9005_send_command(struct dvb_usb_device *d, u8 command, u8 * wbuf,
> rbuf[i] = st->data[i + 7];
> }
>
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
> return ret;
> }
>
> @@ -523,7 +522,7 @@ int af9005_read_eeprom(struct dvb_usb_device *d, u8 address, u8 * values,
> u8 seq;
> int ret, i;
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
>
> memset(st->data, 0, sizeof(st->data));
>
> @@ -559,7 +558,7 @@ int af9005_read_eeprom(struct dvb_usb_device *d, u8 address, u8 * values,
> for (i = 0; i < len; i++)
> values[i] = st->data[6 + i];
> }
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
>
> return ret;
> }
> @@ -847,7 +846,7 @@ static int af9005_rc_query(struct dvb_usb_device *d, u32 * event, int *state)
> return 0;
> }
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
>
> /* deb_info("rc_query\n"); */
> st->data[0] = 3; /* rest of packet length low */
> @@ -890,7 +889,7 @@ static int af9005_rc_query(struct dvb_usb_device *d, u32 * event, int *state)
> }
>
> ret:
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
> return ret;
> }
>
> @@ -1004,20 +1003,8 @@ static struct dvb_usb_device_properties af9005_properties;
> static int af9005_usb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> - struct dvb_usb_device *d;
> - struct af9005_device_state *st;
> - int ret;
> -
> - ret = dvb_usb_device_init(intf, &af9005_properties,
> - THIS_MODULE, &d, adapter_nr);
> -
> - if (ret < 0)
> - return ret;
> -
> - st = d->priv;
> - mutex_init(&st->data_mutex);
> -
> - return 0;
> + return dvb_usb_device_init(intf, &af9005_properties,
> + THIS_MODULE, NULL, adapter_nr);
> }
>
> enum af9005_usb_table_entry {
> diff --git a/drivers/media/usb/dvb-usb/cinergyT2-core.c b/drivers/media/usb/dvb-usb/cinergyT2-core.c
> index 8ac825413d5a..290275bc7fde 100644
> --- a/drivers/media/usb/dvb-usb/cinergyT2-core.c
> +++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c
> @@ -42,7 +42,6 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
> struct cinergyt2_state {
> u8 rc_counter;
> unsigned char data[64];
> - struct mutex data_mutex;
> };
>
> /* We are missing a release hook with usb_device data */
> @@ -56,12 +55,12 @@ static int cinergyt2_streaming_ctrl(struct dvb_usb_adapter *adap, int enable)
> struct cinergyt2_state *st = d->priv;
> int ret;
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
> st->data[0] = CINERGYT2_EP1_CONTROL_STREAM_TRANSFER;
> st->data[1] = enable ? 1 : 0;
>
> ret = dvb_usb_generic_rw(d, st->data, 2, st->data, 64, 0);
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
>
> return ret;
> }
> @@ -71,12 +70,12 @@ static int cinergyt2_power_ctrl(struct dvb_usb_device *d, int enable)
> struct cinergyt2_state *st = d->priv;
> int ret;
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
> st->data[0] = CINERGYT2_EP1_SLEEP_MODE;
> st->data[1] = enable ? 0 : 1;
>
> ret = dvb_usb_generic_rw(d, st->data, 2, st->data, 3, 0);
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
>
> return ret;
> }
> @@ -89,7 +88,7 @@ static int cinergyt2_frontend_attach(struct dvb_usb_adapter *adap)
>
> adap->fe_adap[0].fe = cinergyt2_fe_attach(adap->dev);
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
> st->data[0] = CINERGYT2_EP1_GET_FIRMWARE_VERSION;
>
> ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 3, 0);
> @@ -97,7 +96,7 @@ static int cinergyt2_frontend_attach(struct dvb_usb_adapter *adap)
> deb_rc("cinergyt2_power_ctrl() Failed to retrieve sleep "
> "state info\n");
> }
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
>
> /* Copy this pointer as we are gonna need it in the release phase */
> cinergyt2_usb_device = adap->dev;
> @@ -166,7 +165,7 @@ static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
>
> *state = REMOTE_NO_KEY_PRESSED;
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
> st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS;
>
> ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0);
> @@ -202,29 +201,17 @@ static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
> }
>
> ret:
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
> return ret;
> }
>
> static int cinergyt2_usb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> - struct dvb_usb_device *d;
> - struct cinergyt2_state *st;
> - int ret;
> -
> - ret = dvb_usb_device_init(intf, &cinergyt2_properties,
> - THIS_MODULE, &d, adapter_nr);
> - if (ret < 0)
> - return ret;
> -
> - st = d->priv;
> - mutex_init(&st->data_mutex);
> -
> - return 0;
> + return dvb_usb_device_init(intf, &cinergyt2_properties,
> + THIS_MODULE, NULL, adapter_nr);
> }
>
> -
> static struct usb_device_id cinergyt2_usb_table[] = {
> { USB_DEVICE(USB_VID_TERRATEC, 0x0038) },
> { 0 }
> diff --git a/drivers/media/usb/dvb-usb/cxusb.c b/drivers/media/usb/dvb-usb/cxusb.c
> index 39772812269d..243403081fa5 100644
> --- a/drivers/media/usb/dvb-usb/cxusb.c
> +++ b/drivers/media/usb/dvb-usb/cxusb.c
> @@ -68,7 +68,7 @@ static int cxusb_ctrl_msg(struct dvb_usb_device *d,
>
> wo = (rbuf == NULL || rlen == 0); /* write-only */
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
> st->data[0] = cmd;
> memcpy(&st->data[1], wbuf, wlen);
> if (wo)
> @@ -77,7 +77,7 @@ static int cxusb_ctrl_msg(struct dvb_usb_device *d,
> ret = dvb_usb_generic_rw(d, st->data, 1 + wlen,
> rbuf, rlen, 0);
>
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
> return ret;
> }
>
> @@ -1461,43 +1461,36 @@ static struct dvb_usb_device_properties cxusb_mygica_t230_properties;
> static int cxusb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> - struct dvb_usb_device *d;
> - struct cxusb_state *st;
> -
> if (0 == dvb_usb_device_init(intf, &cxusb_medion_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr) ||
> 0 == dvb_usb_device_init(intf, &cxusb_bluebird_lgh064f_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr) ||
> 0 == dvb_usb_device_init(intf, &cxusb_bluebird_dee1601_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr) ||
> 0 == dvb_usb_device_init(intf, &cxusb_bluebird_lgz201_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr) ||
> 0 == dvb_usb_device_init(intf, &cxusb_bluebird_dtt7579_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr) ||
> 0 == dvb_usb_device_init(intf, &cxusb_bluebird_dualdig4_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr) ||
> 0 == dvb_usb_device_init(intf, &cxusb_bluebird_nano2_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr) ||
> 0 == dvb_usb_device_init(intf,
> &cxusb_bluebird_nano2_needsfirmware_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr) ||
> 0 == dvb_usb_device_init(intf, &cxusb_aver_a868r_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr) ||
> 0 == dvb_usb_device_init(intf,
> &cxusb_bluebird_dualdig4_rev2_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr) ||
> 0 == dvb_usb_device_init(intf, &cxusb_d680_dmb_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr) ||
> 0 == dvb_usb_device_init(intf, &cxusb_mygica_d689_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr) ||
> 0 == dvb_usb_device_init(intf, &cxusb_mygica_t230_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> - 0) {
> - st = d->priv;
> - mutex_init(&st->data_mutex);
> -
> + THIS_MODULE, NULL, adapter_nr) ||
> + 0)
> return 0;
> - }
>
> return -EINVAL;
> }
> diff --git a/drivers/media/usb/dvb-usb/cxusb.h b/drivers/media/usb/dvb-usb/cxusb.h
> index 9f3ee0e47d5c..18acda19527a 100644
> --- a/drivers/media/usb/dvb-usb/cxusb.h
> +++ b/drivers/media/usb/dvb-usb/cxusb.h
> @@ -37,7 +37,6 @@ struct cxusb_state {
> struct i2c_client *i2c_client_tuner;
>
> unsigned char data[MAX_XFER_SIZE];
> - struct mutex data_mutex;
> };
>
> #endif
> diff --git a/drivers/media/usb/dvb-usb/dtt200u.c b/drivers/media/usb/dvb-usb/dtt200u.c
> index f88572c7ae7c..fcbff7fb0c4e 100644
> --- a/drivers/media/usb/dvb-usb/dtt200u.c
> +++ b/drivers/media/usb/dvb-usb/dtt200u.c
> @@ -22,7 +22,6 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
>
> struct dtt200u_state {
> unsigned char data[80];
> - struct mutex data_mutex;
> };
>
> static int dtt200u_power_ctrl(struct dvb_usb_device *d, int onoff)
> @@ -30,23 +29,24 @@ static int dtt200u_power_ctrl(struct dvb_usb_device *d, int onoff)
> struct dtt200u_state *st = d->priv;
> int ret = 0;
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
>
> st->data[0] = SET_INIT;
>
> if (onoff)
> ret = dvb_usb_generic_write(d, st->data, 2);
>
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
> return ret;
> }
>
> static int dtt200u_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
> {
> - struct dtt200u_state *st = adap->dev->priv;
> + struct dvb_usb_device *d = adap->dev;
> + struct dtt200u_state *st = d->priv;
> int ret;
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
> st->data[0] = SET_STREAMING;
> st->data[1] = onoff;
>
> @@ -61,26 +61,27 @@ static int dtt200u_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
> ret = dvb_usb_generic_write(adap->dev, st->data, 1);
>
> ret:
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
>
> return ret;
> }
>
> static int dtt200u_pid_filter(struct dvb_usb_adapter *adap, int index, u16 pid, int onoff)
> {
> - struct dtt200u_state *st = adap->dev->priv;
> + struct dvb_usb_device *d = adap->dev;
> + struct dtt200u_state *st = d->priv;
> int ret;
>
> pid = onoff ? pid : 0;
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
> st->data[0] = SET_PID_FILTER;
> st->data[1] = index;
> st->data[2] = pid & 0xff;
> st->data[3] = (pid >> 8) & 0x1f;
>
> ret = dvb_usb_generic_write(adap->dev, st->data, 4);
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
>
> return ret;
> }
> @@ -91,7 +92,7 @@ static int dtt200u_rc_query(struct dvb_usb_device *d)
> u32 scancode;
> int ret;
>
> - mutex_lock(&st->data_mutex);
> + mutex_lock(&d->data_mutex);
> st->data[0] = GET_RC_CODE;
>
> ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0);
> @@ -126,7 +127,7 @@ static int dtt200u_rc_query(struct dvb_usb_device *d)
> deb_info("st->data: %*ph\n", 5, st->data);
>
> ret:
> - mutex_unlock(&st->data_mutex);
> + mutex_unlock(&d->data_mutex);
> return ret;
> }
>
> @@ -145,24 +146,17 @@ static struct dvb_usb_device_properties wt220u_miglia_properties;
> static int dtt200u_usb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> - struct dvb_usb_device *d;
> - struct dtt200u_state *st;
> -
> if (0 == dvb_usb_device_init(intf, &dtt200u_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr) ||
> 0 == dvb_usb_device_init(intf, &wt220u_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr) ||
> 0 == dvb_usb_device_init(intf, &wt220u_fc_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr) ||
> 0 == dvb_usb_device_init(intf, &wt220u_zl0353_properties,
> - THIS_MODULE, &d, adapter_nr) ||
> + THIS_MODULE, NULL, adapter_nr) ||
> 0 == dvb_usb_device_init(intf, &wt220u_miglia_properties,
> - THIS_MODULE, &d, adapter_nr)) {
> - st = d->priv;
> - mutex_init(&st->data_mutex);
> -
> + THIS_MODULE, NULL, adapter_nr))
> return 0;
> - }
>
> return -ENODEV;
> }
> diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> index 3896ba9a4179..84308569e7dc 100644
> --- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
> +++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> @@ -142,6 +142,7 @@ static int dvb_usb_init(struct dvb_usb_device *d, short *adapter_nums)
> {
> int ret = 0;
>
> + mutex_init(&d->data_mutex);
> mutex_init(&d->usb_mutex);
> mutex_init(&d->i2c_mutex);
>
> diff --git a/drivers/media/usb/dvb-usb/dvb-usb.h b/drivers/media/usb/dvb-usb/dvb-usb.h
> index 1448c3d27ea2..67f898b6f6d0 100644
> --- a/drivers/media/usb/dvb-usb/dvb-usb.h
> +++ b/drivers/media/usb/dvb-usb/dvb-usb.h
> @@ -404,8 +404,12 @@ struct dvb_usb_adapter {
> * Powered is in/decremented for each call to modify the state.
> * @udev: pointer to the device's struct usb_device.
> *
> - * @usb_mutex: semaphore of USB control messages (reading needs two messages)
> - * @i2c_mutex: semaphore for i2c-transfers
> + * @data_mutex: mutex to protect the data structure used to store URB data
> + * @usb_mutex: mutex of USB control messages (reading needs two messages).
> + * Please notice that this mutex is used internally at the generic
> + * URB control functions. So, drivers using dvb_usb_generic_rw() and
> + * derivated functions should not lock it internally.
> + * @i2c_mutex: mutex for i2c-transfers
> *
> * @i2c_adap: device's i2c_adapter if it uses I2CoverUSB
> *
> @@ -433,6 +437,7 @@ struct dvb_usb_device {
> int powered;
>
> /* locking */
> + struct mutex data_mutex;
> struct mutex usb_mutex;
>
> /* i2c */
The patch works for me.
Note that my build environment builds a minimum kernel and only
compiles cinergyT2 parts of the patch.
Thanks, Jörg
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-11-11 16:43 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-06 15:40 [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference Jörg Otte
2016-11-08 18:42 ` Linus Torvalds
2016-11-08 20:22 ` Mauro Carvalho Chehab
2016-11-08 21:15 ` Benjamin Larsson
2016-11-08 21:38 ` Mauro Carvalho Chehab
2016-11-09 19:57 ` Malcolm Priestley
2016-11-09 20:25 ` Mauro Carvalho Chehab
2016-11-09 11:09 ` Jörg Otte
2016-11-09 19:07 ` Linus Torvalds
2016-11-09 20:21 ` Mauro Carvalho Chehab
2016-11-10 8:40 ` Mauro Carvalho Chehab
2016-11-10 11:15 ` Jörg Otte
2016-11-11 13:55 ` Mauro Carvalho Chehab
2016-11-11 16:43 ` Jörg Otte
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).