[PATCH] Permissions don't stick on ConfigFS attributes

[Daniel Phillips]

Hi Joel,

Permissions set on ConfigFS attributes (aka files) do not stick.  The reason 
is that configfs attribute inodes are not pinned and simply disappear after
each file operation.  This is good because it saves memory, but it is not
good to throw the permissions away - you then don't have any way to expose 
configuration tweaks to normal users.  The patch below fixes this by copying 
each file's mode back into the non-transient backing structure on dentry 
delete.

Some general comments on this work, if I may.  First, this is really an 
inspired hack, it gives the user an intuitive way to instance kernel
subsystems.  It turned out to be just the thing for one of my own projects. 
_However_ while it may make sense to you as a programmer that configfs is
separate from sysfs, it certainly will not make sense to users.  Users will
just wonder why they can't create directories in sysfs.  This work needs to
end up as part of sysfs.  It should simply be a new way for a subsystem to
instance a sysfs directory.  (Note!  I haven't actually looked at sysfs yet,
my impression of how it must work is in fact based on reading ConfigFS.)

So: Integrate with sysfs.  To prove itself, it is perfectly fine for configfs
to own a separate namespace, but in my opinion you need to get together with
the sysfs crew and make it one thing, in the fullness of time (i.e., before
hitting mainline).

Terminology skew.  It is a very bad idea to call your configfs files 
"attributes".  Filesystems already have attributes, there is an api for them, 
and they are not the things you call attributes.  You need to put on your 
thinking cap and come up with a new term.  They are really files, but that 
somehow just doesn't sound sufficiently high-tech, does it.  OK, I see you 
inherited this bogosity from sysfs, it is not your fault.  But it is still 
way wrong.

Memory requirements.  ConfigFS pins way too much kernel memory for inodes
and dentries.  You can extend the concept of what happens at the leaf nodes 
(files aka attributes) and copy the useful data back into compact in-memory 
backing store, then just let the vfs objects vanish as each operation 
completes.  Nobody will notice the extra cpu work, but they will certainly 
notice dozens or hundreds of pinned inodes.  Getting rid of the pinned inodes 
is not that hard.

Note that getting rid of the pinned memory will make it an even worse idea to 
use this interface in the block IO path of a cluster, as was proposed at 
Walldorf.   As you noted, Netlink et al are still there and in many cases 
remain the right tools for the job.  ConfigFS is also not a replacement for 
config files.  For one thing, it is volatile and ultimately needs a separate 
persistent store anyway.  Ahem, a config file.  What ConfigFS does better 
than any other interface is allow the user to instance a kernel subsystem.  
There is code out there that tries to do this with insmod, a horribly bad 
idea.  Any such code needs to get the ConfigFS religion right now.

Verbose kernel side interfaces.  My kernel-side implementation of a very 
simple group with single-attribute children is about 150 lines.  If this 
interface takes off and there are, say, 100 kernel classes exposed via 
configfs, is 15,000 lines of kernel source an acceptable overhead?  Not in my 
book.  You need a libconfigfs to encapsulate some of the more common 
situations so that a kernel-side interface can be just half a dozen lines or 
so in those cases.  Of course, it would help to use this a while and find out 
what those common situations really are.

Lindenting.  You have lots of spaces in the wrong places.  It is pervasive 
enough that you might consider just running the whole thing through lindent.

Permissions.  There are still some loose ends, I will play with it some more.

Error reporting.  If a ConfigFS instance doesn't like something the user
throws at it, it just rejects it with EINVAL (your examples silently treat
alpha strings not as EINVAL, but as zero).  Kprinting a detailed error would
help, but you ought to have a ponder about how to make errors more
user-visible and more directly coupled to the culprit.  Error attributes?

Anyway, great stuff!  You have managed to see the forest in spite of all the 
trees in the way.

Regards,

Daniel

diff -up --recursive 2.6.12-mm2.clean/fs/configfs/dir.c 2.6.12-mm2/fs/configfs/dir.c
--- 2.6.12-mm2.clean/fs/configfs/dir.c	2005-08-12 00:53:06.000000000 -0400
+++ 2.6.12-mm2/fs/configfs/dir.c	2005-08-19 18:35:28.000000000 -0400
@@ -64,6 +64,17 @@ static struct dentry_operations configfs
 	.d_delete	= configfs_d_delete,
 };
 
+static int configfs_d_delete_attr(struct dentry *dentry)
+{
+	to_attr(dentry)->ca_mode = dentry->d_inode->i_mode;
+	return 1;
+}
+
+static struct dentry_operations configfs_attr_dentry_ops = {
+	.d_delete = configfs_d_delete_attr,
+	.d_iput = configfs_d_iput,
+};
+
 /*
  * Allocates a new configfs_dirent and links it to the parent configfs_dirent
  */
@@ -245,7 +256,7 @@ static int configfs_attach_attr(struct c
 	if (error)
 		return error;
 
-	dentry->d_op = &configfs_dentry_ops;
+	dentry->d_op = &configfs_attr_dentry_ops;
 	dentry->d_fsdata = configfs_get(sd);
 	sd->s_dentry = dentry;
 	d_rehash(dentry);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/