2018-03-18 06:49:43

by Arvind Yadav

[permalink] [raw]
Subject: [PATCH] vmbus: use put_device() if device_register fail

if device_register() returned an error. Always use put_device()
to give up the reference initialized.

Signed-off-by: Arvind Yadav <[email protected]>
---
drivers/hv/vmbus_drv.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index bc65c4d..25da2f3 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1358,6 +1358,7 @@ int vmbus_device_register(struct hv_device *child_device_obj)
ret = device_register(&child_device_obj->device);
if (ret) {
pr_err("Unable to register child device\n");
+ put_device(&child_device_obj->device);
return ret;
}

--
2.7.4



2018-03-18 22:39:24

by Michael Kelley (EOSG)

[permalink] [raw]
Subject: RE: [PATCH] vmbus: use put_device() if device_register fail

> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf
> Of Arvind Yadav
> Sent: Saturday, March 17, 2018 11:48 PM
> To: Stephen Hemminger <[email protected]>; Haiyang Zhang
> <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: [PATCH] vmbus: use put_device() if device_register fail
>
> if device_register() returned an error. Always use put_device()
> to give up the reference initialized.
>
> Signed-off-by: Arvind Yadav <[email protected]>

Reviewed-by: Michael Kelley <[email protected]>

> ---
> drivers/hv/vmbus_drv.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index bc65c4d..25da2f3 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1358,6 +1358,7 @@ int vmbus_device_register(struct hv_device *child_device_obj)
> ret = device_register(&child_device_obj->device);
> if (ret) {
> pr_err("Unable to register child device\n");
> + put_device(&child_device_obj->device);
> return ret;
> }
>
> --
> 2.7.4


2018-03-19 03:04:19

by KY Srinivasan

[permalink] [raw]
Subject: RE: [PATCH] vmbus: use put_device() if device_register fail



> -----Original Message-----
> From: devel <[email protected]> On Behalf
> Of Arvind Yadav
> Sent: Saturday, March 17, 2018 11:48 PM
> To: Stephen Hemminger <[email protected]>; Haiyang Zhang
> <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: [PATCH] vmbus: use put_device() if device_register fail
>
> if device_register() returned an error. Always use put_device()
> to give up the reference initialized.
>
> Signed-off-by: Arvind Yadav <[email protected]>
> ---
> drivers/hv/vmbus_drv.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index bc65c4d..25da2f3 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1358,6 +1358,7 @@ int vmbus_device_register(struct hv_device
> *child_device_obj)
> ret = device_register(&child_device_obj->device);
> if (ret) {
> pr_err("Unable to register child device\n");
> + put_device(&child_device_obj->device);

If the registration failed; we would not have acquired the reference on the device and so
we should not be dropping the reference in the failure path.

K. Y
> return ret;
> }
>
> --
> 2.7.4
>
> _______________________________________________
> devel mailing list
> [email protected]
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdriverd
> ev.linuxdriverproject.org%2Fmailman%2Flistinfo%2Fdriverdev-
> devel&data=04%7C01%7Ckys%40microsoft.com%7Caf95bfab917f4e1fa4b008
> d58c9c3b72%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63656952
> 5011478334%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIj
> oiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-
> 1&sdata=%2FEb%2FMTY2SMh8sY1tar2c8Om2uKPUZAXIUQkrG0q07CA%3D&r
> eserved=0

2018-03-19 03:41:16

by Michael Kelley (EOSG)

[permalink] [raw]
Subject: RE: [PATCH] vmbus: use put_device() if device_register fail

> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf
> Of KY Srinivasan
> Sent: Sunday, March 18, 2018 8:02 PM
> To: Arvind Yadav <[email protected]>; Stephen Hemminger
> <[email protected]>; Haiyang Zhang <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: RE: [PATCH] vmbus: use put_device() if device_register fail
>
> > -----Original Message-----
> > From: devel <[email protected]> On Behalf
> > Of Arvind Yadav
> > Sent: Saturday, March 17, 2018 11:48 PM
> > To: Stephen Hemminger <[email protected]>; Haiyang Zhang
> > <[email protected]>
> > Cc: [email protected]; [email protected]
> > Subject: [PATCH] vmbus: use put_device() if device_register fail
> >
> > if device_register() returned an error. Always use put_device()
> > to give up the reference initialized.
> >
> > Signed-off-by: Arvind Yadav <[email protected]>
> > ---
> > drivers/hv/vmbus_drv.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index bc65c4d..25da2f3 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -1358,6 +1358,7 @@ int vmbus_device_register(struct hv_device
> > *child_device_obj)
> > ret = device_register(&child_device_obj->device);
> > if (ret) {
> > pr_err("Unable to register child device\n");
> > + put_device(&child_device_obj->device);
>
> If the registration failed; we would not have acquired the reference on the device and so
> we should not be dropping the reference in the failure path.

Looking at the code for device_register() and its constituent parts
device_initialize() and device_add(), Arvind is correct. device_initialize()
creates the object with a reference count of 1. device_add() does not
decrement that reference count or free the object, even if it fails. The
comments for device_register() call this out as well.

Michael

>
> K. Y
> > return ret;
> > }
> >
> > --
> > 2.7.4

2018-03-19 05:54:04

by KY Srinivasan

[permalink] [raw]
Subject: RE: [PATCH] vmbus: use put_device() if device_register fail



> -----Original Message-----
> From: Michael Kelley (EOSG)
> Sent: Sunday, March 18, 2018 8:40 PM
> To: KY Srinivasan <[email protected]>; Arvind Yadav
> <[email protected]>; Stephen Hemminger
> <[email protected]>; Haiyang Zhang <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: RE: [PATCH] vmbus: use put_device() if device_register fail
>
> > -----Original Message-----
> > From: [email protected] <linux-kernel-
> [email protected]> On Behalf
> > Of KY Srinivasan
> > Sent: Sunday, March 18, 2018 8:02 PM
> > To: Arvind Yadav <[email protected]>; Stephen Hemminger
> > <[email protected]>; Haiyang Zhang <[email protected]>
> > Cc: [email protected]; [email protected]
> > Subject: RE: [PATCH] vmbus: use put_device() if device_register fail
> >
> > > -----Original Message-----
> > > From: devel <[email protected]> On
> Behalf
> > > Of Arvind Yadav
> > > Sent: Saturday, March 17, 2018 11:48 PM
> > > To: Stephen Hemminger <[email protected]>; Haiyang Zhang
> > > <[email protected]>
> > > Cc: [email protected]; [email protected]
> > > Subject: [PATCH] vmbus: use put_device() if device_register fail
> > >
> > > if device_register() returned an error. Always use put_device()
> > > to give up the reference initialized.
> > >
> > > Signed-off-by: Arvind Yadav <[email protected]>
> > > ---
> > > drivers/hv/vmbus_drv.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > > index bc65c4d..25da2f3 100644
> > > --- a/drivers/hv/vmbus_drv.c
> > > +++ b/drivers/hv/vmbus_drv.c
> > > @@ -1358,6 +1358,7 @@ int vmbus_device_register(struct hv_device
> > > *child_device_obj)
> > > ret = device_register(&child_device_obj->device);
> > > if (ret) {
> > > pr_err("Unable to register child device\n");
> > > + put_device(&child_device_obj->device);
> >
> > If the registration failed; we would not have acquired the reference on the
> device and so
> > we should not be dropping the reference in the failure path.
>
> Looking at the code for device_register() and its constituent parts
> device_initialize() and device_add(), Arvind is correct. device_initialize()
> creates the object with a reference count of 1. device_add() does not
> decrement that reference count or free the object, even if it fails. The
> comments for device_register() call this out as well.

Yes; agreed. I will take this patch.

K. Y
>
> Michael
>
> >
> > K. Y
> > > return ret;
> > > }
> > >
> > > --
> > > 2.7.4