[patch 12/12] vfs: allow file truncations when both suid and write permissions set

[patch 12/12] vfs: allow file truncations when both suid and write permissions set

[akpm]

From: Amerigo Wang <amwang@redhat.com>

When suid is set and the non-owner user has write permission, any writing
into this file should be allowed and suid should be removed after that.

However, the current kernel only allows writing without truncations, when
we do truncations on that file, we get EPERM.  This is a bug.

Steps to reproduce this bug:

% ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1
% echo h > rootdir/file1
zsh: operation not permitted: rootdir/file1
% ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1
% echo h >> rootdir/file1
% ls -l rootdir/file1
-rwxrwxrwx 1 root root 5 Jun 25 16:34 rootdir/file1

This patch fixes it.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Eric Sandeen <esandeen@redhat.com>
Cc: Eric Paris <eparis@redhat.com>
Cc: Eugene Teo <eteo@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/open.c          |    8 ++++++--
 include/linux/fs.h |    1 +
 mm/filemap.c       |   12 ++++++++++--
 3 files changed, 17 insertions(+), 4 deletions(-)

diff -puN fs/open.c~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set fs/open.c
--- a/fs/open.c~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set
+++ a/fs/open.c
@@ -213,11 +213,15 @@ int do_truncate(struct dentry *dentry, l
 		newattrs.ia_valid |= ATTR_FILE;
 	}
 
+	mutex_lock(&dentry->d_inode->i_mutex);
 	/* Remove suid/sgid on truncate too */
-	newattrs.ia_valid |= should_remove_suid(dentry);
+	err = dentry_remove_suid(dentry);
+	if (err)
+		goto unlock;
 
-	mutex_lock(&dentry->d_inode->i_mutex);
 	err = notify_change(dentry, &newattrs);
+
+ unlock:
 	mutex_unlock(&dentry->d_inode->i_mutex);
 	return err;
 }
diff -puN include/linux/fs.h~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set include/linux/fs.h
--- a/include/linux/fs.h~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set
+++ a/include/linux/fs.h
@@ -2170,6 +2170,7 @@ extern void destroy_inode(struct inode *
 extern struct inode *new_inode(struct super_block *);
 extern int should_remove_suid(struct dentry *);
 extern int file_remove_suid(struct file *);
+extern int dentry_remove_suid(struct dentry *);
 
 extern void __insert_inode_hash(struct inode *, unsigned long hashval);
 extern void remove_inode_hash(struct inode *);
diff -puN mm/filemap.c~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set mm/filemap.c
--- a/mm/filemap.c~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set
+++ a/mm/filemap.c
@@ -1839,9 +1839,11 @@ static int __remove_suid(struct dentry *
 	return notify_change(dentry, &newattrs);
 }
 
-int file_remove_suid(struct file *file)
+/*
+ * Note: you need to hold i_mutex before call this.
+ */
+int dentry_remove_suid(struct dentry *dentry)
 {
-	struct dentry *dentry = file->f_path.dentry;
 	int killsuid = should_remove_suid(dentry);
 	int killpriv = security_inode_need_killpriv(dentry);
 	int error = 0;
@@ -1855,6 +1857,12 @@ int file_remove_suid(struct file *file)
 
 	return error;
 }
+
+int file_remove_suid(struct file *file)
+{
+	struct dentry *dentry = file->f_path.dentry;
+	return dentry_remove_suid(dentry);
+}
 EXPORT_SYMBOL(file_remove_suid);
 
 static size_t __iovec_copy_from_user_inatomic(char *vaddr,
_

Re: [patch 12/12] vfs: allow file truncations when both suid and write permissions set

[OGAWA Hirofumi]

akpm@linux-foundation.org writes:

> diff -puN fs/open.c~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set fs/open.c
> --- a/fs/open.c~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set
> +++ a/fs/open.c
> @@ -213,11 +213,15 @@ int do_truncate(struct dentry *dentry, l
>  		newattrs.ia_valid |= ATTR_FILE;
>  	}
>  
> +	mutex_lock(&dentry->d_inode->i_mutex);
>  	/* Remove suid/sgid on truncate too */
> -	newattrs.ia_valid |= should_remove_suid(dentry);
> +	err = dentry_remove_suid(dentry);
> +	if (err)
> +		goto unlock;

Can't we use ATTR_FORCE for this? Because this calls notify_change()
twice, and I guess this removes s[ug]id even if vmtruncate() (or in
future ->truncate() may return error) or something returned error.

I think it would not be good behavior.

Thanks.

> -	mutex_lock(&dentry->d_inode->i_mutex);
>  	err = notify_change(dentry, &newattrs);
> +
> + unlock:
>  	mutex_unlock(&dentry->d_inode->i_mutex);
>  	return err;
>  }
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [patch 12/12] vfs: allow file truncations when both suid and write permissions set

[Amerigo Wang]

OGAWA Hirofumi wrote:
> akpm@linux-foundation.org writes:
>
>   
>> diff -puN fs/open.c~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set fs/open.c
>> --- a/fs/open.c~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set
>> +++ a/fs/open.c
>> @@ -213,11 +213,15 @@ int do_truncate(struct dentry *dentry, l
>>  		newattrs.ia_valid |= ATTR_FILE;
>>  	}
>>  
>> +	mutex_lock(&dentry->d_inode->i_mutex);
>>  	/* Remove suid/sgid on truncate too */
>> -	newattrs.ia_valid |= should_remove_suid(dentry);
>> +	err = dentry_remove_suid(dentry);
>> +	if (err)
>> +		goto unlock;
>>     
>
> Can't we use ATTR_FORCE for this? Because this calls notify_change()
> twice, and I guess this removes s[ug]id even if vmtruncate() (or in
> future ->truncate() may return error) or something returned error.
>
> I think it would not be good behavior.
>   

Hi, please check:
http://lkml.org/lkml/2009/7/1/459

Re: [patch 12/12] vfs: allow file truncations when both suid and write permissions set

[OGAWA Hirofumi]

Amerigo Wang <amwang@redhat.com> writes:

> OGAWA Hirofumi wrote:
>> akpm@linux-foundation.org writes:
>>
>>   
>>> diff -puN fs/open.c~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set fs/open.c
>>> --- a/fs/open.c~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set
>>> +++ a/fs/open.c
>>> @@ -213,11 +213,15 @@ int do_truncate(struct dentry *dentry, l
>>>  		newattrs.ia_valid |= ATTR_FILE;
>>>  	}
>>>  
>>> +	mutex_lock(&dentry->d_inode->i_mutex);
>>>  	/* Remove suid/sgid on truncate too */
>>> -	newattrs.ia_valid |= should_remove_suid(dentry);
>>> +	err = dentry_remove_suid(dentry);
>>> +	if (err)
>>> +		goto unlock;
>>>     
>>
>> Can't we use ATTR_FORCE for this? Because this calls notify_change()
>> twice, and I guess this removes s[ug]id even if vmtruncate() (or in
>> future ->truncate() may return error) or something returned error.
>>
>> I think it would not be good behavior.
>>   
>
> Hi, please check:
> http://lkml.org/lkml/2009/7/1/459

Sorry for same argument. I see. However, um...

I found this piece in security/selinux/hooks.c

static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
{
	const struct cred *cred = current_cred();

	if (iattr->ia_valid & ATTR_FORCE)
		return 0;

	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
			       ATTR_ATIME_SET | ATTR_MTIME_SET))
		return dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);

	return dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
}

I guess it's assuming the ia_valid doesn't have (ATTR_MODE | ATTR_SIZE),
but truncate() already does it, I don't know whether it's ok. The
definition of ATTR_FORCE is unclear at all, it would be problem. But,
I'm not sure though, I suspect the above code also has problem...

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

Re: [patch 12/12] vfs: allow file truncations when both suid and write permissions set

[OGAWA Hirofumi]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:

> Sorry for same argument. I see. However, um...
>
> I found this piece in security/selinux/hooks.c
>
> static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
> {
> 	const struct cred *cred = current_cred();
>
> 	if (iattr->ia_valid & ATTR_FORCE)
> 		return 0;
>
> 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
> 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
> 		return dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
>
> 	return dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
> }
>
> I guess it's assuming the ia_valid doesn't have (ATTR_MODE | ATTR_SIZE),
> but truncate() already does it, I don't know whether it's ok. The
> definition of ATTR_FORCE is unclear at all, it would be problem. But,
> I'm not sure though, I suspect the above code also has problem...

I found the SuSv3 says for ftruncate():

Upon successful completion, if fildes refers to a regular file,
ftruncate() shall mark for update the last data modification and last
file status change timestamps of the file and the S_ISUID and S_ISGID
bits of the file mode may be cleared. If the ftruncate() function is
unsuccessful, the file is unaffected.

And vmtruncate() can return error easily with RLIMIT_FSIZE or
->s_maxbytes. So, I think clearing s[ug]id first may be bad behavior
without good reason.

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

Re: [patch 12/12] vfs: allow file truncations when both suid and write permissions set

[Amerigo Wang]

OGAWA Hirofumi wrote:
> I found the SuSv3 says for ftruncate():
>
> Upon successful completion, if fildes refers to a regular file,
> ftruncate() shall mark for update the last data modification and last
> file status change timestamps of the file and the S_ISUID and S_ISGID
> bits of the file mode may be cleared. If the ftruncate() function is
> unsuccessful, the file is unaffected.
>
> And vmtruncate() can return error easily with RLIMIT_FSIZE or
> ->s_maxbytes. So, I think clearing s[ug]id first may be bad behavior
> without good reason.
>   

Hmm, you mean we should clear suid after we do notify_change()?
Good point!

I will update the patch now...

Thanks for your review!

Re: [patch 12/12] vfs: allow file truncations when both suid and write permissions set

[OGAWA Hirofumi]

Amerigo Wang <amwang@redhat.com> writes:

> OGAWA Hirofumi wrote:
>> I found the SuSv3 says for ftruncate():
>>
>> Upon successful completion, if fildes refers to a regular file,
>> ftruncate() shall mark for update the last data modification and last
>> file status change timestamps of the file and the S_ISUID and S_ISGID
>> bits of the file mode may be cleared. If the ftruncate() function is
>> unsuccessful, the file is unaffected.
>>
>> And vmtruncate() can return error easily with RLIMIT_FSIZE or
>> ->s_maxbytes. So, I think clearing s[ug]id first may be bad behavior
>> without good reason.
>>   
>
> Hmm, you mean we should clear suid after we do notify_change()?
> Good point!

I guess it would be better than current patch in practice, although it's
not perfect.

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

Re: [patch 12/12] vfs: allow file truncations when both suid and write permissions set

[Amerigo Wang]

OGAWA Hirofumi wrote:
> Amerigo Wang <amwang@redhat.com> writes:
>
>   
>> OGAWA Hirofumi wrote:
>>     
>>> akpm@linux-foundation.org writes:
>>>
>>>   
>>>       
>>>> diff -puN fs/open.c~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set fs/open.c
>>>> --- a/fs/open.c~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set
>>>> +++ a/fs/open.c
>>>> @@ -213,11 +213,15 @@ int do_truncate(struct dentry *dentry, l
>>>>  		newattrs.ia_valid |= ATTR_FILE;
>>>>  	}
>>>>  
>>>> +	mutex_lock(&dentry->d_inode->i_mutex);
>>>>  	/* Remove suid/sgid on truncate too */
>>>> -	newattrs.ia_valid |= should_remove_suid(dentry);
>>>> +	err = dentry_remove_suid(dentry);
>>>> +	if (err)
>>>> +		goto unlock;
>>>>     
>>>>         
>>> Can't we use ATTR_FORCE for this? Because this calls notify_change()
>>> twice, and I guess this removes s[ug]id even if vmtruncate() (or in
>>> future ->truncate() may return error) or something returned error.
>>>
>>> I think it would not be good behavior.
>>>   
>>>       
>> Hi, please check:
>> http://lkml.org/lkml/2009/7/1/459
>>     
>
> Sorry for same argument. I see. However, um...
>
> I found this piece in security/selinux/hooks.c
>
> static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
> {
> 	const struct cred *cred = current_cred();
>
> 	if (iattr->ia_valid & ATTR_FORCE)
> 		return 0;
>
> 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
> 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
> 		return dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
>
> 	return dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
> }
>
> I guess it's assuming the ia_valid doesn't have (ATTR_MODE | ATTR_SIZE),
> but truncate() already does it, I don't know whether it's ok. 

No, here we should only force ATTR_KILL_SUID and/or ATTR_KILL_SGID. 
do_truncate() has ATTR_SIZE and ATTR_FILE.

Re: [patch 12/12] vfs: allow file truncations when both suid and write permissions set

[OGAWA Hirofumi]

Amerigo Wang <amwang@redhat.com> writes:

>> static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
>> {
>> 	const struct cred *cred = current_cred();
>>
>> 	if (iattr->ia_valid & ATTR_FORCE)
>> 		return 0;
>>
>> 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
>> 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
>> 		return dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
>>
>> 	return dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
>> }
>>
>> I guess it's assuming the ia_valid doesn't have (ATTR_MODE | ATTR_SIZE),
>> but truncate() already does it, I don't know whether it's ok. 
>
> No, here we should only force ATTR_KILL_SUID and/or ATTR_KILL_SGID. 
> do_truncate() has ATTR_SIZE and ATTR_FILE.

I guess security module should do,

	ia_valid = iattr->ia_valid;
	if (!(ia_valid & ATTR_FORCE) && (ia_valid & ATTR_FORCE_MASK)) {
		err = dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
		if (err)
                	return err;
		ia_valid &= ~ATTR_FORCE_MASK;
	}
	if (ia_valid & ATTR_NOT_FORCE_MASK)
	 	err = dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
	return err;

or something. Because do_truncate() already do (ATTR_MODE | ATTR_SIZE)
without ATTR_FORCE.

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

Re: [patch 12/12] vfs: allow file truncations when both suid and write permissions set

[OGAWA Hirofumi]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:

> Amerigo Wang <amwang@redhat.com> writes:
>
>>> static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
>>> {
>>> 	const struct cred *cred = current_cred();
>>>
>>> 	if (iattr->ia_valid & ATTR_FORCE)
>>> 		return 0;
>>>
>>> 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
>>> 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
>>> 		return dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
>>>
>>> 	return dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
>>> }
>>>
>>> I guess it's assuming the ia_valid doesn't have (ATTR_MODE | ATTR_SIZE),
>>> but truncate() already does it, I don't know whether it's ok. 
>>
>> No, here we should only force ATTR_KILL_SUID and/or ATTR_KILL_SGID. 
>> do_truncate() has ATTR_SIZE and ATTR_FILE.
>
> I guess security module should do,
>
> 	ia_valid = iattr->ia_valid;
> 	if (!(ia_valid & ATTR_FORCE) && (ia_valid & ATTR_FORCE_MASK)) {
> 		err = dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
> 		if (err)
>                 	return err;
> 		ia_valid &= ~ATTR_FORCE_MASK;
> 	}
> 	if (ia_valid & ATTR_NOT_FORCE_MASK)
> 	 	err = dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
> 	return err;
>
> or something. Because do_truncate() already do (ATTR_MODE | ATTR_SIZE)
> without ATTR_FORCE.

BTW, it seems original code doesn't check ATTR_SIZE if (ATTR_MODE |
ATTR_SIZE), right?  So, ATTR_FORCE is just forcing ATTR_MODE, but I
guess that's problem itself.

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

Re: [patch 12/12] vfs: allow file truncations when both suid and write permissions set

[Amerigo Wang]

OGAWA Hirofumi wrote:
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
>
>   
>> Amerigo Wang <amwang@redhat.com> writes:
>>
>>     
>>>> static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
>>>> {
>>>> 	const struct cred *cred = current_cred();
>>>>
>>>> 	if (iattr->ia_valid & ATTR_FORCE)
>>>> 		return 0;
>>>>
>>>> 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
>>>> 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
>>>> 		return dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
>>>>
>>>> 	return dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
>>>> }
>>>>
>>>> I guess it's assuming the ia_valid doesn't have (ATTR_MODE | ATTR_SIZE),
>>>> but truncate() already does it, I don't know whether it's ok. 
>>>>         
>>> No, here we should only force ATTR_KILL_SUID and/or ATTR_KILL_SGID. 
>>> do_truncate() has ATTR_SIZE and ATTR_FILE.
>>>       
>> I guess security module should do,
>>
>> 	ia_valid = iattr->ia_valid;
>> 	if (!(ia_valid & ATTR_FORCE) && (ia_valid & ATTR_FORCE_MASK)) {
>> 		err = dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
>> 		if (err)
>>                 	return err;
>> 		ia_valid &= ~ATTR_FORCE_MASK;
>> 	}
>> 	if (ia_valid & ATTR_NOT_FORCE_MASK)
>> 	 	err = dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
>> 	return err;
>>
>> or something. Because do_truncate() already do (ATTR_MODE | ATTR_SIZE)
>> without ATTR_FORCE.
>>     
>
> BTW, it seems original code doesn't check ATTR_SIZE if (ATTR_MODE |
> ATTR_SIZE), right?  So, ATTR_FORCE is just forcing ATTR_MODE, but I
> guess that's problem itself.
>   

I am not sure if I understand you correctly... You must be referring 
notify_change(), it seems to do what you said.

But clearly ATTR_FORCE is the way to bypass the security module on 
purpose. I agree that we perhaps should have some wrapper function to do 
this (instead of calling notify_change() twice), but currently this is fine.

Re: [patch 12/12] vfs: allow file truncations when both suid and write permissions set

[OGAWA Hirofumi]

Amerigo Wang <amwang@redhat.com> writes:

> OGAWA Hirofumi wrote:
>> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
>>
>>   
>>> Amerigo Wang <amwang@redhat.com> writes:
>>>
>>>     
>>>>> static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
>>>>> {
>>>>> 	const struct cred *cred = current_cred();
>>>>>
>>>>> 	if (iattr->ia_valid & ATTR_FORCE)
>>>>> 		return 0;
>>>>>
>>>>> 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
>>>>> 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
>>>>> 		return dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
>>>>>
>>>>> 	return dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
>>>>> }
>>>>>
>>>>> I guess it's assuming the ia_valid doesn't have (ATTR_MODE | ATTR_SIZE),
>>>>> but truncate() already does it, I don't know whether it's ok. 
>>>>>         
>>>> No, here we should only force ATTR_KILL_SUID and/or ATTR_KILL_SGID. 
>>>> do_truncate() has ATTR_SIZE and ATTR_FILE.
>>>>       
>>> I guess security module should do,
>>>
>>> 	ia_valid = iattr->ia_valid;
>>> 	if (!(ia_valid & ATTR_FORCE) && (ia_valid & ATTR_FORCE_MASK)) {
>>> 		err = dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
>>> 		if (err)
>>>                 	return err;
>>> 		ia_valid &= ~ATTR_FORCE_MASK;
>>> 	}
>>> 	if (ia_valid & ATTR_NOT_FORCE_MASK)
>>> 	 	err = dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
>>> 	return err;
>>>
>>> or something. Because do_truncate() already do (ATTR_MODE | ATTR_SIZE)
>>> without ATTR_FORCE.
>>>     
>>
>> BTW, it seems original code doesn't check ATTR_SIZE if (ATTR_MODE |
>> ATTR_SIZE), right?  So, ATTR_FORCE is just forcing ATTR_MODE, but I
>> guess that's problem itself.
>>   
>
> I am not sure if I understand you correctly... You must be referring 
> notify_change(), it seems to do what you said.

As far as I can see,

do_truncate pass ATTR_SIZE
	security_inode_setattr()
		dentry_has_perm(cred, NULL, dentry, FILE__WRITE);

do_truncate pass (ATTR_SIZE | ATTR_MODE) [ATTR_MODE is set by ATTR_KILL_*.
                                          This should works fine for
	                                  file owner without patch.]
	security_inode_setattr()
		dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);

Doesn't security module really have to call dentry_has_perm() with
FILE__WRITE if (ATTR_SIZE | ATTR_MODE)?

> But clearly ATTR_FORCE is the way to bypass the security module on
> purpose.

Even if it's true, why don't we change security modules? I'm saying
always bypass was wrong way, if it doesn't want. AFAIK, it's not binary
driver, and it has good source codes.

> I agree that we perhaps should have some wrapper function to do this
> (instead of calling notify_change() twice), but currently this is
> fine.

I don't object to that patch though, but it's fine? Really? The error
handling is wrong, and note to call fs-driver twice means it can be
cause of I/O (network, storage) twice, fsnotify() is also called twice,
and fs-driver can't fix those.

I don't think those are fine at all.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>