mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [patch 0/4] vfs: utimes cleanups
@ 2008-07-01 13:01 Miklos Szeredi
  2008-07-01 13:01 ` [patch 1/4] vfs: utimes: move owner check into inode_change_ok() Miklos Szeredi
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Miklos Szeredi @ 2008-07-01 13:01 UTC (permalink / raw)
  To: viro; +Cc: linux-kernel, linux-fsdevel, hch, akpm

Al,

Could you please take these (mostly) utimes cleanups for 2.6.27?

Thanks,
Miklos
--

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [patch 1/4] vfs: utimes: move owner check into inode_change_ok()
  2008-07-01 13:01 [patch 0/4] vfs: utimes cleanups Miklos Szeredi
@ 2008-07-01 13:01 ` Miklos Szeredi
  2008-07-01 14:09   ` Michael Kerrisk
  2008-07-01 13:01 ` [patch 2/4] vfs: utimes cleanup Miklos Szeredi
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Miklos Szeredi @ 2008-07-01 13:01 UTC (permalink / raw)
  To: viro
  Cc: linux-kernel, linux-fsdevel, hch, akpm, Ulrich Drepper, Michael Kerrisk

[-- Attachment #1: vfs-utimes-move-owner-check-into-inode_change_ok.patch --]
[-- Type: text/plain, Size: 4411 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Add a new ia_valid flag: ATTR_TIMES_SET, to handle the
UTIMES_OMIT/UTIMES_NOW and UTIMES_NOW/UTIMES_OMIT cases.  In these
cases neither ATTR_MTIME_SET nor ATTR_ATIME_SET is in the flags, yet
the POSIX draft specifies that permission checking is performed the
same way as if one or both of the times was explicitly set to a
timestamp.

See the path "vfs: utimensat(): fix error checking for
{UTIME_NOW,UTIME_OMIT} case" by Michael Kerrisk for the patch
introducing this behavior.

This is a cleanup, as well as allowing filesystems (NFS/fuse/...) to
perform their own permission checking instead of the default.

CC: Ulrich Drepper <drepper@redhat.com>
CC: Michael Kerrisk <mtk.manpages@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/attr.c          |    2 +-
 fs/utimes.c        |   17 ++++-------------
 include/linux/fs.h |   33 +++++++++++++++++----------------
 3 files changed, 22 insertions(+), 30 deletions(-)

Index: linux-2.6/fs/attr.c
===================================================================
--- linux-2.6.orig/fs/attr.c	2008-06-27 22:09:08.000000000 +0200
+++ linux-2.6/fs/attr.c	2008-07-01 13:52:20.000000000 +0200
@@ -51,7 +51,7 @@ int inode_change_ok(struct inode *inode,
 	}
 
 	/* Check for setting the inode time. */
-	if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET)) {
+	if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET | ATTR_TIMES_SET)) {
 		if (!is_owner_or_cap(inode))
 			goto error;
 	}
Index: linux-2.6/fs/utimes.c
===================================================================
--- linux-2.6.orig/fs/utimes.c	2008-07-01 08:10:12.000000000 +0200
+++ linux-2.6/fs/utimes.c	2008-07-01 13:52:20.000000000 +0200
@@ -101,7 +101,6 @@ long do_utimes(int dfd, char __user *fil
 		     times[1].tv_nsec == UTIME_NOW)
 		times = NULL;
 
-	/* In most cases, the checks are done in inode_change_ok() */
 	newattrs.ia_valid = ATTR_CTIME | ATTR_MTIME | ATTR_ATIME;
 	if (times) {
 		error = -EPERM;
@@ -123,21 +122,13 @@ long do_utimes(int dfd, char __user *fil
 			newattrs.ia_mtime.tv_nsec = times[1].tv_nsec;
 			newattrs.ia_valid |= ATTR_MTIME_SET;
 		}
-
 		/*
-		 * For the UTIME_OMIT/UTIME_NOW and UTIME_NOW/UTIME_OMIT
-		 * cases, we need to make an extra check that is not done by
-		 * inode_change_ok().
+		 * Tell inode_change_ok(), that this is an explicit time
+		 * update, even if neither ATTR_ATIME_SET nor ATTR_MTIME_SET
+		 * were used.
 		 */
-		if (((times[0].tv_nsec == UTIME_NOW &&
-			    times[1].tv_nsec == UTIME_OMIT)
-		     ||
-		     (times[0].tv_nsec == UTIME_OMIT &&
-			    times[1].tv_nsec == UTIME_NOW))
-		    && !is_owner_or_cap(inode))
-			goto mnt_drop_write_and_out;
+		newattrs.ia_valid |= ATTR_TIMES_SET;
 	} else {
-
 		/*
 		 * If times is NULL (or both times are UTIME_NOW),
 		 * then we need to check permissions, because
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2008-07-01 13:52:19.000000000 +0200
+++ linux-2.6/include/linux/fs.h	2008-07-01 13:52:20.000000000 +0200
@@ -317,22 +317,23 @@ typedef void (dio_iodone_t)(struct kiocb
  * Attribute flags.  These should be or-ed together to figure out what
  * has been changed!
  */
-#define ATTR_MODE	1
-#define ATTR_UID	2
-#define ATTR_GID	4
-#define ATTR_SIZE	8
-#define ATTR_ATIME	16
-#define ATTR_MTIME	32
-#define ATTR_CTIME	64
-#define ATTR_ATIME_SET	128
-#define ATTR_MTIME_SET	256
-#define ATTR_FORCE	512	/* Not a change, but a change it */
-#define ATTR_ATTR_FLAG	1024
-#define ATTR_KILL_SUID	2048
-#define ATTR_KILL_SGID	4096
-#define ATTR_FILE	8192
-#define ATTR_KILL_PRIV	16384
-#define ATTR_OPEN	32768	/* Truncating from open(O_TRUNC) */
+#define ATTR_MODE	(1 << 0)
+#define ATTR_UID	(1 << 1)
+#define ATTR_GID	(1 << 2)
+#define ATTR_SIZE	(1 << 3)
+#define ATTR_ATIME	(1 << 4)
+#define ATTR_MTIME	(1 << 5)
+#define ATTR_CTIME	(1 << 6)
+#define ATTR_ATIME_SET	(1 << 7)
+#define ATTR_MTIME_SET	(1 << 8)
+#define ATTR_FORCE	(1 << 9) /* Not a change, but a change it */
+#define ATTR_ATTR_FLAG	(1 << 10)
+#define ATTR_KILL_SUID	(1 << 11)
+#define ATTR_KILL_SGID	(1 << 12)
+#define ATTR_FILE	(1 << 13)
+#define ATTR_KILL_PRIV	(1 << 14)
+#define ATTR_OPEN	(1 << 15) /* Truncating from open(O_TRUNC) */
+#define ATTR_TIMES_SET	(1 << 16)
 
 /*
  * This is the Inode Attributes structure, used for notify_change().  It

--

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [patch 2/4] vfs: utimes cleanup
  2008-07-01 13:01 [patch 0/4] vfs: utimes cleanups Miklos Szeredi
  2008-07-01 13:01 ` [patch 1/4] vfs: utimes: move owner check into inode_change_ok() Miklos Szeredi
@ 2008-07-01 13:01 ` Miklos Szeredi
  2008-07-01 13:01 ` [patch 3/4] fat: dont call notify_change Miklos Szeredi
  2008-07-01 13:01 ` [patch 4/4] vfs: immutable inode checking cleanup Miklos Szeredi
  3 siblings, 0 replies; 7+ messages in thread
From: Miklos Szeredi @ 2008-07-01 13:01 UTC (permalink / raw)
  To: viro
  Cc: linux-kernel, linux-fsdevel, hch, akpm, Ulrich Drepper, Michael Kerrisk

[-- Attachment #1: vfs-utimes-cleanup.patch --]
[-- Type: text/plain, Size: 3905 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Untange the mess that is do_utimes().  Add kerneldoc comment to
do_utimes().

CC: Ulrich Drepper <drepper@redhat.com>
CC: Michael Kerrisk <mtk.manpages@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/utimes.c |  114 ++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 65 insertions(+), 49 deletions(-)

Index: linux/fs/utimes.c
===================================================================
--- linux.orig/fs/utimes.c	2008-06-13 11:35:24.000000000 +0200
+++ linux/fs/utimes.c	2008-06-13 11:49:37.000000000 +0200
@@ -48,54 +48,15 @@ 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;
+	struct inode *inode = path->dentry->d_inode;
 
-		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);
+	error = mnt_want_write(path->mnt);
 	if (error)
-		goto dput_and_out;
+		goto out;
 
 	if (times && times[0].tv_nsec == UTIME_NOW &&
 		     times[1].tv_nsec == UTIME_NOW)
@@ -145,15 +106,70 @@ long do_utimes(int dfd, char __user *fil
 		}
 	}
 	mutex_lock(&inode->i_mutex);
-	error = notify_change(dentry, &newattrs);
+	error = notify_change(path->dentry, &newattrs);
 	mutex_unlock(&inode->i_mutex);
+
 mnt_drop_write_and_out:
-	mnt_drop_write(mnt);
-dput_and_out:
-	if (f)
-		fput(f);
-	else
+	mnt_drop_write(path->mnt);
+out:
+	return error;
+}
+
+/*
+ * do_utimes - change times on filename or file descriptor
+ * @dfd: open file descriptor, -1 or AT_FDCWD
+ * @filename: path name or NULL
+ * @times: new times or NULL
+ * @flags: zero or more flags (only AT_SYMLINK_NOFOLLOW for the moment)
+ *
+ * If filename is NULL and dfd refers to an open file, then operate on
+ * the file.  Otherwise look up filename, possibly using dfd as a
+ * starting point.
+ *
+ * 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 = -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) {
+		struct file *file;
+
+		if (flags & AT_SYMLINK_NOFOLLOW)
+			goto out;
+
+		file = fget(dfd);
+		error = -EBADF;
+		if (!file)
+			goto out;
+
+		error = utimes_common(&file->f_path, times);
+		fput(file);
+	} else {
+		struct nameidata nd;
+		int lookup_flags = 0;
+
+		if (!(flags & AT_SYMLINK_NOFOLLOW))
+			lookup_flags |= LOOKUP_FOLLOW;
+
+		error = __user_walk_fd(dfd, filename, lookup_flags, &nd);
+		if (error)
+			goto out;
+
+		error = utimes_common(&nd.path, times);
 		path_put(&nd.path);
+	}
+
 out:
 	return error;
 }

--

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [patch 3/4] fat: dont call notify_change
  2008-07-01 13:01 [patch 0/4] vfs: utimes cleanups Miklos Szeredi
  2008-07-01 13:01 ` [patch 1/4] vfs: utimes: move owner check into inode_change_ok() Miklos Szeredi
  2008-07-01 13:01 ` [patch 2/4] vfs: utimes cleanup Miklos Szeredi
@ 2008-07-01 13:01 ` Miklos Szeredi
  2008-07-01 13:01 ` [patch 4/4] vfs: immutable inode checking cleanup Miklos Szeredi
  3 siblings, 0 replies; 7+ messages in thread
From: Miklos Szeredi @ 2008-07-01 13:01 UTC (permalink / raw)
  To: viro; +Cc: linux-kernel, linux-fsdevel, hch, akpm, OGAWA Hirofumi

[-- Attachment #1: fat-dont-call-notify_change.patch --]
[-- Type: text/plain, Size: 2816 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 calls to security_inode_setattr(), fat_setattr() and
fsnotify_change().

This is equivalent to the original.  The reason it is needed, is that
later in the series we move the immutable check into notify_change().
That would break the FAT_IOCTL_SET_ATTRIBUTES ioctl, as it needs to
perform the mode change regardless of the immutability of the file.

[Fix error if fat is built as a module.  Thanks to OGAWA Hirofumi for
noticing.]

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

Index: linux-2.6/fs/fat/file.c
===================================================================
--- linux-2.6.orig/fs/fat/file.c	2008-06-27 22:09:08.000000000 +0200
+++ linux-2.6/fs/fat/file.c	2008-07-01 13:52:26.000000000 +0200
@@ -16,6 +16,8 @@
 #include <linux/writeback.h>
 #include <linux/backing-dev.h>
 #include <linux/blkdev.h>
+#include <linux/fsnotify.h>
+#include <linux/security.h>
 
 int fat_generic_ioctl(struct inode *inode, struct file *filp,
 		      unsigned int cmd, unsigned long arg)
@@ -65,6 +67,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)
@@ -91,11 +94,21 @@ int fat_generic_ioctl(struct inode *inod
 			}
 		}
 
+		/*
+		 * The security check is questionable...  We single
+		 * out the RO attribute for checking by the security
+		 * module, just because it maps to a file mode.
+		 */
+		err = security_inode_setattr(filp->f_path.dentry, &ia);
+		if (err)
+			goto up;
+
 		/* 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;
Index: linux-2.6/security/security.c
===================================================================
--- linux-2.6.orig/security/security.c	2008-06-27 22:09:08.000000000 +0200
+++ linux-2.6/security/security.c	2008-07-01 13:52:26.000000000 +0200
@@ -476,6 +476,7 @@ int security_inode_setattr(struct dentry
 		return 0;
 	return security_ops->inode_setattr(dentry, attr);
 }
+EXPORT_SYMBOL_GPL(security_inode_setattr);
 
 int security_inode_getattr(struct vfsmount *mnt, struct dentry *dentry)
 {

--

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [patch 4/4] vfs: immutable inode checking cleanup
  2008-07-01 13:01 [patch 0/4] vfs: utimes cleanups Miklos Szeredi
                   ` (2 preceding siblings ...)
  2008-07-01 13:01 ` [patch 3/4] fat: dont call notify_change Miklos Szeredi
@ 2008-07-01 13:01 ` Miklos Szeredi
  3 siblings, 0 replies; 7+ messages in thread
From: Miklos Szeredi @ 2008-07-01 13:01 UTC (permalink / raw)
  To: viro
  Cc: linux-kernel, linux-fsdevel, hch, akpm, Ulrich Drepper, Michael Kerrisk

[-- Attachment #1: vfs-immutable-inode-checking-cleanup.patch --]
[-- Type: text/plain, Size: 3952 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().

This should be completely equivalent.

CC: Ulrich Drepper <drepper@redhat.com>
CC: Michael Kerrisk <mtk.manpages@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/attr.c   |    5 +++++
 fs/open.c   |   24 ++----------------------
 fs/utimes.c |    4 ----
 3 files changed, 7 insertions(+), 26 deletions(-)

Index: linux-2.6/fs/attr.c
===================================================================
--- linux-2.6.orig/fs/attr.c	2008-07-01 13:52:20.000000000 +0200
+++ linux-2.6/fs/attr.c	2008-07-01 13:52:29.000000000 +0200
@@ -108,6 +108,11 @@ 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_TIMES_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/open.c
===================================================================
--- linux-2.6.orig/fs/open.c	2008-07-01 13:52:17.000000000 +0200
+++ linux-2.6/fs/open.c	2008-07-01 13:52:29.000000000 +0200
@@ -598,9 +598,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;
@@ -608,8 +605,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);
@@ -633,11 +628,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;
@@ -645,8 +635,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);
@@ -661,18 +649,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;
@@ -688,7 +668,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/utimes.c
===================================================================
--- linux-2.6.orig/fs/utimes.c	2008-07-01 13:52:23.000000000 +0200
+++ linux-2.6/fs/utimes.c	2008-07-01 13:52:29.000000000 +0200
@@ -64,10 +64,6 @@ static int utimes_common(struct path *pa
 
 	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) {

--

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [patch 1/4] vfs: utimes: move owner check into inode_change_ok()
  2008-07-01 13:01 ` [patch 1/4] vfs: utimes: move owner check into inode_change_ok() Miklos Szeredi
@ 2008-07-01 14:09   ` Michael Kerrisk
  2008-07-01 14:23     ` Miklos Szeredi
  0 siblings, 1 reply; 7+ messages in thread
From: Michael Kerrisk @ 2008-07-01 14:09 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: viro, linux-kernel, linux-fsdevel, hch, akpm, Ulrich Drepper,
	Michael Kerrisk

Hi Miklos,

On Tue, Jul 1, 2008 at 3:01 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
>
> Add a new ia_valid flag: ATTR_TIMES_SET, to handle the
> UTIMES_OMIT/UTIMES_NOW and UTIMES_NOW/UTIMES_OMIT cases.  In these
> cases neither ATTR_MTIME_SET nor ATTR_ATIME_SET is in the flags, yet
> the POSIX draft specifies that permission checking is performed the
> same way as if one or both of the times was explicitly set to a
> timestamp.
>
> See the path "vfs: utimensat(): fix error checking for
> {UTIME_NOW,UTIME_OMIT} case" by Michael Kerrisk for the patch
> introducing this behavior.
>
> This is a cleanup, as well as allowing filesystems (NFS/fuse/...) to
> perform their own permission checking instead of the default.

What kernel version/tree is this patch against?

Cheers,

Michael

> CC: Ulrich Drepper <drepper@redhat.com>
> CC: Michael Kerrisk <mtk.manpages@gmail.com>
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> ---
>  fs/attr.c          |    2 +-
>  fs/utimes.c        |   17 ++++-------------
>  include/linux/fs.h |   33 +++++++++++++++++----------------
>  3 files changed, 22 insertions(+), 30 deletions(-)
>
> Index: linux-2.6/fs/attr.c
> ===================================================================
> --- linux-2.6.orig/fs/attr.c    2008-06-27 22:09:08.000000000 +0200
> +++ linux-2.6/fs/attr.c 2008-07-01 13:52:20.000000000 +0200
> @@ -51,7 +51,7 @@ int inode_change_ok(struct inode *inode,
>        }
>
>        /* Check for setting the inode time. */
> -       if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET)) {
> +       if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET | ATTR_TIMES_SET)) {
>                if (!is_owner_or_cap(inode))
>                        goto error;
>        }
> Index: linux-2.6/fs/utimes.c
> ===================================================================
> --- linux-2.6.orig/fs/utimes.c  2008-07-01 08:10:12.000000000 +0200
> +++ linux-2.6/fs/utimes.c       2008-07-01 13:52:20.000000000 +0200
> @@ -101,7 +101,6 @@ long do_utimes(int dfd, char __user *fil
>                     times[1].tv_nsec == UTIME_NOW)
>                times = NULL;
>
> -       /* In most cases, the checks are done in inode_change_ok() */
>        newattrs.ia_valid = ATTR_CTIME | ATTR_MTIME | ATTR_ATIME;
>        if (times) {
>                error = -EPERM;
> @@ -123,21 +122,13 @@ long do_utimes(int dfd, char __user *fil
>                        newattrs.ia_mtime.tv_nsec = times[1].tv_nsec;
>                        newattrs.ia_valid |= ATTR_MTIME_SET;
>                }
> -
>                /*
> -                * For the UTIME_OMIT/UTIME_NOW and UTIME_NOW/UTIME_OMIT
> -                * cases, we need to make an extra check that is not done by
> -                * inode_change_ok().
> +                * Tell inode_change_ok(), that this is an explicit time
> +                * update, even if neither ATTR_ATIME_SET nor ATTR_MTIME_SET
> +                * were used.
>                 */
> -               if (((times[0].tv_nsec == UTIME_NOW &&
> -                           times[1].tv_nsec == UTIME_OMIT)
> -                    ||
> -                    (times[0].tv_nsec == UTIME_OMIT &&
> -                           times[1].tv_nsec == UTIME_NOW))
> -                   && !is_owner_or_cap(inode))
> -                       goto mnt_drop_write_and_out;
> +               newattrs.ia_valid |= ATTR_TIMES_SET;
>        } else {
> -
>                /*
>                 * If times is NULL (or both times are UTIME_NOW),
>                 * then we need to check permissions, because
> Index: linux-2.6/include/linux/fs.h
> ===================================================================
> --- linux-2.6.orig/include/linux/fs.h   2008-07-01 13:52:19.000000000 +0200
> +++ linux-2.6/include/linux/fs.h        2008-07-01 13:52:20.000000000 +0200
> @@ -317,22 +317,23 @@ typedef void (dio_iodone_t)(struct kiocb
>  * Attribute flags.  These should be or-ed together to figure out what
>  * has been changed!
>  */
> -#define ATTR_MODE      1
> -#define ATTR_UID       2
> -#define ATTR_GID       4
> -#define ATTR_SIZE      8
> -#define ATTR_ATIME     16
> -#define ATTR_MTIME     32
> -#define ATTR_CTIME     64
> -#define ATTR_ATIME_SET 128
> -#define ATTR_MTIME_SET 256
> -#define ATTR_FORCE     512     /* Not a change, but a change it */
> -#define ATTR_ATTR_FLAG 1024
> -#define ATTR_KILL_SUID 2048
> -#define ATTR_KILL_SGID 4096
> -#define ATTR_FILE      8192
> -#define ATTR_KILL_PRIV 16384
> -#define ATTR_OPEN      32768   /* Truncating from open(O_TRUNC) */
> +#define ATTR_MODE      (1 << 0)
> +#define ATTR_UID       (1 << 1)
> +#define ATTR_GID       (1 << 2)
> +#define ATTR_SIZE      (1 << 3)
> +#define ATTR_ATIME     (1 << 4)
> +#define ATTR_MTIME     (1 << 5)
> +#define ATTR_CTIME     (1 << 6)
> +#define ATTR_ATIME_SET (1 << 7)
> +#define ATTR_MTIME_SET (1 << 8)
> +#define ATTR_FORCE     (1 << 9) /* Not a change, but a change it */
> +#define ATTR_ATTR_FLAG (1 << 10)
> +#define ATTR_KILL_SUID (1 << 11)
> +#define ATTR_KILL_SGID (1 << 12)
> +#define ATTR_FILE      (1 << 13)
> +#define ATTR_KILL_PRIV (1 << 14)
> +#define ATTR_OPEN      (1 << 15) /* Truncating from open(O_TRUNC) */
> +#define ATTR_TIMES_SET (1 << 16)
>
>  /*
>  * This is the Inode Attributes structure, used for notify_change().  It
>
> --
>



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [patch 1/4] vfs: utimes: move owner check into inode_change_ok()
  2008-07-01 14:09   ` Michael Kerrisk
@ 2008-07-01 14:23     ` Miklos Szeredi
  0 siblings, 0 replies; 7+ messages in thread
From: Miklos Szeredi @ 2008-07-01 14:23 UTC (permalink / raw)
  To: mtk.manpages
  Cc: miklos, viro, linux-kernel, linux-fsdevel, hch, akpm, drepper,
	mtk.manpages

Hi Michael,

On Tue, 1 Jul 2008, Michael Kerrisk wrote:
> On Tue, Jul 1, 2008 at 3:01 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> > From: Miklos Szeredi <mszeredi@suse.cz>
> >
> > Add a new ia_valid flag: ATTR_TIMES_SET, to handle the
> > UTIMES_OMIT/UTIMES_NOW and UTIMES_NOW/UTIMES_OMIT cases.  In these
> > cases neither ATTR_MTIME_SET nor ATTR_ATIME_SET is in the flags, yet
> > the POSIX draft specifies that permission checking is performed the
> > same way as if one or both of the times was explicitly set to a
> > timestamp.
> >
> > See the path "vfs: utimensat(): fix error checking for
> > {UTIME_NOW,UTIME_OMIT} case" by Michael Kerrisk for the patch
> > introducing this behavior.
> >
> > This is a cleanup, as well as allowing filesystems (NFS/fuse/...) to
> > perform their own permission checking instead of the default.
> 
> What kernel version/tree is this patch against?

Against latest git.  2.6.26-rc8-git2 seems to be recent enough.

Miklos

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2008-07-01 14:23 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-07-01 13:01 [patch 0/4] vfs: utimes cleanups Miklos Szeredi
2008-07-01 13:01 ` [patch 1/4] vfs: utimes: move owner check into inode_change_ok() Miklos Szeredi
2008-07-01 14:09   ` Michael Kerrisk
2008-07-01 14:23     ` Miklos Szeredi
2008-07-01 13:01 ` [patch 2/4] vfs: utimes cleanup Miklos Szeredi
2008-07-01 13:01 ` [patch 3/4] fat: dont call notify_change Miklos Szeredi
2008-07-01 13:01 ` [patch 4/4] vfs: immutable inode checking cleanup Miklos Szeredi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®