Permissions don't stick on ConfigFS attributes

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
@@ -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);

A patch for making sysfs attributes persistent has recently made it into Linus' tree.

http://article.gmane.org/gmane.linux.hotplug.devel/7927/match=sysfs+permissions

Jon Smirl
[email protected]

http://article.gmane.org/gmane.linux.hotplug.devel/7927/match=sysfs+permissions

Interesting, it handles more than just the file mode. But does anybody really care about the ctime/atime/mtime in sysfs? I can see how uid and gid could be useful. My way of handling this (by copying out the potentially changed iattrs when the dentry is destroyed) looks more compact than Maneesh's solution, while not being any less effective, once I get it right that is. Does sysfs really need its own setattr?

A quibble: we normally use the term persistent to mean "saved on permanent storage". Going by that, Maneesh just fixed a bug and did not make iattrs persistent.

Regards,

Daniel

The recent changes to sysfs should be ported to configfs to do this.

No, don't. Do you think that Joel would not have already worked with the sysfs people prior to submitting this? No, he did, and we all agreed that it should be kept separate.

That's what sysfs calls its files. They used the same naming scheme there. This is nothing that a user ever cares about or sees.

configfs is not going to have that many nodes at all in memory (compared to sysfs), so I don't think this is a big problem.

So we wait and evolve the interface over time. Like always...

thanks,

greg k-h

Would you care to recap the reasoning, please?

It's wrrrrronnnggg. The best you can defend this with is "it's entrenched".

The current bloat is unconscionable, for the amount of data that is carried. Are you arguing against fixing it? And what makes you think configfs will never have lots of nodes?

Regards,

Daniel

They do two different things, and people interact with them in two different ways. So, they should be two different file systems.

Will a user ever see the word "attribute"? Also, these files represent attributes of the main object in which they are attached to. Hence the name.

Doesn't it currently work the same way as sysfs with the backing store being created on the fly? If not, it should be pretty simple to convert over if people are really worried about memory consumption, but again, why don't we see how many nodes are really used on most systems (don't want to add more complexity and kernel code if you never really need it.)

thanks,

greg k-h

There is only one essential difference: the user can create nodes in configfs. Otherwise, the user does the same with both: display things and tweak things. The big difference is in your mind, not the user's mind. And look above, you're saying "don't patch configfs, copy sysfs instead" and in the next breath you're saying "but oh they're completely different".

For example:

http://linuxcommand.org/man_pages/udev8.html

Yes, regrettable. "Properties" would be tasteful and avoid the collision.

I haven't look at sysfs yet, but the backing store, which is just a tree of configfs_dirents that point at the corresponding dentries, is created on the fly. Currently, directories are all pinned while only files are transient. There is no good reason for this as far as I can see, directories can be transient too.

What I did notice is that my fix for permissions is wrong. I plugged the updated permissions back into the descriptor for all attributes of a config item - not what we want, the update needs to be restricted to just one attribute. The right fix is to store the updated permissions in the configfs_dirent, which already has the field, it is just not used consistently. See, the code is trying to say something to us, I think the message is "read me!"

It will not be hard to fix, the framework seems to be there already. As far as complexity goes, it is trivial and the payoff is significant. Cleaning up a bunch of little stylistic things will have a bigger effect on readability.

Regards,

Daniel

Yeah, I've been meaning to do something, and resusing code is
always a good plan. Hopefully I can get to this soon.

Joel

"I don't know anything about music. In my line you don't have
 to."
        - Elvis Presley

Joel Becker
Senior Member of Technical Staff
Oracle
E-mail: [email protected]
Phone: (650) 506-8127

Ending up with the same code in core kernel in two different places is always a bad plan. Oh man. Just look at these two bodies of code, configfs is mostly just large tracts that are identical to sysfs except for name changes. Listen to what the code is trying to tell you!

SysFS:

struct kobject {
	const char		* k_name;
	char			name[KOBJ_NAME_LEN];
	struct kref		kref;
	struct list_head	entry;
	struct kobject		* parent;
	struct kset		* kset;
	struct kobj_type	* ktype;
	struct dentry		* dentry;
};

ConfigFS:

struct config_item {
	char			*ci_name;
	char			ci_namebuf[CONFIGFS_ITEM_NAME_LEN];
	struct kref		ci_kref;
	struct list_head	ci_entry;
	struct config_item	*ci_parent;
	struct config_group	*ci_group;
	struct config_item_type	*ci_type;
	struct dentry		*ci_dentry;
};

Big difference, huh?

As designer of configfs, could you please offer your take on why it cannot be rolled back into sysfs, considering that it is mostly identical already?

Regards,

Daniel

No, it should go the other way, my fix is better. It would not require sysfs to have its own version of setattr. What I do like about Maneesh's fix is the handling of other inode attributes besides mode flags, however that is a detail, let's get the structural elements right first.

The revised patch fixes the vanishing permissions bug and kills the configfs bogon that made my first attempt subtly wrong (changed permissions for all attribute files instead of just the chmoded one).

diff -up --recursive 2.6.12-mm2.clean/fs/configfs/dir.c 2.6.12-mm2/fs/configfs/dir.c
@@ -64,6 +64,17 @@ static struct dentry_operations configfs
 	.d_delete	= configfs_d_delete,
 };
 
+static int configfs_d_delete_attr(struct dentry *dentry)
+{
+	((struct configfs_dirent *)dentry->d_fsdata)->s_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
  */

@@ -238,14 +249,11 @@ static void configfs_remove_dir(struct c
  */
 static int configfs_attach_attr(struct configfs_dirent * sd, struct dentry * dentry)
 {
-	struct configfs_attribute * attr = sd->s_element;
-	int error;
-
-	error = configfs_create(dentry, (attr->ca_mode & S_IALLUGO) | S_IFREG, init_file);
+	int error = configfs_create(dentry, sd->s_mode, init_file);
 	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);

I am confused. I am beginning to see shades of the devfs problems coming up again. sysfs is built to be world readable by everyone who has it mounted in their namespace. Writable files in sysfs I have never understood.

Given that we now have files which do not conform to one uniform policy for everyone is there any reason why we do not want to allocate a character device major number for all config values and dynamically allocate a minor number for each config value? Giving each config value its own unique entry under /dev.

Device nodes for each writable config value trivially handles persistence and user policy and should be easy to implement in the kernel. We already have a policy engine in userspace, udev to handle all of the chaos.

Why do we need another mechanism?

Are device nodes out of fashion these days?

Eric

Sysfs is not like devfs by nature, it is more like procfs. It exposes properties of a device, not the device itself. It makes perfect sense that some of the properties should be writeable.

/dev is already busy enough without adding masses of entries that are not devices. I don't see that this would simplify the internal implementation either, the opposite actually. The user certainly will not have any use for temporary device numbers in this context.

On the other hand, it is clunky to force an application to go through the same parse/format interface as the user just to get/set a simple integer. Perhaps sysfs needs to be taught how to ioctl these properties. I see exposing property names and operating on them as orthogonal issues that are currently joined at the hip in an unnatural, but fixable way.

We need the mechanism that exposes subsystem instance properties as they appear and disappear with changing configuration. This is a new mechanism anyway, so implementing it using device nodes does not save anything, it only introduces a new requirement to allocate device numbers.

They are, at least for putting things in /dev that are not actual hardware.

Regards,

Daniel