linux-kernel - Re: [PATCH v2] nbd: nbd sysfs framework

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: <20110804231838.GA2978@kroah.com>
Date:	Thu, 4 Aug 2011 16:18:38 -0700
From:	Greg KH <greg@...ah.com>
To:	paul.clements@...eleye.com
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] nbd: nbd sysfs framework

[admin note, bounced to lkml?  wierd...]

On Thu, Aug 04, 2011 at 06:11:27PM -0400, paul.clements@...eleye.com wrote:
> Description: This patch adds a small framework that will simplify adding
> sysfs entries for nbd (coming later). All the locking and structure walking
> are in the main nbd_attr_ handlers and the individual entries' show and store
> handlers will be much simpler. The patch moves the one existing sysfs entry
> (pid) to use the framework. ABI docs included.

Again, I fail to understand _why_ this is needed at all.  You can
trivially add new sysfs files in an attribute group to nbd without
adding this "infrastructure", making the overall patch set much smaller,
right?

Actually, there's a problem in the original code that you should fix
that will show this in an easier way:

>  static int nbd_do_it(struct nbd_device *lo)
>  {
>  	struct request *req;
> -	int ret;
>  
>  	BUG_ON(lo->magic != LO_MAGIC);
>  
>  	lo->pid = current->pid;
> -	ret = sysfs_create_file(&disk_to_dev(lo->disk)->kobj, &pid_attr.attr);

That should be using device_create_file() and not sysfs_create_file, as
that is what is really happening here.

Then just do the same "cast" in each of your new sysfs files that you
want to create.

Actually, you should be setting the groups pointer of the device here to
properly create the sysfs files at the correct time, instead of having
it as-is which races with userspace.  That would make the code even
smaller than it currently is today as well, and make it even easier to
add new sysfs files like you really want to do.

So, in short, no, this patch is still not acceptable.  And in the
future, properly set your Cc: lines.

greg k-h
--
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/