Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Anant Thazhemadam <anant.thazhemadam@gmail.com>
Subject: Re: [PATCH v3] bluetooth: hci_h5: fix memory leak in h5_close
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     linux-kernel-mentees@lists.linuxfoundation.org,
        syzbot+6ce141c55b2f7aafd1c4@syzkaller.appspotmail.com,
        Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20201001194329.9328-1-anant.thazhemadam@gmail.com>
 <20201004051708.21985-1-anant.thazhemadam@gmail.com>
 <407eed16-ba46-0ba6-544f-d5e820a1ced7@redhat.com>
Message-ID: <93ed51be-97b9-c5b4-8448-d06528a1d1af@gmail.com>
Date:   Tue, 6 Oct 2020 08:14:45 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.3.1
MIME-Version: 1.0
In-Reply-To: <407eed16-ba46-0ba6-544f-d5e820a1ced7@redhat.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk

On 05-10-2020 14:48, Hans de Goede wrote:
> To fully fix the memleak you also need to add a kfree_skb(h5->rx_skb);
> call to the end of h5_serdev_remove(), because in the hu->serdev case
> that is where the h5 struct will be free-ed (it is free-ed after that
> function exits).

Hi Hans,

I'm not entirely convinced that it might be entirely the best idea to do
that.

* The bug detected by syzbot only provides us with reproducer and
information about this bug (which gets triggered when !hu->serdev).
Even if like you said, there might be a memory leak unattended to when
hu->serdev exists, then this might not necessarily be the right place to fix
it.

* From what I can see, all the drivers that have modified to provide serdev
support have different close() mechanisms.
However, one thing they do have in common (in this context) is that their
respective serdev_remove() function simply calls hci_uart_unregister_device()
to unregister the device.
It is primarily for this reason that I feel adding a kfree_skb() call at the end
of h5_serdev_remove() might not exactly be the best way we could solve this
(and since this hasn't been picked up by syzbot yet, there's no way to know if
this just fixes things or ends up causing unforeseen complications).

Alternatively, wouldn't freeing h5->rx_skb and assigning it to NULL, for both
hu->serdev and !hu->serdev cases within h5_close() itself be a better
approach? I've also taken the liberty of testing a patch that does this, and it
seems to work correctly too. :)

But then again, I'm not exactly an authority on how this works.

Thanks,
Anant