[patch 00/15] VFS fixes and cleanups

[patch 00/15] VFS fixes and cleanups

[Miklos Szeredi]

These are VFS/filesystem fix and cleanup patches in preparation for
the path_* API and AppArmor.

If there are no objections, please apply.

Thanks,
Miklos

--

[patch 01/15] ecryptfs: clean up (un)lock_parent

[Miklos Szeredi]

[-- Attachment #1: ecryptfs_cleanup.patch --]
[-- Type: text/plain, Size: 1795 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

dget(dentry->d_parent) --> dget_parent(dentry)

unlock_parent() is racy and unnecessary.  Replace single caller with
unlock_dir().

There are several other suspect uses of ->d_parent in ecryptfs...

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: Michael Halcrow <mhalcrow@us.ibm.com>
---
 fs/ecryptfs/inode.c |   13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

Index: linux-2.6/fs/ecryptfs/inode.c
===================================================================
--- linux-2.6.orig/fs/ecryptfs/inode.c	2008-05-05 11:29:21.000000000 +0200
+++ linux-2.6/fs/ecryptfs/inode.c	2008-05-05 11:29:24.000000000 +0200
@@ -37,17 +37,11 @@ static struct dentry *lock_parent(struct
 {
 	struct dentry *dir;
 
-	dir = dget(dentry->d_parent);
+	dir = dget_parent(dentry);
 	mutex_lock_nested(&(dir->d_inode->i_mutex), I_MUTEX_PARENT);
 	return dir;
 }
 
-static void unlock_parent(struct dentry *dentry)
-{
-	mutex_unlock(&(dentry->d_parent->d_inode->i_mutex));
-	dput(dentry->d_parent);
-}
-
 static void unlock_dir(struct dentry *dir)
 {
 	mutex_unlock(&dir->d_inode->i_mutex);
@@ -426,8 +420,9 @@ static int ecryptfs_unlink(struct inode 
 	int rc = 0;
 	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
 	struct inode *lower_dir_inode = ecryptfs_inode_to_lower(dir);
+	struct dentry *lower_dir_dentry;
 
-	lock_parent(lower_dentry);
+	lower_dir_dentry = lock_parent(lower_dentry);
 	rc = vfs_unlink(lower_dir_inode, lower_dentry);
 	if (rc) {
 		printk(KERN_ERR "Error in vfs_unlink; rc = [%d]\n", rc);
@@ -439,7 +434,7 @@ static int ecryptfs_unlink(struct inode 
 	dentry->d_inode->i_ctime = dir->i_ctime;
 	d_drop(dentry);
 out_unlock:
-	unlock_parent(lower_dentry);
+	unlock_dir(lower_dir_dentry);
 	return rc;
 }
 

--

Re: [patch 01/15] ecryptfs: clean up (un)lock_parent

[Christoph Hellwig]

On Mon, May 05, 2008 at 11:54:42AM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> dget(dentry->d_parent) --> dget_parent(dentry)
> 
> unlock_parent() is racy and unnecessary.  Replace single caller with
> unlock_dir().
> 
> There are several other suspect uses of ->d_parent in ecryptfs...

Looks good an the first hunk is an obvious fix.  Should probably go into
.26

[patch 02/15] nfsd: clean up mnt_want_write calls

[Miklos Szeredi]

[-- Attachment #1: nfsd_create_cleanup.patch --]
[-- Type: text/plain, Size: 1982 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Multiple mnt_want_write() calls in the switch statement looks really
ugly.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/nfsd/vfs.c |   25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

Index: linux-2.6/fs/nfsd/vfs.c
===================================================================
--- linux-2.6.orig/fs/nfsd/vfs.c	2008-05-05 11:29:21.000000000 +0200
+++ linux-2.6/fs/nfsd/vfs.c	2008-05-05 11:29:24.000000000 +0200
@@ -1248,36 +1248,34 @@ nfsd_create(struct svc_rqst *rqstp, stru
 		iap->ia_mode = 0;
 	iap->ia_mode = (iap->ia_mode & S_IALLUGO) | type;
 
+	err = nfserr_inval;
+	if (!S_ISREG(type) && !S_ISDIR(type) && !special_file(type)) {
+		printk(KERN_WARNING "nfsd: bad file type %o in nfsd_create\n",
+		       type);
+		goto out;
+	}
+
+	host_err = mnt_want_write(fhp->fh_export->ex_path.mnt);
+	if (host_err)
+		goto out_nfserr;
+
 	/*
 	 * Get the dir op function pointer.
 	 */
 	err = 0;
 	switch (type) {
 	case S_IFREG:
-		host_err = mnt_want_write(fhp->fh_export->ex_path.mnt);
-		if (host_err)
-			goto out_nfserr;
 		host_err = vfs_create(dirp, dchild, iap->ia_mode, NULL);
 		break;
 	case S_IFDIR:
-		host_err = mnt_want_write(fhp->fh_export->ex_path.mnt);
-		if (host_err)
-			goto out_nfserr;
 		host_err = vfs_mkdir(dirp, dchild, iap->ia_mode);
 		break;
 	case S_IFCHR:
 	case S_IFBLK:
 	case S_IFIFO:
 	case S_IFSOCK:
-		host_err = mnt_want_write(fhp->fh_export->ex_path.mnt);
-		if (host_err)
-			goto out_nfserr;
 		host_err = vfs_mknod(dirp, dchild, iap->ia_mode, rdev);
 		break;
-	default:
-	        printk("nfsd: bad file type %o in nfsd_create\n", type);
-		host_err = -EINVAL;
-		goto out_nfserr;
 	}
 	if (host_err < 0) {
 		mnt_drop_write(fhp->fh_export->ex_path.mnt);
@@ -1289,7 +1287,6 @@ nfsd_create(struct svc_rqst *rqstp, stru
 		write_inode_now(dchild->d_inode, 1);
 	}
 
-
 	err2 = nfsd_create_setattr(rqstp, resfhp, iap);
 	if (err2)
 		err = err2;

--

Re: [patch 02/15] nfsd: clean up mnt_want_write calls

[Christoph Hellwig]

On Mon, May 05, 2008 at 11:54:43AM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> Multiple mnt_want_write() calls in the switch statement looks really
> ugly.

agreed.  nice cleanup.

[patch 03/15] cgroup: dont call vfs_mkdir

[Miklos Szeredi]

[-- Attachment #1: cgroup_mkdir_cleanup.patch --]
[-- Type: text/plain, Size: 1561 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

cgroup_clone() calls vfs_mkdir() to create a directory in the cgroup
filesystem.  Replace with explicit call to cgroup_mkdir() and
fsnotify_mkdir().

This is equivalent, except that the following functions are not called
before cgroup_mkdir():

 - may_create()
 - security_inode_mkdir()
 - DQUOT_INIT()

Permission to clone the cgroup has already been checked in
copy_namespaces() (requiring CAP_SYS_ADMIN).  Additional file system
related capability checks are inappropriate and confusing.

The quota check is unnecessary, as quotas don't make any sense for
this filesystem.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: Paul Menage <menage@google.com>
---
 kernel/cgroup.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: linux-2.6/kernel/cgroup.c
===================================================================
--- linux-2.6.orig/kernel/cgroup.c	2008-05-05 11:29:21.000000000 +0200
+++ linux-2.6/kernel/cgroup.c	2008-05-05 11:29:25.000000000 +0200
@@ -25,6 +25,7 @@
 #include <linux/cgroup.h>
 #include <linux/errno.h>
 #include <linux/fs.h>
+#include <linux/fsnotify.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/mm.h>
@@ -2928,7 +2929,9 @@ int cgroup_clone(struct task_struct *tsk
 	}
 
 	/* Create the cgroup directory, which also creates the cgroup */
-	ret = vfs_mkdir(inode, dentry, S_IFDIR | 0755);
+	ret = cgroup_mkdir(inode, dentry, S_IFDIR);
+	if (!ret)
+		fsnotify_mkdir(inode, dentry);
 	child = __d_cgrp(dentry);
 	dput(dentry);
 	if (ret) {

--

Re: [patch 03/15] cgroup: dont call vfs_mkdir

[Christoph Hellwig]

On Mon, May 05, 2008 at 11:54:44AM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> cgroup_clone() calls vfs_mkdir() to create a directory in the cgroup
> filesystem.  Replace with explicit call to cgroup_mkdir() and
> fsnotify_mkdir().
> 
> This is equivalent, except that the following functions are not called
> before cgroup_mkdir():
> 
>  - may_create()
>  - security_inode_mkdir()
>  - DQUOT_INIT()
> 
> Permission to clone the cgroup has already been checked in
> copy_namespaces() (requiring CAP_SYS_ADMIN).  Additional file system
> related capability checks are inappropriate and confusing.
> 
> The quota check is unnecessary, as quotas don't make any sense for
> this filesystem.

Looks correct but I don't think it's a good idea.  Spreading more logic
into filesystems without a good reason is rarely a good idea.

Re: [patch 03/15] cgroup: dont call vfs_mkdir

[Miklos Szeredi]

> > From: Miklos Szeredi <mszeredi@suse.cz>
> > 
> > cgroup_clone() calls vfs_mkdir() to create a directory in the cgroup
> > filesystem.  Replace with explicit call to cgroup_mkdir() and
> > fsnotify_mkdir().
> > 
> > This is equivalent, except that the following functions are not called
> > before cgroup_mkdir():
> > 
> >  - may_create()
> >  - security_inode_mkdir()
> >  - DQUOT_INIT()
> > 
> > Permission to clone the cgroup has already been checked in
> > copy_namespaces() (requiring CAP_SYS_ADMIN).  Additional file system
> > related capability checks are inappropriate and confusing.
> > 
> > The quota check is unnecessary, as quotas don't make any sense for
> > this filesystem.
> 
> Looks correct but I don't think it's a good idea.  Spreading more logic
> into filesystems without a good reason is rarely a good idea.

(Thanks for the review, Christoph)

Agreed completely, but vfs_* aren't for filesystems to call, rather
for entities calling _into_ filesystems from the outside.  This is
actually a very rare thing, so adding some extra logic for the sake of
cleanliness should be OK.

Now it can be argued, that cgroup_clone() is calling into the
filesystem from the outside.  But it's not really doing that, rather
it's making an internal modification to a specific filesystem,
triggered by some external action.

Miklos

Re: [patch 03/15] cgroup: dont call vfs_mkdir

[Christoph Hellwig]

On Mon, May 05, 2008 at 02:32:22PM +0200, Miklos Szeredi wrote:
> > Looks correct but I don't think it's a good idea.  Spreading more logic
> > into filesystems without a good reason is rarely a good idea.
> 
> (Thanks for the review, Christoph)
> 
> Agreed completely, but vfs_* aren't for filesystems to call, rather
> for entities calling _into_ filesystems from the outside.  This is
> actually a very rare thing, so adding some extra logic for the sake of
> cleanliness should be OK.
> 
> Now it can be argued, that cgroup_clone() is calling into the
> filesystem from the outside.  But it's not really doing that, rather
> it's making an internal modification to a specific filesystem,
> triggered by some external action.

I don't think that matters. We're not about overly strict layering, and
especialy this kind where you call into a higher layer to get back into
the lower one is not harmful at all.  For cgroup it's only a small
duplication, but e.g. I don't really like all the duplications in the
reiserfs case.  Unless we have a very good reason why the useage of the
vfs_ function should go away from the filesystem code I don't think
we want this.

Re: [patch 03/15] cgroup: dont call vfs_mkdir

[Miklos Szeredi]

> > > Looks correct but I don't think it's a good idea.  Spreading more logic
> > > into filesystems without a good reason is rarely a good idea.
> > 
> > (Thanks for the review, Christoph)
> > 
> > Agreed completely, but vfs_* aren't for filesystems to call, rather
> > for entities calling _into_ filesystems from the outside.  This is
> > actually a very rare thing, so adding some extra logic for the sake of
> > cleanliness should be OK.
> > 
> > Now it can be argued, that cgroup_clone() is calling into the
> > filesystem from the outside.  But it's not really doing that, rather
> > it's making an internal modification to a specific filesystem,
> > triggered by some external action.
> 
> I don't think that matters. We're not about overly strict layering, and
> especialy this kind where you call into a higher layer to get back into
> the lower one is not harmful at all.  For cgroup it's only a small
> duplication, but e.g. I don't really like all the duplications in the
> reiserfs case.

I think there's some good reasons, other than just to get rid of the
vfs recursion.  I took this change from Jeff Mahoney's patchset.

>  Unless we have a very good reason why the useage of the
> vfs_ function should go away from the filesystem code I don't think
> we want this.

We do have a good reason: r/o bind mounts and AppArmor.  And please
don't tell me, you also think that moving the security hooks to
callers is a good idea ;)  That would actually be a change with a much
larger impact, both in terms of code duplication and of verifying
correctness.

Miklos

Re: [patch 03/15] cgroup: dont call vfs_mkdir

[Christoph Hellwig]

On Mon, May 05, 2008 at 03:29:38PM +0200, Miklos Szeredi wrote:
> We do have a good reason: r/o bind mounts and AppArmor.  And please
> don't tell me, you also think that moving the security hooks to
> callers is a good idea ;)  That would actually be a change with a much
> larger impact, both in terms of code duplication and of verifying
> correctness.

I think AppArmor is a horribly stupid idea to start with, and I'm not
willing up to mess up the kernel tree for it.

Re: [patch 03/15] cgroup: dont call vfs_mkdir

[Miklos Szeredi]

> > We do have a good reason: r/o bind mounts and AppArmor.  And please
> > don't tell me, you also think that moving the security hooks to
> > callers is a good idea ;)  That would actually be a change with a much
> > larger impact, both in terms of code duplication and of verifying
> > correctness.
> 
> I think AppArmor is a horribly stupid idea to start with, and I'm not
> willing up to mess up the kernel tree for it.

Well, I'd hardly call this whole patchset messing up the kernel, and
you seemed to agree with most of the patches.

It may come as a surprise, but all of it (as well as reviews of the
r/o bind patches which found some pretty serious bugs) are because I'm
trying to work on integrating AppArmor cleanly.

You may consider it a stupid idea, and it may or may not be one.  But
please try to be a bit more constructive on the issue.

Thanks,
Miklos

Re: [patch 03/15] cgroup: dont call vfs_mkdir

[Christoph Hellwig]

On Mon, May 05, 2008 at 03:43:37PM +0200, Miklos Szeredi wrote:
> Well, I'd hardly call this whole patchset messing up the kernel, and
> you seemed to agree with most of the patches.

Of course.  Good patches will always again, but preparing for apparmor
is not an excuse for bad patches.

Re: [patch 03/15] cgroup: dont call vfs_mkdir

[Miklos Szeredi]

> > Well, I'd hardly call this whole patchset messing up the kernel, and
> > you seemed to agree with most of the patches.
> 
> Of course.  Good patches will always again, but preparing for apparmor
> is not an excuse for bad patches.

It's not an excuse.  It's one of *several* reasons for why the patches
are good.  And no, "help *cleanly* integrate AA" is not a reason
against patches.

Please try to keep your arguments technical (like code duplication in
reiserfs, etc), which are perfectly valid.  I don't like the reiserfs
duplication, but as I've said, it's needed anyway for some other
reason.  I can dig out the original patch and include the description
in this one if that helps.

And yes, all these are cleanups.  Recursing into the vfs_ functions
from inside the filesystems is plain wrong most of the time.

For example fsnotify_change() call in fat_generic_ioctl() would
actually *break* after the IS_IMMUTABLE() cleanups, because it's
actually calling this function before changing the "SYS" attribute
(which is interpreted as immutable) on the inode.  So without this
cleanup that notify_change() would always fail.

Miklos

Re: [patch 03/15] cgroup: dont call vfs_mkdir

[Paul Menage]

On Mon, May 5, 2008 at 2:54 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
>
>  cgroup_clone() calls vfs_mkdir() to create a directory in the cgroup
>  filesystem.  Replace with explicit call to cgroup_mkdir() and
>  fsnotify_mkdir().
>
>  This is equivalent, except that the following functions are not called
>  before cgroup_mkdir():
>
>   - may_create()
>   - security_inode_mkdir()
>   - DQUOT_INIT()
>
>  Permission to clone the cgroup has already been checked in
>  copy_namespaces() (requiring CAP_SYS_ADMIN).  Additional file system
>  related capability checks are inappropriate and confusing.
>
>  The quota check is unnecessary, as quotas don't make any sense for
>  this filesystem.
>
>  Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
>  CC: Paul Menage <menage@google.com>

This looks like it behaves correctly, but I don't really have any view
on whether the change is the right thing to do - I'll leave that to
the VFS gurus. FWIW, I'd regard cgroup_clone() as being outside the
filesystem rather than inside. It does have some knowledge of the
cgroupfs internals, but it tries to leave as much as possible up to
the real filesystem code.

Paul

[patch 04/15] reiserfs: dont call vfs_rmdir

[Miklos Szeredi]

[-- Attachment #1: reiserfs_rmdir_cleanup.patch --]
[-- Type: text/plain, Size: 3375 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

reiserfs_delete_xattrs() calls vfs_rmdir() to remove the private
directory associated with the extended attributes for an inode.
Replace with explicit call to reiserfs_rmdir().

The extended attribute files reside in a private directory, completely
inaccessible from userspace.  This means, that checking against mounts
on the dentry is not necessary.

The calls to may_delete() and security_inode_rmdir(), as performed by
vfs_rmdir() are also unecessary for this case.

Based on patch by Jeff Mahoney.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
Acked-by: Jeff Mahoney <jeffm@suse.de>
---
 fs/reiserfs/namei.c         |    2 +-
 fs/reiserfs/xattr.c         |   22 +++++++++++++++++++++-
 include/linux/reiserfs_fs.h |    1 +
 3 files changed, 23 insertions(+), 2 deletions(-)

Index: linux-2.6/fs/reiserfs/xattr.c
===================================================================
--- linux-2.6.orig/fs/reiserfs/xattr.c	2008-05-05 11:29:21.000000000 +0200
+++ linux-2.6/fs/reiserfs/xattr.c	2008-05-05 11:29:25.000000000 +0200
@@ -35,6 +35,7 @@
 #include <linux/namei.h>
 #include <linux/errno.h>
 #include <linux/fs.h>
+#include <linux/quotaops.h>
 #include <linux/file.h>
 #include <linux/pagemap.h>
 #include <linux/xattr.h>
@@ -712,6 +713,25 @@ reiserfs_delete_xattrs_filler(void *buf,
 
 }
 
+static int xattr_rmdir(struct inode *dir, struct dentry *dentry)
+{
+	int error;
+
+	DQUOT_INIT(dir);
+
+	mutex_lock_nested(&dentry->d_inode->i_mutex, I_MUTEX_CHILD);
+	dentry_unhash(dentry);
+	error = reiserfs_rmdir(dir, dentry);
+	if (!error)
+		dentry->d_inode->i_flags |= S_DEAD;
+	mutex_unlock(&dentry->d_inode->i_mutex);
+	if (!error)
+		d_delete(dentry);
+	dput(dentry);
+
+	return error;
+}
+
 /* This is called w/ inode->i_mutex downed */
 int reiserfs_delete_xattrs(struct inode *inode)
 {
@@ -746,7 +766,7 @@ int reiserfs_delete_xattrs(struct inode 
 	if (dir->d_inode->i_nlink <= 2) {
 		root = get_xa_root(inode->i_sb, XATTR_REPLACE);
 		reiserfs_write_lock_xattrs(inode->i_sb);
-		err = vfs_rmdir(root->d_inode, dir);
+		err = xattr_rmdir(root->d_inode, dir);
 		reiserfs_write_unlock_xattrs(inode->i_sb);
 		dput(root);
 	} else {
Index: linux-2.6/fs/reiserfs/namei.c
===================================================================
--- linux-2.6.orig/fs/reiserfs/namei.c	2008-05-05 11:29:21.000000000 +0200
+++ linux-2.6/fs/reiserfs/namei.c	2008-05-05 11:29:25.000000000 +0200
@@ -850,7 +850,7 @@ static inline int reiserfs_empty_dir(str
 	return 1;
 }
 
-static int reiserfs_rmdir(struct inode *dir, struct dentry *dentry)
+int reiserfs_rmdir(struct inode *dir, struct dentry *dentry)
 {
 	int retval, err;
 	struct inode *inode;
Index: linux-2.6/include/linux/reiserfs_fs.h
===================================================================
--- linux-2.6.orig/include/linux/reiserfs_fs.h	2008-05-05 11:29:21.000000000 +0200
+++ linux-2.6/include/linux/reiserfs_fs.h	2008-05-05 11:29:25.000000000 +0200
@@ -1911,6 +1911,7 @@ static inline void reiserfs_update_sd(st
 void sd_attrs_to_i_attrs(__u16 sd_attrs, struct inode *inode);
 void i_attrs_to_sd_attrs(struct inode *inode, __u16 * sd_attrs);
 int reiserfs_setattr(struct dentry *dentry, struct iattr *attr);
+int reiserfs_rmdir(struct inode *dir, struct dentry *dentry);
 
 /* namei.c */
 void set_de_name_and_namelen(struct reiserfs_dir_entry *de);

--

[patch 05/15] reiserfs: dont call notify_change

[Miklos Szeredi]

[-- Attachment #1: reiserfs_notify_change_cleanup.patch --]
[-- Type: text/plain, Size: 2128 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Reiserfs calls notify_change() on it's private extended attribute
files to resize and change ownership of these files.  Replace with
reiserfs_setattr().

This is equivalent, except that:

 - i_alloc_sem locking is not performed around reiserfs_setattr() when
   the size is changed
 - security_inode_setattr() is not called before reiserfs_setattr()
 - fsnotify_change() is not called after reiserfs_setattr()

None of the above is necessary, since the files are private to
reiserfs and inaccessible from userspace.

Also remove setting of ctime on the xattr files.

Based on patch by Jeff Mahoney.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
Acked-by: Jeff Mahoney <jeffm@suse.de>
---
 fs/reiserfs/xattr.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Index: linux-2.6/fs/reiserfs/xattr.c
===================================================================
--- linux-2.6.orig/fs/reiserfs/xattr.c	2008-05-05 11:29:25.000000000 +0200
+++ linux-2.6/fs/reiserfs/xattr.c	2008-05-05 11:29:25.000000000 +0200
@@ -458,9 +458,9 @@ reiserfs_xattr_set(struct inode *inode, 
 
 	/* Resize it so we're ok to write there */
 	newattrs.ia_size = buffer_size;
-	newattrs.ia_valid = ATTR_SIZE | ATTR_CTIME;
+	newattrs.ia_valid = ATTR_SIZE;
 	mutex_lock_nested(&xinode->i_mutex, I_MUTEX_XATTR);
-	err = notify_change(dentry, &newattrs);
+	err = reiserfs_setattr(dentry, &newattrs);
 	if (err)
 		goto out_filp;
 
@@ -810,7 +810,7 @@ reiserfs_chown_xattrs_filler(void *buf, 
 	}
 
 	if (!S_ISDIR(xafile->d_inode->i_mode))
-		err = notify_change(xafile, attrs);
+		err = reiserfs_setattr(xafile, attrs);
 	dput(xafile);
 
 	return err;
@@ -843,7 +843,7 @@ int reiserfs_chown_xattrs(struct inode *
 
 	lock_kernel();
 
-	attrs->ia_valid &= (ATTR_UID | ATTR_GID | ATTR_CTIME);
+	attrs->ia_valid &= (ATTR_UID | ATTR_GID);
 	buf.xadir = dir;
 	buf.attrs = attrs;
 	buf.inode = inode;
@@ -854,7 +854,7 @@ int reiserfs_chown_xattrs(struct inode *
 		goto out_dir;
 	}
 
-	err = notify_change(dir, attrs);
+	err = reiserfs_setattr(dir, attrs);
 	unlock_kernel();
 
       out_dir:

--

[patch 06/15] sysfs: dont call notify_change

[Miklos Szeredi]

[-- Attachment #1: sysfs_notify_change_cleanup.patch --]
[-- Type: text/plain, Size: 1424 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

sysfs_chmod_file() calls notify_change() to change the permission bits
on a sysfs file.  Replace with explicit call to sysfs_setattr() and
fsnotify_change().

This is equivalent, except that security_inode_setattr() is not
called.  This function is called by drivers, so the security checks do
not make any sense.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: Greg Kroah-Hartman <gregkh@suse.de>
---
 fs/sysfs/file.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: linux-2.6/fs/sysfs/file.c
===================================================================
--- linux-2.6.orig/fs/sysfs/file.c	2008-05-05 11:29:21.000000000 +0200
+++ linux-2.6/fs/sysfs/file.c	2008-05-05 11:29:26.000000000 +0200
@@ -18,6 +18,7 @@
 #include <linux/poll.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
+#include <linux/fsnotify.h>
 #include <asm/uaccess.h>
 
 #include "sysfs.h"
@@ -585,9 +586,11 @@ int sysfs_chmod_file(struct kobject *kob
 
 	newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
 	newattrs.ia_valid = ATTR_MODE | ATTR_CTIME;
-	rc = notify_change(victim, &newattrs);
+	newattrs.ia_ctime = current_fs_time(inode->i_sb);
+	rc = sysfs_setattr(victim, &newattrs);
 
 	if (rc == 0) {
+		fsnotify_change(victim, newattrs.ia_valid);
 		mutex_lock(&sysfs_mutex);
 		victim_sd->s_mode = newattrs.ia_mode;
 		mutex_unlock(&sysfs_mutex);

--

Re: [patch 06/15] sysfs: dont call notify_change

[Greg KH]

On Mon, May 05, 2008 at 11:54:47AM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> sysfs_chmod_file() calls notify_change() to change the permission bits
> on a sysfs file.  Replace with explicit call to sysfs_setattr() and
> fsnotify_change().
> 
> This is equivalent, except that security_inode_setattr() is not
> called.  This function is called by drivers, so the security checks do
> not make any sense.

Are you sure?  As a user, you can chmod the sysfs file and it will
stick, I thought that is what this function was accomplishing.  You will
want to call the security checks in those cases, right?

thanks,

greg k-h

Re: [patch 06/15] sysfs: dont call notify_change

[Miklos Szeredi]

> > sysfs_chmod_file() calls notify_change() to change the permission bits
> > on a sysfs file.  Replace with explicit call to sysfs_setattr() and
> > fsnotify_change().
> > 
> > This is equivalent, except that security_inode_setattr() is not
> > called.  This function is called by drivers, so the security checks do
> > not make any sense.
> 
> Are you sure?  As a user, you can chmod the sysfs file and it will
> stick,

Right, but that's not sysfs_chmod_file() but sys_chmod(), which calls
notify_change(), which calls security_inode_setattr().

sysfs_chmod_file() is just called by a couple of drivers to change the
file mode during operation, it's never called by user action directly.

Miklos

Re: [patch 06/15] sysfs: dont call notify_change

[Greg KH]

On Tue, May 06, 2008 at 08:28:39AM +0200, Miklos Szeredi wrote:
> > > sysfs_chmod_file() calls notify_change() to change the permission bits
> > > on a sysfs file.  Replace with explicit call to sysfs_setattr() and
> > > fsnotify_change().
> > > 
> > > This is equivalent, except that security_inode_setattr() is not
> > > called.  This function is called by drivers, so the security checks do
> > > not make any sense.
> > 
> > Are you sure?  As a user, you can chmod the sysfs file and it will
> > stick,
> 
> Right, but that's not sysfs_chmod_file() but sys_chmod(), which calls
> notify_change(), which calls security_inode_setattr().
> 
> sysfs_chmod_file() is just called by a couple of drivers to change the
> file mode during operation, it's never called by user action directly.

Ah, sorry, you are correct, no objection from me for this patch:
	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

thanks,

greg k-h

[patch 07/15] hpfs: dont call notify_change

[Miklos Szeredi]

[-- Attachment #1: hpfs_notify_change_cleanup.patch --]
[-- Type: text/plain, Size: 1291 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

hpfs_unlink() calls notify_change() to truncate the file before
deleting.  Replace with explicit call to hpfs_notify_change().

This is equivalent, except that:
 - security_inode_setattr() is not called before hpfs_notify_change()
 - fsnotify_change() is not called after hpfs_notify_change()

The truncation is just an implementation detail, so both the security
check and the notification are unnecessary.

Possibly even the ctime modification is wrong?

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: Mikulas Patocka <mikulas@artax.karlin.mff.cuni.cz>
---
 fs/hpfs/namei.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6/fs/hpfs/namei.c
===================================================================
--- linux-2.6.orig/fs/hpfs/namei.c	2008-05-05 11:29:20.000000000 +0200
+++ linux-2.6/fs/hpfs/namei.c	2008-05-05 11:29:26.000000000 +0200
@@ -426,7 +426,8 @@ again:
 			/*printk("HPFS: truncating file before delete.\n");*/
 			newattrs.ia_size = 0;
 			newattrs.ia_valid = ATTR_SIZE | ATTR_CTIME;
-			err = notify_change(dentry, &newattrs);
+			newattrs.ia_ctime = current_fs_time(inode->i_sb);
+			err = hpfs_notify_change(dentry, &newattrs);
 			put_write_access(inode);
 			if (!err)
 				goto again;

--

Re: [patch 07/15] hpfs: dont call notify_change

[Mikulas Patocka]

On Mon, 5 May 2008, Miklos Szeredi wrote:

> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> hpfs_unlink() calls notify_change() to truncate the file before
> deleting.  Replace with explicit call to hpfs_notify_change().
> 
> This is equivalent, except that:
>  - security_inode_setattr() is not called before hpfs_notify_change()
>  - fsnotify_change() is not called after hpfs_notify_change()
> 
> The truncation is just an implementation detail, so both the security
> check and the notification are unnecessary.
> 
> Possibly even the ctime modification is wrong?

Ok, commit it.

Signed-off-by: Mikulas Patocka <mikulas@artax.karlin.mff.cuni.cz>

> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> CC: Mikulas Patocka <mikulas@artax.karlin.mff.cuni.cz>
> ---
>  fs/hpfs/namei.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/fs/hpfs/namei.c
> ===================================================================
> --- linux-2.6.orig/fs/hpfs/namei.c	2008-05-05 11:29:20.000000000 +0200
> +++ linux-2.6/fs/hpfs/namei.c	2008-05-05 11:29:26.000000000 +0200
> @@ -426,7 +426,8 @@ again:
>  			/*printk("HPFS: truncating file before delete.\n");*/
>  			newattrs.ia_size = 0;
>  			newattrs.ia_valid = ATTR_SIZE | ATTR_CTIME;
> -			err = notify_change(dentry, &newattrs);
> +			newattrs.ia_ctime = current_fs_time(inode->i_sb);
> +			err = hpfs_notify_change(dentry, &newattrs);
>  			put_write_access(inode);
>  			if (!err)
>  				goto again;
> 
> --
> 

[patch 08/15] fat: dont call notify_change

[Miklos Szeredi]

[-- Attachment #1: fat_notify_change_cleanup.patch --]
[-- Type: text/plain, Size: 1749 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

The FAT_IOCTL_SET_ATTRIBUTES ioctl() calls notify_change() to change
the file mode before changing the inode attributes.  Replace with
explicit call to fat_setattr().

This is equivalent, except that security_inode_setattr() is not called
before fat_setattr().  I think this is not needed, since the mode
change is just a side effect of the attribute change.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---
 fs/fat/file.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: linux-2.6/fs/fat/file.c
===================================================================
--- linux-2.6.orig/fs/fat/file.c	2008-05-05 11:29:20.000000000 +0200
+++ linux-2.6/fs/fat/file.c	2008-05-05 11:29:26.000000000 +0200
@@ -16,6 +16,7 @@
 #include <linux/writeback.h>
 #include <linux/backing-dev.h>
 #include <linux/blkdev.h>
+#include <linux/fsnotify.h>
 
 int fat_generic_ioctl(struct inode *inode, struct file *filp,
 		      unsigned int cmd, unsigned long arg)
@@ -65,6 +66,7 @@ int fat_generic_ioctl(struct inode *inod
 
 		/* Equivalent to a chmod() */
 		ia.ia_valid = ATTR_MODE | ATTR_CTIME;
+		ia.ia_ctime = current_fs_time(inode->i_sb);
 		if (is_dir) {
 			ia.ia_mode = MSDOS_MKMODE(attr,
 				S_IRWXUGO & ~sbi->options.fs_dmask)
@@ -92,10 +94,11 @@ int fat_generic_ioctl(struct inode *inod
 		}
 
 		/* This MUST be done before doing anything irreversible... */
-		err = notify_change(filp->f_path.dentry, &ia);
+		err = fat_setattr(filp->f_path.dentry, &ia);
 		if (err)
 			goto up;
 
+		fsnotify_change(filp->f_path.dentry, ia.ia_valid);
 		if (sbi->options.sys_immutable) {
 			if (attr & ATTR_SYS)
 				inode->i_flags |= S_IMMUTABLE;

--

Re: [patch 08/15] fat: dont call notify_change

[OGAWA Hirofumi]

Miklos Szeredi <miklos@szeredi.hu> writes:

> From: Miklos Szeredi <mszeredi@suse.cz>
>
> The FAT_IOCTL_SET_ATTRIBUTES ioctl() calls notify_change() to change
> the file mode before changing the inode attributes.  Replace with
> explicit call to fat_setattr().
>
> This is equivalent, except that security_inode_setattr() is not called
> before fat_setattr().  I think this is not needed, since the mode
> change is just a side effect of the attribute change.
>
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> CC: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Looks good. I'm not sure about the intent of security_inode_setattr()
though.

> Index: linux-2.6/fs/fat/file.c
> ===================================================================
> --- linux-2.6.orig/fs/fat/file.c	2008-05-05 11:29:20.000000000 +0200
> +++ linux-2.6/fs/fat/file.c	2008-05-05 11:29:26.000000000 +0200
> @@ -16,6 +16,7 @@
>  #include <linux/writeback.h>
>  #include <linux/backing-dev.h>
>  #include <linux/blkdev.h>
> +#include <linux/fsnotify.h>
>  
>  int fat_generic_ioctl(struct inode *inode, struct file *filp,
>  		      unsigned int cmd, unsigned long arg)
> @@ -65,6 +66,7 @@ int fat_generic_ioctl(struct inode *inod
>  
>  		/* Equivalent to a chmod() */
>  		ia.ia_valid = ATTR_MODE | ATTR_CTIME;
> +		ia.ia_ctime = current_fs_time(inode->i_sb);
>  		if (is_dir) {
>  			ia.ia_mode = MSDOS_MKMODE(attr,
>  				S_IRWXUGO & ~sbi->options.fs_dmask)
> @@ -92,10 +94,11 @@ int fat_generic_ioctl(struct inode *inod
>  		}
>  
>  		/* This MUST be done before doing anything irreversible... */
> -		err = notify_change(filp->f_path.dentry, &ia);
> +		err = fat_setattr(filp->f_path.dentry, &ia);
>  		if (err)
>  			goto up;
>  
> +		fsnotify_change(filp->f_path.dentry, ia.ia_valid);
>  		if (sbi->options.sys_immutable) {
>  			if (attr & ATTR_SYS)
>  				inode->i_flags |= S_IMMUTABLE;
>
> --

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [patch 08/15] fat: dont call notify_change

[Miklos Szeredi]

> > From: Miklos Szeredi <mszeredi@suse.cz>
> >
> > The FAT_IOCTL_SET_ATTRIBUTES ioctl() calls notify_change() to change
> > the file mode before changing the inode attributes.  Replace with
> > explicit call to fat_setattr().
> >
> > This is equivalent, except that security_inode_setattr() is not called
> > before fat_setattr().  I think this is not needed, since the mode
> > change is just a side effect of the attribute change.
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> > CC: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> 
> Looks good. I'm not sure about the intent of security_inode_setattr()
> though.

security_inode_setattr() will call into the security module (selinux,
smack, apparmor) to check if the file mode change is permitted or not.
It's not really applicable to this case, since AFAICS the mode change
here is just a side effect of the attribute change.

If it's not just a side effect, but another way to change the file
mode, then the whole code is very wrong.  chmod() is perfectly fine
for changing the file mode, there's no need for a separate ioctl to
perform exactly the same task.

Miklos

Re: [patch 08/15] fat: dont call notify_change

[OGAWA Hirofumi]

Miklos Szeredi <miklos@szeredi.hu> writes:

> security_inode_setattr() will call into the security module (selinux,
> smack, apparmor) to check if the file mode change is permitted or not.
> It's not really applicable to this case, since AFAICS the mode change
> here is just a side effect of the attribute change.

Yes.

> If it's not just a side effect, but another way to change the file
> mode, then the whole code is very wrong.  chmod() is perfectly fine
> for changing the file mode, there's no need for a separate ioctl to
> perform exactly the same task.

FAT doesn't have permission, it has just some flags. hidden, system,
read-only, etc., and FAT driver maps read-only to permission bit of
file/dir. Mainly this ioctl is to change FAT specific flags, but doesn't
handle read-only flag as special.

As result, this ioctl change permission.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

[patch 09/15] vfs: immutable inode checking cleanup

[Miklos Szeredi]

[-- Attachment #1: immutable_cleanup.patch --]
[-- Type: text/plain, Size: 3912 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Move the immutable and append-only checks from chmod, chown and utimes
into notify_change().  Checks for immutable and append-only files are
always performed by the VFS and not by the filesystem (see
permission() and may_...() in namei.c), so these belong in
notify_change(), and not in inode_change_ok().

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/attr.c   |    6 ++++++
 fs/open.c   |   24 ++----------------------
 fs/utimes.c |    4 ----
 3 files changed, 8 insertions(+), 26 deletions(-)

Index: linux-2.6/fs/open.c
===================================================================
--- linux-2.6.orig/fs/open.c	2008-05-05 11:29:20.000000000 +0200
+++ linux-2.6/fs/open.c	2008-05-05 11:29:26.000000000 +0200
@@ -582,9 +582,6 @@ asmlinkage long sys_fchmod(unsigned int 
 	err = mnt_want_write(file->f_path.mnt);
 	if (err)
 		goto out_putf;
-	err = -EPERM;
-	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
-		goto out_drop_write;
 	mutex_lock(&inode->i_mutex);
 	if (mode == (mode_t) -1)
 		mode = inode->i_mode;
@@ -592,8 +589,6 @@ asmlinkage long sys_fchmod(unsigned int 
 	newattrs.ia_valid = ATTR_MODE | ATTR_CTIME;
 	err = notify_change(dentry, &newattrs);
 	mutex_unlock(&inode->i_mutex);
-
-out_drop_write:
 	mnt_drop_write(file->f_path.mnt);
 out_putf:
 	fput(file);
@@ -617,11 +612,6 @@ asmlinkage long sys_fchmodat(int dfd, co
 	error = mnt_want_write(nd.path.mnt);
 	if (error)
 		goto dput_and_out;
-
-	error = -EPERM;
-	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
-		goto out_drop_write;
-
 	mutex_lock(&inode->i_mutex);
 	if (mode == (mode_t) -1)
 		mode = inode->i_mode;
@@ -629,8 +619,6 @@ asmlinkage long sys_fchmodat(int dfd, co
 	newattrs.ia_valid = ATTR_MODE | ATTR_CTIME;
 	error = notify_change(nd.path.dentry, &newattrs);
 	mutex_unlock(&inode->i_mutex);
-
-out_drop_write:
 	mnt_drop_write(nd.path.mnt);
 dput_and_out:
 	path_put(&nd.path);
@@ -645,18 +633,10 @@ asmlinkage long sys_chmod(const char __u
 
 static int chown_common(struct dentry * dentry, uid_t user, gid_t group)
 {
-	struct inode * inode;
+	struct inode *inode = dentry->d_inode;
 	int error;
 	struct iattr newattrs;
 
-	error = -ENOENT;
-	if (!(inode = dentry->d_inode)) {
-		printk(KERN_ERR "chown_common: NULL inode\n");
-		goto out;
-	}
-	error = -EPERM;
-	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
-		goto out;
 	newattrs.ia_valid =  ATTR_CTIME;
 	if (user != (uid_t) -1) {
 		newattrs.ia_valid |= ATTR_UID;
@@ -672,7 +652,7 @@ static int chown_common(struct dentry * 
 	mutex_lock(&inode->i_mutex);
 	error = notify_change(dentry, &newattrs);
 	mutex_unlock(&inode->i_mutex);
-out:
+
 	return error;
 }
 
Index: linux-2.6/fs/attr.c
===================================================================
--- linux-2.6.orig/fs/attr.c	2008-05-05 11:29:20.000000000 +0200
+++ linux-2.6/fs/attr.c	2008-05-05 11:29:26.000000000 +0200
@@ -108,6 +108,12 @@ int notify_change(struct dentry * dentry
 	struct timespec now;
 	unsigned int ia_valid = attr->ia_valid;
 
+	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
+			ATTR_ATIME_SET | ATTR_MTIME_SET)) {
+		if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
+			return -EPERM;
+	}
+
 	now = current_fs_time(inode->i_sb);
 
 	attr->ia_ctime = now;
Index: linux-2.6/fs/utimes.c
===================================================================
--- linux-2.6.orig/fs/utimes.c	2008-05-05 11:29:20.000000000 +0200
+++ linux-2.6/fs/utimes.c	2008-05-05 11:29:26.000000000 +0200
@@ -105,10 +105,6 @@ long do_utimes(int dfd, char __user *fil
 	/* Don't worry, the checks are done in inode_change_ok() */
 	newattrs.ia_valid = ATTR_CTIME | ATTR_MTIME | ATTR_ATIME;
 	if (times) {
-		error = -EPERM;
-                if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
-			goto mnt_drop_write_and_out;
-
 		if (times[0].tv_nsec == UTIME_OMIT)
 			newattrs.ia_valid &= ~ATTR_ATIME;
 		else if (times[0].tv_nsec != UTIME_NOW) {

--

Re: [patch 09/15] vfs: immutable inode checking cleanup

[Christoph Hellwig]

On Mon, May 05, 2008 at 11:54:50AM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> Move the immutable and append-only checks from chmod, chown and utimes
> into notify_change().  Checks for immutable and append-only files are
> always performed by the VFS and not by the filesystem (see
> permission() and may_...() in namei.c), so these belong in
> notify_change(), and not in inode_change_ok().

Looks fine to me.

[patch 10/15] vfs: truncate: dont check immutable twice

[Miklos Szeredi]

[-- Attachment #1: truncate_immutable_cleanup.patch --]
[-- Type: text/plain, Size: 739 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

vfs_permission(MAY_WRITE) already checked for the inode being
immutable, so no need to repeat it.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/open.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/fs/open.c
===================================================================
--- linux-2.6.orig/fs/open.c	2008-05-05 11:29:26.000000000 +0200
+++ linux-2.6/fs/open.c	2008-05-05 11:29:27.000000000 +0200
@@ -254,7 +254,7 @@ static long do_sys_truncate(const char _
 		goto mnt_drop_write_and_out;
 
 	error = -EPERM;
-	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
+	if (IS_APPEND(inode))
 		goto mnt_drop_write_and_out;
 
 	error = get_write_access(inode);

--

Re: [patch 10/15] vfs: truncate: dont check immutable twice

[Christoph Hellwig]

On Mon, May 05, 2008 at 11:54:51AM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> vfs_permission(MAY_WRITE) already checked for the inode being
> immutable, so no need to repeat it.

Looks good.

[patch 11/15] vfs: truncate: append-only checking cleanup

[Miklos Szeredi]

[-- Attachment #1: truncate_append_cleanup.patch --]
[-- Type: text/plain, Size: 2245 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Move the append-only checks from truncate to notify_change().  Checks
for append-only files are always performed by the VFS and not by the
filesystem so this belongs in notify_change(), and not in
inode_change_ok().

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/attr.c  |    6 ++++++
 fs/namei.c |    2 --
 fs/open.c  |    8 --------
 3 files changed, 6 insertions(+), 10 deletions(-)

Index: linux-2.6/fs/open.c
===================================================================
--- linux-2.6.orig/fs/open.c	2008-05-05 11:29:27.000000000 +0200
+++ linux-2.6/fs/open.c	2008-05-05 11:29:27.000000000 +0200
@@ -253,10 +253,6 @@ static long do_sys_truncate(const char _
 	if (error)
 		goto mnt_drop_write_and_out;
 
-	error = -EPERM;
-	if (IS_APPEND(inode))
-		goto mnt_drop_write_and_out;
-
 	error = get_write_access(inode);
 	if (error)
 		goto mnt_drop_write_and_out;
@@ -321,10 +317,6 @@ static long do_sys_ftruncate(unsigned in
 	if (small && length > MAX_NON_LFS)
 		goto out_putf;
 
-	error = -EPERM;
-	if (IS_APPEND(inode))
-		goto out_putf;
-
 	error = locks_verify_truncate(inode, file, length);
 	if (!error)
 		error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, file);
Index: linux-2.6/fs/attr.c
===================================================================
--- linux-2.6.orig/fs/attr.c	2008-05-05 11:29:26.000000000 +0200
+++ linux-2.6/fs/attr.c	2008-05-05 11:29:27.000000000 +0200
@@ -114,6 +114,12 @@ int notify_change(struct dentry * dentry
 			return -EPERM;
 	}
 
+	/*
+	 * IS_IMMUTABLE() is checked by caller for truncation
+	 */
+	if ((ia_valid & ATTR_SIZE) && IS_APPEND(inode))
+		return -EPERM;
+
 	now = current_fs_time(inode->i_sb);
 
 	attr->ia_ctime = now;
Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c	2008-05-05 11:29:20.000000000 +0200
+++ linux-2.6/fs/namei.c	2008-05-05 11:29:27.000000000 +0200
@@ -1639,8 +1639,6 @@ int may_open(struct nameidata *nd, int a
 	if (IS_APPEND(inode)) {
 		if  ((flag & FMODE_WRITE) && !(flag & O_APPEND))
 			return -EPERM;
-		if (flag & O_TRUNC)
-			return -EPERM;
 	}
 
 	/* O_NOATIME can only be set by the owner or superuser */

--

Re: [patch 11/15] vfs: truncate: append-only checking cleanup

[Christoph Hellwig]

On Mon, May 05, 2008 at 11:54:52AM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> Move the append-only checks from truncate to notify_change().  Checks
> for append-only files are always performed by the VFS and not by the
> filesystem so this belongs in notify_change(), and not in
> inode_change_ok().

Looks good to me.

[patch 12/15] vfs: create file_truncate() helper

[Miklos Szeredi]

[-- Attachment #1: file_truncate.patch --]
[-- Type: text/plain, Size: 4903 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Clean up do_truncate() API:

  - create a new function file_truncate()
  - remove the 'struct file *' argument from do_truncate()

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/exec.c          |    2 +-
 fs/namei.c         |    3 +--
 fs/open.c          |   22 +++++++++++++++-------
 include/linux/fs.h |    5 +++--
 mm/tiny-shmem.c    |    2 +-
 5 files changed, 21 insertions(+), 13 deletions(-)

Index: linux-2.6/fs/exec.c
===================================================================
--- linux-2.6.orig/fs/exec.c	2008-05-05 11:29:20.000000000 +0200
+++ linux-2.6/fs/exec.c	2008-05-05 11:29:27.000000000 +0200
@@ -1763,7 +1763,7 @@ int do_coredump(long signr, int exit_cod
 		goto close_fail;
 	if (!file->f_op->write)
 		goto close_fail;
-	if (!ispipe && do_truncate(file->f_path.dentry, 0, 0, file) != 0)
+	if (!ispipe && file_truncate(file, 0, 0) != 0)
 		goto close_fail;
 
 	retval = binfmt->core_dump(signr, regs, file, core_limit);
Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c	2008-05-05 11:29:27.000000000 +0200
+++ linux-2.6/fs/namei.c	2008-05-05 11:29:27.000000000 +0200
@@ -1666,8 +1666,7 @@ int may_open(struct nameidata *nd, int a
 			DQUOT_INIT(inode);
 
 			error = do_truncate(dentry, 0,
-					    ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
-					    NULL);
+					    ATTR_MTIME|ATTR_CTIME|ATTR_OPEN);
 		}
 		put_write_access(inode);
 		if (error)
Index: linux-2.6/fs/open.c
===================================================================
--- linux-2.6.orig/fs/open.c	2008-05-05 11:29:27.000000000 +0200
+++ linux-2.6/fs/open.c	2008-05-05 11:29:28.000000000 +0200
@@ -195,8 +195,8 @@ out:
 	return error;
 }
 
-int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
-	struct file *filp)
+static int truncate_common(struct dentry *dentry, loff_t length,
+			   unsigned int time_attrs, struct file *filp)
 {
 	int err;
 	struct iattr newattrs;
@@ -221,6 +221,16 @@ int do_truncate(struct dentry *dentry, l
 	return err;
 }
 
+int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs)
+{
+	return truncate_common(dentry, length, time_attrs, NULL);
+}
+
+int file_truncate(struct file *filp, loff_t length, unsigned int time_attrs)
+{
+	return truncate_common(filp->f_path.dentry, length, time_attrs, filp);
+}
+
 static long do_sys_truncate(const char __user * path, loff_t length)
 {
 	struct nameidata nd;
@@ -268,7 +278,7 @@ static long do_sys_truncate(const char _
 	error = locks_verify_truncate(inode, NULL, length);
 	if (!error) {
 		DQUOT_INIT(inode);
-		error = do_truncate(nd.path.dentry, length, 0, NULL);
+		error = do_truncate(nd.path.dentry, length, 0);
 	}
 
 put_write_and_out:
@@ -290,7 +300,6 @@ asmlinkage long sys_truncate(const char 
 static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
 {
 	struct inode * inode;
-	struct dentry *dentry;
 	struct file * file;
 	int error;
 
@@ -306,8 +315,7 @@ static long do_sys_ftruncate(unsigned in
 	if (file->f_flags & O_LARGEFILE)
 		small = 0;
 
-	dentry = file->f_path.dentry;
-	inode = dentry->d_inode;
+	inode = file->f_path.dentry->d_inode;
 	error = -EINVAL;
 	if (!S_ISREG(inode->i_mode) || !(file->f_mode & FMODE_WRITE))
 		goto out_putf;
@@ -319,7 +327,7 @@ static long do_sys_ftruncate(unsigned in
 
 	error = locks_verify_truncate(inode, file, length);
 	if (!error)
-		error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, file);
+		error = file_truncate(file, length, ATTR_MTIME|ATTR_CTIME);
 out_putf:
 	fput(file);
 out:
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2008-05-05 11:29:20.000000000 +0200
+++ linux-2.6/include/linux/fs.h	2008-05-05 11:29:28.000000000 +0200
@@ -1603,8 +1603,9 @@ static inline int break_lease(struct ino
 
 /* fs/open.c */
 
-extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs,
-		       struct file *filp);
+extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs);
+extern int file_truncate(struct file *filp, loff_t start,
+			 unsigned int time_attrs);
 extern long do_sys_open(int dfd, const char __user *filename, int flags,
 			int mode);
 extern struct file *filp_open(const char *, int, int);
Index: linux-2.6/mm/tiny-shmem.c
===================================================================
--- linux-2.6.orig/mm/tiny-shmem.c	2008-05-05 11:29:20.000000000 +0200
+++ linux-2.6/mm/tiny-shmem.c	2008-05-05 11:29:28.000000000 +0200
@@ -80,7 +80,7 @@ struct file *shmem_file_setup(char *name
 	inode->i_nlink = 0;	/* It is unlinked */
 
 	/* notify everyone as to the change of file size */
-	error = do_truncate(dentry, size, 0, file);
+	error = file_truncate(file, size, 0);
 	if (error < 0)
 		goto close_file;
 

--

Re: [patch 12/15] vfs: create file_truncate() helper

[Christoph Hellwig]

On Mon, May 05, 2008 at 11:54:53AM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> Clean up do_truncate() API:
> 
>   - create a new function file_truncate()
>   - remove the 'struct file *' argument from do_truncate()

file_truncate looks like a nice little cleanup, but please make sure to
add a kerneldoc comment for every newly added global function.

I don't think there's a point to remove the file argument from
do_truncate and add another helper.

Re: [patch 12/15] vfs: create file_truncate() helper

[Miklos Szeredi]

> On Mon, May 05, 2008 at 11:54:53AM +0200, Miklos Szeredi wrote:
> > From: Miklos Szeredi <mszeredi@suse.cz>
> > 
> > Clean up do_truncate() API:
> > 
> >   - create a new function file_truncate()
> >   - remove the 'struct file *' argument from do_truncate()
> 
> file_truncate looks like a nice little cleanup, but please make sure to
> add a kerneldoc comment for every newly added global function.

OK.

> I don't think there's a point to remove the file argument from
> do_truncate and add another helper.

Whatever.

Miklos

[patch 13/15] vfs: utimes cleanup

[Miklos Szeredi]

[-- Attachment #1: utimes_cleanup.patch --]
[-- Type: text/plain, Size: 5043 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Untange the mess that is do_utimes()

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: Ulrich Drepper <drepper@redhat.com>
---
 fs/utimes.c |  163 +++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 91 insertions(+), 72 deletions(-)

Index: linux-2.6/fs/utimes.c
===================================================================
--- linux-2.6.orig/fs/utimes.c	2008-05-05 11:29:26.000000000 +0200
+++ linux-2.6/fs/utimes.c	2008-05-05 11:29:28.000000000 +0200
@@ -53,54 +53,10 @@ static bool nsec_valid(long nsec)
 	return nsec >= 0 && nsec <= 999999999;
 }
 
-/* If times==NULL, set access and modification to current time,
- * must be owner or have write permission.
- * Else, update from *times, must be owner or super user.
- */
-long do_utimes(int dfd, char __user *filename, struct timespec *times, int flags)
+static int utimes_common(struct path *path, struct timespec *times)
 {
 	int error;
-	struct nameidata nd;
-	struct dentry *dentry;
-	struct inode *inode;
 	struct iattr newattrs;
-	struct file *f = NULL;
-	struct vfsmount *mnt;
-
-	error = -EINVAL;
-	if (times && (!nsec_valid(times[0].tv_nsec) ||
-		      !nsec_valid(times[1].tv_nsec))) {
-		goto out;
-	}
-
-	if (flags & ~AT_SYMLINK_NOFOLLOW)
-		goto out;
-
-	if (filename == NULL && dfd != AT_FDCWD) {
-		error = -EINVAL;
-		if (flags & AT_SYMLINK_NOFOLLOW)
-			goto out;
-
-		error = -EBADF;
-		f = fget(dfd);
-		if (!f)
-			goto out;
-		dentry = f->f_path.dentry;
-		mnt = f->f_path.mnt;
-	} else {
-		error = __user_walk_fd(dfd, filename, (flags & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW, &nd);
-		if (error)
-			goto out;
-
-		dentry = nd.path.dentry;
-		mnt = nd.path.mnt;
-	}
-
-	inode = dentry->d_inode;
-
-	error = mnt_want_write(mnt);
-	if (error)
-		goto dput_and_out;
 
 	/* Don't worry, the checks are done in inode_change_ok() */
 	newattrs.ia_valid = ATTR_CTIME | ATTR_MTIME | ATTR_ATIME;
@@ -121,40 +77,103 @@ long do_utimes(int dfd, char __user *fil
 			newattrs.ia_valid |= ATTR_MTIME_SET;
 		}
 	}
+	mutex_lock(&path->dentry->d_inode->i_mutex);
+	error = mnt_want_write(path->mnt);
+	if (!error) {
+		error = notify_change(path->dentry, &newattrs);
+		mnt_drop_write(path->mnt);
+	}
+	mutex_unlock(&path->dentry->d_inode->i_mutex);
+
+	return error;
+}
+
+/*
+ * If times is NULL or both times are either UTIME_OMIT or UTIME_NOW, then
+ * need to check permissions, because inode_change_ok() won't do it.
+ */
+static bool utimes_need_permission(struct timespec *times)
+{
+	return !times || (nsec_special(times[0].tv_nsec) &&
+			  nsec_special(times[1].tv_nsec));
+}
+
+static int do_fd_utimes(int fd, struct timespec *times)
+{
+	int error;
+	struct file *file = fget(fd);
+
+	if (!file)
+		return -EBADF;
+
+	if (utimes_need_permission(times)) {
+		struct inode *inode = file->f_path.dentry->d_inode;
+
+		error = -EACCES;
+		if (IS_IMMUTABLE(inode))
+			goto out_fput;
+
+		if (!is_owner_or_cap(inode) && !(file->f_mode & FMODE_WRITE))
+			goto out_fput;
+	}
+	error = utimes_common(&file->f_path, times);
+
+ out_fput:
+	fput(file);
+
+	return error;
+}
+
+/* If times==NULL, set access and modification to current time,
+ * must be owner or have write permission.
+ * Else, update from *times, must be owner or super user.
+ */
+long do_utimes(int dfd, char __user *filename, struct timespec *times,
+	       int flags)
+{
+	int error;
+	struct nameidata nd;
+	int lookup_flags;
+
+	if (times && (!nsec_valid(times[0].tv_nsec) ||
+		      !nsec_valid(times[1].tv_nsec))) {
+		return -EINVAL;
+	}
+
+	if (filename == NULL && dfd != AT_FDCWD) {
+		if (flags)
+			return -EINVAL;
+
+		return do_fd_utimes(dfd, times);
+	}
+
+	if (flags & ~AT_SYMLINK_NOFOLLOW)
+		return -EINVAL;
+
+	lookup_flags = (flags & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;
+	error = __user_walk_fd(dfd, filename, lookup_flags, &nd);
+	if (error)
+		return error;
+
+
+	if (utimes_need_permission(times)) {
+		struct inode *inode = nd.path.dentry->d_inode;
 
-	/*
-	 * If times is NULL or both times are either UTIME_OMIT or
-	 * UTIME_NOW, then need to check permissions, because
-	 * inode_change_ok() won't do it.
-	 */
-	if (!times || (nsec_special(times[0].tv_nsec) &&
-		       nsec_special(times[1].tv_nsec))) {
 		error = -EACCES;
                 if (IS_IMMUTABLE(inode))
-			goto mnt_drop_write_and_out;
+			goto out_path_put;
 
 		if (!is_owner_or_cap(inode)) {
-			if (f) {
-				if (!(f->f_mode & FMODE_WRITE))
-					goto mnt_drop_write_and_out;
-			} else {
-				error = vfs_permission(&nd, MAY_WRITE);
-				if (error)
-					goto mnt_drop_write_and_out;
-			}
+			error = vfs_permission(&nd, MAY_WRITE);
+			if (error)
+				goto out_path_put;
 		}
 	}
-	mutex_lock(&inode->i_mutex);
-	error = notify_change(dentry, &newattrs);
-	mutex_unlock(&inode->i_mutex);
-mnt_drop_write_and_out:
-	mnt_drop_write(mnt);
-dput_and_out:
-	if (f)
-		fput(f);
-	else
-		path_put(&nd.path);
-out:
+	error = utimes_common(&nd.path, times);
+
+ out_path_put:
+	path_put(&nd.path);
+
 	return error;
 }
 

--

Re: [patch 13/15] vfs: utimes cleanup

[Christoph Hellwig]

On Mon, May 05, 2008 at 11:54:54AM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> Untange the mess that is do_utimes()

A good idea to untangle this, but I'm not entirely happy with how it's
done.

utimes_need_permission is a good helper and fine with me.

utimes_common is a good idea aswell, but I'd rather take the permission
checks into it aswell, even if that means a little flag telling if
file->f_mode should be checked or vfs_permission().

do_fd_utimes sounds fine, but I don't like that name.  do_futimes maybe?

and when the fd-side is sorted out the path side should probably be a
helper aswell.  Then sys_utime/sys_utimes/arhc bits could call it directly,
with the initial check in do_utimes separated out into a helper ala
utimes_need_permission.  do_utimes should probably become do_futimesat
at the point.

Re: [patch 13/15] vfs: utimes cleanup

[Miklos Szeredi]

> > From: Miklos Szeredi <mszeredi@suse.cz>
> > 
> > Untange the mess that is do_utimes()
> 
> A good idea to untangle this, but I'm not entirely happy with how it's
> done.
> 
> utimes_need_permission is a good helper and fine with me.
> 
> utimes_common is a good idea aswell, but I'd rather take the permission
> checks into it aswell, even if that means a little flag telling if
> file->f_mode should be checked or vfs_permission().

How would that be better?  There's zero commonality between the two
kinds of permission checks (other than utimes_need_permission()).

> do_fd_utimes sounds fine, but I don't like that name.  do_futimes maybe?

Whatever you prefer.  It's a static function, so it's not really a big
issue.

> and when the fd-side is sorted out the path side should probably be a
> helper aswell.  Then sys_utime/sys_utimes/arhc bits could call it directly,
> with the initial check in do_utimes separated out into a helper ala
> utimes_need_permission.  do_utimes should probably become do_futimesat
> at the point.

OK, makes sense.

Miklos

Re: [patch 13/15] vfs: utimes cleanup

[Christoph Hellwig]

On Mon, May 05, 2008 at 02:39:47PM +0200, Miklos Szeredi wrote:
> > checks into it aswell, even if that means a little flag telling if
> > file->f_mode should be checked or vfs_permission().
> 
> How would that be better?  There's zero commonality between the two
> kinds of permission checks (other than utimes_need_permission()).

it looks very similar.  but actually given that the next patch removes
the IS_IMMUTABLE check in the fd case it isn't anymore.  You're probably
right that it doesn't make sense to move it to the common one.

[patch 14/15] vfs: utimes immutable fix

[Miklos Szeredi]

[-- Attachment #1: utimes_immutable_fix.patch --]
[-- Type: text/plain, Size: 965 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

If updating file times to the current time and using a file
descriptor, then don't check for immutable inode, only if the file is
opened for write.  In this case immutability has been checked at open
time.  This is the same as how write() and ftruncate() are handled.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: Ulrich Drepper <drepper@redhat.com>
---
 fs/utimes.c |    3 ---
 1 file changed, 3 deletions(-)

Index: linux-2.6/fs/utimes.c
===================================================================
--- linux-2.6.orig/fs/utimes.c	2008-05-05 11:29:28.000000000 +0200
+++ linux-2.6/fs/utimes.c	2008-05-05 11:29:28.000000000 +0200
@@ -110,9 +110,6 @@ static int do_fd_utimes(int fd, struct t
 		struct inode *inode = file->f_path.dentry->d_inode;
 
 		error = -EACCES;
-		if (IS_IMMUTABLE(inode))
-			goto out_fput;
-
 		if (!is_owner_or_cap(inode) && !(file->f_mode & FMODE_WRITE))
 			goto out_fput;
 	}

--

Re: [patch 14/15] vfs: utimes immutable fix

[Christoph Hellwig]

On Mon, May 05, 2008 at 11:54:55AM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> If updating file times to the current time and using a file
> descriptor, then don't check for immutable inode, only if the file is
> opened for write.  In this case immutability has been checked at open
> time.  This is the same as how write() and ftruncate() are handled.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> CC: Ulrich Drepper <drepper@redhat.com>
> ---
>  fs/utimes.c |    3 ---
>  1 file changed, 3 deletions(-)
> 
> Index: linux-2.6/fs/utimes.c
> ===================================================================
> --- linux-2.6.orig/fs/utimes.c	2008-05-05 11:29:28.000000000 +0200
> +++ linux-2.6/fs/utimes.c	2008-05-05 11:29:28.000000000 +0200
> @@ -110,9 +110,6 @@ static int do_fd_utimes(int fd, struct t
>  		struct inode *inode = file->f_path.dentry->d_inode;
>  
>  		error = -EACCES;
> -		if (IS_IMMUTABLE(inode))
> -			goto out_fput;
> -

Looks good, but please re-order it before the cleanup so we can put it
in for .26.

[patch 15/15] vfs: splice remove_suid() cleanup

[Miklos Szeredi]

[-- Attachment #1: splice_remove_suid_cleanup.patch --]
[-- Type: text/plain, Size: 3197 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

generic_file_splice_write() duplicates remove_suid() just because it
doesn't hold i_mutex.  But it grabs i_mutex inside splice_from_pipe()
anyway, so this is rather pointless.

Move locking to generic_file_splice_write() and call remove_suid() and
__splice_from_pipe() instead.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: Jens Axboe <jens.axboe@oracle.com>
---
 fs/splice.c        |   29 +++++++++++++----------------
 include/linux/fs.h |    1 -
 mm/filemap.c       |    2 +-
 3 files changed, 14 insertions(+), 18 deletions(-)

Index: linux-2.6/fs/splice.c
===================================================================
--- linux-2.6.orig/fs/splice.c	2008-05-05 11:29:20.000000000 +0200
+++ linux-2.6/fs/splice.c	2008-05-05 11:29:29.000000000 +0200
@@ -811,24 +811,19 @@ generic_file_splice_write(struct pipe_in
 {
 	struct address_space *mapping = out->f_mapping;
 	struct inode *inode = mapping->host;
-	int killsuid, killpriv;
+	struct splice_desc sd = {
+		.total_len = len,
+		.flags = flags,
+		.pos = *ppos,
+		.u.file = out,
+	};
 	ssize_t ret;
-	int err = 0;
 
-	killpriv = security_inode_need_killpriv(out->f_path.dentry);
-	killsuid = should_remove_suid(out->f_path.dentry);
-	if (unlikely(killsuid || killpriv)) {
-		mutex_lock(&inode->i_mutex);
-		if (killpriv)
-			err = security_inode_killpriv(out->f_path.dentry);
-		if (!err && killsuid)
-			err = __remove_suid(out->f_path.dentry, killsuid);
-		mutex_unlock(&inode->i_mutex);
-		if (err)
-			return err;
-	}
-
-	ret = splice_from_pipe(pipe, out, ppos, len, flags, pipe_to_file);
+	inode_double_lock(inode, pipe->inode);
+	ret = remove_suid(out->f_path.dentry);
+	if (likely(!ret))
+		ret = __splice_from_pipe(pipe, &sd, pipe_to_file);
+	inode_double_unlock(inode, pipe->inode);
 	if (ret > 0) {
 		unsigned long nr_pages;
 
@@ -840,6 +835,8 @@ generic_file_splice_write(struct pipe_in
 		 * sync it.
 		 */
 		if (unlikely((out->f_flags & O_SYNC) || IS_SYNC(inode))) {
+			int err;
+
 			mutex_lock(&inode->i_mutex);
 			err = generic_osync_inode(inode, mapping,
 						  OSYNC_METADATA|OSYNC_DATA);
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2008-05-05 11:29:28.000000000 +0200
+++ linux-2.6/include/linux/fs.h	2008-05-05 11:29:29.000000000 +0200
@@ -1822,7 +1822,6 @@ extern void iget_failed(struct inode *);
 extern void clear_inode(struct inode *);
 extern void destroy_inode(struct inode *);
 extern struct inode *new_inode(struct super_block *);
-extern int __remove_suid(struct dentry *, int);
 extern int should_remove_suid(struct dentry *);
 extern int remove_suid(struct dentry *);
 
Index: linux-2.6/mm/filemap.c
===================================================================
--- linux-2.6.orig/mm/filemap.c	2008-05-05 11:29:20.000000000 +0200
+++ linux-2.6/mm/filemap.c	2008-05-05 11:29:29.000000000 +0200
@@ -1655,7 +1655,7 @@ int should_remove_suid(struct dentry *de
 }
 EXPORT_SYMBOL(should_remove_suid);
 
-int __remove_suid(struct dentry *dentry, int kill)
+static int __remove_suid(struct dentry *dentry, int kill)
 {
 	struct iattr newattrs;
 

--

Re: [patch 15/15] vfs: splice remove_suid() cleanup

[Christoph Hellwig]

On Mon, May 05, 2008 at 11:54:56AM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> generic_file_splice_write() duplicates remove_suid() just because it
> doesn't hold i_mutex.  But it grabs i_mutex inside splice_from_pipe()
> anyway, so this is rather pointless.
> 
> Move locking to generic_file_splice_write() and call remove_suid() and
> __splice_from_pipe() instead.

Looks good to me.  I wonder whether we should kill splice_from_pipe
entirely and always leave the locking to the caller.

Re: [patch 15/15] vfs: splice remove_suid() cleanup

[Jens Axboe]

On Mon, May 05 2008, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> generic_file_splice_write() duplicates remove_suid() just because it
> doesn't hold i_mutex.  But it grabs i_mutex inside splice_from_pipe()
> anyway, so this is rather pointless.
> 
> Move locking to generic_file_splice_write() and call remove_suid() and
> __splice_from_pipe() instead.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> CC: Jens Axboe <jens.axboe@oracle.com>

Looks good to me, applied.

-- 
Jens Axboe