lkml.org 
[lkml]   [2017]   [Jul]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC PATCH] Bluetooth: btusb: Fix memory leak in play_deferred
    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
    > spinlock.

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

    >
    >>>> if (err < 0)
    >>>> break;
    >>>
    >>> 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:
    >> submit_or_queue_tx_urb
    >> if (!suspending)
    >> submit_tx_urb
    >> else
    >> 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
    >> later?
    >
    > 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:

    spin_lock_irq(&data->txlock);
    play_deferred(data);
    clear_bit(BTUSB_SUSPENDING, &data->flags);
    spin_unlock_irq(&data->txlock);


    spin_unlock_irqrestore(&data->txlock, flags);

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


    >
    > Regards
    > Oliver
    >
    >
    >
    >


    \
     
     \ /
      Last update: 2017-07-12 04:28    [W:4.105 / U:0.152 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site