Return-Path: MIME-Version: 1.0 In-Reply-To: <7127E793-A2AE-413D-AA65-239B23229380@holtmann.org> References: <1508830195-13824-1-git-send-email-jaganathx.kanakkassery@intel.com> <20171024085831.GA3889@x1c> <7127E793-A2AE-413D-AA65-239B23229380@holtmann.org> From: Jaganath K Date: Wed, 25 Oct 2017 11:14:42 +0530 Message-ID: Subject: Re: [PATCH] Bluetooth: Fix potential memory leak To: Marcel Holtmann Cc: Johan Hedberg , "open list:BLUETOOTH DRIVERS" , Jaganath Kanakkassery Content-Type: text/plain; charset="UTF-8" List-ID: Hi Marcel, Johan, On Tue, Oct 24, 2017 at 6:13 PM, Marcel Holtmann wrote: > Hi Johan, > >>>> index 1fba2a0..58045ee 100644 >>>> --- a/net/bluetooth/mgmt.c >>>> +++ b/net/bluetooth/mgmt.c >>>> @@ -6383,6 +6383,7 @@ static int remove_advertising(struct sock *sk, struct hci_dev *hdev, >>>> if (skb_queue_empty(&req.cmd_q) || >>>> !hdev_is_powered(hdev) || >>>> hci_dev_test_flag(hdev, HCI_ADVERTISING)) { >>>> + skb_queue_purge(&req.cmd_q); >>>> rp.instance = cp->instance; >>>> err = mgmt_cmd_complete(sk, hdev->id, >>>> MGMT_OP_REMOVE_ADVERTISING, >>> >>> this does not look right to me. It most likely has side affects. The >>> fix must be differently if there is a memory leak. >> >> Actually, it looks like the right fix to me. We don't have a separate >> helper to clean up a hci_request that never got used, and >> skb_queue_purge is what other places in the code seem to be doing in >> this kind of cases. > > then we need a helper to do this. This is pretty bad style if we hook into the queue directly with a skb_queue_purge. Since that cmd_q should be a hidden detail. I have raised v1 with the suggested changes. Thanks, Jaganath