move IMMUTABLE|APPEND checks to notify_change()

[PATCH] move IMMUTABLE|APPEND checks to notify_change()

This patch moves lots of IMMUTABLE and APPEND flag checks scattered all around to more logical place in notify_change().

@@ -109,6 +109,9 @@ int notify_change(struct dentry * dentry

	struct timespec now;
	unsigned int ia_valid = attr->ia_valid;

+ if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
+ return -EPERM;
+

	mode = inode->i_mode;
	now = current_fs_time(inode->i_sb);

@@ -252,10 +252,6 @@ static long do_sys_truncate(const char _

	if (IS_RDONLY(inode))
		goto dput_and_out;

- error = -EPERM;
- if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
- goto dput_and_out;
-

	/*
	 * Make sure that there are no leases.
	 */
@@ -316,10 +312,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);
@@ -385,10 +377,6 @@ asmlinkage long sys_utime(char __user * 
	/* 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 dput_and_out;
-

		error = get_user(newattrs.ia_atime.tv_sec, &times->actime);
		newattrs.ia_atime.tv_nsec = 0;
		if (!error)
@@ -398,15 +386,9 @@ asmlinkage long sys_utime(char __user * 
			goto dput_and_out;

		newattrs.ia_valid |= ATTR_ATIME_SET | ATTR_MTIME_SET;

- } else {
- error = -EACCES;
- if (IS_IMMUTABLE(inode))
- goto dput_and_out;
-
- if (current->fsuid != inode->i_uid &&
+ } else if (current->fsuid != inode->i_uid &&

		    (error = vfs_permission(&nd, MAY_WRITE)) != 0)
-			goto dput_and_out;
-	}
+		goto dput_and_out;
	mutex_lock(&inode->i_mutex);
	error = notify_change(nd.dentry, &newattrs);
	mutex_unlock(&inode->i_mutex);
@@ -442,24 +424,14 @@ 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 dput_and_out;
-

		newattrs.ia_atime.tv_sec = times[0].tv_sec;
		newattrs.ia_atime.tv_nsec = times[0].tv_usec * 1000;
		newattrs.ia_mtime.tv_sec = times[1].tv_sec;
		newattrs.ia_mtime.tv_nsec = times[1].tv_usec * 1000;
		newattrs.ia_valid |= ATTR_ATIME_SET | ATTR_MTIME_SET;

- } else {
- error = -EACCES;
- if (IS_IMMUTABLE(inode))
- goto dput_and_out;
-
- if (current->fsuid != inode->i_uid &&
+ } else if (current->fsuid != inode->i_uid &&

		    (error = vfs_permission(&nd, MAY_WRITE)) != 0)
-			goto dput_and_out;
-	}
+		goto dput_and_out;
	mutex_lock(&inode->i_mutex);
	error = notify_change(nd.dentry, &newattrs);
	mutex_unlock(&inode->i_mutex);
@@ -638,9 +610,6 @@ asmlinkage long sys_fchmod(unsigned int 
	err = -EROFS;
	if (IS_RDONLY(inode))
		goto out_putf;
-	err = -EPERM;
-	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
-		goto out_putf;
	mutex_lock(&inode->i_mutex);
	if (mode == (mode_t) -1)
		mode = inode->i_mode;
@@ -672,10 +641,6 @@ asmlinkage long sys_fchmodat(int dfd, co
	if (IS_RDONLY(inode))
		goto dput_and_out;

- error = -EPERM;
- if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
- goto dput_and_out;
-

	mutex_lock(&inode->i_mutex);
	if (mode == (mode_t) -1)
		mode = inode->i_mode;
@@ -709,9 +674,6 @@ static int chown_common(struct dentry * 
	error = -EROFS;
	if (IS_RDONLY(inode))
		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;

NAK. For example, you are allowed to do unames(file, NULL) on any file you own or can write to, whether it's append-only or not. With your change that gets -EPERM.

Do you meant utimes(file, NULL)?
But is it correct behaviour? Why then do you get -EPERM on utimes(file, smth) if the file is append-only? And why do you get -EACCESS on utimes(file, NULL), if this file is immutable?

Could you explain, why is it done so?

Thanks,
Dmitry.

Al, will you ACK the patch allowing to set current times in notify_change() for APPEND files?

However, I'd like to see an explanation on dim@'s question why semantics are different and current time is allowed to be set, while arbitrary time is not.

Kirill

Does such check in notify_change() looks better for you?

notify_change():

       if (IS_IMMUTABLE(inode))
               return -EPERM;
       if (IS_APPEND(inode) &&
                       (ia_valid & ~(ATTR_CTIME | ATTR_MTIME | ATTR_ATIME)))
               return -EPERM;

Thanks,
Kirill

RTFPOSIX...

Short version:
* immutable files are immutable, including metadata
* append-only files may be touched (when you write to the end), which
means that you can touch them. Which is what utimes(file, NULL) does.
* you can not truncate append-only file, overwrite already written
data or set timestamps to arbitrary values.

That's where the difference between utimes(file, NULL) and utimes(file, p) is - the former basically is a write-without-write ("touch foo") and the latter directly assigns to timestamps. Permissions needed for these are obviously different.

Please, read POSIX/SuS when modifying behaviour of syscalls. Really.