virtio: release virtio index when fail to device_register

PATCH In review weiping zhang 3 participants 8 messages Nov 29, 2017 – Dec 5, 2017

Subsystems (2)

Stable branch virtio core

lore

Share & tools

lore

weiping zhang

author

2017-11-29 01:23 UTC #0 lore raw

PATCH 1/1 virtio: release virtio index when fail to device_register

index can be reused by other virtio device.

Signed-off-by weiping zhang <[email protected]>

---
 drivers/virtio/virtio.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/virtio/virtio.c +2 -0

index 48230a5..bf7ff39 100644
@@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev)
 	/* device_register() causes the bus infrastructure to look for a
 	 * matching driver. */
 	err = device_register(&dev->dev);
+	if (err)
+		ida_simple_remove(&virtio_index_ida, dev->index);
 out:
 	if (err)
 		virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);

Signature · 1 line

2.9.4

Cornelia Huck

Reviewed-by on v1 1/1

2017-11-29 09:50 UTC #1 lore raw

Quoted 3 lines · On Wed, 29 Nov 2017 09:23:01 +0800 weiping zhang <[email protected]> wrote:

index can be reused by other virtio device.
Signed-off-by weiping zhang <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

Quoted 9 lines

---
 drivers/virtio/virtio.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/virtio/virtio.c +0 -0

index 48230a5..bf7ff39 100644

drivers/virtio/virtio.c int register_virtio_device(struct virtio_device *dev) · line 340

Quoted 8 lines

drivers/virtio/virtio.c +2 -0

@@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev)
 	/* device_register() causes the bus infrastructure to look for a
 	 * matching driver. */
 	err = device_register(&dev->dev);
+	if (err)
+		ida_simple_remove(&virtio_index_ida, dev->index);
 out:
 	if (err)
 		virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);

I think your patch is correct (giving up the index if we failed to register), but this made me look at our error handling if a virtio device failed to register.

We hold an extra reference to the struct device, even after a failed register, and it is the responsibility of the caller to give up that reference once no longer needed. As callers toregister_virtio_device() embed the struct virtio_device, it needs to be their responsibility. Looking at the existing callers,

- ccw does a put_device
- pci, mmio and remoteproc do nothing, causing a leak
- vop does a free on the embedding structure, which is a big no-no

Thoughts?

Michael S. Tsirkin

maintainer

2017-11-29 13:55 UTC #6 lore raw

In reply to weiping zhang

Quoted 3 lines · weiping zhang

index can be reused by other virtio device.
Signed-off-by weiping zhang <[email protected]>

Thanks!
I've queued this up, this is needed on stable as well.

Quoted 19 lines

---
 drivers/virtio/virtio.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/virtio/virtio.c +2 -0

index 48230a5..bf7ff39 100644
@@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev)
 	/* device_register() causes the bus infrastructure to look for a
 	 * matching driver. */
 	err = device_register(&dev->dev);
+	if (err)
+		ida_simple_remove(&virtio_index_ida, dev->index);
 out:
 	if (err)
 		virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);

Signature · 1 line

2.9.4

Michael S. Tsirkin

maintainer

2017-11-29 13:55 UTC #7 lore raw

In reply to weiping zhang

Quoted 3 lines · weiping zhang

index can be reused by other virtio device.
Signed-off-by weiping zhang <[email protected]>

Thanks!
I've queued this up, this is needed on stable as well.

Quoted 19 lines

---
 drivers/virtio/virtio.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/virtio/virtio.c +2 -0

index 48230a5..bf7ff39 100644
@@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev)
 	/* device_register() causes the bus infrastructure to look for a
 	 * matching driver. */
 	err = device_register(&dev->dev);
+	if (err)
+		ida_simple_remove(&virtio_index_ida, dev->index);
 out:
 	if (err)
 		virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);

Signature · 1 line

2.9.4

weiping zhang

author

2017-12-01 16:55 UTC #2 lore raw

In reply to Cornelia Huck

Quoted 37 lines · Cornelia Huck

Quoted 3 lines · On Wed, 29 Nov 2017 09:23:01 +0800 weiping zhang <[email protected]> wrote:
index can be reused by other virtio device.
Signed-off-by weiping zhang <[email protected]>
Reviewed-by Cornelia Huck <[email protected]>
Quoted 9 lines
---
 drivers/virtio/virtio.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/virtio/virtio.c +0 -0
index 48230a5..bf7ff39 100644
drivers/virtio/virtio.c int register_virtio_device(struct virtio_device *dev) · line 340
Quoted 8 lines
drivers/virtio/virtio.c +2 -0
@@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev)
 	/* device_register() causes the bus infrastructure to look for a
 	 * matching driver. */
 	err = device_register(&dev->dev);
+	if (err)
+		ida_simple_remove(&virtio_index_ida, dev->index);
 out:
 	if (err)
 		virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
I think your patch is correct (giving up the index if we failed to register), but this made me look at our error handling if a virtio device failed to register.
We hold an extra reference to the struct device, even after a failed register, and it is the responsibility of the caller to give up that reference once no longer needed. As callers toregister_virtio_device() embed the struct virtio_device, it needs to be their responsibility. Looking at the existing callers,

Quoted 5 lines

- ccw does a put_device
- pci, mmio and remoteproc do nothing, causing a leak
- vop does a free on the embedding structure, which is a big no-no
Thoughts?

Sorry to relay late and thanks for your review.
Do you mean the "extra reference to the struct device" caused by the following code?

err = device_register(&dev->dev);
device_add(dev)
get_device(dev)
If I'm understand right, I think there is no extra reference if we fail virtio_register_device, because if device_register we don't get a reference.

--
Thanks
weiping

weiping zhang

author

2017-12-01 17:30 UTC #3 lore raw

2017-12-02 0:55 GMT+08:00 weiping zhang <[email protected]>:

Quoted 47 lines · Cornelia Huck

Quoted 37 lines
Quoted 3 lines · On Wed, 29 Nov 2017 09:23:01 +0800 weiping zhang <[email protected]> wrote:
index can be reused by other virtio device.
Signed-off-by weiping zhang <[email protected]>
Reviewed-by Cornelia Huck <[email protected]>
Quoted 9 lines
---
 drivers/virtio/virtio.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/virtio/virtio.c +0 -0
index 48230a5..bf7ff39 100644
drivers/virtio/virtio.c int register_virtio_device(struct virtio_device *dev) · line 340
Quoted 8 lines
drivers/virtio/virtio.c +2 -0
@@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev)
    /* device_register() causes the bus infrastructure to look for a
     * matching driver. */
    err = device_register(&dev->dev);
+   if (err)
+           ida_simple_remove(&virtio_index_ida, dev->index);
 out:
    if (err)
            virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
I think your patch is correct (giving up the index if we failed to register), but this made me look at our error handling if a virtio device failed to register.
We hold an extra reference to the struct device, even after a failed register, and it is the responsibility of the caller to give up that reference once no longer needed. As callers toregister_virtio_device() embed the struct virtio_device, it needs to be their responsibility. Looking at the existing callers,
Quoted 5 lines
- ccw does a put_device
- pci, mmio and remoteproc do nothing, causing a leak
- vop does a free on the embedding structure, which is a big no-no
Thoughts?
Sorry to relay late and thanks for your review.
Do you mean the "extra reference to the struct device" caused by the following code?

Quoted 6 lines

err = device_register(&dev->dev);
device_add(dev)
get_device(dev)
If I'm understand right, I think there is no extra reference if we fail virtio_register_device, because if device_register we don't get a reference.

I go through these callers, virtio_mmio_probe should do more cleanup. I'll sent a patch fix that.

Quoted 3 lines

--
Thanks
weiping

Cornelia Huck

2017-12-04 09:38 UTC #4 lore raw

In reply to weiping zhang

Quoted 17 lines · On Sat, 2 Dec 2017 00:55:39 +0800 weiping zhang <[email protected]> wrote:

Quoted 11 lines · Cornelia Huck
We hold an extra reference to the struct device, even after a failed register, and it is the responsibility of the caller to give up that reference once no longer needed. As callers toregister_virtio_device() embed the struct virtio_device, it needs to be their responsibility. Looking at the existing callers,
- ccw does a put_device
- pci, mmio and remoteproc do nothing, causing a leak
- vop does a free on the embedding structure, which is a big no-no
Thoughts?
Sorry to relay late and thanks for your review.
Do you mean the "extra reference to the struct device" caused by the following code?

Quoted 6 lines

err = device_register(&dev->dev);
device_add(dev)
get_device(dev)
If I'm understand right, I think there is no extra reference if we fail virtio_register_device, because if device_register we don't get a reference.

The device_initialize() already gives you a reference. If device_add() fails, it has cleaned up any additional reference it might have obtained, but the initial reference is still there and needs to be released by the caller.

weiping zhang

author

2017-12-05 01:30 UTC #5 lore raw

2017-12-04 17:38 GMT+08:00 Cornelia Huck <[email protected]>:

Quoted 27 lines

Quoted 17 lines · On Sat, 2 Dec 2017 00:55:39 +0800 weiping zhang <[email protected]> wrote:
Quoted 11 lines · Cornelia Huck
We hold an extra reference to the struct device, even after a failed register, and it is the responsibility of the caller to give up that reference once no longer needed. As callers toregister_virtio_device() embed the struct virtio_device, it needs to be their responsibility. Looking at the existing callers,
- ccw does a put_device
- pci, mmio and remoteproc do nothing, causing a leak
- vop does a free on the embedding structure, which is a big no-no
Thoughts?
Sorry to relay late and thanks for your review.
Do you mean the "extra reference to the struct device" caused by the following code?
Quoted 6 lines
err = device_register(&dev->dev);
device_add(dev)
get_device(dev)
If I'm understand right, I think there is no extra reference if we fail virtio_register_device, because if device_register we don't get a reference.

Quoted 4 lines

The device_initialize() already gives you a reference. If device_add() fails, it has cleaned up any additional reference it might have obtained, but the initial reference is still there and needs to be released by the caller.

Thanks your clarify, I also notice the comments at device_register, device_initialize, device_add,

* 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.

--
Thanks
weiping