[PATCH] Bluetooth: revert: hci_h5: close serdev device and free hu in h5_close

[PATCH] Bluetooth: revert: hci_h5: close serdev device and free hu in h5_close

[Hans de Goede]

There have been multiple revisions of the patch fix the h5->rx_skb
leak. Accidentally the first revision (which is buggy) and v5 have
both been merged:

v1 commit 70f259a3f427 ("Bluetooth: hci_h5: close serdev device and free
hu in h5_close");
v5 commit 855af2d74c87 ("Bluetooth: hci_h5: fix memory leak in h5_close")

The correct v5 makes changes slightly higher up in the h5_close()
function, which allowed both versions to get merged without conflict.

The changes from v1 unconditionally frees the h5 data struct, this
is wrong because in the serdev enumeration case the memory is
allocated in h5_serdev_probe() like this:

        h5 = devm_kzalloc(dev, sizeof(*h5), GFP_KERNEL);

So its lifetime is tied to the lifetime of the driver being bound
to the serdev and it is automatically freed when the driver gets
unbound. In the serdev case the same h5 struct is re-used over
h5_close() and h5_open() calls and thus MUST not be free-ed in
h5_close().

The serdev_device_close() added to h5_close() is incorrect in the
same way, serdev_device_close() is called on driver unbound too and
also MUST no be called from h5_close().

This reverts the changes made by merging v1 of the patch, so that
just the changes of the correct v5 remain.

Cc: Anant Thazhemadam <anant.thazhemadam@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_h5.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index 2cb5a48121f1..7be16a7f653b 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -251,12 +251,8 @@ static int h5_close(struct hci_uart *hu)
 	if (h5->vnd && h5->vnd->close)
 		h5->vnd->close(h5);
 
-	if (hu->serdev)
-		serdev_device_close(hu->serdev);
-
-	kfree_skb(h5->rx_skb);
-	kfree(h5);
-	h5 = NULL;
+	if (!hu->serdev)
+		kfree(h5);
 
 	return 0;
 }
-- 
2.28.0

RE: Bluetooth: revert: hci_h5: close serdev device and free hu in h5_close

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=389079

---Test result---

##############################
Test: CheckPatch - PASS

##############################
Test: CheckGitLint - PASS

##############################
Test: CheckBuildK - PASS



---
Regards,
Linux Bluetooth

Re: [PATCH] Bluetooth: revert: hci_h5: close serdev device and free hu in h5_close

[Marcel Holtmann]

Hi Hans,

> There have been multiple revisions of the patch fix the h5->rx_skb
> leak. Accidentally the first revision (which is buggy) and v5 have
> both been merged:
> 
> v1 commit 70f259a3f427 ("Bluetooth: hci_h5: close serdev device and free
> hu in h5_close");
> v5 commit 855af2d74c87 ("Bluetooth: hci_h5: fix memory leak in h5_close")
> 
> The correct v5 makes changes slightly higher up in the h5_close()
> function, which allowed both versions to get merged without conflict.
> 
> The changes from v1 unconditionally frees the h5 data struct, this
> is wrong because in the serdev enumeration case the memory is
> allocated in h5_serdev_probe() like this:
> 
>        h5 = devm_kzalloc(dev, sizeof(*h5), GFP_KERNEL);
> 
> So its lifetime is tied to the lifetime of the driver being bound
> to the serdev and it is automatically freed when the driver gets
> unbound. In the serdev case the same h5 struct is re-used over
> h5_close() and h5_open() calls and thus MUST not be free-ed in
> h5_close().
> 
> The serdev_device_close() added to h5_close() is incorrect in the
> same way, serdev_device_close() is called on driver unbound too and
> also MUST no be called from h5_close().
> 
> This reverts the changes made by merging v1 of the patch, so that
> just the changes of the correct v5 remain.
> 
> Cc: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/bluetooth/hci_h5.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel