On Thu, 1 Dec 2022 02:00:57 +0000
"Tian, Kevin" <kevin.tian@xxxxxxxxx> wrote:
> > From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Sent: Thursday, December 1, 2022 7:06 AM
> >
> > [Cc +vfio-ap, vfio-ccw]
> >
> > On Fri, 18 Nov 2022 11:28:27 +0800
> > ruanjinjie <ruanjinjie@xxxxxxxxxx> wrote:
> >
> > > Inject fault while probing module, if device_register() fails,
> > > but the refcount of kobject is not decreased to 0, the name
> > > allocated in dev_set_name() is leaked. Fix this by calling
> > > put_device(), so that name can be freed in callback function
> > > kobject_cleanup().
>
> It's not just about the name. The problem of kboject not being
> released is a bigger one.
>
> put_device() is always required no matter device_register()
> succeeds or not:
>
> * 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.
> */
> int device_register(struct device *dev)
>
> > > @@ -1430,8 +1430,10 @@ static int __init mbochs_dev_init(void)
> > > dev_set_name(&mbochs_dev, "%s", MBOCHS_NAME);
> > >
> > > ret = device_register(&mbochs_dev);
> > > - if (ret)
> > > + if (ret) {
> > > + put_device(&mbochs_dev);
> > > goto err_class;
> > > + }
> > >
> > > ret = mdev_register_parent(&mbochs_parent, &mbochs_dev,
> > &mbochs_driver,
> > > mbochs_mdev_types,
> >
> >
> > vfio-ap has a similar unwind as the sample drivers, but actually makes
> > an attempt to catch this ex:
> >
> > ...
> > ret = device_register(&matrix_dev->device);
> > if (ret)
> > goto matrix_reg_err;
> >
> > ret = driver_register(&matrix_driver);
> > if (ret)
> > goto matrix_drv_err;
> >
> > return 0;
> >
> > matrix_drv_err:
> > device_unregister(&matrix_dev->device);
> > matrix_reg_err:
> > put_device(&matrix_dev->device);
> > ...
> >
> > So of the vfio drivers calling device_register(), vfio-ap is the only
> > one that does a put_device() if device_register() fails, but it also
> > seems sketchy to call both device_unregister() and put_device() in the
> > case that we exit via matrix_drv_err.
> >
> > I wonder if all of these shouldn't adopt a flow like:
> >
> > ret = device_register(&dev);
> > if (ret)
> > goto err1;
> >
> > ....
> >
> > return 0;
> >
> > err2:
> > device_del(&dev);
> > err1:
> > put_device(&dev);
> >
>
> It's kind of a mixed model.
>
> With above unwind it's clearer to use device_initialize() and device_add() instead.
That would go against the comment for device_register() recommending
that device_initialize() and device_add() should only be called
separately if we have a clearly defined need. I can only imagine a
patch bot would quickly come along to rectify the situation if we
simply open code device_register() for aesthetics. I don't see that
splitting device_unregister() for the purpose of having a common unwind
path necessitates any changes relative to device_register().
> Otherwise what this patch does looks better IMHO:
>
> ret = device_register(&dev);
> if (ret) {
> put_device(&dev);
> goto err1;
> }
>
> ...
>
> return 0;
>
> err2:
> device_unregister(&dev);
> err1:
> earlier_unwind();
>
This is essentially what was originally proposed. It could also be
called a "mixed model", implementing part of the unwind in the error
branch before jumping to the common unwind. As demonstrated below,
every current vfio driver calling device_register() follows a similar
goto unwind stack as found in the sample drivers, which makes it
trivially easy to split the device_unregister() call and add a goto
target in between.
Either way, they're equivalent and I'll take whichever version
addresses all the vfio related use cases and gets acks from their
maintainers. Thanks,
Alex
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index c2a65808605a..54aba7cceb33 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -199,8 +199,9 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
return 0;
out_unreg:
- device_unregister(&parent->dev);
+ device_del(&parent->dev);
out_free:
+ put_device(&parent->dev);
dev_set_drvdata(&sch->dev, NULL);
return ret;
}
diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index f43cfeabd2cc..997b524bdd2b 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -122,7 +122,7 @@ static int vfio_ap_matrix_dev_create(void)
return 0;
matrix_drv_err:
- device_unregister(&matrix_dev->device);
+ device_del(&matrix_dev->device);
matrix_reg_err:
put_device(&matrix_dev->device);
matrix_alloc_err:
diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
index 8b5a3a778a25..e54eb752e1ba 100644
--- a/samples/vfio-mdev/mbochs.c
+++ b/samples/vfio-mdev/mbochs.c
@@ -1430,7 +1430,7 @@ static int __init mbochs_dev_init(void)
ret = device_register(&mbochs_dev);
if (ret)
- goto err_class;
+ goto err_put;
ret = mdev_register_parent(&mbochs_parent, &mbochs_dev, &mbochs_driver,
mbochs_mdev_types,
@@ -1441,8 +1441,9 @@ static int __init mbochs_dev_init(void)
return 0;
err_device:
- device_unregister(&mbochs_dev);
-err_class:
+ device_del(&mbochs_dev);
+err_put:
+ put_device(&mbochs_dev);
class_destroy(mbochs_class);
err_driver:
mdev_unregister_driver(&mbochs_driver);
diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
index 721fb06c6413..e8400fdab71d 100644
--- a/samples/vfio-mdev/mdpy.c
+++ b/samples/vfio-mdev/mdpy.c
@@ -717,7 +717,7 @@ static int __init mdpy_dev_init(void)
ret = device_register(&mdpy_dev);
if (ret)
- goto err_class;
+ goto err_put;
ret = mdev_register_parent(&mdpy_parent, &mdpy_dev, &mdpy_driver,
mdpy_mdev_types,
@@ -728,8 +728,9 @@ static int __init mdpy_dev_init(void)
return 0;
err_device:
- device_unregister(&mdpy_dev);
-err_class:
+ device_del(&mdpy_dev);
+err_put:
+ put_device(&mdpy_dev);
class_destroy(mdpy_class);
err_driver:
mdev_unregister_driver(&mdpy_driver);
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index 3c2a421b9b69..e887de672c52 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -1330,7 +1330,7 @@ static int __init mtty_dev_init(void)
ret = device_register(&mtty_dev.dev);
if (ret)
- goto err_class;
+ goto err_put;
ret = mdev_register_parent(&mtty_dev.parent, &mtty_dev.dev,
&mtty_driver, mtty_mdev_types,
@@ -1340,8 +1340,9 @@ static int __init mtty_dev_init(void)
return 0;
err_device:
- device_unregister(&mtty_dev.dev);
-err_class:
+ device_del(&mtty_dev.dev);
+err_put:
+ put_device(&mtty_dev.dev);
class_destroy(mtty_dev.vd_class);
err_driver:
mdev_unregister_driver(&mtty_driver);
