netdev - Re: [PATCH v3 01/20] 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: <20191210182211.GD46@ziepe.ca>
Date:   Tue, 10 Dec 2019 14:22:11 -0400
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Cc:     davem@...emloft.net, gregkh@...uxfoundation.org,
        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,
        Kiran Patil <kiran.patil@...el.com>
Subject: Re: [PATCH v3 01/20] virtual-bus: Implementation of Virtual Bus

On Mon, Dec 09, 2019 at 02:49:16PM -0800, Jeff Kirsher wrote:

> +#define to_virtbus_dev(x)	(container_of((x), struct virtbus_device, dev))
> +#define to_virtbus_drv(x)	(container_of((x), struct virtbus_driver, \
> +				 driver))

Please use static inlines for things like this, it makes the type
system clearer

> +/**
> + * virtbus_dev_register - add a virtual bus device
> + * @vdev: virtual bus device to add
> + */
> +int virtbus_dev_register(struct virtbus_device *vdev)
> +{
> +	int ret;
> +
> +	device_initialize(&vdev->dev);

I generally try to discourage the pattern where the device_initialize
is inside a function called register.

The kref inside the struct device should be the only kref for this
memory, and the kref system should be setup close to allocating the
memory. Any non-trivial user tends to require access to the kref
before calling register (which should be done last)

> +	/* All device IDs are automatically allocated */
> +	ret = ida_simple_get(&virtbus_dev_ida, 0, 0, GFP_KERNEL);
> +	if (ret < 0)
> +		return ret;

Should this be a cyclic allocation?

> +
> +	vdev->id = ret;
> +	dev_set_name(&vdev->dev, "%s.%d", vdev->name, vdev->id);

This is also stuff that can be useful to do early as the driver can
then use functions like dev_warn/etc

> +struct virtbus_object {
> +	struct virtbus_device vdev;
> +	char name[];
> +};

This whole virtbus_object makes no sense to me

> +
> +/**
> + * virtbus_dev_release - Destroy a virtbus device
> + * @vdev: virtual device to release
> + *
> + * Note that the vdev->data which is separately allocated needs to be
> + * separately freed on it own.

How will that happen?

> + */
> +static void virtbus_dev_release(struct device *dev)
> +{
> +	struct virtbus_object *vo = container_of(dev, struct virtbus_object,
> +						 vdev.dev);
> +
> +	ida_simple_remove(&virtbus_dev_ida, vo->vdev.id);
> +	kfree(vo);
> +}

Is something wrong with my search? I couldn't find a user for this?

If the virtbus framework wants to provide a release function then it
should also provide the alloc and require that the virtbus_device be
at offset 0 in the caller's struct so that the above kfree can work.
(ie like netdev does with the whole priv thing)

I have no idea what the virtbus_object is supposed to be doing here.

> +struct virtbus_device {
> +	const char *name;
> +	int id;

id is always positive, should be unsigned

Jason