linux-kernel - RE: [EXTERNAL] 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] [day] [month] [year] [list]

Message-ID: <PUZP153MB0749F19E7A3DC1C2DCBF16ABBE4DA@PUZP153MB0749.APCP153.PROD.OUTLOOK.COM>
Date:   Mon, 5 Jun 2023 03:00:15 +0000
From:   Saurabh Singh Sengar <ssengar@...rosoft.com>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Saurabh Sengar <ssengar@...ux.microsoft.com>
CC:     KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        Dexuan Cui <decui@...rosoft.com>,
        "Michael Kelley (LINUX)" <mikelley@...rosoft.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>
Subject: RE: [EXTERNAL] Re: [PATCH 1/5] uio: Add hv_vmbus_client driver



> -----Original Message-----
> From: Greg KH <gregkh@...uxfoundation.org>
> Sent: Sunday, June 4, 2023 12:40 PM
> To: Saurabh Sengar <ssengar@...ux.microsoft.com>
> Cc: KY Srinivasan <kys@...rosoft.com>; Haiyang Zhang
> <haiyangz@...rosoft.com>; wei.liu@...nel.org; Dexuan Cui
> <decui@...rosoft.com>; Michael Kelley (LINUX) <mikelley@...rosoft.com>;
> linux-kernel@...r.kernel.org; linux-hyperv@...r.kernel.org
> Subject: [EXTERNAL] 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?

I will update it in Documentation/driver-api/uio-howto.rst.

> 	
> > + */
> > +
> > +#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?

will remove in V2.

> 
> > +#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.
> 

Will remove.

> 
> > +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?

Yes, I am using  "--strict" switch for checkpatch.pl, still didn't get this warning.
However, I will use sysfs_emit() in V2.

> 
> > +	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.

Thanks for pointing this out, will look into this.

> 
> > +	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"?

Will fix.

> 
> > +MODULE_VERSION(DRIVER_VERSION);
> 
> This means nothing, please drop.

OK.

Thanks for review.
- Saurabh

> 
> thanks,
> 
> greg k-h