[PATCH v4] Bluetooth: virtio: Fix virtbt_probe() init and cleanup

[PATCH v4] Bluetooth: virtio: Fix virtbt_probe() init and cleanup

[Haoxiang Li]

virtbt_probe() allocates vbt before setting up the virtqueues, but some
failure paths return without freeing it.

The probe path also registers the HCI device before the virtio transport
is opened. Since hci_register_dev() makes the HCI device visible and queues
power_on work, move it after virtio_device_ready() and virtbt_open_vdev()
so the transport is ready before the HCI core can use it.

On failures after DRIVER_OK, reset and close the virtio device before
deleting the virtqueues and freeing vbt. This also cancels pending rx work
before vbt is freed.

Fixes: afd2daa26c7a ("Bluetooth: Add support for virtio transport driver")
Fixes: dc65b4b0f90a ("Bluetooth: virtio_bt: fix device removal")
Cc: stable@vger.kernel.org
Signed-off-by: Haoxiang Li <haoxiang_li2024@163.com>
---
Changes in v2:
 - Rework virtbt_probe() error paths into an unwind ladder.
 - Free vbt on probe failures.
 - Reset the virtio device and unregister the HCI device before freeing it
   when virtbt_open_vdev() fails.
 - Close the virtio device before unregistering the HCI device in remove().

   Thanks Dan for the suggestions. The blog is very helpful.

Changes in v3:
 - Remove virtio_reset_device() from the virtbt_open_vdev() failure path.

Changes in v4:
 - Move hci_register_dev() after virtio_device_ready() and virtbt_open_vdev().
 - Reset and close the virtio device on probe failures after DRIVER_OK. Thanks, Luiz!
---
 drivers/bluetooth/virtio_bt.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c
index 140ab55c9fc5..e7e79ba3c1f7 100644
--- a/drivers/bluetooth/virtio_bt.c
+++ b/drivers/bluetooth/virtio_bt.c
@@ -311,12 +311,12 @@ static int virtbt_probe(struct virtio_device *vdev)
 
 	err = virtio_find_vqs(vdev, VIRTBT_NUM_VQS, vbt->vqs, vqs_info, NULL);
 	if (err)
-		return err;
+		goto err_free_vbt;
 
 	hdev = hci_alloc_dev();
 	if (!hdev) {
 		err = -ENOMEM;
-		goto failed;
+		goto err_del_vqs;
 	}
 
 	vbt->hdev = hdev;
@@ -383,23 +383,28 @@ static int virtbt_probe(struct virtio_device *vdev)
 	if (virtio_has_feature(vdev, VIRTIO_BT_F_AOSP_EXT))
 		hci_set_aosp_capable(hdev);
 
-	if (hci_register_dev(hdev) < 0) {
-		hci_free_dev(hdev);
-		err = -EBUSY;
-		goto failed;
-	}
-
 	virtio_device_ready(vdev);
 	err = virtbt_open_vdev(vbt);
 	if (err)
-		goto open_failed;
+		goto err_close_vdev;
+
+	err = hci_register_dev(hdev);
+	if (err < 0) {
+		err = -EBUSY;
+		goto err_close_vdev;
+	}
 
 	return 0;
 
-open_failed:
+err_close_vdev:
+	virtio_reset_device(vdev);
+	virtbt_close_vdev(vbt);
 	hci_free_dev(hdev);
-failed:
+err_del_vqs:
 	vdev->config->del_vqs(vdev);
+err_free_vbt:
+	vdev->priv = NULL;
+	kfree(vbt);
 	return err;
 }
 
-- 
2.25.1

Re: [PATCH v4] Bluetooth: virtio: Fix virtbt_probe() init and cleanup

[Dan Carpenter]

On Thu, Jul 09, 2026 at 07:47:45PM +0800, Haoxiang Li wrote:
> virtbt_probe() allocates vbt before setting up the virtqueues, but some
> failure paths return without freeing it.
> 
> The probe path also registers the HCI device before the virtio transport
> is opened. Since hci_register_dev() makes the HCI device visible and queues
> power_on work, move it after virtio_device_ready() and virtbt_open_vdev()
> so the transport is ready before the HCI core can use it.
> 
> On failures after DRIVER_OK, reset and close the virtio device before
> deleting the virtqueues and freeing vbt. This also cancels pending rx work
> before vbt is freed.
> 
> Fixes: afd2daa26c7a ("Bluetooth: Add support for virtio transport driver")
> Fixes: dc65b4b0f90a ("Bluetooth: virtio_bt: fix device removal")
> Cc: stable@vger.kernel.org
> Signed-off-by: Haoxiang Li <haoxiang_li2024@163.com>
> ---
> Changes in v2:
>  - Rework virtbt_probe() error paths into an unwind ladder.
>  - Free vbt on probe failures.
>  - Reset the virtio device and unregister the HCI device before freeing it
>    when virtbt_open_vdev() fails.
>  - Close the virtio device before unregistering the HCI device in remove().
> 
>    Thanks Dan for the suggestions. The blog is very helpful.
> 
> Changes in v3:
>  - Remove virtio_reset_device() from the virtbt_open_vdev() failure path.
> 
> Changes in v4:
>  - Move hci_register_dev() after virtio_device_ready() and virtbt_open_vdev().
>  - Reset and close the virtio device on probe failures after DRIVER_OK. Thanks, Luiz!

These are Sashiko warnings.  To be honest, I would feel really
uncomfortable blindly applying them without testing.  If someone
can test, then great.  Otherwise, I would probably apply v3.  The
stuff that Sashiko complained about was all pre-existing issues
even though for the last one it said it wasn't but it was.

regards,
dan carpenter

Re: [PATCH v4] Bluetooth: virtio: Fix virtbt_probe() init and cleanup

[Michael S. Tsirkin]

On Thu, Jul 09, 2026 at 03:28:17PM +0300, Dan Carpenter wrote:
> On Thu, Jul 09, 2026 at 07:47:45PM +0800, Haoxiang Li wrote:
> > virtbt_probe() allocates vbt before setting up the virtqueues, but some
> > failure paths return without freeing it.
> > 
> > The probe path also registers the HCI device before the virtio transport
> > is opened. Since hci_register_dev() makes the HCI device visible and queues
> > power_on work, move it after virtio_device_ready() and virtbt_open_vdev()
> > so the transport is ready before the HCI core can use it.
> > 
> > On failures after DRIVER_OK, reset and close the virtio device before
> > deleting the virtqueues and freeing vbt. This also cancels pending rx work
> > before vbt is freed.
> > 
> > Fixes: afd2daa26c7a ("Bluetooth: Add support for virtio transport driver")
> > Fixes: dc65b4b0f90a ("Bluetooth: virtio_bt: fix device removal")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Haoxiang Li <haoxiang_li2024@163.com>
> > ---
> > Changes in v2:
> >  - Rework virtbt_probe() error paths into an unwind ladder.
> >  - Free vbt on probe failures.
> >  - Reset the virtio device and unregister the HCI device before freeing it
> >    when virtbt_open_vdev() fails.
> >  - Close the virtio device before unregistering the HCI device in remove().
> > 
> >    Thanks Dan for the suggestions. The blog is very helpful.
> > 
> > Changes in v3:
> >  - Remove virtio_reset_device() from the virtbt_open_vdev() failure path.
> > 
> > Changes in v4:
> >  - Move hci_register_dev() after virtio_device_ready() and virtbt_open_vdev().
> >  - Reset and close the virtio device on probe failures after DRIVER_OK. Thanks, Luiz!
> 
> These are Sashiko warnings.  To be honest, I would feel really
> uncomfortable blindly applying them without testing.  If someone
> can test, then great.  Otherwise, I would probably apply v3.  The
> stuff that Sashiko complained about was all pre-existing issues
> even though for the last one it said it wasn't but it was.
> 
> regards,
> dan carpenter

why make changes at all if no one can test. in fact, why have a driver
then.

-- 
MST

Re: [PATCH v4] Bluetooth: virtio: Fix virtbt_probe() init and cleanup

[Dan Carpenter]

On Thu, Jul 09, 2026 at 08:36:32AM -0400, Michael S. Tsirkin wrote:
> 
> why make changes at all if no one can test. in fact, why have a driver
> then.

It would be interesting to see what proportion of kernel patches are
actually tested...  Testing the code is often impossible because you
need the hardware.

In drivers/staging probably very few patches are tested.  Every couple
years I look at the data from where the problems come from and it's
normally from complicated changes from the driver maintainer.  The
number of bugs introduced by checkpatch and static checker fixes is
really tiny.

It's about risk vs reward.  Fixing a security issue is a huge reward.
Cleaning up the code.  Fixing obvious leaks and static checker issues.
Those things are all valuable because they raise the standards and
they prevent copy and paste bugs.

I consider a few things:

1. Is it a security fix?  I recently fixed some memory corruption and
   broke a driver.  I tried to be careful, I wrote a long commit message
   describing my thinking, but I still messed up.  And that's okay
   because fixing security bugs is important.
2. Is the code new?  If it is then there are probably very few users,
   and the original developer is still around so it's pretty safe to
   change.
3. Is it an error path?  Code on error paths is hard to test in the
   best of times.  The risk is very low.
4. Is the change small and obvious?

On the other hand, I often leave known bugs.  In this case, we're talking
about a use after free if the driver fails to probe.  That's not a
security thing.  It's unlikely to ever affect anyone in real life.  The
fix affects the success path so it could easily cause the driver to stop
working.

regards,
dan carpenter

Re: [PATCH v4] Bluetooth: virtio: Fix virtbt_probe() init and cleanup

[Michael S. Tsirkin]

On Thu, Jul 09, 2026 at 04:44:31PM +0300, Dan Carpenter wrote:
> On Thu, Jul 09, 2026 at 08:36:32AM -0400, Michael S. Tsirkin wrote:
> > 
> > why make changes at all if no one can test. in fact, why have a driver
> > then.
> 
> It would be interesting to see what proportion of kernel patches are
> actually tested...  Testing the code is often impossible because you
> need the hardware.

Sure I agree - if I am refactoring kernel APIs I would often
compile the driver and that is it.
But that is different from poking at a driver specifically.
If I do that then yes I expect the patch to be tested.



> In drivers/staging probably very few patches are tested.  Every couple
> years I look at the data from where the problems come from and it's
> normally from complicated changes from the driver maintainer.  The
> number of bugs introduced by checkpatch and static checker fixes is
> really tiny.
> 
> It's about risk vs reward.  Fixing a security issue is a huge reward.
> Cleaning up the code.  Fixing obvious leaks and static checker issues.
> Those things are all valuable because they raise the standards and
> they prevent copy and paste bugs.
> 
> I consider a few things:
> 
> 1. Is it a security fix?  I recently fixed some memory corruption and
>    broke a driver.  I tried to be careful, I wrote a long commit message
>    describing my thinking, but I still messed up.  And that's okay
>    because fixing security bugs is important.
> 2. Is the code new?  If it is then there are probably very few users,
>    and the original developer is still around so it's pretty safe to
>    change.
> 3. Is it an error path?  Code on error paths is hard to test in the
>    best of times.  The risk is very low.
> 4. Is the change small and obvious?
> 
> On the other hand, I often leave known bugs.  In this case, we're talking
> about a use after free if the driver fails to probe.  That's not a
> security thing.  It's unlikely to ever affect anyone in real life.  The
> fix affects the success path so it could easily cause the driver to stop
> working.

Exactly, agree on all points.

> regards,
> dan carpenter

Re: [PATCH v4] Bluetooth: virtio: Fix virtbt_probe() init and cleanup

[Michael S. Tsirkin]

Thanks for the patch! one question:

On Thu, Jul 09, 2026 at 07:47:45PM +0800, Haoxiang Li wrote:
> virtbt_probe() allocates vbt before setting up the virtqueues, but some
> failure paths return without freeing it.
> 
> The probe path also registers the HCI device before the virtio transport
> is opened. Since hci_register_dev() makes the HCI device visible and queues
> power_on work, move it after virtio_device_ready() and virtbt_open_vdev()
> so the transport is ready before the HCI core can use it.

Sounds good, and yes it is a spec violation to kick vq before virtio_device_ready

> On failures after DRIVER_OK, reset and close the virtio device before
> deleting the virtqueues and freeing vbt. This also cancels pending rx work
> before vbt is freed.

what "this" cancels pending work? And how can we have work since device was
not registered?

> Fixes: afd2daa26c7a ("Bluetooth: Add support for virtio transport driver")
> Fixes: dc65b4b0f90a ("Bluetooth: virtio_bt: fix device removal")
> Cc: stable@vger.kernel.org
> Signed-off-by: Haoxiang Li <haoxiang_li2024@163.com>
> ---
> Changes in v2:
>  - Rework virtbt_probe() error paths into an unwind ladder.
>  - Free vbt on probe failures.
>  - Reset the virtio device and unregister the HCI device before freeing it
>    when virtbt_open_vdev() fails.
>  - Close the virtio device before unregistering the HCI device in remove().
> 
>    Thanks Dan for the suggestions. The blog is very helpful.
> 
> Changes in v3:
>  - Remove virtio_reset_device() from the virtbt_open_vdev() failure path.
> 
> Changes in v4:
>  - Move hci_register_dev() after virtio_device_ready() and virtbt_open_vdev().
>  - Reset and close the virtio device on probe failures after DRIVER_OK. Thanks, Luiz!
> ---
>  drivers/bluetooth/virtio_bt.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c
> index 140ab55c9fc5..e7e79ba3c1f7 100644
> --- a/drivers/bluetooth/virtio_bt.c
> +++ b/drivers/bluetooth/virtio_bt.c
> @@ -311,12 +311,12 @@ static int virtbt_probe(struct virtio_device *vdev)
>  
>  	err = virtio_find_vqs(vdev, VIRTBT_NUM_VQS, vbt->vqs, vqs_info, NULL);
>  	if (err)
> -		return err;
> +		goto err_free_vbt;
>  
>  	hdev = hci_alloc_dev();
>  	if (!hdev) {
>  		err = -ENOMEM;
> -		goto failed;
> +		goto err_del_vqs;
>  	}
>  
>  	vbt->hdev = hdev;
> @@ -383,23 +383,28 @@ static int virtbt_probe(struct virtio_device *vdev)
>  	if (virtio_has_feature(vdev, VIRTIO_BT_F_AOSP_EXT))
>  		hci_set_aosp_capable(hdev);
>  
> -	if (hci_register_dev(hdev) < 0) {
> -		hci_free_dev(hdev);
> -		err = -EBUSY;
> -		goto failed;
> -	}
> -
>  	virtio_device_ready(vdev);
>  	err = virtbt_open_vdev(vbt);
>  	if (err)
> -		goto open_failed;
> +		goto err_close_vdev;
> +
> +	err = hci_register_dev(hdev);
> +	if (err < 0) {
> +		err = -EBUSY;
> +		goto err_close_vdev;
> +	}
>  
>  	return 0;
>  
> -open_failed:
> +err_close_vdev:
> +	virtio_reset_device(vdev);
> +	virtbt_close_vdev(vbt);
>  	hci_free_dev(hdev);
> -failed:
> +err_del_vqs:
>  	vdev->config->del_vqs(vdev);
> +err_free_vbt:
> +	vdev->priv = NULL;
> +	kfree(vbt);
>  	return err;
>  }
>  
> -- 
> 2.25.1