[PATCH v3 0/5] proper cleanup if fail to register_virtio_device

[PATCH v3 0/5] proper cleanup if fail to register_virtio_device

[weiping zhang]

Hi,

Patch1 add a helper to get virtio_device's status which will be used
later.
Patch2~4: check virtio_device's status is RTIO_CONFIG_S_ACKNOWLEDGE
or not, if so use put_device otherwise use kfree.
Patch5: add comments for virtio_register_device help caller do a
proper cleanup if got failure.

weiping zhang (5):
  virtio: add helper virtio_get_status
  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
  virtio: add comments for virtio_register_device

 drivers/misc/mic/vop/vop_main.c        | 17 +++++++++++------
 drivers/remoteproc/remoteproc_virtio.c | 10 +++++++++-
 drivers/virtio/virtio.c                | 19 +++++++++++++++++++
 drivers/virtio/virtio_pci_common.c     |  5 ++++-
 include/linux/virtio_config.h          |  2 ++
 5 files changed, 45 insertions(+), 8 deletions(-)

-- 
2.9.4

[PATCH v3 1/5] virtio: add helper virtio_get_status

[weiping zhang]

add helper function to simplify dev->config->get_status().

Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
 drivers/virtio/virtio.c       | 6 ++++++
 include/linux/virtio_config.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index bf7ff39..c5b057bd 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -165,6 +165,12 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
 }
 EXPORT_SYMBOL_GPL(virtio_add_status);
 
+unsigned int virtio_get_status(struct virtio_device *dev)
+{
+	return dev->config->get_status(dev);
+}
+EXPORT_SYMBOL_GPL(virtio_get_status);
+
 int virtio_finalize_features(struct virtio_device *dev)
 {
 	int ret = dev->config->finalize_features(dev);
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 5559a2d..30972c4 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -429,6 +429,8 @@ static inline void virtio_cwrite64(struct virtio_device *vdev,
 	vdev->config->set(vdev, offset, &val, sizeof(val));
 }
 
+unsigned int virtio_get_status(struct virtio_device *dev);
+
 /* Conditional config space accessors. */
 #define virtio_cread_feature(vdev, fbit, structname, member, ptr)	\
 	({								\
-- 
2.9.4

[PATCH v3 2/5] virtio_pci: don't kfree device on register failure

[weiping zhang]

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@didichuxing.com>
---
 drivers/virtio/virtio_pci_common.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 1c4797e..21a2ce0 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -564,7 +564,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 (VIRTIO_CONFIG_S_ACKNOWLEDGE & virtio_get_status(&vp_dev->vdev))
+		put_device(&vp_dev->vdev.dev);
+	else
+		kfree(vp_dev);
 	return rc;
 }
 
-- 
2.9.4

[PATCH v3 3/5] virtio_vop: don't kfree device on register failure

[weiping zhang]

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@didichuxing.com>
---
 drivers/misc/mic/vop/vop_main.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index a341938..53eaa9d 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);
 }
 
 /*
@@ -512,7 +514,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 (VIRTIO_CONFIG_S_ACKNOWLEDGE & virtio_get_status(&vdev->vdev))
+		put_device(&vdev->vdev.dev);
+	else
+		kfree(vdev);
 	return ret;
 }
 
@@ -568,7 +573,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

[PATCH v3 4/5] virtio_remoteproc: don't kfree device on register failure

[weiping zhang]

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@didichuxing.com>
---
 drivers/remoteproc/remoteproc_virtio.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 2946348..ad5d6d1 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -327,14 +327,22 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 
 	ret = register_virtio_device(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 (VIRTIO_CONFIG_S_ACKNOWLEDGE & virtio_get_status(vdev))
+		put_device(&vdev->dev);
+	else {
+		kref_put(&rvdev->refcount, rproc_vdev_release);
+		put_device(&rproc->dev);
+	}
+
 	return ret;
 }
 
-- 
2.9.4

[PATCH v3 5/5] virtio: add comments for virtio_register_device

[weiping zhang]

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.
*/
virtio_register_device may fail before/after call device_register, the
caller should do a proper cleanup. Caller cann't use kfree directly,
if virtio_register_device has already called device_register. Caller
cann't use put_device directly, if virtio_register_device has not yet
call device_register, because kobject_put may give a warning cause
dev->kobj has not been initialized.

Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
 drivers/virtio/virtio.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index c5b057bd..4f0718b 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -309,6 +309,19 @@ void unregister_virtio_driver(struct virtio_driver *driver)
 }
 EXPORT_SYMBOL_GPL(unregister_virtio_driver);
 
+/**
+ * register_virtio_device - register virtio device
+ *
+ * @dev	: virtio device interested
+ *
+ * This funciton may fail after call device_register, as mentioned at
+ * drivers/base/core.c we must use put_device(), _never_ directly free @dev.
+ * The caller should take care of the status of @dev and determine to use
+ * put_device or kfree. If @dev in VIRTIO_CONFIG_S_ACKNOWLEDGE status that
+ * means caller should use put_device otherwise use kfree directly.
+ *
+ * Returns: 0 on success, error on failure.
+ */
 int register_virtio_device(struct virtio_device *dev)
 {
 	int err;
-- 
2.9.4

Re: [PATCH v3 5/5] virtio: add comments for virtio_register_device

[Cornelia Huck]

On Sun, 17 Dec 2017 21:48:05 +0800
weiping zhang <zwp10758@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.
> */
> virtio_register_device may fail before/after call device_register, the
> caller should do a proper cleanup. Caller cann't use kfree directly,
> if virtio_register_device has already called device_register. Caller
> cann't use put_device directly, if virtio_register_device has not yet
> call device_register, because kobject_put may give a warning cause
> dev->kobj has not been initialized.

This comment makes me inclined to think that we should also rethink
register_virtio_device(). On failure, we cannot do kfree() due to
driver core interaction; but we cannot do a put_device() either, since
the refcount may not yet have been initialized -- unless we check the
device status, which triggers I/O (at least on s390).

We really want to do the same cleanup on error in every case. What
about splitting device_register() into device_initialize() and
device_add()? If we move device_initialize() before getting an index,
we should be fine with doing put_device() on error in every case.

> 
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
>  drivers/virtio/virtio.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index c5b057bd..4f0718b 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -309,6 +309,19 @@ void unregister_virtio_driver(struct virtio_driver *driver)
>  }
>  EXPORT_SYMBOL_GPL(unregister_virtio_driver);
>  
> +/**
> + * register_virtio_device - register virtio device
> + *
> + * @dev	: virtio device interested
> + *
> + * This funciton may fail after call device_register, as mentioned at
> + * drivers/base/core.c we must use put_device(), _never_ directly free @dev.
> + * The caller should take care of the status of @dev and determine to use
> + * put_device or kfree. If @dev in VIRTIO_CONFIG_S_ACKNOWLEDGE status that
> + * means caller should use put_device otherwise use kfree directly.
> + *
> + * Returns: 0 on success, error on failure.
> + */
>  int register_virtio_device(struct virtio_device *dev)
>  {
>  	int err;

Re: [PATCH v3 5/5] virtio: add comments for virtio_register_device

[weiping zhang]

2017-12-19 19:11 GMT+08:00 Cornelia Huck <cohuck@redhat.com>:
> On Sun, 17 Dec 2017 21:48:05 +0800
> weiping zhang <zwp10758@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.
>> */
>> virtio_register_device may fail before/after call device_register, the
>> caller should do a proper cleanup. Caller cann't use kfree directly,
>> if virtio_register_device has already called device_register. Caller
>> cann't use put_device directly, if virtio_register_device has not yet
>> call device_register, because kobject_put may give a warning cause
>> dev->kobj has not been initialized.
>
> This comment makes me inclined to think that we should also rethink
> register_virtio_device(). On failure, we cannot do kfree() due to
> driver core interaction; but we cannot do a put_device() either, since
> the refcount may not yet have been initialized -- unless we check the
> device status, which triggers I/O (at least on s390).
>
> We really want to do the same cleanup on error in every case. What
> about splitting device_register() into device_initialize() and
> device_add()? If we move device_initialize() before getting an index,
> we should be fine with doing put_device() on error in every case.
>
Good idea (^_^), I''ll apply it at V4.