[PATCH] Permissions don't stick on ConfigFS attributes

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

Re: [PATCH] Permissions don't stick on ConfigFS attributes

[Jon Smirl]

On 8/19/05, Daniel Phillips <phillips@istop.com> wrote:
> 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.

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
jonsmirl@gmail.com

Re: [PATCH] Permissions don't stick on ConfigFS attributes

[Greg KH]

On Sat, Aug 20, 2005 at 10:50:51AM +1000, Daniel Phillips wrote:
> Hi Joel,
> 
> Permissions set on ConfigFS attributes (aka files) do not stick.

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

> So: Integrate with sysfs.

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.

> Terminology skew.  It is a very bad idea to call your configfs files 
> "attributes".

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.

> Memory requirements.  ConfigFS pins way too much kernel memory for inodes
> and dentries.

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.

> 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.

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

thanks,

greg k-h

Re: [PATCH] Permissions don't stick on ConfigFS attributes

[Daniel Phillips]

On Saturday 20 August 2005 13:01, Greg KH wrote:
> On Sat, Aug 20, 2005 at 10:50:51AM +1000, Daniel Phillips wrote:
> > So: Integrate with sysfs.
>
> 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.

Would you care to recap the reasoning, please?

> > Terminology skew.  It is a very bad idea to call your configfs files
> > "attributes".
>
> 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.

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

> > Memory requirements.  ConfigFS pins way too much kernel memory for inodes
> > and dentries.
>
> 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.

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

Re: [PATCH] Permissions don't stick on ConfigFS attributes

[Greg KH]

On Sat, Aug 20, 2005 at 01:23:29PM +1000, Daniel Phillips wrote:
> On Saturday 20 August 2005 13:01, Greg KH wrote:
> > On Sat, Aug 20, 2005 at 10:50:51AM +1000, Daniel Phillips wrote:
> > > So: Integrate with sysfs.
> >
> > 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.
> 
> Would you care to recap the reasoning, please?

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

> > > Terminology skew.  It is a very bad idea to call your configfs files
> > > "attributes".
> >
> > 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.
> 
> It's wrrrrronnnggg.  The best you can defend this with is "it's entrenched".

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.

> > > Memory requirements.  ConfigFS pins way too much kernel memory for inodes
> > > and dentries.
> >
> > 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.
> 
> 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?

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

Re: [PATCH] Permissions don't stick on ConfigFS attributes

[Daniel Phillips]

On Saturday 20 August 2005 13:33, Greg KH wrote:
> On Sat, Aug 20, 2005 at 01:23:29PM +1000, Daniel Phillips wrote:
> > On Saturday 20 August 2005 13:01, Greg KH wrote:
> > > On Sat, Aug 20, 2005 at 10:50:51AM +1000, Daniel Phillips wrote:
> > > > So: Integrate with sysfs.
> > >
> > > 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.
> >
> > Would you care to recap the reasoning, please?
>
> They do two different things, and people interact with them in two
> different ways.

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".

> > > > Terminology skew.  It is a very bad idea to call your configfs files
> > > > "attributes".
> > >
> > > 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.
> >
> > It's wrrrrronnnggg.  The best you can defend this with is "it's
> > entrenched".
>
> Will a user ever see the word "attribute"?

For example:

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

> Also, these files represent 
> attributes of the main object in which they are attached to.  Hence the
> name.

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

> > > > Memory requirements.  ConfigFS pins way too much kernel memory for
> > > > inodes and dentries.
> > >
> > > 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.
> >
> > 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?
>
> Doesn't it currently work the same way as sysfs with the backing store
> being created on the fly?

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!"

> 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.)

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

Re: [PATCH] Permissions don't stick on ConfigFS attributes

[Daniel Phillips]

On Saturday 20 August 2005 11:22, Jon Smirl wrote:
> 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

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

Re: [PATCH] Permissions don't stick on ConfigFS attributes

[Joel Becker]

On Fri, Aug 19, 2005 at 08:01:17PM -0700, Greg KH wrote:
> The recent changes to sysfs should be ported to configfs to do this.

	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: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH] Permissions don't stick on ConfigFS attributes

[Daniel Phillips]

On Saturday 20 August 2005 16:31, Joel Becker wrote:
> On Fri, Aug 19, 2005 at 08:01:17PM -0700, Greg KH wrote:
> > The recent changes to sysfs should be ported to configfs to do this.
>
> 	Yeah, I've been meaning to do something, and resusing code is
> always a good plan.

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

[PATCH] Permissions don't stick on ConfigFS attributes (revised)

[Daniel Phillips]

On Saturday 20 August 2005 13:01, Greg KH wrote:
> On Sat, Aug 20, 2005 at 10:50:51AM +1000, Daniel Phillips wrote:
> > Permissions set on ConfigFS attributes (aka files) do not stick.
>
> The recent changes to sysfs should be ported to configfs to do this.

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
--- 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-20 16:16:34.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)
+{
+	((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);

Re: [PATCH] Permissions don't stick on ConfigFS attributes

[Eric W. Biederman]

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

Re: [PATCH] Permissions don't stick on ConfigFS attributes

[Daniel Phillips]

On Monday 22 August 2005 00:49, Eric W. Biederman wrote:
> 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.

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.

> 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.

/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.

> 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?

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.

> Are device nodes out of fashion these days?

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

Regards,

Daniel