Linux Virtualization - Dec 2017 - [PATCH v4 3/4] virtio_vop: don't kfree device on register failure

Hi,

The main change is split device_register into 2 sperate calls:
device_initalize() and device_add, and then the caller can use
put_device safety when fail to register_virtio_device.

v3->v4:
 * split device_register into device_initialize and devicea_add that
   the caller can always use put_device when fail to register virtio
   device.

v2->v3:
 * virtio: add new helper do get device's status then determine use
   put_device or kfree.

v1->v2:
 * virtio_pci: add comments in commit message for why using put_device
 * virtio_vop: also use put_device int _vop_remove_device
weiping zhang (4):
  virtio: split device_register into device_initialize and device_add
  virtio_pci: don't kfree device on register failure
  virtio_vop: don't kfree device on register failure
  virtio_remoteproc: don't kfree device on register failure

 drivers/misc/mic/vop/vop_main.c        | 20 +++++++++++++-------
 drivers/remoteproc/remoteproc_virtio.c | 13 +++++++++++--
 drivers/virtio/virtio.c                | 18 +++++++++++++++---
 drivers/virtio/virtio_pci_common.c     |  8 ++++++--
 4 files changed, 45 insertions(+), 14 deletions(-)

-- 
2.9.4

In order to make caller do a simple cleanup, we split device_register
into device_initialize and device_add. device_initialize always sucess,
the caller can always use put_device when fail to register virtio_device
no matter fail at ida_simple_get or at device_add.

Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>
Suggested-by: Cornelia Huck <cohuck at redhat.com>
---
 drivers/virtio/virtio.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index bf7ff39..3c9f211 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -303,11 +303,21 @@ void unregister_virtio_driver(struct virtio_driver
*driver)
 }
 EXPORT_SYMBOL_GPL(unregister_virtio_driver);
 
+/**
+ * register_virtio_device - register virtio device
+ * @dev        : virtio device interested
+ *
+ * If an error occurs, the caller must use put_device, instead of kfree,
because
+ * device_initialize and device_add will increase @dev->dev's reference
count.
+ *
+ * Returns: 0 on suceess, -error on failure
+ */
 int register_virtio_device(struct virtio_device *dev)
 {
 	int err;
 
 	dev->dev.bus = &virtio_bus;
+	device_initialize(&dev->dev);
 
 	/* Assign a unique device index and hence name. */
 	err = ida_simple_get(&virtio_index_ida, 0, 0, GFP_KERNEL);
@@ -330,9 +340,11 @@ int register_virtio_device(struct virtio_device *dev)
 
 	INIT_LIST_HEAD(&dev->vqs);
 
-	/* device_register() causes the bus infrastructure to look for a
-	 * matching driver. */
-	err = device_register(&dev->dev);
+	/*
+	 * device_add() causes the bus infrastructure to look for a matching
+	 * driver.
+	 */
+	err = device_add(&dev->dev);
 	if (err)
 		ida_simple_remove(&virtio_index_ida, dev->index);
 out:
-- 
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>
---
 drivers/virtio/virtio_pci_common.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
index 1c4797e..48d4d1c 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,7 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 	pci_set_master(pci_dev);
 
 	rc = register_virtio_device(&vp_dev->vdev);
+	reg_dev = vp_dev;
 	if (rc)
 		goto err_register;
 
@@ -564,7 +565,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(&vp_dev->vdev.dev);
+	else
+		kfree(vp_dev);
 	return rc;
 }
 
-- 
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 vdev until vdev->vdev.dev.release be called.

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

diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index a341938..3633202 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);
 }
 
 /*
@@ -466,7 +468,7 @@ static int _vop_add_device(struct mic_device_desc __iomem
*d,
 			   unsigned int offset, struct vop_device *vpdev,
 			   int dnode)
 {
-	struct _vop_vdev *vdev;
+	struct _vop_vdev *vdev, *reg_dev = NULL;
 	int ret;
 	u8 type = ioread8(&d->type);
 
@@ -497,6 +499,7 @@ static int _vop_add_device(struct mic_device_desc __iomem
*d,
 	vdev->c2h_vdev_db = ioread8(&vdev->dc->c2h_vdev_db);
 
 	ret = register_virtio_device(&vdev->vdev);
+	reg_dev = vdev;
 	if (ret) {
 		dev_err(_vop_dev(vdev),
 			"Failed to register vop device %u type %u\n",
@@ -512,7 +515,10 @@ static int _vop_add_device(struct mic_device_desc __iomem
*d,
 free_irq:
 	vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
 kfree:
-	kfree(vdev);
+	if (reg_dev)
+		put_device(&vdev->vdev.dev);
+	else
+		kfree(vdev);
 	return ret;
 }
 
@@ -568,7 +574,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 check if we call device_register
or not, if called, put vdev.dev, and then rproc->dev's cleanup will be
done in rproc_virtio_dev_release, otherwise we do cleanup directly.

Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>
---
 drivers/remoteproc/remoteproc_virtio.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_virtio.c
b/drivers/remoteproc/remoteproc_virtio.c
index 2946348..1073ea3 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -304,7 +304,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 {
 	struct rproc *rproc = rvdev->rproc;
 	struct device *dev = &rproc->dev;
-	struct virtio_device *vdev = &rvdev->vdev;
+	struct virtio_device *vdev = &rvdev->vdev, *reg_dev = NULL;
 	int ret;
 
 	vdev->id.device	= id,
@@ -326,15 +326,24 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 	kref_get(&rvdev->refcount);
 
 	ret = register_virtio_device(vdev);
+	reg_dev = vdev;
 	if (ret) {
-		put_device(&rproc->dev);
 		dev_err(dev, "failed to register vdev: %d\n", ret);
 		goto out;
 	}
 
 	dev_info(dev, "registered %s (type %d)\n",
dev_name(&vdev->dev), id);
 
+	return 0;
+
 out:
+	if (reg_dev)
+		put_device(&vdev->dev);
+	else {
+		kref_put(&rvdev->refcount, rproc_vdev_release);
+		put_device(&rproc->dev);
+	}
+
 	return ret;
 }
 
-- 
2.9.4

On Wed, 20 Dec 2017 12:26:25 +0800
weiping zhang <zwp10758 at gmail.com> wrote:

[you used a different mail address in your From: than in your s-o-b:;
same for the other patches]
> In order to make caller do a simple cleanup, we split device_register
> into device_initialize and device_add. device_initialize always sucess,
s/success/succeeds/
> the caller can always use put_device when fail to register virtio_device
"so the caller can always use put_device when register_virtio_device
failed,"
> no matter fail at ida_simple_get or at device_add.
"no matter whether it failed..."
> 
> Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>
> Suggested-by: Cornelia Huck <cohuck at redhat.com>
> ---
>  drivers/virtio/virtio.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index bf7ff39..3c9f211 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -303,11 +303,21 @@ void unregister_virtio_driver(struct virtio_driver
*driver)
>  }
>  EXPORT_SYMBOL_GPL(unregister_virtio_driver);
>  
> +/**
> + * register_virtio_device - register virtio device
> + * @dev        : virtio device interested
"virtio device to be registered"
> + *
> + * If an error occurs, the caller must use put_device, instead of kfree,
because
> + * device_initialize and device_add will increase @dev->dev's
reference count.
That's not correct: It's not because of device_add increasing the
reference count (it releases it again on failure), but because another
code path may have obtained a reference.

What about:

"On error, the caller must call put_device on &@dev->dev (and not
kfree), as another code path may have obtained a reference to @dev."
> + *
> + * Returns: 0 on suceess, -error on failure
> + */
>  int register_virtio_device(struct virtio_device *dev)
>  {
>  	int err;
>  
>  	dev->dev.bus = &virtio_bus;
> +	device_initialize(&dev->dev);
>  
>  	/* Assign a unique device index and hence name. */
>  	err = ida_simple_get(&virtio_index_ida, 0, 0, GFP_KERNEL);
> @@ -330,9 +340,11 @@ int register_virtio_device(struct virtio_device *dev)
>  
>  	INIT_LIST_HEAD(&dev->vqs);
>  
> -	/* device_register() causes the bus infrastructure to look for a
> -	 * matching driver. */
> -	err = device_register(&dev->dev);
> +	/*
> +	 * device_add() causes the bus infrastructure to look for a matching
> +	 * driver.
FWIW, I would just have done s/device_register/device_add/ in the
comment, but this is ok as well.
> +	 */
> +	err = device_add(&dev->dev);
>  	if (err)
>  		ida_simple_remove(&virtio_index_ida, dev->index);
>  out:
Your code change is fine.

On Wed, 20 Dec 2017 12:26:43 +0800
weiping zhang <zwp10758 at gmail.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_dev until vp_dev->vdev.dev.release be called.
> 
> Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>
> ---
>  drivers/virtio/virtio_pci_common.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
> index 1c4797e..48d4d1c 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;
Not sure if I would have used a pointer for this purpose, but as that
is what Michael had proposed...
>  	int rc;
>  
>  	/* allocate our structure and fill it out */
> @@ -551,6 +551,7 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
>  	pci_set_master(pci_dev);
>  
>  	rc = register_virtio_device(&vp_dev->vdev);
> +	reg_dev = vp_dev;
>  	if (rc)
>  		goto err_register;
>  
> @@ -564,7 +565,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(&vp_dev->vdev.dev);
> +	else
> +		kfree(vp_dev);
>  	return rc;
>  }
>  
Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Wed, 20 Dec 2017 12:27:04 +0800
weiping zhang <zwp10758 at gmail.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 vdev until vdev->vdev.dev.release be called.
> 
> Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>
> ---
>  drivers/misc/mic/vop/vop_main.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/misc/mic/vop/vop_main.c
b/drivers/misc/mic/vop/vop_main.c
> index a341938..3633202 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);
>  }
>  
>  /*
> @@ -466,7 +468,7 @@ static int _vop_add_device(struct mic_device_desc
__iomem *d,
>  			   unsigned int offset, struct vop_device *vpdev,
>  			   int dnode)
>  {
> -	struct _vop_vdev *vdev;
> +	struct _vop_vdev *vdev, *reg_dev = NULL;
Similarly, not a fan of that pointer variable, but it's fine.
>  	int ret;
>  	u8 type = ioread8(&d->type);
>  
> @@ -497,6 +499,7 @@ static int _vop_add_device(struct mic_device_desc
__iomem *d,
>  	vdev->c2h_vdev_db = ioread8(&vdev->dc->c2h_vdev_db);
>  
>  	ret = register_virtio_device(&vdev->vdev);
> +	reg_dev = vdev;
>  	if (ret) {
>  		dev_err(_vop_dev(vdev),
>  			"Failed to register vop device %u type %u\n",
> @@ -512,7 +515,10 @@ static int _vop_add_device(struct mic_device_desc
__iomem *d,
>  free_irq:
>  	vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
>  kfree:
> -	kfree(vdev);
> +	if (reg_dev)
> +		put_device(&vdev->vdev.dev);
> +	else
> +		kfree(vdev);
>  	return ret;
>  }
>  
> @@ -568,7 +574,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));
Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Wed, 20 Dec 2017 12:27:33 +0800
weiping zhang <zwp10758 at gmail.com> wrote:
> rproc_virtio_dev_release will be called iff virtio_device.dev's
> refer count became to 0. Here we should check if we call device_register
"reference count drops to 0"

s/call/called/
> or not, if called, put vdev.dev, and then rproc->dev's cleanup will
be
> done in rproc_virtio_dev_release, otherwise we do cleanup directly.
> 
> Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>
> ---
>  drivers/remoteproc/remoteproc_virtio.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_virtio.c
b/drivers/remoteproc/remoteproc_virtio.c
> index 2946348..1073ea3 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -304,7 +304,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int
id)
>  {
>  	struct rproc *rproc = rvdev->rproc;
>  	struct device *dev = &rproc->dev;
> -	struct virtio_device *vdev = &rvdev->vdev;
> +	struct virtio_device *vdev = &rvdev->vdev, *reg_dev = NULL;
>  	int ret;
>  
>  	vdev->id.device	= id,
> @@ -326,15 +326,24 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev,
int id)
>  	kref_get(&rvdev->refcount);
>  
>  	ret = register_virtio_device(vdev);
> +	reg_dev = vdev;
>  	if (ret) {
> -		put_device(&rproc->dev);
>  		dev_err(dev, "failed to register vdev: %d\n", ret);
>  		goto out;
>  	}
>  
>  	dev_info(dev, "registered %s (type %d)\n",
dev_name(&vdev->dev), id);
>  
> +	return 0;
> +
>  out:
> +	if (reg_dev)
> +		put_device(&vdev->dev);
> +	else {
> +		kref_put(&rvdev->refcount, rproc_vdev_release);
> +		put_device(&rproc->dev);
> +	}
> +
>  	return ret;
>  }
>  
I think in this case using the marker makes a straightforward cleanup
way too complicated. There's a single way we can get to the out label,
and that's when register_virtio_device() failed. Switching
put_device(&rproc->dev) to put_device(@vdev->dev) (what your first
patch did) seems like the way to go.

(It also may be good to cc: the maintainers for this driver.)