archive mirror
 help / color / mirror / Atom feed
From: Hans de Goede <>
To: Anant Thazhemadam <>
	Johan Hedberg <>,
	Marcel Holtmann <>,,,
Subject: Re: [Linux-kernel-mentees] [PATCH v2] bluetooth: hci_h5: close serdev device and free hu in h5_close
Date: Fri, 2 Oct 2020 12:22:43 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>


On 10/1/20 9:43 PM, Anant Thazhemadam wrote:
> When h5_close() gets called, the memory allocated for the hu gets
> freed only if hu->serdev doesn't exist. This leads to a memory leak.
> So when h5_close() is requested, close the serdev device instance and
> free the memory allocated to the hu entirely instead.
> Fixes: ce945552fde4 ("Bluetooth: hci_h5: Add support for serdev enumerated devices")
> Reported-by:
> Tested-by:
> Signed-off-by: Anant Thazhemadam <>

So 2 comments to this:

1. You claim this change fixes a memory-leak, but in the serdev 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. If you had looked at where the h5 struct gets allocated
in h5_close()-s counterpart, h5_open() then you could have seen
this there:

         if (hu->serdev) {
                 h5 = serdev_device_get_drvdata(hu->serdev);
         } else {
                 h5 = kzalloc(sizeof(*h5), GFP_KERNEL);
                 if (!h5)
                         return -ENOMEM;

So it is very clear here, that the kzalloc only happens in the
if (!hu->serdev) which clearly makes the kfree() have the same
condition the right thing todo. In the hu->serdev the data which
was allocated by h5_serdev_probe() is used instead and no alloc
happens inside h5_open()

In general when looking at resource teardown you should also look
at where they are allocated in the mirror function
and the teardown should mirror the alloc code.

So the main change of your commit is wrong:


2. You are making multiple changes in a single commit here, I count at
least 3. When ever you are making multiple changes like this, you should really
split the commit up in 1 commit per change and explain in each commit
message why that change is being made (why it is necessary). Writing
commit messages like this also leads to you double-checking your own
work by carefully considering why you are making the changes.

So about the 3 different changes:

2a) Make the kfree(h5) call unconditionally, which as mentioned above
is wrong.

2b) Introduce a call to kfree_skb(h5->rx_skb); which is not mentioned in
the commit message at all.  This looks like it would actually be a sensible
change, at least in the "if (!hu->serdev)" case.  When using the serdev
interface then just freeing it will leave a dangling pointer, so it
would be better (for both the hu->serdev and the !hu->serdev cases)
to call h5_reset_rx() on close instead which does:

         if (h5->rx_skb) {
                 h5->rx_skb = NULL;

2c) Introduce a call to serdev_device_close(), without really explaining
why in the commit message. Again if you would have looked at the mirror
h5_close() function then you see no serdev_device_open() there.
Actually serdev_device_open() is not called anywhere in the hci_h5.c code.

Digging a little deeper (using grep) shows that hci_uart_register_device()
calls serdev_device_open() and hci_uart_register_device() gets called from:
h5_serdev_probe(), likewise hci_uart_unregister_device() calls
serdev_device_close() for us and that gets called from h5_serdev_remove(),
so there is no need to call serdev_device_close() from h5_close() and doing
so will actually break things, because then the serdev will be left closed
on a subsequent h5_open() call.



> ---
> Changes in v2:
> 	* Fixed the Fixes tag
>   drivers/bluetooth/hci_h5.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
> index e41854e0d79a..3d1585add572 100644
> --- a/drivers/bluetooth/hci_h5.c
> +++ b/drivers/bluetooth/hci_h5.c
> @@ -248,8 +248,12 @@ static int h5_close(struct hci_uart *hu)
>   	if (h5->vnd && h5->vnd->close)
>   		h5->vnd->close(h5);
> -	if (!hu->serdev)
> -		kfree(h5);
> +	if (hu->serdev)
> +		serdev_device_close(hu->serdev);
> +
> +	kfree_skb(h5->rx_skb);
> +	kfree(h5);
> +	h5 = NULL;
>   	return 0;
>   }

Linux-kernel-mentees mailing list

  reply	other threads:[~2020-10-02 10:22 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-01 19:43 [Linux-kernel-mentees] [PATCH v2] bluetooth: hci_h5: close serdev device and free hu in h5_close Anant Thazhemadam
2020-10-02 10:22 ` Hans de Goede [this message]
2020-10-02 10:55   ` Anant Thazhemadam
2020-10-03 22:07   ` Anant Thazhemadam
2020-10-04  5:17 ` [Linux-kernel-mentees] [PATCH v3] bluetooth: hci_h5: fix memory leak " Anant Thazhemadam
2020-10-05  9:18   ` Hans de Goede
2020-10-06  2:44     ` Anant Thazhemadam
2020-10-06  6:30       ` Hans de Goede

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).