linux-kernel - Re: [RFC PATCH] Bluetooth: btusb: Fix memory leak in play

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <596D6F6A.1090004@rock-chips.com>
Date:   Tue, 18 Jul 2017 10:16:10 +0800
From:   jeffy <jeffy.chen@...k-chips.com>
To:     Oliver Neukum <oneukum@...e.com>,
        Marcel Holtmann <marcel@...tmann.org>
CC:     LKML <linux-kernel@...r.kernel.org>, xiyou.wangcong@...il.com,
        Brian Norris <briannorris@...omium.org>,
        Douglas Anderson <dianders@...omium.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        "Gustavo F. Padovan" <gustavo@...ovan.org>
Subject: Re: [RFC PATCH] Bluetooth: btusb: Fix memory leak in play_deferred

Hi Oliver,
Thanks for your reply.

On 07/17/2017 11:26 PM, Oliver Neukum wrote:
> Am Mittwoch, den 12.07.2017, 10:27 +0800 schrieb jeffy:
>> Hi Oliver,
>>
>> Thanx for your comments, and sorry for reply late.
>>
>>
>>> 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
>
> You need to fix the GFP_KERNEL therein.
oh, i see the problem.
>
>>>> 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...
>
> I do not get your reasoning there. If an URB has executed, it belongs
> onto the anchor for URBs to be used again.
the urbs we submit here are referenced but unanchored, so i think we can:
1/ unreference it here and put it in tx_anchor, and let urb core to do 
the unachor(and unreference)
or
2/ we unreference it in the complete callback.

i'll send a new version for 2/
>
>>>> 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?
>
> Yes
>
> 	Regards
> 		Oliver
>
>
>
>