[PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly

[PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly

[Jeff Layton]

This should fix all of the filesystems in the mainline kernels to handle
ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
just a matter of making sure that they call generic_attrkill early in
the setattr inode op.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 arch/powerpc/platforms/cell/spufs/inode.c |    1 +
 fs/9p/vfs_inode.c                         |    1 +
 fs/affs/inode.c                           |    1 +
 fs/afs/inode.c                            |    3 +++
 fs/coda/inode.c                           |    1 +
 fs/configfs/inode.c                       |    4 +++-
 fs/ext2/inode.c                           |    1 +
 fs/ext3/inode.c                           |    5 ++++-
 fs/ext4/inode.c                           |    5 ++++-
 fs/fuse/dir.c                             |    2 ++
 fs/gfs2/ops_inode.c                       |    2 ++
 fs/hostfs/hostfs_kern.c                   |    2 ++
 fs/hpfs/inode.c                           |    1 +
 fs/hugetlbfs/inode.c                      |    5 ++++-
 fs/jffs2/fs.c                             |    1 +
 fs/jfs/acl.c                              |    2 ++
 fs/ocfs2/file.c                           |    2 ++
 fs/proc/base.c                            |    3 +++
 fs/proc/generic.c                         |    3 +++
 fs/proc/proc_sysctl.c                     |    3 +++
 fs/ramfs/file-nommu.c                     |    5 ++++-
 fs/reiserfs/inode.c                       |    6 +++++-
 fs/smbfs/inode.c                          |    2 ++
 fs/sysfs/inode.c                          |    5 ++++-
 fs/ufs/truncate.c                         |    5 ++++-
 fs/xfs/linux-2.6/xfs_iops.c               |    5 ++++-
 mm/shmem.c                                |    2 ++
 27 files changed, 69 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c
index b3d0dd1..66144a1 100644
--- a/arch/powerpc/platforms/cell/spufs/inode.c
+++ b/arch/powerpc/platforms/cell/spufs/inode.c
@@ -97,6 +97,7 @@ spufs_setattr(struct dentry *dentry, struct iattr *attr)
 {
 	struct inode *inode = dentry->d_inode;
 
+	generic_attrkill(inode->i_mode, attr);
 	if ((attr->ia_valid & ATTR_SIZE) &&
 	    (attr->ia_size != inode->i_size))
 		return -EINVAL;
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index e5c45ee..a90ebf1 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -756,6 +756,7 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr)
 		return PTR_ERR(fid);
 
 	v9fs_blank_wstat(&wstat);
+	generic_attrkill(dentry->d_inode->i_mode, iattr);
 	if (iattr->ia_valid & ATTR_MODE)
 		wstat.mode = unixmode2p9mode(v9ses, iattr->ia_mode);
 
diff --git a/fs/affs/inode.c b/fs/affs/inode.c
index 4609a6c..e8dbedf 100644
--- a/fs/affs/inode.c
+++ b/fs/affs/inode.c
@@ -218,6 +218,7 @@ affs_notify_change(struct dentry *dentry, struct iattr *attr)
 
 	pr_debug("AFFS: notify_change(%lu,0x%x)\n",inode->i_ino,attr->ia_valid);
 
+	generic_attrkill(inode->i_mode, attr);
 	error = inode_change_ok(inode,attr);
 	if (error)
 		goto out;
diff --git a/fs/afs/inode.c b/fs/afs/inode.c
index 84750c8..8c43c93 100644
--- a/fs/afs/inode.c
+++ b/fs/afs/inode.c
@@ -371,6 +371,9 @@ int afs_setattr(struct dentry *dentry, struct iattr *attr)
 	       vnode->fid.vid, vnode->fid.vnode, dentry->d_name.name,
 	       attr->ia_valid);
 
+	/* FIXME: is this necessary? */
+	generic_attrkill(dentry->d_inode->i_mode, attr);
+
 	if (!(attr->ia_valid & (ATTR_SIZE | ATTR_MODE | ATTR_UID | ATTR_GID |
 				ATTR_MTIME))) {
 		_leave(" = 0 [unsupported]");
diff --git a/fs/coda/inode.c b/fs/coda/inode.c
index 342f4e0..7dec870 100644
--- a/fs/coda/inode.c
+++ b/fs/coda/inode.c
@@ -239,6 +239,7 @@ int coda_setattr(struct dentry *de, struct iattr *iattr)
 	memset(&vattr, 0, sizeof(vattr)); 
 
 	inode->i_ctime = CURRENT_TIME_SEC;
+	generic_attrkill(inode->i_mode, iattr);
 	coda_iattr_to_vattr(iattr, &vattr);
 	vattr.va_type = C_VNON; /* cannot set type */
 
diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
index dbd257d..d3d6637 100644
--- a/fs/configfs/inode.c
+++ b/fs/configfs/inode.c
@@ -59,7 +59,7 @@ int configfs_setattr(struct dentry * dentry, struct iattr * iattr)
 	struct inode * inode = dentry->d_inode;
 	struct configfs_dirent * sd = dentry->d_fsdata;
 	struct iattr * sd_iattr;
-	unsigned int ia_valid = iattr->ia_valid;
+	unsigned int ia_valid;
 	int error;
 
 	if (!sd)
@@ -67,6 +67,7 @@ int configfs_setattr(struct dentry * dentry, struct iattr * iattr)
 
 	sd_iattr = sd->s_iattr;
 
+	generic_attrkill(inode->i_mode, iattr);
 	error = inode_change_ok(inode, iattr);
 	if (error)
 		return error;
@@ -90,6 +91,7 @@ int configfs_setattr(struct dentry * dentry, struct iattr * iattr)
 
 	/* attributes were changed atleast once in past */
 
+	ia_valid = iattr->ia_valid;
 	if (ia_valid & ATTR_UID)
 		sd_iattr->ia_uid = iattr->ia_uid;
 	if (ia_valid & ATTR_GID)
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index c1dcb68..353472c 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1405,6 +1405,7 @@ int ext2_setattr(struct dentry *dentry, struct iattr *iattr)
 	struct inode *inode = dentry->d_inode;
 	int error;
 
+	generic_attrkill(inode->i_mode, iattr);
 	error = inode_change_ok(inode, iattr);
 	if (error)
 		return error;
diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
index 2d6324a..cabb57b 100644
--- a/fs/ext3/inode.c
+++ b/fs/ext3/inode.c
@@ -2989,7 +2989,10 @@ int ext3_setattr(struct dentry *dentry, struct iattr *attr)
 {
 	struct inode *inode = dentry->d_inode;
 	int error, rc = 0;
-	const unsigned int ia_valid = attr->ia_valid;
+	unsigned int ia_valid;
+
+	generic_attrkill(inode->i_mode, attr);
+	ia_valid = attr->ia_valid;
 
 	error = inode_change_ok(inode, attr);
 	if (error)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index a49bb00..c39f472 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3047,7 +3047,10 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 {
 	struct inode *inode = dentry->d_inode;
 	int error, rc = 0;
-	const unsigned int ia_valid = attr->ia_valid;
+	unsigned int ia_valid;
+
+	generic_attrkill(inode->i_mode, attr);
+	ia_valid = attr->ia_valid;
 
 	error = inode_change_ok(inode, attr);
 	if (error)
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index d1acab9..df6a911 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1005,6 +1005,8 @@ static int fuse_setattr(struct dentry *entry, struct iattr *attr)
 	struct fuse_attr_out outarg;
 	int err;
 
+	generic_attrkill(inode->i_mode, attr);
+
 	if (fc->flags & FUSE_DEFAULT_PERMISSIONS) {
 		err = inode_change_ok(inode, attr);
 		if (err)
diff --git a/fs/gfs2/ops_inode.c b/fs/gfs2/ops_inode.c
index 5b8b994..f0d8532 100644
--- a/fs/gfs2/ops_inode.c
+++ b/fs/gfs2/ops_inode.c
@@ -994,6 +994,8 @@ static int gfs2_setattr(struct dentry *dentry, struct iattr *attr)
 	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
 		goto out;
 
+	generic_attrkill(inode->i_mode, attr);
+
 	error = inode_change_ok(inode, attr);
 	if (error)
 		goto out;
diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 4880256..370f71e 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -821,6 +821,8 @@ int hostfs_setattr(struct dentry *dentry, struct iattr *attr)
 
 	int fd = HOSTFS_I(dentry->d_inode)->fd;
 
+	generic_attrkill(dentry->d_inode->i_mode, attr);
+
 	err = inode_change_ok(dentry->d_inode, attr);
 	if (err)
 		return err;
diff --git a/fs/hpfs/inode.c b/fs/hpfs/inode.c
index 85d3e1d..1387839 100644
--- a/fs/hpfs/inode.c
+++ b/fs/hpfs/inode.c
@@ -265,6 +265,7 @@ int hpfs_notify_change(struct dentry *dentry, struct iattr *attr)
 	struct inode *inode = dentry->d_inode;
 	int error=0;
 	lock_kernel();
+	generic_attrkill(inode->i_mode, attr);
 	if ( ((attr->ia_valid & ATTR_SIZE) && attr->ia_size > inode->i_size) ||
 	     (hpfs_sb(inode->i_sb)->sb_root == inode->i_ino) ) {
 		error = -EINVAL;
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index b35a298..fde0875 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -452,10 +452,13 @@ static int hugetlbfs_setattr(struct dentry *dentry, struct iattr *attr)
 {
 	struct inode *inode = dentry->d_inode;
 	int error;
-	unsigned int ia_valid = attr->ia_valid;
+	unsigned int ia_valid;
 
 	BUG_ON(!inode);
 
+	generic_attrkill(inode->i_mode, attr);
+	ia_valid = attr->ia_valid;
+
 	error = inode_change_ok(inode, attr);
 	if (error)
 		goto out;
diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index 1d3b7a9..597f903 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -37,6 +37,7 @@ static int jffs2_do_setattr (struct inode *inode, struct iattr *iattr)
 	uint32_t alloclen;
 	int ret;
 	D1(printk(KERN_DEBUG "jffs2_setattr(): ino #%lu\n", inode->i_ino));
+	generic_attrkill(inode->i_mode, iattr);
 	ret = inode_change_ok(inode, iattr);
 	if (ret)
 		return ret;
diff --git a/fs/jfs/acl.c b/fs/jfs/acl.c
index 4d84bdc..1630ba0 100644
--- a/fs/jfs/acl.c
+++ b/fs/jfs/acl.c
@@ -227,6 +227,8 @@ int jfs_setattr(struct dentry *dentry, struct iattr *iattr)
 	struct inode *inode = dentry->d_inode;
 	int rc;
 
+	generic_attrkill(inode->i_mode, iattr);
+
 	rc = inode_change_ok(inode, iattr);
 	if (rc)
 		return rc;
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index 0d9f96e..d48c83d 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -989,6 +989,8 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
 	mlog_entry("(0x%p, '%.*s')\n", dentry,
 	           dentry->d_name.len, dentry->d_name.name);
 
+	generic_attrkill(inode->i_mode, attr);
+
 	if (attr->ia_valid & ATTR_MODE)
 		mlog(0, "mode change: %d\n", attr->ia_mode);
 	if (attr->ia_valid & ATTR_UID)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 614851a..3b0f6c1 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -349,6 +349,9 @@ static int proc_setattr(struct dentry *dentry, struct iattr *attr)
 	int error;
 	struct inode *inode = dentry->d_inode;
 
+	/* FIXME: is this necessary? */
+	generic_attrkill(inode->i_mode, attr);
+
 	if (attr->ia_valid & ATTR_MODE)
 		return -EPERM;
 
diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index 1bdb624..1e31ae7 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -239,6 +239,9 @@ static int proc_notify_change(struct dentry *dentry, struct iattr *iattr)
 	struct proc_dir_entry *de = PDE(inode);
 	int error;
 
+	/* FIXME: is this necessary? */
+	generic_attrkill(inode->i_mode, iattr);
+
 	error = inode_change_ok(inode, iattr);
 	if (error)
 		goto out;
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 680c429..7b50364 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -425,6 +425,9 @@ static int proc_sys_setattr(struct dentry *dentry, struct iattr *attr)
 	struct inode *inode = dentry->d_inode;
 	int error;
 
+	/* FIXME: is this necessary */
+	generic_attrkill(inode->i_mode, attr);
+
 	if (attr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
 		return -EPERM;
 
diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
index 0989bc2..a3ff4b3 100644
--- a/fs/ramfs/file-nommu.c
+++ b/fs/ramfs/file-nommu.c
@@ -192,9 +192,12 @@ static int ramfs_nommu_resize(struct inode *inode, loff_t newsize, loff_t size)
 static int ramfs_nommu_setattr(struct dentry *dentry, struct iattr *ia)
 {
 	struct inode *inode = dentry->d_inode;
-	unsigned int old_ia_valid = ia->ia_valid;
+	unsigned int old_ia_valid;
 	int ret = 0;
 
+	generic_attrkill(inode->i_mode, ia);
+	old_ia_valid = ia->ia_valid;
+
 	/* POSIX UID/GID verification for setting inode attributes */
 	ret = inode_change_ok(inode, ia);
 	if (ret)
diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
index 9525f9f..48db0df 100644
--- a/fs/reiserfs/inode.c
+++ b/fs/reiserfs/inode.c
@@ -3062,7 +3062,11 @@ int reiserfs_setattr(struct dentry *dentry, struct iattr *attr)
 {
 	struct inode *inode = dentry->d_inode;
 	int error;
-	unsigned int ia_valid = attr->ia_valid;
+	unsigned int ia_valid;
+
+	generic_attrkill(inode->i_mode, attr);
+	ia_valid = attr->ia_valid;
+
 	reiserfs_write_lock(inode->i_sb);
 	if (attr->ia_valid & ATTR_SIZE) {
 		/* version 2 items will be caught by the s_maxbytes check
diff --git a/fs/smbfs/inode.c b/fs/smbfs/inode.c
index ed851e6..98c4158 100644
--- a/fs/smbfs/inode.c
+++ b/fs/smbfs/inode.c
@@ -682,6 +682,8 @@ smb_notify_change(struct dentry *dentry, struct iattr *attr)
 	if (error)
 		goto out;
 
+	generic_attrkill(inode->i_mode, attr);
+
 	if ((error = inode_change_ok(inode, attr)) < 0)
 		goto out;
 
diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index 782c480..e9a6dfb 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -38,7 +38,7 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
 	struct inode * inode = dentry->d_inode;
 	struct sysfs_dirent * sd = dentry->d_fsdata;
 	struct iattr * sd_iattr;
-	unsigned int ia_valid = iattr->ia_valid;
+	unsigned int ia_valid;
 	int error;
 
 	if (!sd)
@@ -46,6 +46,9 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
 
 	sd_iattr = sd->s_iattr;
 
+	generic_attrkill(inode->i_mode, iattr);
+	ia_valid = iattr->ia_valid;
+
 	error = inode_change_ok(inode, iattr);
 	if (error)
 		return error;
diff --git a/fs/ufs/truncate.c b/fs/ufs/truncate.c
index 311ded3..28cb078 100644
--- a/fs/ufs/truncate.c
+++ b/fs/ufs/truncate.c
@@ -510,9 +510,12 @@ out:
 static int ufs_setattr(struct dentry *dentry, struct iattr *attr)
 {
 	struct inode *inode = dentry->d_inode;
-	unsigned int ia_valid = attr->ia_valid;
+	unsigned int ia_valid;
 	int error;
 
+	generic_attrkill(inode->i_mode, attr);
+	ia_valid = attr->ia_valid;
+
 	error = inode_change_ok(inode, attr);
 	if (error)
 		return error;
diff --git a/fs/xfs/linux-2.6/xfs_iops.c b/fs/xfs/linux-2.6/xfs_iops.c
index e0e06dd..cfbed9e 100644
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -651,12 +651,15 @@ xfs_vn_setattr(
 	struct iattr	*attr)
 {
 	struct inode	*inode = dentry->d_inode;
-	unsigned int	ia_valid = attr->ia_valid;
+	unsigned int	ia_valid;
 	bhv_vnode_t	*vp = vn_from_inode(inode);
 	bhv_vattr_t	vattr = { 0 };
 	int		flags = 0;
 	int		error;
 
+	generic_attrkill(inode->i_mode, attr);
+	ia_valid = attr->ia_valid;
+
 	if (ia_valid & ATTR_UID) {
 		vattr.va_mask |= XFS_AT_UID;
 		vattr.va_uid = attr->ia_uid;
diff --git a/mm/shmem.c b/mm/shmem.c
index 29f8d07..de54808 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -718,6 +718,8 @@ static int shmem_notify_change(struct dentry *dentry, struct iattr *attr)
 	struct page *page = NULL;
 	int error;
 
+	generic_attrkill(inode->i_mode, attr);
+
 	if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
 		if (attr->ia_size < inode->i_size) {
 			/*
-- 
1.5.2.2

Re: [Jfs-discussion] [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly

[Dave Kleikamp]

On Mon, 2007-08-20 at 16:53 -0400, Jeff Layton wrote:
> This should fix all of the filesystems in the mainline kernels to handle
> ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
> just a matter of making sure that they call generic_attrkill early in
> the setattr inode op.

Here's my ack for the jfs piece.

> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
Acked-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>

-- 
David Kleikamp
IBM Linux Technology Center

Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly

[Timothy Shimmin]

Jeff Layton wrote:
> This should fix all of the filesystems in the mainline kernels to handle
> ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
> just a matter of making sure that they call generic_attrkill early in
> the setattr inode op.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/xfs/linux-2.6/xfs_iops.c               |    5 ++++-
> --- a/fs/xfs/linux-2.6/xfs_iops.c
> +++ b/fs/xfs/linux-2.6/xfs_iops.c
> @@ -651,12 +651,15 @@ xfs_vn_setattr(
>  	struct iattr	*attr)
>  {
>  	struct inode	*inode = dentry->d_inode;
> -	unsigned int	ia_valid = attr->ia_valid;
> +	unsigned int	ia_valid;
>  	bhv_vnode_t	*vp = vn_from_inode(inode);
>  	bhv_vattr_t	vattr = { 0 };
>  	int		flags = 0;
>  	int		error;
>  
> +	generic_attrkill(inode->i_mode, attr);
> +	ia_valid = attr->ia_valid;
> +
>  	if (ia_valid & ATTR_UID) {
>  		vattr.va_mask |= XFS_AT_UID;
>  		vattr.va_uid = attr->ia_uid;

Looks reasonable to me for XFS.
Acked-by: Tim Shimmin <tes@sgi.com>

So before, this clearing would happen directly in notify_change()
and now this won't happen until notify_change() calls i_op->setattr
which for a particular fs it can call generic_attrkill() to do it.
So I guess for the cases where i_op->setattr is called outside of
via notify_change, we don't normally have ATTR_KILL_SUID/SGID
set so that nothing will happen there?
I guess just wondering the effect with having the code on all
setattr's. (I'm not familiar with the code path)

--Tim

Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly

[Jeff Layton]

On Tue, 21 Aug 2007 15:35:08 +1000
Timothy Shimmin <tes@sgi.com> wrote:

> Jeff Layton wrote:
> > This should fix all of the filesystems in the mainline kernels to handle
> > ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
> > just a matter of making sure that they call generic_attrkill early in
> > the setattr inode op.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/xfs/linux-2.6/xfs_iops.c               |    5 ++++-
> > --- a/fs/xfs/linux-2.6/xfs_iops.c
> > +++ b/fs/xfs/linux-2.6/xfs_iops.c
> > @@ -651,12 +651,15 @@ xfs_vn_setattr(
> >  	struct iattr	*attr)
> >  {
> >  	struct inode	*inode = dentry->d_inode;
> > -	unsigned int	ia_valid = attr->ia_valid;
> > +	unsigned int	ia_valid;
> >  	bhv_vnode_t	*vp = vn_from_inode(inode);
> >  	bhv_vattr_t	vattr = { 0 };
> >  	int		flags = 0;
> >  	int		error;
> >  
> > +	generic_attrkill(inode->i_mode, attr);
> > +	ia_valid = attr->ia_valid;
> > +
> >  	if (ia_valid & ATTR_UID) {
> >  		vattr.va_mask |= XFS_AT_UID;
> >  		vattr.va_uid = attr->ia_uid;
> 
> Looks reasonable to me for XFS.
> Acked-by: Tim Shimmin <tes@sgi.com>
> 
> So before, this clearing would happen directly in notify_change()
> and now this won't happen until notify_change() calls i_op->setattr
> which for a particular fs it can call generic_attrkill() to do it.
> So I guess for the cases where i_op->setattr is called outside of
> via notify_change, we don't normally have ATTR_KILL_SUID/SGID
> set so that nothing will happen there?

Right. If neither ATTR_KILL bit is set then generic_attrkill is a
noop.

> I guess just wondering the effect with having the code on all
> setattr's. (I'm not familiar with the code path)
> 

These bits are referenced in very few places in the current kernel
tree -- mostly in the VFS layer. The *only* place I see that they
actually get interpreted into a mode change is in notify_change. So
places that call setattr ops w/o going through notify_change are
not likely to have those bits set.

But hypothetically, if a fs did set ATTR_KILL_* and call setattr
directly, then the setattr would now include a mode change that
clears setuid or setgid bits where it may not have before.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly

[Jan Harkes]

On Mon, Aug 20, 2007 at 04:53:22PM -0400, Jeff Layton wrote:
> This should fix all of the filesystems in the mainline kernels to handle
> ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
> just a matter of making sure that they call generic_attrkill early in
> the setattr inode op.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

Coda part looks fine to me. Couldn't test it beyond 'it compiles and
doesn't oops', because our userspace client unconditionally strips
setuid/setgid bits anyways.

Acked-by: Jan Harkes <jaharkes@cs.cmu.edu>

Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly

[Josef Sipek]

On Tue, Aug 21, 2007 at 07:35:51AM -0400, Jeff Layton wrote:
> On Tue, 21 Aug 2007 15:35:08 +1000
> Timothy Shimmin <tes@sgi.com> wrote:
> 
> > Jeff Layton wrote:
> > > This should fix all of the filesystems in the mainline kernels to handle
> > > ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
> > > just a matter of making sure that they call generic_attrkill early in
> > > the setattr inode op.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > ---
> > >  fs/xfs/linux-2.6/xfs_iops.c               |    5 ++++-
> > > --- a/fs/xfs/linux-2.6/xfs_iops.c
> > > +++ b/fs/xfs/linux-2.6/xfs_iops.c
> > > @@ -651,12 +651,15 @@ xfs_vn_setattr(
> > >  	struct iattr	*attr)
> > >  {
> > >  	struct inode	*inode = dentry->d_inode;
> > > -	unsigned int	ia_valid = attr->ia_valid;
> > > +	unsigned int	ia_valid;
> > >  	bhv_vnode_t	*vp = vn_from_inode(inode);
> > >  	bhv_vattr_t	vattr = { 0 };
> > >  	int		flags = 0;
> > >  	int		error;
> > >  
> > > +	generic_attrkill(inode->i_mode, attr);
> > > +	ia_valid = attr->ia_valid;
> > > +
> > >  	if (ia_valid & ATTR_UID) {
> > >  		vattr.va_mask |= XFS_AT_UID;
> > >  		vattr.va_uid = attr->ia_uid;
> > 
> > Looks reasonable to me for XFS.
> > Acked-by: Tim Shimmin <tes@sgi.com>
> > 
> > So before, this clearing would happen directly in notify_change()
> > and now this won't happen until notify_change() calls i_op->setattr
> > which for a particular fs it can call generic_attrkill() to do it.
> > So I guess for the cases where i_op->setattr is called outside of
> > via notify_change, we don't normally have ATTR_KILL_SUID/SGID
> > set so that nothing will happen there?
> 
> Right. If neither ATTR_KILL bit is set then generic_attrkill is a
> noop.
> 
> > I guess just wondering the effect with having the code on all
> > setattr's. (I'm not familiar with the code path)
> > 
> 
> These bits are referenced in very few places in the current kernel
> tree -- mostly in the VFS layer. The *only* place I see that they
> actually get interpreted into a mode change is in notify_change. So
> places that call setattr ops w/o going through notify_change are
> not likely to have those bits set.
> 
> But hypothetically, if a fs did set ATTR_KILL_* and call setattr
> directly, then the setattr would now include a mode change that
> clears setuid or setgid bits where it may not have before.

It almost sounds like an argument for a new inode op (NULL would use
generic_attr_kill).

Josef 'Jeff' Sipek.

-- 
A CRAY is the only computer that runs an endless loop in just 4 hours...

Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly

[Jeff Layton]

On Tue, 21 Aug 2007 17:21:28 -0400
Josef Sipek <jsipek@fsl.cs.sunysb.edu> wrote:

> On Tue, Aug 21, 2007 at 07:35:51AM -0400, Jeff Layton wrote:
> > On Tue, 21 Aug 2007 15:35:08 +1000
> > Timothy Shimmin <tes@sgi.com> wrote:
> > 
> > > Jeff Layton wrote:
> > > > This should fix all of the filesystems in the mainline kernels to handle
> > > > ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
> > > > just a matter of making sure that they call generic_attrkill early in
> > > > the setattr inode op.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > > ---
> > > >  fs/xfs/linux-2.6/xfs_iops.c               |    5 ++++-
> > > > --- a/fs/xfs/linux-2.6/xfs_iops.c
> > > > +++ b/fs/xfs/linux-2.6/xfs_iops.c
> > > > @@ -651,12 +651,15 @@ xfs_vn_setattr(
> > > >  	struct iattr	*attr)
> > > >  {
> > > >  	struct inode	*inode = dentry->d_inode;
> > > > -	unsigned int	ia_valid = attr->ia_valid;
> > > > +	unsigned int	ia_valid;
> > > >  	bhv_vnode_t	*vp = vn_from_inode(inode);
> > > >  	bhv_vattr_t	vattr = { 0 };
> > > >  	int		flags = 0;
> > > >  	int		error;
> > > >  
> > > > +	generic_attrkill(inode->i_mode, attr);
> > > > +	ia_valid = attr->ia_valid;
> > > > +
> > > >  	if (ia_valid & ATTR_UID) {
> > > >  		vattr.va_mask |= XFS_AT_UID;
> > > >  		vattr.va_uid = attr->ia_uid;
> > > 
> > > Looks reasonable to me for XFS.
> > > Acked-by: Tim Shimmin <tes@sgi.com>
> > > 
> > > So before, this clearing would happen directly in notify_change()
> > > and now this won't happen until notify_change() calls i_op->setattr
> > > which for a particular fs it can call generic_attrkill() to do it.
> > > So I guess for the cases where i_op->setattr is called outside of
> > > via notify_change, we don't normally have ATTR_KILL_SUID/SGID
> > > set so that nothing will happen there?
> > 
> > Right. If neither ATTR_KILL bit is set then generic_attrkill is a
> > noop.
> > 
> > > I guess just wondering the effect with having the code on all
> > > setattr's. (I'm not familiar with the code path)
> > > 
> > 
> > These bits are referenced in very few places in the current kernel
> > tree -- mostly in the VFS layer. The *only* place I see that they
> > actually get interpreted into a mode change is in notify_change. So
> > places that call setattr ops w/o going through notify_change are
> > not likely to have those bits set.
> > 
> > But hypothetically, if a fs did set ATTR_KILL_* and call setattr
> > directly, then the setattr would now include a mode change that
> > clears setuid or setgid bits where it may not have before.
> 

I should probably clarify -- in the hypothetical situation above,
the setattr function would have to call generic_attrkill (as most
filesystems should do with this change).

> It almost sounds like an argument for a new inode op (NULL would use
> generic_attr_kill).
> 

That's not a bad idea at all. I suppose that would be easier than
modifying every fs like this, and it does seem like it might be
cleaner. I need to mull it over, but that might be the best
solution.

-- 
Jeff Layton <jlayton@redhat.com>