Re: sysfs_chmod_file() broken in 2.6.33-rc4-git6
Eric W. Biederman
> 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 <ebie...@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 <t...@kernel.org>
> Acked-by: Serge Hallyn <se...@us.ibm.com>
> Signed-off-by: Eric W. Biederman <ebie...@aristanetworks.com>
> Signed-off-by: Greg Kroah-Hartman <gre...@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;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Jean Delvare
Yes, this fixes my problem. Thanks for the fast fix!
--
Jean Delvare
Greg KH
Great, Eric, care to resend this so that I can submit it for inclusion?
thanks,
greg k-h
Jean Delvare
Ping Eric, the patch is still not in Linus' tree.
--
Jean Delvare
Greg KH
Nor mine :)
Eric, care to send it to me?
thanks,
greg k-h
Eric W. Biederman
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
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 <ebie...@xmission.com>
---
fs/sysfs/inode.c | 35 +++++++++++++++++------------------
1 files changed, 17 insertions(+), 18 deletions(-)
--
1.6.5.2.143.g8cc62
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 <ebie...@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
Jean Delvare
>
> 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 <ebie...@xmission.com>
Tested-by: Jean Delvare <kh...@linux-fr.org>
--
Jean Delvare
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
Greg KH
>
> 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
Eric W. Biederman
> 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