| 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: <DM6PR11MB28414BEF48AB56336F3456DEDDD50@DM6PR11MB2841.namprd11.prod.outlook.com>
Date: Tue, 21 Apr 2020 23:27:03 +0000
From: "Ertman, David M" <david.m.ertman@...el.com>
To: Jason Gunthorpe <jgg@...pe.ca>
CC: "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
"nhorman@...hat.com" <nhorman@...hat.com>,
"sassmann@...hat.com" <sassmann@...hat.com>,
"parav@...lanox.com" <parav@...lanox.com>,
"galpress@...zon.com" <galpress@...zon.com>,
"selvin.xavier@...adcom.com" <selvin.xavier@...adcom.com>,
"sriharsha.basavapatna@...adcom.com"
<sriharsha.basavapatna@...adcom.com>,
"benve@...co.com" <benve@...co.com>,
"bharat@...lsio.com" <bharat@...lsio.com>,
"xavier.huwei@...wei.com" <xavier.huwei@...wei.com>,
"yishaih@...lanox.com" <yishaih@...lanox.com>,
"leonro@...lanox.com" <leonro@...lanox.com>,
"mkalderon@...vell.com" <mkalderon@...vell.com>,
"aditr@...are.com" <aditr@...are.com>,
"ranjani.sridharan@...ux.intel.com"
<ranjani.sridharan@...ux.intel.com>,
"pierre-louis.bossart@...ux.intel.com"
<pierre-louis.bossart@...ux.intel.com>,
"Patil, Kiran" <kiran.patil@...el.com>,
"Bowers, AndrewX" <andrewx.bowers@...el.com>
Subject: RE: [net-next 1/9] Implementation of Virtual Bus
> -----Original Message-----
> From: Jason Gunthorpe <jgg@...pe.ca>
> Sent: Monday, April 20, 2020 5:45 PM
> To: Ertman, David M <david.m.ertman@...el.com>
> Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@...el.com>; davem@...emloft.net;
> gregkh@...uxfoundation.org; netdev@...r.kernel.org; linux-
> rdma@...r.kernel.org; nhorman@...hat.com; sassmann@...hat.com;
> parav@...lanox.com; galpress@...zon.com;
> selvin.xavier@...adcom.com; sriharsha.basavapatna@...adcom.com;
> benve@...co.com; bharat@...lsio.com; xavier.huwei@...wei.com;
> yishaih@...lanox.com; leonro@...lanox.com; mkalderon@...vell.com;
> aditr@...are.com; ranjani.sridharan@...ux.intel.com; pierre-
> louis.bossart@...ux.intel.com; Patil, Kiran <kiran.patil@...el.com>; Bowers,
> AndrewX <andrewx.bowers@...el.com>
> Subject: Re: [net-next 1/9] Implementation of Virtual Bus
>
> On Mon, Apr 20, 2020 at 11:16:38PM +0000, Ertman, David M wrote:
> > > > +/**
> > > > + * virtbus_register_device - add a virtual bus device
> > > > + * @vdev: virtual bus device to add
> > > > + */
> > > > +int virtbus_register_device(struct virtbus_device *vdev)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + /* Do this first so that all error paths perform a put_device */
> > > > + device_initialize(&vdev->dev);
> > > > +
> > > > + if (!vdev->release) {
> > > > + ret = -EINVAL;
> > > > + dev_err(&vdev->dev, "virtbus_device MUST have a .release
> > > callback that does something.\n");
> > > > + goto device_pre_err;
> > >
> > > This does put_device but the release() hasn't been set yet. Doesn't it
> > > leak memory?
> >
> > The KO registering the virtbus_device is responsible for allocating
> > and freeing the memory for the virtbus_device (which should be done
> > in the release() function). If there is no release function
> > defined, then the originating KO needs to handle this. We are
> > trying to not recreate the platform_bus, so the design philosophy
> > behind virtual_bus is minimalist.
>
> Oh, a precondition assertion should just be written as something like:
>
> if (WARN_ON(!vdev->release))
> return -EINVAL;
>
> And done before device_initialize
>
> But I wouldn't bother, things will just reliably crash on null pointer
> exceptions if a driver mis-uses the API.
>
Done.
> > > > + }
> > > > +
> > > > + /* All device IDs are automatically allocated */
> > > > + ret = ida_simple_get(&virtbus_dev_ida, 0, 0, GFP_KERNEL);
> > > > + if (ret < 0) {
> > > > + dev_err(&vdev->dev, "get IDA idx for virtbus device
> failed!\n");
> > > > + goto device_pre_err;
> > >
> > > Do this before device_initialize()
> >
> > The memory for virtbus device is allocated by the KO registering the
> > virtbus_device before it calls virtbus_register_device(). If this
> > function is exiting on an error, then we have to do a put_device()
> > so that the release is called (if it exists) to clean up the memory.
>
> put_device() must call virtbus_release_device(), which does
> ida_simple_remove() on vdev->id which hasn't been set yet.
>
> Also ->release wasn't initialized at this point so its leaks memory..
->release assignment moved to before ida_simple_get evaluation,
and added a define for VIRTBUS_INVALID_ID and a check in release
to not do ida_simple_remove for an invalid ID.
>
> > The ida_simple_get is not used until later in the function when
> > setting the vdev->id. It doesn't matter what IDA it is used, as
> > long as it is unique. So, since we cannot have the error state
> > before the device_initialize, there is no reason to have the
> > ida_sinple_get before the device_initialization.
>
> I was a bit wrong on this advice because no matter what you have to do
> put_device(), so you need to add some kind of flag that the vdev->id
> is not valid.
>
Did just that 😊
> It is ugly. It is nicer to arrange thing so initialization is done
> after kalloc but before device_initialize. For instance look how
> vdpa_alloc_device() and vdpa_register() work, very clean, very simple
> goto error unwinds everywhere.
>
> > GregKH was pretty insistent that all error paths out of this
> > function go through a put_device() when possible.
>
> After device_initialize() is called all error paths must go through
> put_device.
>
> > > Can't understand why vdev->name is being passed in with the struct,
> > > why not just a function argument?
> >
> > This avoids having the calling KO have to manage a separate piece of
> memory
> > to hold the name during the call to virtbus_device_regsiter. It is a cleaner
> > memory model to just store it once in the virtbus_device itself. This name
> is
> > the abbreviated name without the ID appended on the end, which will be
> used
> > for matching drivers and devices.
>
> Your other explanation was better. This would be less confusing if it
> was called match_name/device_label/driver_key or something, as it is
> not the 'name'.
>
changing the vdev->name to vdev->match_name
> > > > + * virtbus_unregister_device - remove a virtual bus device
> > > > + * @vdev: virtual bus device we are removing
> > > > + */
> > > > +void virtbus_unregister_device(struct virtbus_device *vdev)
> > > > +{
> > > > + device_del(&vdev->dev);
> > > > + put_device(&vdev->dev);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(virtbus_unregister_device);
> > >
> > > Just inline this as wrapper around device_unregister
> >
> > I thought that EXPORT_SYMBOL makes inline meaningless?
> > But, putting device_unregister here is a good catch.
>
> I mean move it to the header file and inline it
Done.
>
> Jason
-DaveE
Powered by blists - more mailing lists