Re: sysfs_chmod_file() broken in 2.6.33-rc4-git6

Re: sysfs_chmod_file() broken in 2.6.33-rc4-git6

[Eric W. Biederman]

Jean Delvare <khali@linux-fr.org> writes:

> Hi Eric, Greg,
>
> While working on a driver using 2.6.33-rc4-git6 as my environment, I
> have found that sysfs_chmod_file() called from a kernel driver no
> longer works. It doesn't return any error nor print any message in the
> logs, it simply has no effect. The same code worked fine using kernel
> 2.6.32. I have bisected it down to the following commit:
>
> commit e61ab4ae48fbf477f5b9fcbec9e1b8dc789920d0
> Author: Eric W. Biederman <ebiederm@xmission.com>
> Date:   Fri Nov 20 16:08:53 2009 -0800
>
>     sysfs: Implement sysfs_getattr & sysfs_permission
>     
>     With the implementation of sysfs_getattr and sysfs_permission
>     sysfs becomes able to lazily propogate inode attribute changes
>     from the sysfs_dirents to the vfs inodes.   This paves the way
>     for deleting significant chunks of now unnecessary code.
>     
>     While doing this we did not reference sysfs_setattr from
>     sysfs_symlink_inode_operations so I added along with
>     sysfs_getattr and sysfs_permission.
>     
>     Acked-by: Tejun Heo <tj@kernel.org>
>     Acked-by: Serge Hallyn <serue@us.ibm.com>
>     Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
>     Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
>
> Eric, can you please look at your code again and see if you can find
> any obvious issue?

Does this fix your issue?

Eric


diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index 220b758..6a06a1d 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -81,24 +81,23 @@ int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr * iattr)
 		if (!sd_attrs)
 			return -ENOMEM;
 		sd->s_iattr = sd_attrs;
-	} else {
-		/* attributes were changed at least once in past */
-		iattrs = &sd_attrs->ia_iattr;
-
-		if (ia_valid & ATTR_UID)
-			iattrs->ia_uid = iattr->ia_uid;
-		if (ia_valid & ATTR_GID)
-			iattrs->ia_gid = iattr->ia_gid;
-		if (ia_valid & ATTR_ATIME)
-			iattrs->ia_atime = iattr->ia_atime;
-		if (ia_valid & ATTR_MTIME)
-			iattrs->ia_mtime = iattr->ia_mtime;
-		if (ia_valid & ATTR_CTIME)
-			iattrs->ia_ctime = iattr->ia_ctime;
-		if (ia_valid & ATTR_MODE) {
-			umode_t mode = iattr->ia_mode;
-			iattrs->ia_mode = sd->s_mode = mode;
-		}
+	}
+	/* attributes were changed at least once in past */
+	iattrs = &sd_attrs->ia_iattr;
+
+	if (ia_valid & ATTR_UID)
+		iattrs->ia_uid = iattr->ia_uid;
+	if (ia_valid & ATTR_GID)
+		iattrs->ia_gid = iattr->ia_gid;
+	if (ia_valid & ATTR_ATIME)
+		iattrs->ia_atime = iattr->ia_atime;
+	if (ia_valid & ATTR_MTIME)
+		iattrs->ia_mtime = iattr->ia_mtime;
+	if (ia_valid & ATTR_CTIME)
+		iattrs->ia_ctime = iattr->ia_ctime;
+	if (ia_valid & ATTR_MODE) {
+		umode_t mode = iattr->ia_mode;
+		iattrs->ia_mode = sd->s_mode = mode;
 	}
 	return 0;
 }

Re: sysfs_chmod_file() broken in 2.6.33-rc4-git6

[Jean Delvare]

On Wed, 20 Jan 2010 06:01:02 -0800, Eric W. Biederman wrote:
> Jean Delvare <khali@linux-fr.org> writes:
> 
> > Hi Eric, Greg,
> >
> > While working on a driver using 2.6.33-rc4-git6 as my environment, I
> > have found that sysfs_chmod_file() called from a kernel driver no
> > longer works. It doesn't return any error nor print any message in the
> > logs, it simply has no effect. The same code worked fine using kernel
> > 2.6.32. I have bisected it down to the following commit:
> >
> > commit e61ab4ae48fbf477f5b9fcbec9e1b8dc789920d0
> > Author: Eric W. Biederman <ebiederm@xmission.com>
> > Date:   Fri Nov 20 16:08:53 2009 -0800
> >
> >     sysfs: Implement sysfs_getattr & sysfs_permission
> >     
> >     With the implementation of sysfs_getattr and sysfs_permission
> >     sysfs becomes able to lazily propogate inode attribute changes
> >     from the sysfs_dirents to the vfs inodes.   This paves the way
> >     for deleting significant chunks of now unnecessary code.
> >     
> >     While doing this we did not reference sysfs_setattr from
> >     sysfs_symlink_inode_operations so I added along with
> >     sysfs_getattr and sysfs_permission.
> >     
> >     Acked-by: Tejun Heo <tj@kernel.org>
> >     Acked-by: Serge Hallyn <serue@us.ibm.com>
> >     Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> >     Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> >
> > Eric, can you please look at your code again and see if you can find
> > any obvious issue?
> 
> Does this fix your issue?
> 
> Eric
> 
> 
> diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
> index 220b758..6a06a1d 100644
> --- a/fs/sysfs/inode.c
> +++ b/fs/sysfs/inode.c
> @@ -81,24 +81,23 @@ int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr * iattr)
>  		if (!sd_attrs)
>  			return -ENOMEM;
>  		sd->s_iattr = sd_attrs;
> -	} else {
> -		/* attributes were changed at least once in past */
> -		iattrs = &sd_attrs->ia_iattr;
> -
> -		if (ia_valid & ATTR_UID)
> -			iattrs->ia_uid = iattr->ia_uid;
> -		if (ia_valid & ATTR_GID)
> -			iattrs->ia_gid = iattr->ia_gid;
> -		if (ia_valid & ATTR_ATIME)
> -			iattrs->ia_atime = iattr->ia_atime;
> -		if (ia_valid & ATTR_MTIME)
> -			iattrs->ia_mtime = iattr->ia_mtime;
> -		if (ia_valid & ATTR_CTIME)
> -			iattrs->ia_ctime = iattr->ia_ctime;
> -		if (ia_valid & ATTR_MODE) {
> -			umode_t mode = iattr->ia_mode;
> -			iattrs->ia_mode = sd->s_mode = mode;
> -		}
> +	}
> +	/* attributes were changed at least once in past */
> +	iattrs = &sd_attrs->ia_iattr;
> +
> +	if (ia_valid & ATTR_UID)
> +		iattrs->ia_uid = iattr->ia_uid;
> +	if (ia_valid & ATTR_GID)
> +		iattrs->ia_gid = iattr->ia_gid;
> +	if (ia_valid & ATTR_ATIME)
> +		iattrs->ia_atime = iattr->ia_atime;
> +	if (ia_valid & ATTR_MTIME)
> +		iattrs->ia_mtime = iattr->ia_mtime;
> +	if (ia_valid & ATTR_CTIME)
> +		iattrs->ia_ctime = iattr->ia_ctime;
> +	if (ia_valid & ATTR_MODE) {
> +		umode_t mode = iattr->ia_mode;
> +		iattrs->ia_mode = sd->s_mode = mode;
>  	}
>  	return 0;
>  }

Yes, this fixes my problem. Thanks for the fast fix!

-- 
Jean Delvare

Re: sysfs_chmod_file() broken in 2.6.33-rc4-git6

[Greg KH]

On Wed, Jan 20, 2010 at 04:00:08PM +0100, Jean Delvare wrote:
> On Wed, 20 Jan 2010 06:01:02 -0800, Eric W. Biederman wrote:
> > Jean Delvare <khali@linux-fr.org> writes:
> > 
> > > Hi Eric, Greg,
> > >
> > > While working on a driver using 2.6.33-rc4-git6 as my environment, I
> > > have found that sysfs_chmod_file() called from a kernel driver no
> > > longer works. It doesn't return any error nor print any message in the
> > > logs, it simply has no effect. The same code worked fine using kernel
> > > 2.6.32. I have bisected it down to the following commit:
> > >
> > > commit e61ab4ae48fbf477f5b9fcbec9e1b8dc789920d0
> > > Author: Eric W. Biederman <ebiederm@xmission.com>
> > > Date:   Fri Nov 20 16:08:53 2009 -0800
> > >
> > >     sysfs: Implement sysfs_getattr & sysfs_permission
> > >     
> > >     With the implementation of sysfs_getattr and sysfs_permission
> > >     sysfs becomes able to lazily propogate inode attribute changes
> > >     from the sysfs_dirents to the vfs inodes.   This paves the way
> > >     for deleting significant chunks of now unnecessary code.
> > >     
> > >     While doing this we did not reference sysfs_setattr from
> > >     sysfs_symlink_inode_operations so I added along with
> > >     sysfs_getattr and sysfs_permission.
> > >     
> > >     Acked-by: Tejun Heo <tj@kernel.org>
> > >     Acked-by: Serge Hallyn <serue@us.ibm.com>
> > >     Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> > >     Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> > >
> > > Eric, can you please look at your code again and see if you can find
> > > any obvious issue?
> > 
> > Does this fix your issue?
> > 
> > Eric
> > 
> > 
> > diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
> > index 220b758..6a06a1d 100644
> > --- a/fs/sysfs/inode.c
> > +++ b/fs/sysfs/inode.c
> > @@ -81,24 +81,23 @@ int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr * iattr)
> >  		if (!sd_attrs)
> >  			return -ENOMEM;
> >  		sd->s_iattr = sd_attrs;
> > -	} else {
> > -		/* attributes were changed at least once in past */
> > -		iattrs = &sd_attrs->ia_iattr;
> > -
> > -		if (ia_valid & ATTR_UID)
> > -			iattrs->ia_uid = iattr->ia_uid;
> > -		if (ia_valid & ATTR_GID)
> > -			iattrs->ia_gid = iattr->ia_gid;
> > -		if (ia_valid & ATTR_ATIME)
> > -			iattrs->ia_atime = iattr->ia_atime;
> > -		if (ia_valid & ATTR_MTIME)
> > -			iattrs->ia_mtime = iattr->ia_mtime;
> > -		if (ia_valid & ATTR_CTIME)
> > -			iattrs->ia_ctime = iattr->ia_ctime;
> > -		if (ia_valid & ATTR_MODE) {
> > -			umode_t mode = iattr->ia_mode;
> > -			iattrs->ia_mode = sd->s_mode = mode;
> > -		}
> > +	}
> > +	/* attributes were changed at least once in past */
> > +	iattrs = &sd_attrs->ia_iattr;
> > +
> > +	if (ia_valid & ATTR_UID)
> > +		iattrs->ia_uid = iattr->ia_uid;
> > +	if (ia_valid & ATTR_GID)
> > +		iattrs->ia_gid = iattr->ia_gid;
> > +	if (ia_valid & ATTR_ATIME)
> > +		iattrs->ia_atime = iattr->ia_atime;
> > +	if (ia_valid & ATTR_MTIME)
> > +		iattrs->ia_mtime = iattr->ia_mtime;
> > +	if (ia_valid & ATTR_CTIME)
> > +		iattrs->ia_ctime = iattr->ia_ctime;
> > +	if (ia_valid & ATTR_MODE) {
> > +		umode_t mode = iattr->ia_mode;
> > +		iattrs->ia_mode = sd->s_mode = mode;
> >  	}
> >  	return 0;
> >  }
> 
> Yes, this fixes my problem. Thanks for the fast fix!

Great, Eric, care to resend this so that I can submit it for inclusion?

thanks,

greg k-h

Re: sysfs_chmod_file() broken in 2.6.33-rc4-git6

[Jean Delvare]

On Thu, 21 Jan 2010 20:19:13 -0800, Greg KH wrote:
> On Wed, Jan 20, 2010 at 04:00:08PM +0100, Jean Delvare wrote:
> > On Wed, 20 Jan 2010 06:01:02 -0800, Eric W. Biederman wrote:
> > > Does this fix your issue?
> > > 
> > > Eric
> > > 
> > > 
> > > diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
> > > index 220b758..6a06a1d 100644
> > > --- a/fs/sysfs/inode.c
> > > +++ b/fs/sysfs/inode.c
> > > @@ -81,24 +81,23 @@ int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr * iattr)
> > >  		if (!sd_attrs)
> > >  			return -ENOMEM;
> > >  		sd->s_iattr = sd_attrs;
> > > -	} else {
> > > -		/* attributes were changed at least once in past */
> > > -		iattrs = &sd_attrs->ia_iattr;
> > > -
> > > -		if (ia_valid & ATTR_UID)
> > > -			iattrs->ia_uid = iattr->ia_uid;
> > > -		if (ia_valid & ATTR_GID)
> > > -			iattrs->ia_gid = iattr->ia_gid;
> > > -		if (ia_valid & ATTR_ATIME)
> > > -			iattrs->ia_atime = iattr->ia_atime;
> > > -		if (ia_valid & ATTR_MTIME)
> > > -			iattrs->ia_mtime = iattr->ia_mtime;
> > > -		if (ia_valid & ATTR_CTIME)
> > > -			iattrs->ia_ctime = iattr->ia_ctime;
> > > -		if (ia_valid & ATTR_MODE) {
> > > -			umode_t mode = iattr->ia_mode;
> > > -			iattrs->ia_mode = sd->s_mode = mode;
> > > -		}
> > > +	}
> > > +	/* attributes were changed at least once in past */
> > > +	iattrs = &sd_attrs->ia_iattr;
> > > +
> > > +	if (ia_valid & ATTR_UID)
> > > +		iattrs->ia_uid = iattr->ia_uid;
> > > +	if (ia_valid & ATTR_GID)
> > > +		iattrs->ia_gid = iattr->ia_gid;
> > > +	if (ia_valid & ATTR_ATIME)
> > > +		iattrs->ia_atime = iattr->ia_atime;
> > > +	if (ia_valid & ATTR_MTIME)
> > > +		iattrs->ia_mtime = iattr->ia_mtime;
> > > +	if (ia_valid & ATTR_CTIME)
> > > +		iattrs->ia_ctime = iattr->ia_ctime;
> > > +	if (ia_valid & ATTR_MODE) {
> > > +		umode_t mode = iattr->ia_mode;
> > > +		iattrs->ia_mode = sd->s_mode = mode;
> > >  	}
> > >  	return 0;
> > >  }
> > 
> > Yes, this fixes my problem. Thanks for the fast fix!
> 
> Great, Eric, care to resend this so that I can submit it for inclusion?

Ping Eric, the patch is still not in Linus' tree.

-- 
Jean Delvare

Re: sysfs_chmod_file() broken in 2.6.33-rc4-git6

[Greg KH]

On Wed, Feb 03, 2010 at 09:11:38AM +0100, Jean Delvare wrote:
> On Thu, 21 Jan 2010 20:19:13 -0800, Greg KH wrote:
> > On Wed, Jan 20, 2010 at 04:00:08PM +0100, Jean Delvare wrote:
> > > On Wed, 20 Jan 2010 06:01:02 -0800, Eric W. Biederman wrote:
> > > > Does this fix your issue?
> > > > 
> > > > Eric
> > > > 
> > > > 
> > > > diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
> > > > index 220b758..6a06a1d 100644
> > > > --- a/fs/sysfs/inode.c
> > > > +++ b/fs/sysfs/inode.c
> > > > @@ -81,24 +81,23 @@ int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr * iattr)
> > > >  		if (!sd_attrs)
> > > >  			return -ENOMEM;
> > > >  		sd->s_iattr = sd_attrs;
> > > > -	} else {
> > > > -		/* attributes were changed at least once in past */
> > > > -		iattrs = &sd_attrs->ia_iattr;
> > > > -
> > > > -		if (ia_valid & ATTR_UID)
> > > > -			iattrs->ia_uid = iattr->ia_uid;
> > > > -		if (ia_valid & ATTR_GID)
> > > > -			iattrs->ia_gid = iattr->ia_gid;
> > > > -		if (ia_valid & ATTR_ATIME)
> > > > -			iattrs->ia_atime = iattr->ia_atime;
> > > > -		if (ia_valid & ATTR_MTIME)
> > > > -			iattrs->ia_mtime = iattr->ia_mtime;
> > > > -		if (ia_valid & ATTR_CTIME)
> > > > -			iattrs->ia_ctime = iattr->ia_ctime;
> > > > -		if (ia_valid & ATTR_MODE) {
> > > > -			umode_t mode = iattr->ia_mode;
> > > > -			iattrs->ia_mode = sd->s_mode = mode;
> > > > -		}
> > > > +	}
> > > > +	/* attributes were changed at least once in past */
> > > > +	iattrs = &sd_attrs->ia_iattr;
> > > > +
> > > > +	if (ia_valid & ATTR_UID)
> > > > +		iattrs->ia_uid = iattr->ia_uid;
> > > > +	if (ia_valid & ATTR_GID)
> > > > +		iattrs->ia_gid = iattr->ia_gid;
> > > > +	if (ia_valid & ATTR_ATIME)
> > > > +		iattrs->ia_atime = iattr->ia_atime;
> > > > +	if (ia_valid & ATTR_MTIME)
> > > > +		iattrs->ia_mtime = iattr->ia_mtime;
> > > > +	if (ia_valid & ATTR_CTIME)
> > > > +		iattrs->ia_ctime = iattr->ia_ctime;
> > > > +	if (ia_valid & ATTR_MODE) {
> > > > +		umode_t mode = iattr->ia_mode;
> > > > +		iattrs->ia_mode = sd->s_mode = mode;
> > > >  	}
> > > >  	return 0;
> > > >  }
> > > 
> > > Yes, this fixes my problem. Thanks for the fast fix!
> > 
> > Great, Eric, care to resend this so that I can submit it for inclusion?
> 
> Ping Eric, the patch is still not in Linus' tree.

Nor mine :)

Eric, care to send it to me?

thanks,

greg k-h

Re: sysfs_chmod_file() broken in 2.6.33-rc4-git6

[Eric W. Biederman]

Greg KH <gregkh@suse.de> writes:

> On Wed, Feb 03, 2010 at 09:11:38AM +0100, Jean Delvare wrote:
>> On Thu, 21 Jan 2010 20:19:13 -0800, Greg KH wrote:
>> > On Wed, Jan 20, 2010 at 04:00:08PM +0100, Jean Delvare wrote:
>> > > On Wed, 20 Jan 2010 06:01:02 -0800, Eric W. Biederman wrote:
>> > > > Does this fix your issue?
>> > > > 
>> > > > Eric
>> > > > 
>> > > > 
>> > > > diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
>> > > > index 220b758..6a06a1d 100644
>> > > > --- a/fs/sysfs/inode.c
>> > > > +++ b/fs/sysfs/inode.c
>> > > > @@ -81,24 +81,23 @@ int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr * iattr)
>> > > >  		if (!sd_attrs)
>> > > >  			return -ENOMEM;
>> > > >  		sd->s_iattr = sd_attrs;
>> > > > -	} else {
>> > > > -		/* attributes were changed at least once in past */
>> > > > -		iattrs = &sd_attrs->ia_iattr;
>> > > > -
>> > > > -		if (ia_valid & ATTR_UID)
>> > > > -			iattrs->ia_uid = iattr->ia_uid;
>> > > > -		if (ia_valid & ATTR_GID)
>> > > > -			iattrs->ia_gid = iattr->ia_gid;
>> > > > -		if (ia_valid & ATTR_ATIME)
>> > > > -			iattrs->ia_atime = iattr->ia_atime;
>> > > > -		if (ia_valid & ATTR_MTIME)
>> > > > -			iattrs->ia_mtime = iattr->ia_mtime;
>> > > > -		if (ia_valid & ATTR_CTIME)
>> > > > -			iattrs->ia_ctime = iattr->ia_ctime;
>> > > > -		if (ia_valid & ATTR_MODE) {
>> > > > -			umode_t mode = iattr->ia_mode;
>> > > > -			iattrs->ia_mode = sd->s_mode = mode;
>> > > > -		}
>> > > > +	}
>> > > > +	/* attributes were changed at least once in past */
>> > > > +	iattrs = &sd_attrs->ia_iattr;
>> > > > +
>> > > > +	if (ia_valid & ATTR_UID)
>> > > > +		iattrs->ia_uid = iattr->ia_uid;
>> > > > +	if (ia_valid & ATTR_GID)
>> > > > +		iattrs->ia_gid = iattr->ia_gid;
>> > > > +	if (ia_valid & ATTR_ATIME)
>> > > > +		iattrs->ia_atime = iattr->ia_atime;
>> > > > +	if (ia_valid & ATTR_MTIME)
>> > > > +		iattrs->ia_mtime = iattr->ia_mtime;
>> > > > +	if (ia_valid & ATTR_CTIME)
>> > > > +		iattrs->ia_ctime = iattr->ia_ctime;
>> > > > +	if (ia_valid & ATTR_MODE) {
>> > > > +		umode_t mode = iattr->ia_mode;
>> > > > +		iattrs->ia_mode = sd->s_mode = mode;
>> > > >  	}
>> > > >  	return 0;
>> > > >  }
>> > > 
>> > > Yes, this fixes my problem. Thanks for the fast fix!
>> > 
>> > Great, Eric, care to resend this so that I can submit it for inclusion?
>> 
>> Ping Eric, the patch is still not in Linus' tree.
>
> Nor mine :)
>
> Eric, care to send it to me?

Apologies for the long delay.  I have been extremely out of it lately.
I am should have a signed off by version sent shortly.  I looked and
this problem also exists in 2.6.32 but hides because in that case we
directly update the vfs inode so sysfs_chmod works until the vfs inode
is evicted.   I will send you a patch for 2.6.32 -stable as well.

Eric

[PATCH] sysfs: sysfs_sd_setattr set iattrs unconditionally

[Eric W. Biederman]

There is currently a bug in sysfs_sd_setattr inherited from
sysfs_setattr in 2.6.32 where the first time we set the attributes
on a sysfs file we allocate backing store but do not set the
backing store attributes.  Resulting in overly restrictive
permissions on sysfs files.

The fix is to simply modify the code so that it always executes
when we update the sysfs attributes, as we did in 2.6.31 and earlier.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 fs/sysfs/inode.c |   35 +++++++++++++++++------------------
 1 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index 220b758..6a06a1d 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -81,24 +81,23 @@ int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr * iattr)
 		if (!sd_attrs)
 			return -ENOMEM;
 		sd->s_iattr = sd_attrs;
-	} else {
-		/* attributes were changed at least once in past */
-		iattrs = &sd_attrs->ia_iattr;
-
-		if (ia_valid & ATTR_UID)
-			iattrs->ia_uid = iattr->ia_uid;
-		if (ia_valid & ATTR_GID)
-			iattrs->ia_gid = iattr->ia_gid;
-		if (ia_valid & ATTR_ATIME)
-			iattrs->ia_atime = iattr->ia_atime;
-		if (ia_valid & ATTR_MTIME)
-			iattrs->ia_mtime = iattr->ia_mtime;
-		if (ia_valid & ATTR_CTIME)
-			iattrs->ia_ctime = iattr->ia_ctime;
-		if (ia_valid & ATTR_MODE) {
-			umode_t mode = iattr->ia_mode;
-			iattrs->ia_mode = sd->s_mode = mode;
-		}
+	}
+	/* attributes were changed at least once in past */
+	iattrs = &sd_attrs->ia_iattr;
+
+	if (ia_valid & ATTR_UID)
+		iattrs->ia_uid = iattr->ia_uid;
+	if (ia_valid & ATTR_GID)
+		iattrs->ia_gid = iattr->ia_gid;
+	if (ia_valid & ATTR_ATIME)
+		iattrs->ia_atime = iattr->ia_atime;
+	if (ia_valid & ATTR_MTIME)
+		iattrs->ia_mtime = iattr->ia_mtime;
+	if (ia_valid & ATTR_CTIME)
+		iattrs->ia_ctime = iattr->ia_ctime;
+	if (ia_valid & ATTR_MODE) {
+		umode_t mode = iattr->ia_mode;
+		iattrs->ia_mode = sd->s_mode = mode;
 	}
 	return 0;
 }
-- 
1.6.5.2.143.g8cc62

[PATCH] sysfs: sysfs_setattr set iattrs unconditionally

[Eric W. Biederman]

There is currently a bug in sysfs_setattr where the first time
we set the attributes on a sysfs file we allocate backing store
but do not set the backing store attributes.  Resulting in overly
restrictive permissions on sysfs files.

This bug is masked by the fact we that we do update the vfs inode
so it will only show up after the vfs inode has been evicted
causing surprise sysfs attribute changes.

The fix is to simply modify the code so that it always executes
when we update the sysfs attributes, as we did in 2.6.31 and earlier.

This is a backported version of my similiar change for 2.6.33-rc6
against sysfs_sd_setattr, for the stable 2.6.32 tree.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 fs/sysfs/inode.c |   47 +++++++++++++++++++++++------------------------
 1 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index e28cecf..02a022a 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -94,30 +94,29 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
 		if (!sd_attrs)
 			return -ENOMEM;
 		sd->s_iattr = sd_attrs;
-	} else {
-		/* attributes were changed at least once in past */
-		iattrs = &sd_attrs->ia_iattr;
-
-		if (ia_valid & ATTR_UID)
-			iattrs->ia_uid = iattr->ia_uid;
-		if (ia_valid & ATTR_GID)
-			iattrs->ia_gid = iattr->ia_gid;
-		if (ia_valid & ATTR_ATIME)
-			iattrs->ia_atime = timespec_trunc(iattr->ia_atime,
-					inode->i_sb->s_time_gran);
-		if (ia_valid & ATTR_MTIME)
-			iattrs->ia_mtime = timespec_trunc(iattr->ia_mtime,
-					inode->i_sb->s_time_gran);
-		if (ia_valid & ATTR_CTIME)
-			iattrs->ia_ctime = timespec_trunc(iattr->ia_ctime,
-					inode->i_sb->s_time_gran);
-		if (ia_valid & ATTR_MODE) {
-			umode_t mode = iattr->ia_mode;
-
-			if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
-				mode &= ~S_ISGID;
-			iattrs->ia_mode = sd->s_mode = mode;
-		}
+	}
+	/* attributes were changed at least once in past */
+	iattrs = &sd_attrs->ia_iattr;
+
+	if (ia_valid & ATTR_UID)
+		iattrs->ia_uid = iattr->ia_uid;
+	if (ia_valid & ATTR_GID)
+		iattrs->ia_gid = iattr->ia_gid;
+	if (ia_valid & ATTR_ATIME)
+		iattrs->ia_atime = timespec_trunc(iattr->ia_atime,
+			inode->i_sb->s_time_gran);
+	if (ia_valid & ATTR_MTIME)
+		iattrs->ia_mtime = timespec_trunc(iattr->ia_mtime,
+			inode->i_sb->s_time_gran);
+	if (ia_valid & ATTR_CTIME)
+		iattrs->ia_ctime = timespec_trunc(iattr->ia_ctime,
+			inode->i_sb->s_time_gran);
+	if (ia_valid & ATTR_MODE) {
+		umode_t mode = iattr->ia_mode;
+
+		if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
+			mode &= ~S_ISGID;
+		iattrs->ia_mode = sd->s_mode = mode;
 	}
 	return error;
 }
-- 
1.6.5.2.143.g8cc62

Re: [PATCH] sysfs: sysfs_sd_setattr set iattrs unconditionally

[Jean Delvare]

On Wed, 03 Feb 2010 23:13:24 -0800, Eric W. Biederman wrote:
> 
> There is currently a bug in sysfs_sd_setattr inherited from
> sysfs_setattr in 2.6.32 where the first time we set the attributes
> on a sysfs file we allocate backing store but do not set the
> backing store attributes.  Resulting in overly restrictive
> permissions on sysfs files.
> 
> The fix is to simply modify the code so that it always executes
> when we update the sysfs attributes, as we did in 2.6.31 and earlier.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

Tested-by: Jean Delvare <khali@linux-fr.org>

> ---
>  fs/sysfs/inode.c |   35 +++++++++++++++++------------------
>  1 files changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
> index 220b758..6a06a1d 100644
> --- a/fs/sysfs/inode.c
> +++ b/fs/sysfs/inode.c
> @@ -81,24 +81,23 @@ int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr * iattr)
>  		if (!sd_attrs)
>  			return -ENOMEM;
>  		sd->s_iattr = sd_attrs;
> -	} else {
> -		/* attributes were changed at least once in past */
> -		iattrs = &sd_attrs->ia_iattr;
> -
> -		if (ia_valid & ATTR_UID)
> -			iattrs->ia_uid = iattr->ia_uid;
> -		if (ia_valid & ATTR_GID)
> -			iattrs->ia_gid = iattr->ia_gid;
> -		if (ia_valid & ATTR_ATIME)
> -			iattrs->ia_atime = iattr->ia_atime;
> -		if (ia_valid & ATTR_MTIME)
> -			iattrs->ia_mtime = iattr->ia_mtime;
> -		if (ia_valid & ATTR_CTIME)
> -			iattrs->ia_ctime = iattr->ia_ctime;
> -		if (ia_valid & ATTR_MODE) {
> -			umode_t mode = iattr->ia_mode;
> -			iattrs->ia_mode = sd->s_mode = mode;
> -		}
> +	}
> +	/* attributes were changed at least once in past */
> +	iattrs = &sd_attrs->ia_iattr;
> +
> +	if (ia_valid & ATTR_UID)
> +		iattrs->ia_uid = iattr->ia_uid;
> +	if (ia_valid & ATTR_GID)
> +		iattrs->ia_gid = iattr->ia_gid;
> +	if (ia_valid & ATTR_ATIME)
> +		iattrs->ia_atime = iattr->ia_atime;
> +	if (ia_valid & ATTR_MTIME)
> +		iattrs->ia_mtime = iattr->ia_mtime;
> +	if (ia_valid & ATTR_CTIME)
> +		iattrs->ia_ctime = iattr->ia_ctime;
> +	if (ia_valid & ATTR_MODE) {
> +		umode_t mode = iattr->ia_mode;
> +		iattrs->ia_mode = sd->s_mode = mode;
>  	}
>  	return 0;
>  }


-- 
Jean Delvare

Re: [PATCH] sysfs: sysfs_sd_setattr set iattrs unconditionally

[Eric W. Biederman]

Greg I think this patch has slipped through the cracks.
Do I need to resend this regression fix or send it directly to Linus?

chmod and friends have been broken on sysfs since 2.6.32...

Eric

ebiederm@xmission.com (Eric W. Biederman) writes:

> There is currently a bug in sysfs_sd_setattr inherited from
> sysfs_setattr in 2.6.32 where the first time we set the attributes
> on a sysfs file we allocate backing store but do not set the
> backing store attributes.  Resulting in overly restrictive
> permissions on sysfs files.
>
> The fix is to simply modify the code so that it always executes
> when we update the sysfs attributes, as we did in 2.6.31 and earlier.
>
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> ---
>  fs/sysfs/inode.c |   35 +++++++++++++++++------------------
>  1 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
> index 220b758..6a06a1d 100644
> --- a/fs/sysfs/inode.c
> +++ b/fs/sysfs/inode.c
> @@ -81,24 +81,23 @@ int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr * iattr)
>  		if (!sd_attrs)
>  			return -ENOMEM;
>  		sd->s_iattr = sd_attrs;
> -	} else {
> -		/* attributes were changed at least once in past */
> -		iattrs = &sd_attrs->ia_iattr;
> -
> -		if (ia_valid & ATTR_UID)
> -			iattrs->ia_uid = iattr->ia_uid;
> -		if (ia_valid & ATTR_GID)
> -			iattrs->ia_gid = iattr->ia_gid;
> -		if (ia_valid & ATTR_ATIME)
> -			iattrs->ia_atime = iattr->ia_atime;
> -		if (ia_valid & ATTR_MTIME)
> -			iattrs->ia_mtime = iattr->ia_mtime;
> -		if (ia_valid & ATTR_CTIME)
> -			iattrs->ia_ctime = iattr->ia_ctime;
> -		if (ia_valid & ATTR_MODE) {
> -			umode_t mode = iattr->ia_mode;
> -			iattrs->ia_mode = sd->s_mode = mode;
> -		}
> +	}
> +	/* attributes were changed at least once in past */
> +	iattrs = &sd_attrs->ia_iattr;
> +
> +	if (ia_valid & ATTR_UID)
> +		iattrs->ia_uid = iattr->ia_uid;
> +	if (ia_valid & ATTR_GID)
> +		iattrs->ia_gid = iattr->ia_gid;
> +	if (ia_valid & ATTR_ATIME)
> +		iattrs->ia_atime = iattr->ia_atime;
> +	if (ia_valid & ATTR_MTIME)
> +		iattrs->ia_mtime = iattr->ia_mtime;
> +	if (ia_valid & ATTR_CTIME)
> +		iattrs->ia_ctime = iattr->ia_ctime;
> +	if (ia_valid & ATTR_MODE) {
> +		umode_t mode = iattr->ia_mode;
> +		iattrs->ia_mode = sd->s_mode = mode;
>  	}
>  	return 0;
>  }
> -- 
> 1.6.5.2.143.g8cc62

Re: [PATCH] sysfs: sysfs_sd_setattr set iattrs unconditionally

[Greg KH]

On Thu, Feb 11, 2010 at 07:33:29AM -0800, Eric W. Biederman wrote:
> 
> Greg I think this patch has slipped through the cracks.
> Do I need to resend this regression fix or send it directly to Linus?
> 
> chmod and friends have been broken on sysfs since 2.6.32...

Sorry, I was travelling last week, it's still in my queue, I'll get it
to Linus soon.

thanks,

greg k-h

Re: [PATCH] sysfs: sysfs_sd_setattr set iattrs unconditionally

[Eric W. Biederman]

Greg KH <gregkh@suse.de> writes:

> On Thu, Feb 11, 2010 at 07:33:29AM -0800, Eric W. Biederman wrote:
>> 
>> Greg I think this patch has slipped through the cracks.
>> Do I need to resend this regression fix or send it directly to Linus?
>> 
>> chmod and friends have been broken on sysfs since 2.6.32...
>
> Sorry, I was travelling last week, it's still in my queue, I'll get it
> to Linus soon.

That is a better excuse than my nasty cold.  Thanks for taking care of
this.

Eric