netdev - Re: [RFC PATCH v4 01/25] virtual-bus: Implementation of Virtual Bus

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200214203455.GX31668@ziepe.ca>
Date:   Fri, 14 Feb 2020 16:34:55 -0400
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>, davem@...emloft.net,
        Dave Ertman <david.m.ertman@...el.com>, 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, Kiran Patil <kiran.patil@...el.com>,
        Andrew Bowers <andrewx.bowers@...el.com>
Subject: Re: [RFC PATCH v4 01/25] virtual-bus: Implementation of Virtual Bus

On Fri, Feb 14, 2020 at 09:02:40AM -0800, Greg KH wrote:
> > +/**
> > + * virtbus_dev_register - add a virtual bus device
> > + * @vdev: virtual bus device to add
> > + */
> > +int virtbus_dev_register(struct virtbus_device *vdev)
> > +{
> > +	int ret;
> > +
> > +	if (!vdev->release) {
> > +		dev_err(&vdev->dev, "virtbus_device .release callback NULL\n");
> 
> "virtbus_device MUST have a .release callback that does something!\n" 
> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	device_initialize(&vdev->dev);
> > +
> > +	vdev->dev.bus = &virtual_bus_type;
> > +	vdev->dev.release = virtbus_dev_release;
> > +	/* 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");
> > +		put_device(&vdev->dev);
> 
> If you allocate the number before device_initialize(), no need to call
> put_device().  Just a minor thing, no big deal.

If *_regster does put_device on error then it must always do
put_device on any error, for instance the above return -EINVAL with
no put_device leaks memory.

Generally I find the design and audit of drivers simpler if the
register doesn't do device_initialize or put_device - have them
distinct and require the caller to manage this.

For instance look at ice_init_peer_devices() and ask who frees
the alloc_ordered_workqueue() if virtbus_dev_register() fails..

It is not all easy to tell if this is right or not..

> > +	put_device(&vdev->dev);
> > +	ida_simple_remove(&virtbus_dev_ida, vdev->id);
> 
> You need to do this before put_device().

Shouldn't it be in the release function? The ida index should not be
re-used until the kref goes to zero..

> > +struct virtbus_device {
> > +	struct device dev;
> > +	const char *name;
> > +	void (*release)(struct virtbus_device *);
> > +	int id;
> > +	const struct virtbus_dev_id *matched_element;
> > +};
> 
> Any reason you need to make "struct virtbus_device" a public structure
> at all? 

The general point of this scheme is to do this in a public header:

+struct iidc_virtbus_object {
+	struct virtbus_device vdev;
+	struct iidc_peer_dev *peer_dev;
+};

And then this when the driver binds:

+int irdma_probe(struct virtbus_device *vdev)
+{
+       struct iidc_virtbus_object *vo =
+                       container_of(vdev, struct iidc_virtbus_object, vdev);
+       struct iidc_peer_dev *ldev = vo->peer_dev;

So the virtbus_device is in a public header to enable the container_of
construction.

Jason