Linux Virtualization - Dec 2017 - [PATCH v2 0/3] fix cleanup for fail to register_virtio_device

This series fix the cleanup for the caller of register_virtio_device,
the main work is use put_device instead of kfree.

V1->V2:
 * virtio_pci: add comments for the reason use put_device
 * virtio vop: also use put_device in in _vop_remove_device()

weiping zhang (3):
  virtio_pci: use put_device instead of kfree
  virtio: use put_device instead of kfree
  virtio: put reference count of virtio_device.dev

 drivers/misc/mic/vop/vop_main.c        | 18 ++++++++++--------
 drivers/remoteproc/remoteproc_virtio.c |  2 +-
 drivers/virtio/virtio_pci_common.c     | 17 +++++++++--------
 3 files changed, 20 insertions(+), 17 deletions(-)

-- 
2.9.4

As mentioned at drivers/base/core.c:
/*
 * NOTE: _Never_ directly free @dev after calling this function, even
 * if it returned an error! Always use put_device() to give up the
 * reference initialized in this function instead.
 */
so we don't free vp_dev until vp_dev->vdev.dev.release be called.

Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>
Reviewed-by: Cornelia Huck <cohuck at redhat.com>
---
 drivers/virtio/virtio_pci_common.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
index 1c4797e..91d20f7 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 	pci_set_master(pci_dev);
 
 	rc = register_virtio_device(&vp_dev->vdev);
-	if (rc)
-		goto err_register;
+	if (rc) {
+		if (vp_dev->ioaddr)
+		     virtio_pci_legacy_remove(vp_dev);
+		else
+		     virtio_pci_modern_remove(vp_dev);
+		pci_disable_device(pci_dev);
+		put_device(&vp_dev->vdev.dev);
+	}
 
-	return 0;
+	return rc;
 
-err_register:
-	if (vp_dev->ioaddr)
-	     virtio_pci_legacy_remove(vp_dev);
-	else
-	     virtio_pci_modern_remove(vp_dev);
 err_probe:
 	pci_disable_device(pci_dev);
 err_enable_device:
-- 
2.9.4

As mentioned at drivers/base/core.c:
/*
 * NOTE: _Never_ directly free @dev after calling this function, even
 * if it returned an error! Always use put_device() to give up the
 * reference initialized in this function instead.
 */
so we don't free vp_vdev until vp_vdev.dev.release be called.

Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>
---
 drivers/misc/mic/vop/vop_main.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index a341938..456e969 100644
--- a/drivers/misc/mic/vop/vop_main.c
+++ b/drivers/misc/mic/vop/vop_main.c
@@ -452,10 +452,12 @@ static irqreturn_t vop_virtio_intr_handler(int irq, void
*data)
 
 static void vop_virtio_release_dev(struct device *_d)
 {
-	/*
-	 * No need for a release method similar to virtio PCI.
-	 * Provide an empty one to avoid getting a warning from core.
-	 */
+	struct virtio_device *vdev +			container_of(_d, struct virtio_device, dev);
+	struct _vop_vdev *vop_vdev +			container_of(vdev, struct _vop_vdev, vdev);
+
+	kfree(vop_vdev);
 }
 
 /*
@@ -501,7 +503,9 @@ static int _vop_add_device(struct mic_device_desc __iomem
*d,
 		dev_err(_vop_dev(vdev),
 			"Failed to register vop device %u type %u\n",
 			offset, type);
-		goto free_irq;
+		vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
+		put_device(&vdev->vdev.dev);
+		return ret;
 	}
 	writeq((u64)vdev, &vdev->dc->vdev);
 	dev_dbg(_vop_dev(vdev), "%s: registered vop device %u type %u vdev
%p\n",
@@ -509,8 +513,6 @@ static int _vop_add_device(struct mic_device_desc __iomem
*d,
 
 	return 0;
 
-free_irq:
-	vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
 kfree:
 	kfree(vdev);
 	return ret;
@@ -568,7 +570,7 @@ static int _vop_remove_device(struct mic_device_desc __iomem
*d,
 		iowrite8(-1, &dc->h2c_vdev_db);
 		if (status & VIRTIO_CONFIG_S_DRIVER_OK)
 			wait_for_completion(&vdev->reset_done);
-		kfree(vdev);
+		put_device(&vdev->vdev.dev);
 		iowrite8(1, &dc->guest_ack);
 		dev_dbg(&vpdev->dev, "%s %d guest_ack %d\n",
 			__func__, __LINE__, ioread8(&dc->guest_ack));
-- 
2.9.4

rproc_virtio_dev_release will be called iff virtio_device.dev's
refer count became to 0. Here we should put vdev.dev, and then
rproc->dev's cleanup will be done in rproc_virtio_dev_release.

Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>
Reviewed-by: Cornelia Huck <cohuck at redhat.com>
---
 drivers/remoteproc/remoteproc_virtio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_virtio.c
b/drivers/remoteproc/remoteproc_virtio.c
index 2946348..b0633fd 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -327,7 +327,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 
 	ret = register_virtio_device(vdev);
 	if (ret) {
-		put_device(&rproc->dev);
+		put_device(&vdev->dev);
 		dev_err(dev, "failed to register vdev: %d\n", ret);
 		goto out;
 	}
-- 
2.9.4

On Tue, 12 Dec 2017 21:24:33 +0800
weiping zhang <zhangweiping at didichuxing.com> wrote:
> As mentioned at drivers/base/core.c:
> /*
>  * NOTE: _Never_ directly free @dev after calling this function, even
>  * if it returned an error! Always use put_device() to give up the
>  * reference initialized in this function instead.
>  */
> so we don't free vp_vdev until vp_vdev.dev.release be called.
> 
> Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>
> ---
>  drivers/misc/mic/vop/vop_main.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Tue, Dec 12, 2017 at 09:24:02PM +0800, weiping zhang
wrote:
> As mentioned at drivers/base/core.c:
> /*
>  * NOTE: _Never_ directly free @dev after calling this function, even
>  * if it returned an error! Always use put_device() to give up the
>  * reference initialized in this function instead.
>  */
> so we don't free vp_dev until vp_dev->vdev.dev.release be called.
seeing as 5739411acbaa63a6c22c91e340fdcdbcc7d82a51 adding these
annotations went to stable, should this go there too?
> Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>
> Reviewed-by: Cornelia Huck <cohuck at redhat.com>
OK but this relies on users knowing that register_virtio_device
calls device_register. I think we want to add a comment
to register_virtio_device.

Also the cleanup is uglified.

I really think the right thing would be to change device_register making
it safe to kfree. People have the right to expect register on failure to
have no effect.

That just might be too hard to implement though.

For now, my suggestion - add a variable.
> ---
>  drivers/virtio/virtio_pci_common.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
> index 1c4797e..91d20f7 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
>  	pci_set_master(pci_dev);
>  
>  	rc = register_virtio_device(&vp_dev->vdev);
> -	if (rc)
> -		goto err_register;
> +	if (rc) {
> +		if (vp_dev->ioaddr)
> +		     virtio_pci_legacy_remove(vp_dev);
> +		else
> +		     virtio_pci_modern_remove(vp_dev);
> +		pci_disable_device(pci_dev);
> +		put_device(&vp_dev->vdev.dev);
> +	}
>  
> -	return 0;
> +	return rc;
>  
> -err_register:
> -	if (vp_dev->ioaddr)
> -	     virtio_pci_legacy_remove(vp_dev);
> -	else
> -	     virtio_pci_modern_remove(vp_dev);
>  err_probe:
>  	pci_disable_device(pci_dev);
>  err_enable_device:
> -- 
> 2.9.4
I'd prefer something like the below.

--->

virtio_pci: don't kfree device on register failure

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>

---

diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
index 1c4797e..995ab03 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -513,7 +513,7 @@ static void virtio_pci_release_dev(struct device *_d)
 static int virtio_pci_probe(struct pci_dev *pci_dev,
 			    const struct pci_device_id *id)
 {
-	struct virtio_pci_device *vp_dev;
+	struct virtio_pci_device *vp_dev, *reg_dev = NULL;
 	int rc;
 
 	/* allocate our structure and fill it out */
@@ -551,6 +551,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 	pci_set_master(pci_dev);
 
 	rc = register_virtio_device(&vp_dev->vdev);
+	/* NOTE: device is considered registered even if register failed. */
+	reg_dev = vp_dev;
 	if (rc)
 		goto err_register;
 
@@ -564,7 +566,10 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 err_probe:
 	pci_disable_device(pci_dev);
 err_enable_device:
-	kfree(vp_dev);
+	if (reg_dev)
+		put_device(dev);
+	else
+		kfree(vp_dev);
 	return rc;
 }