If the hub gets disconnected while the core is still activating it, this can result in leaking memory of few USB structures.
This will happen if we have done a kref_get() from hub_activate() and scheduled a delayed work item for HUB_INIT2/3. Now if hub_disconnect() gets called before the delayed work expires, then we will cancel the work from hub_quiesce(), but wouldn't do a kref_put(). And so the unbalance.
kmemleak reports this as (with the commit e50293ef9775 backported to 3.10 kernel with other changes, though the same is true for mainline as well):
unreferenced object 0xffffffc08af5b800 (size 1024): comm "khubd", pid 73, jiffies 4295051211 (age 6482.350s) hex dump (first 32 bytes): 30 68 f3 8c c0 ff ff ff 00 a0 b2 2e c0 ff ff ff 0h.............. 01 00 00 00 00 00 00 00 00 94 7d 40 c0 ff ff ff ..........}@.... backtrace: [<ffffffc0003079ec>] create_object+0x148/0x2a0 [<ffffffc000cc150c>] kmemleak_alloc+0x80/0xbc [<ffffffc000303a7c>] kmem_cache_alloc_trace+0x120/0x1ac [<ffffffc0006fa610>] hub_probe+0x120/0xb84 [<ffffffc000702b20>] usb_probe_interface+0x1ec/0x298 [<ffffffc0005d50cc>] driver_probe_device+0x160/0x374 [<ffffffc0005d5308>] __device_attach+0x28/0x4c [<ffffffc0005d3164>] bus_for_each_drv+0x78/0xac [<ffffffc0005d4ee0>] device_attach+0x6c/0x9c [<ffffffc0005d42b8>] bus_probe_device+0x28/0xa0 [<ffffffc0005d23a4>] device_add+0x324/0x604 [<ffffffc000700fcc>] usb_set_configuration+0x660/0x6cc [<ffffffc00070a350>] generic_probe+0x44/0x84 [<ffffffc000702914>] usb_probe_device+0x54/0x74 [<ffffffc0005d50cc>] driver_probe_device+0x160/0x374 [<ffffffc0005d5308>] __device_attach+0x28/0x4c
Fix this by putting the reference in hub_quiesce() if we canceled a pending work.
CC: stable@vger.kernel.org #4.4+ Fixes: e50293ef9775 ("USB: fix invalid memory access in hub_activate()") Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- Greg,
This is tested over 3.10 with backported patches only, sorry didn't had a mainline setup to test this out. :(
drivers/usb/core/hub.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index bee13517676f..3173693fa8e3 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -1315,7 +1315,8 @@ static void hub_quiesce(struct usb_hub *hub, enum hub_quiescing_type type) struct usb_device *hdev = hub->hdev; int i;
- cancel_delayed_work_sync(&hub->init_work); + if (cancel_delayed_work_sync(&hub->init_work)) + kref_put(&hub->kref, hub_release);
/* hub_wq and related activity won't re-trigger */ hub->quiescing = 1;
On Wed, 27 Jul 2016, Viresh Kumar wrote:
If the hub gets disconnected while the core is still activating it, this can result in leaking memory of few USB structures.
This will happen if we have done a kref_get() from hub_activate() and scheduled a delayed work item for HUB_INIT2/3. Now if hub_disconnect() gets called before the delayed work expires, then we will cancel the work from hub_quiesce(), but wouldn't do a kref_put(). And so the unbalance.
kmemleak reports this as (with the commit e50293ef9775 backported to 3.10 kernel with other changes, though the same is true for mainline as well):
...
Fix this by putting the reference in hub_quiesce() if we canceled a pending work.
CC: stable@vger.kernel.org #4.4+ Fixes: e50293ef9775 ("USB: fix invalid memory access in hub_activate()") Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Greg,
This is tested over 3.10 with backported patches only, sorry didn't had a mainline setup to test this out. :(
Arg. This is exactly the sort of thing I should have foreseen when writing the earlier commit.
drivers/usb/core/hub.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index bee13517676f..3173693fa8e3 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -1315,7 +1315,8 @@ static void hub_quiesce(struct usb_hub *hub, enum hub_quiescing_type type) struct usb_device *hdev = hub->hdev; int i;
- cancel_delayed_work_sync(&hub->init_work);
- if (cancel_delayed_work_sync(&hub->init_work))
kref_put(&hub->kref, hub_release);
/* hub_wq and related activity won't re-trigger */ hub->quiescing = 1;
Another possibility is to remove the cancel_delayed_work_sync call entirely. Either way, you can add
Acked-by: Alan Stern stern@rowland.harvard.edu
Alan Stern
On 28-07-16, 10:13, Alan Stern wrote:
On Wed, 27 Jul 2016, Viresh Kumar wrote:
If the hub gets disconnected while the core is still activating it, this can result in leaking memory of few USB structures.
This will happen if we have done a kref_get() from hub_activate() and scheduled a delayed work item for HUB_INIT2/3. Now if hub_disconnect() gets called before the delayed work expires, then we will cancel the work from hub_quiesce(), but wouldn't do a kref_put(). And so the unbalance.
kmemleak reports this as (with the commit e50293ef9775 backported to 3.10 kernel with other changes, though the same is true for mainline as well):
...
Fix this by putting the reference in hub_quiesce() if we canceled a pending work.
CC: stable@vger.kernel.org #4.4+ Fixes: e50293ef9775 ("USB: fix invalid memory access in hub_activate()") Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Greg,
This is tested over 3.10 with backported patches only, sorry didn't had a mainline setup to test this out. :(
Arg. This is exactly the sort of thing I should have foreseen when writing the earlier commit.
drivers/usb/core/hub.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index bee13517676f..3173693fa8e3 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -1315,7 +1315,8 @@ static void hub_quiesce(struct usb_hub *hub, enum hub_quiescing_type type) struct usb_device *hdev = hub->hdev; int i;
- cancel_delayed_work_sync(&hub->init_work);
- if (cancel_delayed_work_sync(&hub->init_work))
kref_put(&hub->kref, hub_release);
/* hub_wq and related activity won't re-trigger */ hub->quiescing = 1;
Another possibility is to remove the cancel_delayed_work_sync call entirely. Either way, you can add
Acked-by: Alan Stern stern@rowland.harvard.edu
Thanks Alan, I thought about that as well but didn't like it much as that is unnecessary overhead (timer+work). I mean, we know that the work is of no use anymore, why keep it around and let it fire? Also, in the worst case it may end up waking up an idle CPU, which isn't good as well.
linaro-kernel@lists.linaro.org