From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756531AbdGLC1R (ORCPT ); Tue, 11 Jul 2017 22:27:17 -0400 Received: from regular1.263xmail.com ([211.150.99.141]:58258 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755938AbdGLC1P (ORCPT ); Tue, 11 Jul 2017 22:27:15 -0400 X-263anti-spam: KSV:0;BIG:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ADDR-CHECKED4: 1 X-ABS-CHECKED: 1 X-SKE-CHECKED: 1 X-ANTISPAM-LEVEL: 2 X-RL-SENDER: jeffy.chen@rock-chips.com X-FST-TO: oliver@neukum.org X-SENDER-IP: 103.29.142.67 X-LOGIN-NAME: jeffy.chen@rock-chips.com X-UNIQUE-TAG: <0ec75412c2fab8e6ba98634a077eab50> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <596588F8.5070402@rock-chips.com> Date: Wed, 12 Jul 2017 10:27:04 +0800 From: jeffy User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20130126 Thunderbird/19.0 MIME-Version: 1.0 To: Oliver Neukum , Marcel Holtmann CC: LKML , xiyou.wangcong@gmail.com, Brian Norris , Douglas Anderson , Johan Hedberg , "Gustavo F. Padovan" Subject: Re: [RFC PATCH] Bluetooth: btusb: Fix memory leak in play_deferred References: <1498126243-4771-1-git-send-email-jeffy.chen@rock-chips.com> <65CF80EF-EA58-4126-889D-07A8FF9D52DB@holtmann.org> <594C8F0D.4000100@rock-chips.com> <1499168300.17946.3.camel@neukum.org> In-Reply-To: <1499168300.17946.3.camel@neukum.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > > >