linux-kernel - RE: [PATCH V4 1/1] Drivers: hv: Implement the file copy service

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: <bfeacfefc0f04367ae447840cfe3abc5@BY2PR03MB299.namprd03.prod.outlook.com>
Date:	Sun, 9 Feb 2014 01:56:02 +0000
From:	KY Srinivasan <kys@...rosoft.com>
To:	Greg KH <gregkh@...uxfoundation.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
	"olaf@...fle.de" <olaf@...fle.de>,
	"apw@...onical.com" <apw@...onical.com>,
	"jasowang@...hat.com" <jasowang@...hat.com>
Subject: RE: [PATCH V4 1/1] Drivers: hv: Implement the file copy service



> -----Original Message-----
> From: Greg KH [mailto:gregkh@...uxfoundation.org]
> Sent: Friday, February 07, 2014 3:17 PM
> To: KY Srinivasan
> Cc: linux-kernel@...r.kernel.org; devel@...uxdriverproject.org; olaf@...fle.de;
> apw@...onical.com; jasowang@...hat.com
> Subject: Re: [PATCH V4 1/1] Drivers: hv: Implement the file copy service
> 
> On Tue, Jan 21, 2014 at 05:43:53PM -0800, K. Y. Srinivasan wrote:
> > +/*
> > + * Create a char device that can support read/write for passing
> > + * the payload.
> > + */
> > +static struct cdev fcopy_cdev;
> > +static struct class *cl;
> > +static struct device *sysfs_dev;
> 
> Why not just be a misc device, you only want 1 minor number for a char
> device:
> 
> > +static int fcopy_dev_init(void)
> > +{
> > +	int result;
> > +
> > +	result = alloc_chrdev_region(&fcopy_dev, 1, 1, "hv_fcopy");
> 
> See, one minor.
> 
> > +	if (result < 0) {
> > +		pr_err("Cannot get major number\n");
> > +		return result;
> > +	}
> > +
> > +	cl = class_create(THIS_MODULE, "chardev");
> 
> That's a _really_ generic name, come on, you know better than that.
> 
> > +	if (IS_ERR(cl)) {
> > +		pr_err("Error creating fcopy class.\n");
> 
> Your error string is wrong :(
> 
> > +		result = PTR_ERR(cl);
> > +		goto err_unregister;
> > +	}
> > +
> > +	sysfs_dev = device_create(cl, NULL, fcopy_dev, "%s", "hv_fcopy");
> 
> A device at the root of sysfs?  No, you have a bus to hang devices off
> of, use that.  What do you need this device for anyway?
> 
> > +	if (IS_ERR(sysfs_dev)) {
> > +		pr_err("Device creation failed\n");
> > +		result = PTR_ERR(cl);
> > +		goto err_destroy_class;
> > +	}
> > +
> > +	cdev_init(&fcopy_cdev, &fcopy_fops);
> > +	fcopy_cdev.owner = THIS_MODULE;
> > +	fcopy_cdev.ops = &fcopy_fops;
> > +
> > +	result = cdev_add(&fcopy_cdev, fcopy_dev, 1);
> 
> Ah, to get udev to pay attention to the char device, no, just use a misc
> device, should make this whole code a lot simpler and more "obvious" as
> to what you want/need.
> 
> > +	if (result) {
> > +		pr_err("Cannot cdev_add\n");
> > +		goto err_destroy_device;
> > +	}
> > +	return result;
> > +
> > +err_destroy_device:
> > +	device_destroy(cl, fcopy_dev);
> > +err_destroy_class:
> > +	class_destroy(cl);
> > +err_unregister:
> > +	unregister_chrdev_region(fcopy_dev, 1);
> > +	return result;
> 
> 
> Ugh, I hate the cdev interface, one of these days I'll fix it up, it's
> so unwieldy...
> 
> > +static void fcopy_dev_deinit(void)
> > +{
> > +	/*
> > +	 * first kill the daemon.
> > +	 */
> > +	if (dtp != NULL)
> > +		send_sig(SIGKILL, dtp, 0);
> 
> We kill userspace daemon's from the kernel?  That's a recipe for
> disaster...
> 
> Why?  What does it matter here if the daemon keeps running, it should
> fail gracefully if the character device is removed, right?  If not, that
> needs to be fixed anyway.

Greg,

Thanks for the detailed comments; I will address these in the next version.


Regards,

K. Y
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/