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

From: jeffy
Date: Tue Jul 11 2017 - 22:27:30 EST

Hi Oliver,

Thanx for your comments, and sorry for reply late.

On 07/04/2017 07:38 PM, Oliver Neukum wrote:
Am Freitag, den 23.06.2017, 11:46 +0800 schrieb jeffy:


drivers/bluetooth/btusb.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 278e811..b469f9b 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3254,11 +3254,12 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)

static void play_deferred(struct btusb_data *data)
+ struct hci_dev *hdev = data->hdev;
struct urb *urb;
int err;

while ((urb = usb_get_from_anchor(&data->deferred))) {
- err = usb_submit_urb(urb, GFP_ATOMIC);
+ err = submit_tx_urb(hdev, urb);

If you do that you have to change submit_tx_urb() to be called under a

sorry, why we need that? since submit_tx_urb is basically usb_anchor_urb/usb_submit_urb/usb_free_urb

if (err < 0)

so why not just fix the memory leak here and instead call submit_tx_urb. I am not sure that is actually the right approach. Why anchor this URB now to the TX anchor now? Is that actually safe?

the current flow is:
if (!suspending)
put into deferred anchor
wake btusb

retry the deferred urbs in deferred anchor(using usb_submit_urb)
after resumed

so i think there are 2 problems here:
1/ error handling, compare submit_tx_urb to usb_submit_urb, it freed
urb->setup_packet when failed to submit

In theory yes. If we ever put control URBs on the deferred anchor.

2/ memory leak:
in usb_submit_urb, we ref that urb
in __usb_hcd_giveback_urb, we unanchor it, and then unref it.

so i think the usb_submit_urb expected the urb not just be referenced,
but also anchored?

It expects that in the sense that it reacts to anchorings, but they are
not required.

or referenced, but the caller would unref it himself

The caller is responsible for its own references.
hmm, maybe unref it in the complete callback(btusb_tx_complete?), and if we do so, we may need to detect which urb came from here...

and for tx_anchor, we put urb in it, and kill them all during suspending
to prevent transfer. so i guess it would be safe to put deferred urb in
to it after resume too?
but i don't know much about usb/btusb, so i could be wrong all about that :)

IIRC the reason for directly submitting them was the spinlock.
sorry, i'm not clear about this, could you help to explain more? do you mean txlock?

the current play_deferred is called under txlock locked, and submit_tx_urb not:

clear_bit(BTUSB_SUSPENDING, &data->flags);

spin_unlock_irqrestore(&data->txlock, flags);

if (!suspending)
return submit_tx_urb(hdev, urb);