* [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred
@ 2017-07-18 2:06 Jeffy Chen
2017-07-18 6:44 ` Marcel Holtmann
0 siblings, 1 reply; 11+ messages in thread
From: Jeffy Chen @ 2017-07-18 2:06 UTC (permalink / raw)
To: linux-kernel
Cc: xiyou.wangcong, briannorris, dianders, Jeffy Chen, Johan Hedberg,
Marcel Holtmann, Gustavo Padovan, linux-bluetooth
Currently we are calling usb_submit_urb directly to submit deferred tx
urbs after unanchor them.
So the usb_giveback_urb_bh would failed to unref it in usb_unanchor_urb
and cause memory leak:
unreferenced object 0xffffffc0ce0fa400 (size 256):
...
backtrace:
[<ffffffc00034a9a8>] __save_stack_trace+0x48/0x6c
[<ffffffc00034b088>] create_object+0x138/0x254
[<ffffffc0009d5504>] kmemleak_alloc+0x58/0x8c
[<ffffffc000345f78>] __kmalloc+0x1d4/0x2a0
[<ffffffc0006765bc>] usb_alloc_urb+0x30/0x60
[<ffffffbffc128598>] alloc_ctrl_urb+0x38/0x120 [btusb]
[<ffffffbffc129e7c>] btusb_send_frame+0x64/0xf8 [btusb]
Free those urbs after completed to avoid the leak, and also fix the
error handling.
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---
Changes in v2:
Call usb_free_urb instead of reusing submit_tx_urb.
drivers/bluetooth/btusb.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 0d533b2..a22a08b 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3260,19 +3260,33 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
return 0;
}
+static void btusb_deferred_tx_complete(struct urb *urb)
+{
+ btusb_tx_complete(urb);
+ usb_free_urb(urb);
+}
+
static void play_deferred(struct btusb_data *data)
{
struct urb *urb;
int err;
while ((urb = usb_get_from_anchor(&data->deferred))) {
+ /* Add a hook to free urb after completed */
+ urb->complete = btusb_deferred_tx_complete;
+
err = usb_submit_urb(urb, GFP_ATOMIC);
- if (err < 0)
- break;
+ if (err < 0) {
+ if (err != -EPERM && err != -ENODEV)
+ BT_ERR("%s urb %p submission failed (%d)",
+ data->hdev->name, urb, -err);
+ kfree(urb->setup_packet);
+ usb_free_urb(urb);
+ continue;
+ }
data->tx_in_flight++;
}
- usb_scuttle_anchored_urbs(&data->deferred);
}
static int btusb_resume(struct usb_interface *intf)
--
2.1.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred
2017-07-18 2:06 [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred Jeffy Chen
@ 2017-07-18 6:44 ` Marcel Holtmann
2017-07-18 7:30 ` Oliver Neukum
0 siblings, 1 reply; 11+ messages in thread
From: Marcel Holtmann @ 2017-07-18 6:44 UTC (permalink / raw)
To: Jeffy Chen, Oliver Neukum
Cc: linux-kernel, xiyou.wangcong, briannorris, dianders,
Johan Hedberg, Gustavo F. Padovan, linux-bluetooth
Hi Oliver,
> Currently we are calling usb_submit_urb directly to submit deferred tx
> urbs after unanchor them.
>
> So the usb_giveback_urb_bh would failed to unref it in usb_unanchor_urb
> and cause memory leak:
> unreferenced object 0xffffffc0ce0fa400 (size 256):
> ...
> backtrace:
> [<ffffffc00034a9a8>] __save_stack_trace+0x48/0x6c
> [<ffffffc00034b088>] create_object+0x138/0x254
> [<ffffffc0009d5504>] kmemleak_alloc+0x58/0x8c
> [<ffffffc000345f78>] __kmalloc+0x1d4/0x2a0
> [<ffffffc0006765bc>] usb_alloc_urb+0x30/0x60
> [<ffffffbffc128598>] alloc_ctrl_urb+0x38/0x120 [btusb]
> [<ffffffbffc129e7c>] btusb_send_frame+0x64/0xf8 [btusb]
>
> Free those urbs after completed to avoid the leak, and also fix the
> error handling.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
> Changes in v2:
> Call usb_free_urb instead of reusing submit_tx_urb.
>
> drivers/bluetooth/btusb.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 0d533b2..a22a08b 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -3260,19 +3260,33 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
> return 0;
> }
>
> +static void btusb_deferred_tx_complete(struct urb *urb)
> +{
> + btusb_tx_complete(urb);
> + usb_free_urb(urb);
> +}
> +
> static void play_deferred(struct btusb_data *data)
> {
> struct urb *urb;
> int err;
>
> while ((urb = usb_get_from_anchor(&data->deferred))) {
> + /* Add a hook to free urb after completed */
> + urb->complete = btusb_deferred_tx_complete;
> +
> err = usb_submit_urb(urb, GFP_ATOMIC);
> - if (err < 0)
> - break;
> + if (err < 0) {
> + if (err != -EPERM && err != -ENODEV)
> + BT_ERR("%s urb %p submission failed (%d)",
> + data->hdev->name, urb, -err);
> + kfree(urb->setup_packet);
> + usb_free_urb(urb);
> + continue;
> + }
>
> data->tx_in_flight++;
> }
> - usb_scuttle_anchored_urbs(&data->deferred);
> }
can I get an ack from you on this one?
Regards
Marcel
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred
2017-07-18 6:44 ` Marcel Holtmann
@ 2017-07-18 7:30 ` Oliver Neukum
2017-07-18 8:08 ` jeffy
0 siblings, 1 reply; 11+ messages in thread
From: Oliver Neukum @ 2017-07-18 7:30 UTC (permalink / raw)
To: Marcel Holtmann, Jeffy Chen
Cc: linux-kernel, xiyou.wangcong, briannorris, dianders,
Johan Hedberg, Gustavo F. Padovan, linux-bluetooth
Am Dienstag, den 18.07.2017, 08:44 +0200 schrieb Marcel Holtmann:
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 0d533b2..a22a08b 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -3260,19 +3260,33 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
> > return 0;
> > }
> >
> > +static void btusb_deferred_tx_complete(struct urb *urb)
> > +{
> > + btusb_tx_complete(urb);
> > + usb_free_urb(urb);
> > +}
> > +
> > static void play_deferred(struct btusb_data *data)
> > {
> > struct urb *urb;
> > int err;
> >
> > while ((urb = usb_get_from_anchor(&data->deferred))) {
> > + /* Add a hook to free urb after completed */
> > + urb->complete = btusb_deferred_tx_complete;
> > +
> > err = usb_submit_urb(urb, GFP_ATOMIC);
> > - if (err < 0)
> > - break;
> > + if (err < 0) {
> > + if (err != -EPERM && err != -ENODEV)
> > + BT_ERR("%s urb %p submission failed (%d)",
> > + data->hdev->name, urb, -err);
> > + kfree(urb->setup_packet);
> > + usb_free_urb(urb);
> > + continue;
> > + }
> >
> > data->tx_in_flight++;
> > }
> > - usb_scuttle_anchored_urbs(&data->deferred);
> > }
>
> can I get an ack from you on this one?
Hi,
I am afraid not. We cannot silently drop one part of a transmission.
I am afraid that the correct algorithm, if we encounter an error at
that stage, is to abort the operation and report an error.
Regards
Oliver
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred
2017-07-18 7:30 ` Oliver Neukum
@ 2017-07-18 8:08 ` jeffy
2017-07-18 8:41 ` Oliver Neukum
0 siblings, 1 reply; 11+ messages in thread
From: jeffy @ 2017-07-18 8:08 UTC (permalink / raw)
To: Oliver Neukum, Marcel Holtmann
Cc: linux-kernel, xiyou.wangcong, briannorris, dianders,
Johan Hedberg, Gustavo F. Padovan, linux-bluetooth
Hi Oliver,
On 07/18/2017 03:30 PM, Oliver Neukum wrote:
> Am Dienstag, den 18.07.2017, 08:44 +0200 schrieb Marcel Holtmann:
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index 0d533b2..a22a08b 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -3260,19 +3260,33 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
>>> return 0;
>>> }
>>>
>>> +static void btusb_deferred_tx_complete(struct urb *urb)
>>> +{
>>> + btusb_tx_complete(urb);
>>> + usb_free_urb(urb);
>>> +}
>>> +
>>> static void play_deferred(struct btusb_data *data)
>>> {
>>> struct urb *urb;
>>> int err;
>>>
>>> while ((urb = usb_get_from_anchor(&data->deferred))) {
>>> + /* Add a hook to free urb after completed */
>>> + urb->complete = btusb_deferred_tx_complete;
>>> +
>>> err = usb_submit_urb(urb, GFP_ATOMIC);
>>> - if (err < 0)
>>> - break;
>>> + if (err < 0) {
>>> + if (err != -EPERM && err != -ENODEV)
>>> + BT_ERR("%s urb %p submission failed (%d)",
>>> + data->hdev->name, urb, -err);
>>> + kfree(urb->setup_packet);
>>> + usb_free_urb(urb);
>>> + continue;
>>> + }
>>>
>>> data->tx_in_flight++;
>>> }
>>> - usb_scuttle_anchored_urbs(&data->deferred);
>>> }
>>
>> can I get an ack from you on this one?
>
> Hi,
>
> I am afraid not. We cannot silently drop one part of a transmission.
> I am afraid that the correct algorithm, if we encounter an error at
> that stage, is to abort the operation and report an error.
>
so i should break the loop when error happens right?
and i uploaded 2 version of patches, which one do you prefer to go on?
> Regards
> Oliver
>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred
2017-07-18 8:08 ` jeffy
@ 2017-07-18 8:41 ` Oliver Neukum
2017-07-18 9:36 ` jeffy
0 siblings, 1 reply; 11+ messages in thread
From: Oliver Neukum @ 2017-07-18 8:41 UTC (permalink / raw)
To: jeffy, Marcel Holtmann
Cc: briannorris, dianders, Johan Hedberg, xiyou.wangcong,
Gustavo F. Padovan, linux-bluetooth, linux-kernel
Am Dienstag, den 18.07.2017, 16:08 +0800 schrieb jeffy:
> > I am afraid not. We cannot silently drop one part of a transmission.
> > I am afraid that the correct algorithm, if we encounter an error at
> > that stage, is to abort the operation and report an error.
> >
> so i should break the loop when error happens right?
Yes
>
> and i uploaded 2 version of patches, which one do you prefer to go on?
Where to?
Regards
Oliver
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred
2017-07-18 8:41 ` Oliver Neukum
@ 2017-07-18 9:36 ` jeffy
2017-07-18 9:41 ` Oliver Neukum
0 siblings, 1 reply; 11+ messages in thread
From: jeffy @ 2017-07-18 9:36 UTC (permalink / raw)
To: Oliver Neukum, Marcel Holtmann
Cc: briannorris, dianders, Johan Hedberg, xiyou.wangcong,
Gustavo F. Padovan, linux-bluetooth, linux-kernel
Hi Oliver,
On 07/18/2017 04:41 PM, Oliver Neukum wrote:
> Am Dienstag, den 18.07.2017, 16:08 +0800 schrieb jeffy:
>>> I am afraid not. We cannot silently drop one part of a transmission.
>>> I am afraid that the correct algorithm, if we encounter an error at
>>> that stage, is to abort the operation and report an error.
>>>
>> so i should break the loop when error happens right?
>
> Yes
ok, i'll do that.
>
>>
>> and i uploaded 2 version of patches, which one do you prefer to go on?
>
> Where to?
https://patchwork.kernel.org/patch/9847037
and
https://patchwork.kernel.org/patch/9846617
>
> Regards
> Oliver
>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred
2017-07-18 9:36 ` jeffy
@ 2017-07-18 9:41 ` Oliver Neukum
2017-07-18 9:56 ` jeffy
0 siblings, 1 reply; 11+ messages in thread
From: Oliver Neukum @ 2017-07-18 9:41 UTC (permalink / raw)
To: jeffy, Marcel Holtmann
Cc: briannorris, dianders, Johan Hedberg, xiyou.wangcong,
Gustavo F. Padovan, linux-bluetooth, linux-kernel
Am Dienstag, den 18.07.2017, 17:36 +0800 schrieb jeffy:
> Hi Oliver,
>
> On 07/18/2017 04:41 PM, Oliver Neukum wrote:
> >
> > Am Dienstag, den 18.07.2017, 16:08 +0800 schrieb jeffy:
> > >
> > > >
> > > > I am afraid not. We cannot silently drop one part of a transmission.
> > > > I am afraid that the correct algorithm, if we encounter an error at
> > > > that stage, is to abort the operation and report an error.
> > > >
> > > so i should break the loop when error happens right?
> >
> > Yes
> ok, i'll do that.
> >
> >
> > >
> > >
> > > and i uploaded 2 version of patches, which one do you prefer to go on?
> >
> > Where to?
> https://patchwork.kernel.org/patch/9847037
> and
> https://patchwork.kernel.org/patch/9846617
I think that as soon as one URB fails, you should not even try
to submit any other deferred URBs. You are taking one out from
the middle of a sequence. That cannot be right.
Regards
Oliver
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred
2017-07-18 9:41 ` Oliver Neukum
@ 2017-07-18 9:56 ` jeffy
2017-07-18 12:29 ` Oliver Neukum
0 siblings, 1 reply; 11+ messages in thread
From: jeffy @ 2017-07-18 9:56 UTC (permalink / raw)
To: Oliver Neukum, Marcel Holtmann
Cc: briannorris, dianders, Johan Hedberg, xiyou.wangcong,
Gustavo F. Padovan, linux-bluetooth, linux-kernel
Hi Oliver,
On 07/18/2017 05:41 PM, Oliver Neukum wrote:
> Am Dienstag, den 18.07.2017, 17:36 +0800 schrieb jeffy:
>> Hi Oliver,
>>
>> On 07/18/2017 04:41 PM, Oliver Neukum wrote:
>>>
>>> Am Dienstag, den 18.07.2017, 16:08 +0800 schrieb jeffy:
>>>>
>>>>>
>>>>> I am afraid not. We cannot silently drop one part of a transmission.
>>>>> I am afraid that the correct algorithm, if we encounter an error at
>>>>> that stage, is to abort the operation and report an error.
>>>>>
>>>> so i should break the loop when error happens right?
>>>
>>> Yes
>> ok, i'll do that.
>>>
>>>
>>>>
>>>>
>>>> and i uploaded 2 version of patches, which one do you prefer to go on?
>>>
>>> Where to?
>> https://patchwork.kernel.org/patch/9847037
>> and
>> https://patchwork.kernel.org/patch/9846617
>
> I think that as soon as one URB fails, you should not even try
> to submit any other deferred URBs. You are taking one out from
> the middle of a sequence. That cannot be right.
ok, that make sense.
new patch sent :)
>
> Regards
> Oliver
>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred
2017-07-18 9:56 ` jeffy
@ 2017-07-18 12:29 ` Oliver Neukum
2017-07-18 13:13 ` jeffy
0 siblings, 1 reply; 11+ messages in thread
From: Oliver Neukum @ 2017-07-18 12:29 UTC (permalink / raw)
To: jeffy, Marcel Holtmann
Cc: briannorris, dianders, Johan Hedberg, xiyou.wangcong,
Gustavo F. Padovan, linux-bluetooth, linux-kernel
Am Dienstag, den 18.07.2017, 17:56 +0800 schrieb jeffy:
> Hi Oliver,
>
> > I think that as soon as one URB fails, you should not even try
> > to submit any other deferred URBs. You are taking one out from
> > the middle of a sequence. That cannot be right.
> ok, that make sense.
>
> new patch sent :)
Where to?
Regards
Oliver
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred
2017-07-18 12:29 ` Oliver Neukum
@ 2017-07-18 13:13 ` jeffy
0 siblings, 0 replies; 11+ messages in thread
From: jeffy @ 2017-07-18 13:13 UTC (permalink / raw)
To: Oliver Neukum, Marcel Holtmann
Cc: briannorris, dianders, Johan Hedberg, xiyou.wangcong,
Gustavo F. Padovan, linux-bluetooth, linux-kernel
Hi Oliver,
On 07/18/2017 08:29 PM, Oliver Neukum wrote:
> Am Dienstag, den 18.07.2017, 17:56 +0800 schrieb jeffy:
>> Hi Oliver,
>>
>>> I think that as soon as one URB fails, you should not even try
>>> to submit any other deferred URBs. You are taking one out from
>>> the middle of a sequence. That cannot be right.
>> ok, that make sense.
>>
>> new patch sent :)
>
> Where to?
>
sorry, just noticed that you're not in the auto-generated CC list by
patman...i'll resend it, thanks:)
> Regards
> Oliver
>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred
@ 2017-07-18 2:05 Jeffy Chen
0 siblings, 0 replies; 11+ messages in thread
From: Jeffy Chen @ 2017-07-18 2:05 UTC (permalink / raw)
To: linux-kernel; +Cc: xiyou.wangcong, briannorris, dianders, Jeffy Chen
Currently we are calling usb_submit_urb directly to submit deferred tx
urbs after unanchor them.
So the usb_giveback_urb_bh would failed to unref it in usb_unanchor_urb
and cause memory leak:
unreferenced object 0xffffffc0ce0fa400 (size 256):
...
backtrace:
[<ffffffc00034a9a8>] __save_stack_trace+0x48/0x6c
[<ffffffc00034b088>] create_object+0x138/0x254
[<ffffffc0009d5504>] kmemleak_alloc+0x58/0x8c
[<ffffffc000345f78>] __kmalloc+0x1d4/0x2a0
[<ffffffc0006765bc>] usb_alloc_urb+0x30/0x60
[<ffffffbffc128598>] alloc_ctrl_urb+0x38/0x120 [btusb]
[<ffffffbffc129e7c>] btusb_send_frame+0x64/0xf8 [btusb]
Free those urbs after completed to avoid the leak, and also fix the
error handling.
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---
Changes in v2:
Call usb_free_urb instead of reusing submit_tx_urb.
drivers/bluetooth/btusb.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 0d533b2..a22a08b 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3260,19 +3260,33 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
return 0;
}
+static void btusb_deferred_tx_complete(struct urb *urb)
+{
+ btusb_tx_complete(urb);
+ usb_free_urb(urb);
+}
+
static void play_deferred(struct btusb_data *data)
{
struct urb *urb;
int err;
while ((urb = usb_get_from_anchor(&data->deferred))) {
+ /* Add a hook to free urb after completed */
+ urb->complete = btusb_deferred_tx_complete;
+
err = usb_submit_urb(urb, GFP_ATOMIC);
- if (err < 0)
- break;
+ if (err < 0) {
+ if (err != -EPERM && err != -ENODEV)
+ BT_ERR("%s urb %p submission failed (%d)",
+ data->hdev->name, urb, -err);
+ kfree(urb->setup_packet);
+ usb_free_urb(urb);
+ continue;
+ }
data->tx_in_flight++;
}
- usb_scuttle_anchored_urbs(&data->deferred);
}
static int btusb_resume(struct usb_interface *intf)
--
2.1.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-07-18 13:13 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-18 2:06 [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred Jeffy Chen
2017-07-18 6:44 ` Marcel Holtmann
2017-07-18 7:30 ` Oliver Neukum
2017-07-18 8:08 ` jeffy
2017-07-18 8:41 ` Oliver Neukum
2017-07-18 9:36 ` jeffy
2017-07-18 9:41 ` Oliver Neukum
2017-07-18 9:56 ` jeffy
2017-07-18 12:29 ` Oliver Neukum
2017-07-18 13:13 ` jeffy
-- strict thread matches above, loose matches on Subject: below --
2017-07-18 2:05 Jeffy Chen
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).