* Re: sysfs_chmod_file() broken in 2.6.33-rc4-git6 [not found] <20100120131602.12aa48f7@hyperion.delvare> @ 2010-01-20 14:01 ` Eric W. Biederman 2010-01-20 15:00 ` Jean Delvare 0 siblings, 1 reply; 12+ messages in thread From: Eric W. Biederman @ 2010-01-20 14:01 UTC (permalink / raw) To: Jean Delvare; +Cc: Greg Kroah-Hartman, LKML, Tejun Heo, Serge Hallyn 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; } ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: sysfs_chmod_file() broken in 2.6.33-rc4-git6 2010-01-20 14:01 ` sysfs_chmod_file() broken in 2.6.33-rc4-git6 Eric W. Biederman @ 2010-01-20 15:00 ` Jean Delvare 2010-01-22 4:19 ` Greg KH 0 siblings, 1 reply; 12+ messages in thread From: Jean Delvare @ 2010-01-20 15:00 UTC (permalink / raw) To: Eric W. Biederman; +Cc: Greg Kroah-Hartman, LKML, Tejun Heo, Serge Hallyn 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: sysfs_chmod_file() broken in 2.6.33-rc4-git6 2010-01-20 15:00 ` Jean Delvare @ 2010-01-22 4:19 ` Greg KH 2010-02-03 8:11 ` Jean Delvare 0 siblings, 1 reply; 12+ messages in thread From: Greg KH @ 2010-01-22 4:19 UTC (permalink / raw) To: Jean Delvare; +Cc: Eric W. Biederman, LKML, Tejun Heo, Serge Hallyn 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: sysfs_chmod_file() broken in 2.6.33-rc4-git6 2010-01-22 4:19 ` Greg KH @ 2010-02-03 8:11 ` Jean Delvare 2010-02-03 14:22 ` Greg KH 0 siblings, 1 reply; 12+ messages in thread From: Jean Delvare @ 2010-02-03 8:11 UTC (permalink / raw) To: Greg KH, Eric W. Biederman; +Cc: LKML, Tejun Heo, Serge Hallyn 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: sysfs_chmod_file() broken in 2.6.33-rc4-git6 2010-02-03 8:11 ` Jean Delvare @ 2010-02-03 14:22 ` Greg KH 2010-02-04 7:05 ` Eric W. Biederman ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Greg KH @ 2010-02-03 14:22 UTC (permalink / raw) To: Jean Delvare; +Cc: Eric W. Biederman, LKML, Tejun Heo, Serge Hallyn 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: sysfs_chmod_file() broken in 2.6.33-rc4-git6 2010-02-03 14:22 ` Greg KH @ 2010-02-04 7:05 ` Eric W. Biederman 2010-02-04 7:13 ` [PATCH] sysfs: sysfs_sd_setattr set iattrs unconditionally Eric W. Biederman 2010-02-04 7:32 ` [PATCH] sysfs: sysfs_setattr " Eric W. Biederman 2 siblings, 0 replies; 12+ messages in thread From: Eric W. Biederman @ 2010-02-04 7:05 UTC (permalink / raw) To: Greg KH; +Cc: Jean Delvare, LKML, Tejun Heo, Serge Hallyn 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] sysfs: sysfs_sd_setattr set iattrs unconditionally 2010-02-03 14:22 ` Greg KH 2010-02-04 7:05 ` Eric W. Biederman @ 2010-02-04 7:13 ` Eric W. Biederman 2010-02-04 8:36 ` Jean Delvare 2010-02-11 15:33 ` Eric W. Biederman 2010-02-04 7:32 ` [PATCH] sysfs: sysfs_setattr " Eric W. Biederman 2 siblings, 2 replies; 12+ messages in thread From: Eric W. Biederman @ 2010-02-04 7:13 UTC (permalink / raw) To: Greg KH; +Cc: Jean Delvare, LKML, Tejun Heo, Serge Hallyn 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] sysfs: sysfs_sd_setattr set iattrs unconditionally 2010-02-04 7:13 ` [PATCH] sysfs: sysfs_sd_setattr set iattrs unconditionally Eric W. Biederman @ 2010-02-04 8:36 ` Jean Delvare 2010-02-11 15:33 ` Eric W. Biederman 1 sibling, 0 replies; 12+ messages in thread From: Jean Delvare @ 2010-02-04 8:36 UTC (permalink / raw) To: Eric W. Biederman; +Cc: Greg KH, LKML, Tejun Heo, Serge Hallyn 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sysfs: sysfs_sd_setattr set iattrs unconditionally 2010-02-04 7:13 ` [PATCH] sysfs: sysfs_sd_setattr set iattrs unconditionally Eric W. Biederman 2010-02-04 8:36 ` Jean Delvare @ 2010-02-11 15:33 ` Eric W. Biederman 2010-02-11 15:52 ` Greg KH 1 sibling, 1 reply; 12+ messages in thread From: Eric W. Biederman @ 2010-02-11 15:33 UTC (permalink / raw) To: Greg KH; +Cc: Jean Delvare, LKML, Tejun Heo, Serge Hallyn 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sysfs: sysfs_sd_setattr set iattrs unconditionally 2010-02-11 15:33 ` Eric W. Biederman @ 2010-02-11 15:52 ` Greg KH 2010-02-11 16:05 ` Eric W. Biederman 0 siblings, 1 reply; 12+ messages in thread From: Greg KH @ 2010-02-11 15:52 UTC (permalink / raw) To: Eric W. Biederman; +Cc: Jean Delvare, LKML, Tejun Heo, Serge Hallyn 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sysfs: sysfs_sd_setattr set iattrs unconditionally 2010-02-11 15:52 ` Greg KH @ 2010-02-11 16:05 ` Eric W. Biederman 0 siblings, 0 replies; 12+ messages in thread From: Eric W. Biederman @ 2010-02-11 16:05 UTC (permalink / raw) To: Greg KH; +Cc: Jean Delvare, LKML, Tejun Heo, Serge Hallyn 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] sysfs: sysfs_setattr set iattrs unconditionally 2010-02-03 14:22 ` Greg KH 2010-02-04 7:05 ` Eric W. Biederman 2010-02-04 7:13 ` [PATCH] sysfs: sysfs_sd_setattr set iattrs unconditionally Eric W. Biederman @ 2010-02-04 7:32 ` Eric W. Biederman 2 siblings, 0 replies; 12+ messages in thread From: Eric W. Biederman @ 2010-02-04 7:32 UTC (permalink / raw) To: Greg KH; +Cc: Jean Delvare, LKML, Tejun Heo, Serge Hallyn, stable 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2010-02-11 16:05 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20100120131602.12aa48f7@hyperion.delvare>
2010-01-20 14:01 ` sysfs_chmod_file() broken in 2.6.33-rc4-git6 Eric W. Biederman
2010-01-20 15:00 ` Jean Delvare
2010-01-22 4:19 ` Greg KH
2010-02-03 8:11 ` Jean Delvare
2010-02-03 14:22 ` Greg KH
2010-02-04 7:05 ` Eric W. Biederman
2010-02-04 7:13 ` [PATCH] sysfs: sysfs_sd_setattr set iattrs unconditionally Eric W. Biederman
2010-02-04 8:36 ` Jean Delvare
2010-02-11 15:33 ` Eric W. Biederman
2010-02-11 15:52 ` Greg KH
2010-02-11 16:05 ` Eric W. Biederman
2010-02-04 7:32 ` [PATCH] sysfs: sysfs_setattr " Eric W. Biederman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).