| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5774398d6f69f0708e1fc040be4b7dd398a884c8.camel@linux.ibm.com>
Date: Fri, 02 Dec 2022 10:08:30 -0500
From: Eric Farman <farman@...ux.ibm.com>
To: "Tian, Kevin" <kevin.tian@...el.com>,
Alex Williamson <alex.williamson@...hat.com>
Cc: ruanjinjie <ruanjinjie@...wei.com>,
"kwankhede@...dia.com" <kwankhede@...dia.com>,
"kraxel@...hat.com" <kraxel@...hat.com>,
"cjia@...dia.com" <cjia@...dia.com>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"akrowiak@...ux.ibm.com" <akrowiak@...ux.ibm.com>,
"pasic@...ux.ibm.com" <pasic@...ux.ibm.com>,
"jjherne@...ux.ibm.com" <jjherne@...ux.ibm.com>,
"mjrosato@...ux.ibm.com" <mjrosato@...ux.ibm.com>
Subject: Re: [PATCH] vfio/mdev: fix possible memory leak in module init funcs
On Fri, 2022-12-02 at 05:28 +0000, Tian, Kevin wrote:
> > From: Alex Williamson <alex.williamson@...hat.com>
> > 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@...el.com>
>
> >
> > 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;
> > }
Ugh, sorry. I had this partially right at one point but missed this
scenario when fixing the parts I had wrong.
This is fine by me, thanks for the fixup:
Reviewed-by: Eric Farman <farman@...ux.ibm.com>
> > 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);
>
Powered by blists - more mailing lists