Re: [PATCH] Bluetooth: virtio_bt: unregister HCI device on open failure

[Dan Carpenter]

On Wed, Jun 24, 2026 at 04:43:33PM +0800, Haoxiang Li wrote:
> virtbt_probe() registers the HCI device before calling
> virtbt_open_vdev(). If opening the virtio Bluetooth
> device fails, the error path frees the HCI device without
> unregistering it.
> 
> Fixes: dc65b4b0f90a ("Bluetooth: virtio_bt: fix device removal")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Haoxiang Li <haoxiang_li2024@xxxxxxx>
> ---
>  drivers/bluetooth/virtio_bt.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c
> index 140ab55c9fc5..bf6827431bb8 100644
> --- a/drivers/bluetooth/virtio_bt.c
> +++ b/drivers/bluetooth/virtio_bt.c
> @@ -397,6 +397,7 @@ static int virtbt_probe(struct virtio_device *vdev)
>  	return 0;
>  
>  open_failed:
> +	hci_unregister_dev(hdev);
>  	hci_free_dev(hdev);
>  failed:
>  	vdev->config->del_vqs(vdev);

I have written a blog about how to write error handling.
https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

Originally this code using One Err style error handling where every
error path just did "goto fail".  It's also using ComeFrom label names
which don't say what the goto does only where the goto is...  Ideally if
hci_register_dev() failed it would use the unwind ladder to clean up it
instead calls hci_free_dev() and then goto fail.

The beauty of writing a normal kernel style unwind ladder is that it
writes the cleanup function automatically...  Let's look at the cleanup
function here.

   406  static void virtbt_remove(struct virtio_device *vdev)
   407  {
   408          struct virtio_bluetooth *vbt = vdev->priv;
   409          struct hci_dev *hdev = vbt->hdev;
   410  
   411          hci_unregister_dev(hdev);
   412          virtio_reset_device(vdev);
   413          virtbt_close_vdev(vbt);

I'm really uncomfortable with having the hci_unregister_dev()
before the close.  Potential use after free?

   414  
   415          hci_free_dev(hdev);
   416          vbt->hdev = NULL;
   417  
   418          vdev->config->del_vqs(vdev);
   419          kfree(vbt);

The probe function should free "vbt" but it doesn't so that's
another leak.

   420  }

So this fix is fine but it's also only a partial fix.

regards,
dan carpenter