linux-kernel - Re: [PATCH 1/5] uio: Add hv_vmbus

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: <2023060421-unclothed-hungry-cb3e@gregkh>
Date:   Sun, 4 Jun 2023 09:09:57 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Saurabh Sengar <ssengar@...ux.microsoft.com>
Cc:     kys@...rosoft.com, haiyangz@...rosoft.com, wei.liu@...nel.org,
        decui@...rosoft.com, mikelley@...rosoft.com,
        linux-kernel@...r.kernel.org, linux-hyperv@...r.kernel.org
Subject: Re: [PATCH 1/5] uio: Add hv_vmbus_client driver

On Fri, Jun 02, 2023 at 12:57:05AM -0700, Saurabh Sengar wrote:
> + * Since the driver does not declare any device ids, you must allocate
> + * id and bind the device to the driver yourself.  For example:
> + * driverctl -b vmbus set-override <dev uuid> uio_hv_vmbus_client

Shouldn't this be documented somewhere?

> + */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/uio_driver.h>
> +#include <linux/hyperv.h>
> +#include <linux/vmalloc.h>
> +#include <linux/sysfs.h>
> +
> +#define DRIVER_VERSION	"0.0.1"

Why this number?  Why not "1"?

The whole "driver version" thing doesn't really make sense here, we
should just drop it from the uio later, right?

> +#define DRIVER_AUTHOR	"Saurabh Sengar <ssengar@...rosoft.com>"
> +#define DRIVER_DESC	"Generic UIO driver for low speed VMBus devices"
> +
> +#define DEFAULT_HV_RING_SIZE	VMBUS_RING_SIZE(3 * HV_HYP_PAGE_SIZE)
> +static int ring_size = DEFAULT_HV_RING_SIZE;
> +
> +struct uio_hv_vmbus_dev {
> +	struct uio_info info;
> +	struct hv_device *device;
> +	atomic_t refcnt;

Why is this refcnt needed?

Have you seen the other threads about how attempting to block multiple
opens in UIO drivers really does not work at all?  Please drop all of
that logic, it is not correct.


> +static ssize_t ring_size_show(struct device *dev, struct device_attribute *attr,
> +			      char *buf)
> +{
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", ring_size);

Did you use checkpatch?

It should have said to use sysfs_emit(), right?

> +	ret = sysfs_create_file(&dev->device.kobj, &dev_attr_ring_size.attr);

If you ever have to use a sysfs_* call in a driver, that's a huge hint
something is wrong.  Please fix this to not race with userspace and
loose.

> +	if (ret)
> +		dev_notice(&dev->device, "sysfs create ring size file failed; %d\n", ret);

Why not just use dev_err() for this and other errors?  Why "notice"?

> +MODULE_VERSION(DRIVER_VERSION);

This means nothing, please drop.

thanks,

greg k-h