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
