RE: [PATCH] vfio/mdev: fix possible memory leak in module init funcs

[Tian, Kevin]

> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Sent: Friday, December 2, 2022 5:10 AM
> 
> > 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.

OK. Actually looking into other users of device_register() I can
see both schemes are used, so...

> 
> 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,

... agree either way is fine. Since your version fixes all vfio cases,
feel free to include:

Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>

> 
> 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);