Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1689609 > unrolled thread

[RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred

Started byJeffy Chen <jeffy.chen@rock-chips.com>
First post2017-07-18 04:10 +0200
Last post2017-07-18 15:20 +0200
Articles 10 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-07-18 04:10 +0200
    Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred Marcel Holtmann <marcel@holtmann.org> - 2017-07-18 08:50 +0200
      Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in  play_deferred Oliver Neukum <oneukum@suse.com> - 2017-07-18 09:40 +0200
        Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred jeffy <jeffy.chen@rock-chips.com> - 2017-07-18 10:10 +0200
          Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in  play_deferred Oliver Neukum <oneukum@suse.com> - 2017-07-18 10:50 +0200
            Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred jeffy <jeffy.chen@rock-chips.com> - 2017-07-18 11:40 +0200
              Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in  play_deferred Oliver Neukum <oneukum@suse.com> - 2017-07-18 11:50 +0200
                Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred jeffy <jeffy.chen@rock-chips.com> - 2017-07-18 12:00 +0200
                  Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in  play_deferred Oliver Neukum <oneukum@suse.com> - 2017-07-18 14:40 +0200
                    Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred jeffy <jeffy.chen@rock-chips.com> - 2017-07-18 15:20 +0200

#1689609 — [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred

FromJeffy Chen <jeffy.chen@rock-chips.com>
Date2017-07-18 04:10 +0200
Subject[RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred
Message-ID<u4q7w-4KD-1@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1689776

FromMarcel Holtmann <marcel@holtmann.org>
Date2017-07-18 08:50 +0200
Message-ID<u4uut-7ol-13@gated-at.bofh.it>
In reply to#1689609
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

[toc] | [prev] | [next] | [standalone]

#1689825 — Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred

FromOliver Neukum <oneukum@suse.com>
Date2017-07-18 09:40 +0200
SubjectRe: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred
Message-ID<u4vgS-7Vc-21@gated-at.bofh.it>
In reply to#1689776
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

[toc] | [prev] | [next] | [standalone]

#1689853

Fromjeffy <jeffy.chen@rock-chips.com>
Date2017-07-18 10:10 +0200
Message-ID<u4vJU-8k9-13@gated-at.bofh.it>
In reply to#1689825
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
>
>
>
>

[toc] | [prev] | [next] | [standalone]

#1689880 — Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred

FromOliver Neukum <oneukum@suse.com>
Date2017-07-18 10:50 +0200
SubjectRe: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred
Message-ID<u4wmC-51-15@gated-at.bofh.it>
In reply to#1689853
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

[toc] | [prev] | [next] | [standalone]

#1689944

Fromjeffy <jeffy.chen@rock-chips.com>
Date2017-07-18 11:40 +0200
Message-ID<u4x90-CI-37@gated-at.bofh.it>
In reply to#1689880
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
>
>
>
>

[toc] | [prev] | [next] | [standalone]

#1689961 — Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred

FromOliver Neukum <oneukum@suse.com>
Date2017-07-18 11:50 +0200
SubjectRe: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred
Message-ID<u4xiH-HH-43@gated-at.bofh.it>
In reply to#1689944
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

[toc] | [prev] | [next] | [standalone]

#1689978

Fromjeffy <jeffy.chen@rock-chips.com>
Date2017-07-18 12:00 +0200
Message-ID<u4xsn-Lp-39@gated-at.bofh.it>
In reply to#1689961
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
>
>
>
>

[toc] | [prev] | [next] | [standalone]

#1690234 — Re: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred

FromOliver Neukum <oneukum@suse.com>
Date2017-07-18 14:40 +0200
SubjectRe: [RFC PATCH v2] Bluetooth: btusb: Fix memory leak in play_deferred
Message-ID<u4zXc-2qo-19@gated-at.bofh.it>
In reply to#1689978
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

[toc] | [prev] | [next] | [standalone]

#1690253

Fromjeffy <jeffy.chen@rock-chips.com>
Date2017-07-18 15:20 +0200
Message-ID<u4AzT-2Sx-7@gated-at.bofh.it>
In reply to#1690234
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
>
>
>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web