* [PATCH v2 0/3] fix cleanup for fail to register_virtio_device
@ 2017-12-12 13:13 weiping zhang
2017-12-12 13:24 ` [PATCH v2 1/3] virtio_pci: use put_device instead of kfree weiping zhang
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: weiping zhang @ 2017-12-12 13:13 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/3] virtio_pci: use put_device instead of kfree
2017-12-12 13:13 [PATCH v2 0/3] fix cleanup for fail to register_virtio_device weiping zhang
@ 2017-12-12 13:24 ` weiping zhang
2017-12-14 19:13 ` Michael S. Tsirkin
2017-12-12 13:24 ` [PATCH v2 2/3] virtio: " weiping zhang
2017-12-12 13:25 ` [PATCH v2 3/3] virtio: put reference count of virtio_device.dev weiping zhang
2 siblings, 1 reply; 11+ messages in thread
From: weiping zhang @ 2017-12-12 13:24 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
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>
Reviewed-by: Cornelia Huck <cohuck@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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] virtio: use put_device instead of kfree
2017-12-12 13:13 [PATCH v2 0/3] fix cleanup for fail to register_virtio_device weiping zhang
2017-12-12 13:24 ` [PATCH v2 1/3] virtio_pci: use put_device instead of kfree weiping zhang
@ 2017-12-12 13:24 ` weiping zhang
2017-12-13 8:27 ` Cornelia Huck
2017-12-12 13:25 ` [PATCH v2 3/3] virtio: put reference count of virtio_device.dev weiping zhang
2 siblings, 1 reply; 11+ messages in thread
From: weiping zhang @ 2017-12-12 13:24 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
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@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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] virtio: put reference count of virtio_device.dev
2017-12-12 13:13 [PATCH v2 0/3] fix cleanup for fail to register_virtio_device weiping zhang
2017-12-12 13:24 ` [PATCH v2 1/3] virtio_pci: use put_device instead of kfree weiping zhang
2017-12-12 13:24 ` [PATCH v2 2/3] virtio: " weiping zhang
@ 2017-12-12 13:25 ` weiping zhang
2 siblings, 0 replies; 11+ messages in thread
From: weiping zhang @ 2017-12-12 13:25 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
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@didichuxing.com>
Reviewed-by: Cornelia Huck <cohuck@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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] virtio: use put_device instead of kfree
2017-12-12 13:24 ` [PATCH v2 2/3] virtio: " weiping zhang
@ 2017-12-13 8:27 ` Cornelia Huck
0 siblings, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2017-12-13 8:27 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
On Tue, 12 Dec 2017 21:24:33 +0800
weiping zhang <zhangweiping@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@didichuxing.com>
> ---
> drivers/misc/mic/vop/vop_main.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree
2017-12-12 13:24 ` [PATCH v2 1/3] virtio_pci: use put_device instead of kfree weiping zhang
@ 2017-12-14 19:13 ` Michael S. Tsirkin
2017-12-15 1:38 ` weiping zhang
2017-12-15 12:21 ` Cornelia Huck
0 siblings, 2 replies; 11+ messages in thread
From: Michael S. Tsirkin @ 2017-12-14 19:13 UTC (permalink / raw)
To: weiping zhang; +Cc: cohuck, Greg Kroah-Hartman, virtualization
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@didichuxing.com>
> Reviewed-by: Cornelia Huck <cohuck@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@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;
}
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree
2017-12-14 19:13 ` Michael S. Tsirkin
@ 2017-12-15 1:38 ` weiping zhang
2017-12-15 1:48 ` Michael S. Tsirkin
2017-12-15 12:21 ` Cornelia Huck
1 sibling, 1 reply; 11+ messages in thread
From: weiping zhang @ 2017-12-15 1:38 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Cornelia Huck, weiping zhang, Greg Kroah-Hartman, virtualization
2017-12-15 3:13 GMT+08:00 Michael S. Tsirkin <mst@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?
>
just let people know the detail reason of using put_device.
>> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
>> Reviewed-by: Cornelia Huck <cohuck@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@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;
> }
looks more cleaner and same coding style.
Need I send V3 or apply your patch directly ?
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree
2017-12-15 1:38 ` weiping zhang
@ 2017-12-15 1:48 ` Michael S. Tsirkin
2017-12-15 12:32 ` Cornelia Huck
0 siblings, 1 reply; 11+ messages in thread
From: Michael S. Tsirkin @ 2017-12-15 1:48 UTC (permalink / raw)
To: weiping zhang
Cc: Cornelia Huck, weiping zhang, Greg Kroah-Hartman, virtualization
On Fri, Dec 15, 2017 at 09:38:42AM +0800, weiping zhang wrote:
> 2017-12-15 3:13 GMT+08:00 Michael S. Tsirkin <mst@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?
> >
> just let people know the detail reason of using put_device.
> >> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> >> Reviewed-by: Cornelia Huck <cohuck@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@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;
> > }
> looks more cleaner and same coding style.
> Need I send V3 or apply your patch directly ?
Pls post v3 updating all patches to this style.
Also just to make sure, none of this is a regression and none
of this causes actual known issues right?
I think it's preferrable to defer to next merge cycle unless this
is a regression.
> > _______________________________________________
> > Virtualization mailing list
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree
2017-12-14 19:13 ` Michael S. Tsirkin
2017-12-15 1:38 ` weiping zhang
@ 2017-12-15 12:21 ` Cornelia Huck
2017-12-16 22:13 ` Michael S. Tsirkin
1 sibling, 1 reply; 11+ messages in thread
From: Cornelia Huck @ 2017-12-15 12:21 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Greg Kroah-Hartman, weiping zhang, virtualization
On Thu, 14 Dec 2017 21:13:28 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> 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@didichuxing.com>
> > Reviewed-by: Cornelia Huck <cohuck@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.
Yes. The main problem is that device_register() at some point makes the
structure visible to others, at which point they may obtain a
reference. If that happened, you cannot clean up unless that other
party gave up their reference -- which means your only chance to get
this right is the current put_device() approach.
It *is* problematic if all of that stuff is hidden behind too many
calling layers. If you have the device_initialize() -> device_add()
calling sequence, having to do a put_device() on failure is much more
obvious. But as you usually don't pass in a pure struct device but
something embedding it, the put_device() needs to be done on the
outermost level.
Commenting can help here, as would probably a static checker for that
code pattern.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree
2017-12-15 1:48 ` Michael S. Tsirkin
@ 2017-12-15 12:32 ` Cornelia Huck
0 siblings, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2017-12-15 12:32 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Greg Kroah-Hartman, weiping zhang, virtualization
On Fri, 15 Dec 2017 03:48:09 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> Also just to make sure, none of this is a regression and none
> of this causes actual known issues right?
>
> I think it's preferrable to defer to next merge cycle unless this
> is a regression.
I noticed this while looking at the cleanup path for
regsiter_virtio_device(), I don't think any actual problems have been
seen in the wild (just a latent bug). Deferring to the next merge
window seems reasonable.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree
2017-12-15 12:21 ` Cornelia Huck
@ 2017-12-16 22:13 ` Michael S. Tsirkin
0 siblings, 0 replies; 11+ messages in thread
From: Michael S. Tsirkin @ 2017-12-16 22:13 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Greg Kroah-Hartman, weiping zhang, virtualization
On Fri, Dec 15, 2017 at 01:21:27PM +0100, Cornelia Huck wrote:
> On Thu, 14 Dec 2017 21:13:28 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > 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@didichuxing.com>
> > > Reviewed-by: Cornelia Huck <cohuck@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.
>
> Yes. The main problem is that device_register() at some point makes the
> structure visible to others, at which point they may obtain a
> reference. If that happened, you cannot clean up unless that other
> party gave up their reference -- which means your only chance to get
> this right is the current put_device() approach.
>
> It *is* problematic if all of that stuff is hidden behind too many
> calling layers. If you have the device_initialize() -> device_add()
> calling sequence, having to do a put_device() on failure is much more
> obvious. But as you usually don't pass in a pure struct device but
> something embedding it, the put_device() needs to be done on the
> outermost level.
>
> Commenting can help here, as would probably a static checker for that
> code pattern.
A semantic patch is probably the best we can do here.
--
MST
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-12-16 22:13 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-12 13:13 [PATCH v2 0/3] fix cleanup for fail to register_virtio_device weiping zhang
2017-12-12 13:24 ` [PATCH v2 1/3] virtio_pci: use put_device instead of kfree weiping zhang
2017-12-14 19:13 ` Michael S. Tsirkin
2017-12-15 1:38 ` weiping zhang
2017-12-15 1:48 ` Michael S. Tsirkin
2017-12-15 12:32 ` Cornelia Huck
2017-12-15 12:21 ` Cornelia Huck
2017-12-16 22:13 ` Michael S. Tsirkin
2017-12-12 13:24 ` [PATCH v2 2/3] virtio: " weiping zhang
2017-12-13 8:27 ` Cornelia Huck
2017-12-12 13:25 ` [PATCH v2 3/3] virtio: put reference count of virtio_device.dev weiping zhang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.