Messages in this thread | | | Subject | Re: [PATCH v3] bluetooth: hci_h5: fix memory leak in h5_close | From | Hans de Goede <> | Date | Tue, 6 Oct 2020 08:30:05 +0200 |
| |
Hi,
On 10/6/20 4:44 AM, Anant Thazhemadam wrote: > 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?
That will indeed also fix the leak in both cases and is more consistent wrt when the free_skb happens. So this sounds good to me, go for it.
Regards,
Hans
> 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 >
| |