In article <[email protected]>, Greg KH <[email protected]> wrote:
>On Fri, Sep 20, 2002 at 07:55:22PM -0700, David Brownell wrote:
>>
>> How about a facility to create the character (or block?) special file
>> node right there in the driverfs directory? Optional of course.
>
>No, Linus has stated that this is not ok to do. See the lkml archives
>for the whole discussion about this.
I'm not totally against it, it's just that it has some issues:
- naming policy in general. Trivially handled by just always calling
the special node something truly boring and nautral like "node", and
be done with it. The _path_ is the real name, the "node" would be
just an openable entity.
- the issue of persistent permissions and ownership.
The latter is the real problem. And I personally think the only sane
policy is to just let "/sbin/hotplug" handle it, which definitely
implies _not_ having the kernel create the real device node. That way
user-space can have any policy it damn well pleases, including having
some default heuristics along with "a priori known nodes".
But clearly that user-space hotplug entity needs to know major and minor
numbers in order to create the real device node, and that's where the
"node" thing may be acceptable - as a template, nothing more. Although
I suspect that there are other, simpler and more acceptable templates
(ie export the dang thing as just a "node" text-file, which describes
the majors and minors and "char vs block" issues)
Linus
On Sun, 22 Sep 2002, Linus Torvalds wrote:
> In article <[email protected]>, Greg KH <[email protected]> wrote:
> >On Fri, Sep 20, 2002 at 07:55:22PM -0700, David Brownell wrote:
> >>
> >> How about a facility to create the character (or block?) special file
> >> node right there in the driverfs directory? Optional of course.
> >
> >No, Linus has stated that this is not ok to do. See the lkml archives
> >for the whole discussion about this.
>
> I'm not totally against it, it's just that it has some issues:
>
> - naming policy in general. Trivially handled by just always calling
> the special node something truly boring and nautral like "node", and
> be done with it. The _path_ is the real name, the "node" would be
> just an openable entity.
>
> - the issue of persistent permissions and ownership.
>
> The latter is the real problem. And I personally think the only sane
> policy is to just let "/sbin/hotplug" handle it, which definitely
> implies _not_ having the kernel create the real device node. That way
> user-space can have any policy it damn well pleases, including having
> some default heuristics along with "a priori known nodes".
>
> But clearly that user-space hotplug entity needs to know major and minor
> numbers in order to create the real device node, and that's where the
> "node" thing may be acceptable - as a template, nothing more. Although
> I suspect that there are other, simpler and more acceptable templates
> (ie export the dang thing as just a "node" text-file, which describes
> the majors and minors and "char vs block" issues)
The concern that I had was that people would actually start using the
stupid thing, and start to rely on the path and existence of them. But, I
suppose if people really _want_ to do that, we shouldn't stop them from
hurting themselves. ;)
Userspace does need to know major/minor, as well as block vs. char. We
could encode those in one file each, but a device node gives us all three
in a standard format.
So, appended is a patch that allows you to create a device node in
driverfs. The API is:
int device_create_node(struct device *device, mode_t mode, kdev_t kdev);
The name is "node", and it's created in the device's directory. There are
plans to call /sbin/hotplug after the device is registered with its class.
It's then that the kernel will know what type of device it is, and what
the major/minor should be. The device path will be passed to
/sbin/hotplug, which can then query the node as a template for creating
other device nodes.
Note that ->fops is not set, though it appears that a request_module()
happpens when you try to write to it.
Also, the owner of the device is root, and I easily make the permissions
more stringent (i.e. make it S_IRUGO only always).
Attached is a simple little module to illustrate the use of the API. It
creates a device 'foo0' and a node inside its directory once registered:
# tree /sys/root/foo0/
/sys/root/foo0/
|-- name
|-- node
`-- power
# ls -l /sys/root/foo0/node
cr--r--r-- 1 root root 10, 16 Sep 23 10:58
/sys/root/foo0/node
Will submit as a BK patch pending feedback..
-pat
===== drivers/base/fs/device.c 1.20 vs edited =====
--- 1.20/drivers/base/fs/device.c Mon Aug 26 08:39:22 2002
+++ edited/drivers/base/fs/device.c Tue Sep 24 10:47:48 2002
@@ -80,6 +80,24 @@
};
/**
+ * device_create_node - create a device node for a device
+ * @dev: device we're creating the node for
+ * @mode: permissions of the device
+ * @kdev: major/minor of the device
+ *
+ */
+int device_create_node(struct device * dev, mode_t mode, kdev_t kdev)
+{
+ struct attribute attr = {
+ .name = "node",
+ .mode = mode,
+ };
+ printk("creating node (kdev = %d, mode = %d, dev = %s)\n",
+ kdev_t_to_nr(kdev),mode,dev->bus_id);
+ return driverfs_create_node(&attr,&dev->dir,kdev);
+}
+
+/**
* device_create_file - create a driverfs file for a device
* @dev: device requesting file
* @entry: entry describing file
@@ -243,3 +261,4 @@
EXPORT_SYMBOL(device_create_file);
EXPORT_SYMBOL(device_remove_file);
+EXPORT_SYMBOL(device_create_node);
===== fs/driverfs/inode.c 1.50 vs edited =====
--- 1.50/fs/driverfs/inode.c Wed Sep 11 13:47:46 2002
+++ edited/fs/driverfs/inode.c Tue Sep 24 10:51:26 2002
@@ -145,8 +145,6 @@
if (dentry->d_inode)
return -EEXIST;
- /* only allow create if ->d_fsdata is not NULL (so we can assume it
- * comes from the driverfs API below. */
inode = driverfs_get_inode(dir->i_sb, mode, dev);
if (inode) {
d_instantiate(dentry, inode);
@@ -598,6 +596,30 @@
up(&parent->dentry->d_inode->i_sem);
return error;
}
+
+int driverfs_create_node(struct attribute * attr,
+ struct driver_dir_entry * parent, kdev_t kdev)
+{
+ struct dentry * dentry;
+ int error = 0;
+
+ if (!attr || !parent)
+ return -EINVAL;
+
+ if (!parent->dentry)
+ return -EINVAL;
+
+ down(&parent->dentry->d_inode->i_sem);
+ dentry = get_dentry(parent->dentry,attr->name);
+ if (!IS_ERR(dentry)) {
+ error = driverfs_mknod(parent->dentry->d_inode,dentry,
+ attr->mode,kdev_t_to_nr(kdev));
+ } else
+ error = PTR_ERR(dentry);
+ up(&parent->dentry->d_inode->i_sem);
+ return error;
+}
+
/**
* driverfs_create_symlink - make a symlink
===== include/linux/device.h 1.38 vs edited =====
--- 1.38/include/linux/device.h Mon Sep 23 18:23:03 2002
+++ edited/include/linux/device.h Tue Sep 24 11:01:26 2002
@@ -346,6 +346,8 @@
extern int device_create_file(struct device *device, struct device_attribute * entry);
extern void device_remove_file(struct device * dev, struct device_attribute * attr);
+extern int device_create_node(struct device *device, mode_t mode, kdev_t kdev);
+
/*
* Platform "fixup" functions - allow the platform to have their say
* about devices and actions that the general device layer doesn't
===== include/linux/driverfs_fs.h 1.13 vs edited =====
--- 1.13/include/linux/driverfs_fs.h Thu Aug 1 12:07:33 2002
+++ edited/include/linux/driverfs_fs.h Tue Sep 24 10:54:16 2002
@@ -26,6 +26,8 @@
#ifndef _DRIVER_FS_H_
#define _DRIVER_FS_H_
+#include <linux/kdev_t.h>
+
struct driver_dir_entry;
struct attribute;
@@ -57,6 +59,10 @@
extern int
driverfs_create_file(struct attribute * attr,
struct driver_dir_entry * parent);
+
+
+extern int driverfs_create_node(struct attribute * attr,
+ struct driver_dir_entry * parent, kdev_t kdev);
extern int
driverfs_create_symlink(struct driver_dir_entry * parent,
On Tue, 24 Sep 2002, Patrick Mochel wrote:
>
> Also, the owner of the device is root, and I easily make the permissions
> more stringent (i.e. make it S_IRUGO only always).
Well, that would _have_ to be the case, right now you give read access to
every single device exported this way. Not acceptable.
I really suspect that it would be better to not export the device itself,
but export just device data. In particular, that avoids the security
issues altogether, and it's most likely what a hotplug agent really wants
anyway (and the pure node is useless without a hotplug agent, as the
default kernel permissions would have to be so anal as to make it not
interesting).
So I'd suggest you just export a text-file that describes the thing.
Something like
- legacy name (the kernel knows about these anyway, see /proc/mounts and
friends)
- major number, minor number) and char vs block
that way a simple script can just basically do the equivalent of "MAKEDEV"
at hotplug time using the legacy name as a key to whatever permission and
ownership heuristics it has (the way MAKEDEV already does)
Linus
On Tue, Sep 24, 2002 at 11:55:00AM -0700, Linus Torvalds wrote:
>
> So I'd suggest you just export a text-file that describes the thing.
> Something like
>
> - legacy name (the kernel knows about these anyway, see /proc/mounts and
> friends)
I would like to see all of the /proc/mounts and friends info that
"knows" about the legacy name, to disappear if possible. Yes, I know
the root filesystem logic will have to stay, but I don't want to see
this be used like the devfs name is used throughout the kernel. That
info should not be in the kernel, it's up to the user what to name their
"USB mouse, connected to the EHCI host controller's 3 hub port".
> - major number, minor number) and char vs block
Yes, this info is needed, and if presented in a file, that would be
fine. It was just that the device node was a nice compact version of
this :)
But I can see how the device node would be abused, and it's fine with me
if it isn't present in driverfs.
thanks,
greg k-h
On Tue, 24 Sep 2002, Greg KH wrote:
> On Tue, Sep 24, 2002 at 11:55:00AM -0700, Linus Torvalds wrote:
> >
> > So I'd suggest you just export a text-file that describes the thing.
> > Something like
> >
> > - legacy name (the kernel knows about these anyway, see /proc/mounts and
> > friends)
>
> I would like to see all of the /proc/mounts and friends info that
> "knows" about the legacy name, to disappear if possible. Yes, I know
> the root filesystem logic will have to stay, but I don't want to see
> this be used like the devfs name is used throughout the kernel. That
> info should not be in the kernel, it's up to the user what to name their
> "USB mouse, connected to the EHCI host controller's 3 hub port".
I agree. We can create a file in userspace that the hotplug agent can
query for what to name the device. It's gonna have to do this anyway, for
user-defined policy. Putting the legacy name in that puts all names in
one place.
> > - major number, minor number) and char vs block
>
> Yes, this info is needed, and if presented in a file, that would be
> fine. It was just that the device node was a nice compact version of
> this :)
Exactly. If we go with a text file, and we enforce our own 1 value/file
semantics, we end up with a file for each:
- major
- minor
- type (char vs. block)
- legacy name
If we punt the legacy name to userspace, a device node becomes ideal for
expressing the data.
> But I can see how the device node would be abused, and it's fine with me
> if it isn't present in driverfs.
I didn't even want it in the first place. ;) But, it's a lot easier to
express the data /sbin/hotplug needs than three separate files.
Also note that there may be multiple 'node' files (or equivalent) for
devices that support multiple interfaces. E.g. mice in the input layer
have an general event device and a mouse event device. The patch I posted
wouldn't work, since the name is hardcoded to 'node'. But, it would be
trival to append or prepend the name of the interface to 'node'.
We could do that for each of the text files, but that would unnecessarily
clutter the device's directory.
As far as abuse, we could force the permissions to 000, which would still
allow us to stat the device. We appear to still do a request_module() on
open(), but read() and write() should fail..
-pat