"Serge E. Hallyn" <serue@xxxxxxxxxx> writes: > Quoting Eric W. Biederman (ebiederm@xxxxxxxxxxxx): >> From: Eric W. Biederman <ebiederm@xxxxxxxxxxxx> >> >> Cleanly separate the work that is specific to setting the >> attributes of a sysfs_dirent from what is needed to update >> the attributes of a vfs inode. >> >> Additionally grab the sysfs_mutex to keep any nasties from >> surprising us when updating the sysfs_dirent. >> >> Acked-by: Tejun Heo <tj@xxxxxxxxxx> >> Signed-off-by: Eric W. Biederman <ebiederm@xxxxxxxxxxxxxxxxxx> >> --- >> fs/sysfs/inode.c | 52 ++++++++++++++++++++++++++++++++-------------------- >> fs/sysfs/sysfs.h | 1 + >> 2 files changed, 33 insertions(+), 20 deletions(-) >> >> diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c >> index fed7a74..fccfb55 100644 >> --- a/fs/sysfs/inode.c >> +++ b/fs/sysfs/inode.c >> @@ -64,30 +64,15 @@ struct sysfs_inode_attrs *sysfs_init_inode_attrs(struct sysfs_dirent *sd) >> >> return attrs; >> } >> -int sysfs_setattr(struct dentry * dentry, struct iattr * iattr) >> + >> +int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr * iattr) >> { >> - struct inode * inode = dentry->d_inode; >> - struct sysfs_dirent * sd = dentry->d_fsdata; >> struct sysfs_inode_attrs *sd_attrs; >> struct iattr *iattrs; >> unsigned int ia_valid = iattr->ia_valid; >> - int error; >> - >> - if (!sd) >> - return -EINVAL; >> >> sd_attrs = sd->s_iattr; >> >> - error = inode_change_ok(inode, iattr); >> - if (error) >> - return error; >> - >> - iattr->ia_valid &= ~ATTR_SIZE; /* ignore size changes */ >> - >> - error = inode_setattr(inode, iattr); >> - if (error) >> - return error; >> - >> if (!sd_attrs) { >> /* setting attributes for the first time, allocate now */ >> sd_attrs = sysfs_init_inode_attrs(sd); >> @@ -110,12 +95,39 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr) >> iattrs->ia_ctime = iattr->ia_ctime; >> 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 0; >> +} >> + >> +int sysfs_setattr(struct dentry * dentry, struct iattr * iattr) >> +{ >> + struct inode * inode = dentry->d_inode; >> + struct sysfs_dirent * sd = dentry->d_fsdata; >> + int error; >> + >> + if (!sd) >> + return -EINVAL; >> + >> + error = inode_change_ok(inode, iattr); >> + if (error) >> + return error; >> + >> + iattr->ia_valid &= ~ATTR_SIZE; /* ignore size changes */ >> + if (iattr->ia_valid & ATTR_MODE) { >> + if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID)) >> + iattr->ia_mode &= ~S_ISGID; >> + } > > Was it a bug that before this patch this wasn't cleared before the > actual inode_setattr()? Not strictly as inode_setattr performs the exact same check. > Since the S_ISGID will be set for the *new* gid, that is, > iattr->ia_gid, shouldn't the user be required to be > in_group_p(iattr->i_gid)? Note you haven't done the > inode_setattr() yet. Interesting point, yes I am potentially testing the wrong gid there. The dangers of moving code around. Thank you for catching that. !#@#!# Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html