nfsd: EACCES vs EPERM on utime()/utimes() calls

nfsd: EACCES vs EPERM on utime()/utimes() calls

[Stanislav Kholmanskikh]

Hello.

As the man page for utime/utimes states [1], EPERM is returned if the 
second argument of utime/utimes is not NULL and:
  * the caller's effective user id does not match the owner of the file
  * the caller does not have write access to the file
  * the caller is not privileged

However, I don't see this behavior with NFS, I see EACCES is generated 
instead.

Please, consider this test:

#include <stdio.h>
#include <sys/types.h>
#include <sys/time.h>
#include <utime.h>

int main(int argc, char *argv[])
{
	struct utimbuf u = { .actime = 0, .modtime = 0 };
	struct timeval tv = { .tv_sec = 0, .tv_usec = 0 };

	if (utime(argv[1], &u))
		perror("utime() failed");

	if (utimes(argv[1], &tv))
		perror("utimes() failed");
	
	return 0;
}

In my environment the kernel is 4.1.0-rc6 x86_64, and there are 2 NFS 
mounted directories:
127.0.0.1:/opt/export/disk0 on /mnt/lin type nfs (rw,vers=3,addr=127.0.0.1)
192.168.0.12:/export/bla on /mnt/sol type nfs (rw,vers=3,addr=192.168.0.12)

/mnt/sol is from Solaris 11.2 x86_64. /opt/export/disk0 is handled by 
the in-kernel nfs server.

Execution of the above test program gives:

* the server is Linux -> EACCES is generated:

[stas@ol6-x64 tmp]$ ls -l /mnt/lin/test_file
-rw-r--r-- 1 root root 0 Jun  1 17:28 /mnt/lin/test_file
[stas@ol6-x64 tmp]$ strace -e utime,utimes ./utime_test /mnt/lin/test_file
utime("/mnt/lin/test_file", [0, 0])     = -1 EACCES (Permission denied)
utime() failed: Permission denied
utimes("/mnt/lin/test_file", {{0, 0}, {0, 0}}) = -1 EACCES (Permission 
denied)
utimes() failed: Permission denied

* the server is Solaris 11.2 -> EPERM is generated

[stas@ol6-x64 tmp]$ ls -l /mnt/sol/test_file
-rw-r--r--+ 1 root root 0 Jun  1  2015 /mnt/sol/test_file
[stas@ol6-x64 tmp]$ strace -e utime,utimes ./utime_test /mnt/sol/test_file
utime("/mnt/sol/test_file", [0, 0])     = -1 EPERM (Operation not permitted)
utime() failed: Operation not permitted
utimes("/mnt/sol/test_file", {{0, 0}, {0, 0}}) = -1 EPERM (Operation not 
permitted)
utimes() failed: Operation not permitted

* on a local ext4 file system EPERM is generated:

[stas@ol6-x64 tmp]$ ls -l /tmp/test_file
-rw-r--r-- 1 root root 0 Jun  1 17:51 /tmp/test_file
[stas@ol6-x64 tmp]$ strace -e utime,utimes ./utime_test /tmp/test_file
utime("/tmp/test_file", [0, 0])         = -1 EPERM (Operation not permitted)
utime() failed: Operation not permitted
utimes("/tmp/test_file", {{0, 0}, {0, 0}}) = -1 EPERM (Operation not 
permitted)
utimes() failed: Operation not permitted

Plus EPERM is generated when the NFS server is FreeBSD 9.1.

Could anybody, clarify, if the described behavior a bug in the Linux NFS 
server implementation or not?

Thank you.

[1] http://man7.org/linux/man-pages/man2/utime.2.html

PS: this all was found using utime06, utimes01 LTP test cases.

Re: nfsd: EACCES vs EPERM on utime()/utimes() calls

[J. Bruce Fields]

On Mon, Jun 01, 2015 at 06:01:02PM +0300, Stanislav Kholmanskikh wrote:
> Hello.
> 
> As the man page for utime/utimes states [1], EPERM is returned if
> the second argument of utime/utimes is not NULL and:
>  * the caller's effective user id does not match the owner of the file
>  * the caller does not have write access to the file
>  * the caller is not privileged
> 
> However, I don't see this behavior with NFS, I see EACCES is
> generated instead.

Agreed that it's probably a server bug.  (Have you run across a case
where this makes a difference?)

Looking at nfsd_setattr()....  The main work is done by notify_change(),
which is probably doing the right thing.  But before that there's an
fh_verify()--looks like that is expected to fail in your case.  I bet
that's the cause.

--b.

> 
> Please, consider this test:
> 
> #include <stdio.h>
> #include <sys/types.h>
> #include <sys/time.h>
> #include <utime.h>
> 
> int main(int argc, char *argv[])
> {
> 	struct utimbuf u = { .actime = 0, .modtime = 0 };
> 	struct timeval tv = { .tv_sec = 0, .tv_usec = 0 };
> 
> 	if (utime(argv[1], &u))
> 		perror("utime() failed");
> 
> 	if (utimes(argv[1], &tv))
> 		perror("utimes() failed");
> 	
> 	return 0;
> }
> 
> In my environment the kernel is 4.1.0-rc6 x86_64, and there are 2
> NFS mounted directories:
> 127.0.0.1:/opt/export/disk0 on /mnt/lin type nfs (rw,vers=3,addr=127.0.0.1)
> 192.168.0.12:/export/bla on /mnt/sol type nfs (rw,vers=3,addr=192.168.0.12)
> 
> /mnt/sol is from Solaris 11.2 x86_64. /opt/export/disk0 is handled
> by the in-kernel nfs server.
> 
> Execution of the above test program gives:
> 
> * the server is Linux -> EACCES is generated:
> 
> [stas@ol6-x64 tmp]$ ls -l /mnt/lin/test_file
> -rw-r--r-- 1 root root 0 Jun  1 17:28 /mnt/lin/test_file
> [stas@ol6-x64 tmp]$ strace -e utime,utimes ./utime_test /mnt/lin/test_file
> utime("/mnt/lin/test_file", [0, 0])     = -1 EACCES (Permission denied)
> utime() failed: Permission denied
> utimes("/mnt/lin/test_file", {{0, 0}, {0, 0}}) = -1 EACCES
> (Permission denied)
> utimes() failed: Permission denied
> 
> * the server is Solaris 11.2 -> EPERM is generated
> 
> [stas@ol6-x64 tmp]$ ls -l /mnt/sol/test_file
> -rw-r--r--+ 1 root root 0 Jun  1  2015 /mnt/sol/test_file
> [stas@ol6-x64 tmp]$ strace -e utime,utimes ./utime_test /mnt/sol/test_file
> utime("/mnt/sol/test_file", [0, 0])     = -1 EPERM (Operation not permitted)
> utime() failed: Operation not permitted
> utimes("/mnt/sol/test_file", {{0, 0}, {0, 0}}) = -1 EPERM (Operation
> not permitted)
> utimes() failed: Operation not permitted
> 
> * on a local ext4 file system EPERM is generated:
> 
> [stas@ol6-x64 tmp]$ ls -l /tmp/test_file
> -rw-r--r-- 1 root root 0 Jun  1 17:51 /tmp/test_file
> [stas@ol6-x64 tmp]$ strace -e utime,utimes ./utime_test /tmp/test_file
> utime("/tmp/test_file", [0, 0])         = -1 EPERM (Operation not permitted)
> utime() failed: Operation not permitted
> utimes("/tmp/test_file", {{0, 0}, {0, 0}}) = -1 EPERM (Operation not
> permitted)
> utimes() failed: Operation not permitted
> 
> Plus EPERM is generated when the NFS server is FreeBSD 9.1.
> 
> Could anybody, clarify, if the described behavior a bug in the Linux
> NFS server implementation or not?
> 
> Thank you.
> 
> [1] http://man7.org/linux/man-pages/man2/utime.2.html
> 
> PS: this all was found using utime06, utimes01 LTP test cases.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: nfsd: EACCES vs EPERM on utime()/utimes() calls

[Stanislav Kholmanskikh]

On 06/02/2015 12:23 AM, bfields@fieldses.org wrote:
> On Mon, Jun 01, 2015 at 06:01:02PM +0300, Stanislav Kholmanskikh wrote:
>> Hello.
>>
>> As the man page for utime/utimes states [1], EPERM is returned if
>> the second argument of utime/utimes is not NULL and:
>>   * the caller's effective user id does not match the owner of the file
>>   * the caller does not have write access to the file
>>   * the caller is not privileged
>>
>> However, I don't see this behavior with NFS, I see EACCES is
>> generated instead.
>
> Agreed that it's probably a server bug.  (Have you run across a case
> where this makes a difference?)

Thank you.

No, I've not seen such a real-word scenario.

I have these LTP test cases failing:

* 
https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utime/utime06.c
* 
https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utimes/utimes01.c

and it makes me a bit nervous :)

>
> Looking at nfsd_setattr()....  The main work is done by notify_change(),
> which is probably doing the right thing.  But before that there's an
> fh_verify()--looks like that is expected to fail in your case.  I bet
> that's the cause.

Ok.

I doubt I can fix it by myself (at least quickly). So I am happy if 
anyone more experienced will look at it as well :)

Anyway, if nobody is interested, I'll give it a try, but later.

Thanks again.

>
> --b.
>
>>
>> Please, consider this test:
>>
>> #include <stdio.h>
>> #include <sys/types.h>
>> #include <sys/time.h>
>> #include <utime.h>
>>
>> int main(int argc, char *argv[])
>> {
>> 	struct utimbuf u = { .actime = 0, .modtime = 0 };
>> 	struct timeval tv = { .tv_sec = 0, .tv_usec = 0 };
>>
>> 	if (utime(argv[1], &u))
>> 		perror("utime() failed");
>>
>> 	if (utimes(argv[1], &tv))
>> 		perror("utimes() failed");
>> 	
>> 	return 0;
>> }
>>
>> In my environment the kernel is 4.1.0-rc6 x86_64, and there are 2
>> NFS mounted directories:
>> 127.0.0.1:/opt/export/disk0 on /mnt/lin type nfs (rw,vers=3,addr=127.0.0.1)
>> 192.168.0.12:/export/bla on /mnt/sol type nfs (rw,vers=3,addr=192.168.0.12)
>>
>> /mnt/sol is from Solaris 11.2 x86_64. /opt/export/disk0 is handled
>> by the in-kernel nfs server.
>>
>> Execution of the above test program gives:
>>
>> * the server is Linux -> EACCES is generated:
>>
>> [stas@ol6-x64 tmp]$ ls -l /mnt/lin/test_file
>> -rw-r--r-- 1 root root 0 Jun  1 17:28 /mnt/lin/test_file
>> [stas@ol6-x64 tmp]$ strace -e utime,utimes ./utime_test /mnt/lin/test_file
>> utime("/mnt/lin/test_file", [0, 0])     = -1 EACCES (Permission denied)
>> utime() failed: Permission denied
>> utimes("/mnt/lin/test_file", {{0, 0}, {0, 0}}) = -1 EACCES
>> (Permission denied)
>> utimes() failed: Permission denied
>>
>> * the server is Solaris 11.2 -> EPERM is generated
>>
>> [stas@ol6-x64 tmp]$ ls -l /mnt/sol/test_file
>> -rw-r--r--+ 1 root root 0 Jun  1  2015 /mnt/sol/test_file
>> [stas@ol6-x64 tmp]$ strace -e utime,utimes ./utime_test /mnt/sol/test_file
>> utime("/mnt/sol/test_file", [0, 0])     = -1 EPERM (Operation not permitted)
>> utime() failed: Operation not permitted
>> utimes("/mnt/sol/test_file", {{0, 0}, {0, 0}}) = -1 EPERM (Operation
>> not permitted)
>> utimes() failed: Operation not permitted
>>
>> * on a local ext4 file system EPERM is generated:
>>
>> [stas@ol6-x64 tmp]$ ls -l /tmp/test_file
>> -rw-r--r-- 1 root root 0 Jun  1 17:51 /tmp/test_file
>> [stas@ol6-x64 tmp]$ strace -e utime,utimes ./utime_test /tmp/test_file
>> utime("/tmp/test_file", [0, 0])         = -1 EPERM (Operation not permitted)
>> utime() failed: Operation not permitted
>> utimes("/tmp/test_file", {{0, 0}, {0, 0}}) = -1 EPERM (Operation not
>> permitted)
>> utimes() failed: Operation not permitted
>>
>> Plus EPERM is generated when the NFS server is FreeBSD 9.1.
>>
>> Could anybody, clarify, if the described behavior a bug in the Linux
>> NFS server implementation or not?
>>
>> Thank you.
>>
>> [1] http://man7.org/linux/man-pages/man2/utime.2.html
>>
>> PS: this all was found using utime06, utimes01 LTP test cases.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: nfsd: EACCES vs EPERM on utime()/utimes() calls

[Kinglong Mee]

On 6/3/2015 12:09 AM, Stanislav Kholmanskikh wrote:
> On 06/02/2015 12:23 AM, bfields@fieldses.org wrote:
>> On Mon, Jun 01, 2015 at 06:01:02PM +0300, Stanislav Kholmanskikh wrote:
>>> Hello.
>>>
>>> As the man page for utime/utimes states [1], EPERM is returned if
>>> the second argument of utime/utimes is not NULL and:
>>>   * the caller's effective user id does not match the owner of the file
>>>   * the caller does not have write access to the file
>>>   * the caller is not privileged
>>>
>>> However, I don't see this behavior with NFS, I see EACCES is
>>> generated instead.
>>
>> Agreed that it's probably a server bug.  (Have you run across a case
>> where this makes a difference?)
> 
> Thank you.
> 
> No, I've not seen such a real-word scenario.
> 
> I have these LTP test cases failing:
> 
> * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utime/utime06.c
> * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utimes/utimes01.c
> 
> and it makes me a bit nervous :)
> 
>>
>> Looking at nfsd_setattr()....  The main work is done by notify_change(),
>> which is probably doing the right thing.  But before that there's an
>> fh_verify()--looks like that is expected to fail in your case.  I bet
>> that's the cause.

Yes, it is.

nfsd do the permission checking before notify_change() as,

/* This assumes  NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));

return -EACCES for non-owner user.

> 
> I doubt I can fix it by myself (at least quickly). So I am happy if anyone more experienced will look at it as well :)
> 
> Anyway, if nobody is interested, I'll give it a try, but later.

Here is a diff patch for this problem, please try testing.
If okay, I will send an official patch.

Note: must apply the following patch first in the url,
http://git.linux-nfs.org/?p=bfields/linux.git;a=commitdiff;h=cc265089ce1b176dde963c74b53593446ee7f99a

thanks,
Kinglong Mee
-------------------------------------------------------------
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 84d770b..2533088 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -407,10 +371,23 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 	bool		get_write_count;
 	int		size_change = 0;
 
-	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
+	if (iap->ia_valid & ATTR_SIZE) {
 		accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
-	if (iap->ia_valid & ATTR_SIZE)
 		ftype = S_IFREG;
+	}
+
+	/*
+	 * According to utimes_common(),
+	 *
+	 * If times is NULL (or both times are UTIME_NOW),
+	 * then we need to check permissions, because
+	 * inode_change_ok() won't do it.
+	 */
+	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME)) {
+		accmode |= NFSD_MAY_OWNER_OVERRIDE;
+		if (!(iap->ia_valid & (ATTR_ATIME_SET | ATTR_MTIME_SET)))
+			accmode |= NFSD_MAY_WRITE;
+	}
 
 	/* Callers that do fh_verify should do the fh_want_write: */
 	get_write_count = !fhp->fh_dentry;

Re: nfsd: EACCES vs EPERM on utime()/utimes() calls

[J. Bruce Fields]

On Thu, Jun 04, 2015 at 08:43:14PM +0800, Kinglong Mee wrote:
> On 6/3/2015 12:09 AM, Stanislav Kholmanskikh wrote:
> > On 06/02/2015 12:23 AM, bfields@fieldses.org wrote:
> >> On Mon, Jun 01, 2015 at 06:01:02PM +0300, Stanislav Kholmanskikh wrote:
> >>> Hello.
> >>>
> >>> As the man page for utime/utimes states [1], EPERM is returned if
> >>> the second argument of utime/utimes is not NULL and:
> >>>   * the caller's effective user id does not match the owner of the file
> >>>   * the caller does not have write access to the file
> >>>   * the caller is not privileged
> >>>
> >>> However, I don't see this behavior with NFS, I see EACCES is
> >>> generated instead.
> >>
> >> Agreed that it's probably a server bug.  (Have you run across a case
> >> where this makes a difference?)
> > 
> > Thank you.
> > 
> > No, I've not seen such a real-word scenario.
> > 
> > I have these LTP test cases failing:
> > 
> > * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utime/utime06.c
> > * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utimes/utimes01.c
> > 
> > and it makes me a bit nervous :)
> > 
> >>
> >> Looking at nfsd_setattr()....  The main work is done by notify_change(),
> >> which is probably doing the right thing.  But before that there's an
> >> fh_verify()--looks like that is expected to fail in your case.  I bet
> >> that's the cause.
> 
> Yes, it is.
> 
> nfsd do the permission checking before notify_change() as,
> 
> /* This assumes  NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
> err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));
> 
> return -EACCES for non-owner user.
> 
> > 
> > I doubt I can fix it by myself (at least quickly). So I am happy if anyone more experienced will look at it as well :)
> > 
> > Anyway, if nobody is interested, I'll give it a try, but later.
> 
> Here is a diff patch for this problem, please try testing.
> If okay, I will send an official patch.
> 
> Note: must apply the following patch first in the url,
> http://git.linux-nfs.org/?p=bfields/linux.git;a=commitdiff;h=cc265089ce1b176dde963c74b53593446ee7f99a

We could do that if we have to.

I wonder if we could instead skip the fh_verify's inode_permission call
entirely?  Most callers of fh_verify don't need the inode_permission
call at all as far as I can tell, because the following vfs operation
does permission checking already.

We still need some nfs-specific checking, I guess (e.g. to make sure we
aren't allowing setattr on a read-only export of a writeable mount), but
the inode_permission itself I think we can usually skip.

Andreas Gruenbacher has also been complaining that the redundant
inode_permission calls make the richacl work more difficult, I can't
remember why exactly.

--b.

> 
> thanks,
> Kinglong Mee
> -------------------------------------------------------------
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 84d770b..2533088 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -407,10 +371,23 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
>  	bool		get_write_count;
>  	int		size_change = 0;
>  
> -	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
> +	if (iap->ia_valid & ATTR_SIZE) {
>  		accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
> -	if (iap->ia_valid & ATTR_SIZE)
>  		ftype = S_IFREG;
> +	}
> +
> +	/*
> +	 * According to utimes_common(),
> +	 *
> +	 * If times is NULL (or both times are UTIME_NOW),
> +	 * then we need to check permissions, because
> +	 * inode_change_ok() won't do it.
> +	 */
> +	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME)) {
> +		accmode |= NFSD_MAY_OWNER_OVERRIDE;
> +		if (!(iap->ia_valid & (ATTR_ATIME_SET | ATTR_MTIME_SET)))
> +			accmode |= NFSD_MAY_WRITE;
> +	}
>  
>  	/* Callers that do fh_verify should do the fh_want_write: */
>  	get_write_count = !fhp->fh_dentry;

Re: nfsd: EACCES vs EPERM on utime()/utimes() calls

[Al Viro]

On Thu, Jun 04, 2015 at 04:27:25PM -0400, J. Bruce Fields wrote:

> I wonder if we could instead skip the fh_verify's inode_permission call
> entirely?  Most callers of fh_verify don't need the inode_permission
> call at all as far as I can tell, because the following vfs operation
> does permission checking already.

In case of notify_change() we only pass a dentry, so anything mount-dependent
must be done in callers...  Actually, I wonder how does tomoyo and its ilk
interact with nfsd?

Re: nfsd: EACCES vs EPERM on utime()/utimes() calls

[Kinglong Mee]

On 6/5/2015 4:27 AM, J. Bruce Fields wrote:
> On Thu, Jun 04, 2015 at 08:43:14PM +0800, Kinglong Mee wrote:
>> On 6/3/2015 12:09 AM, Stanislav Kholmanskikh wrote:
>>> On 06/02/2015 12:23 AM, bfields@fieldses.org wrote:
>>>> On Mon, Jun 01, 2015 at 06:01:02PM +0300, Stanislav Kholmanskikh wrote:
>>>>> Hello.
>>>>>
>>>>> As the man page for utime/utimes states [1], EPERM is returned if
>>>>> the second argument of utime/utimes is not NULL and:
>>>>>   * the caller's effective user id does not match the owner of the file
>>>>>   * the caller does not have write access to the file
>>>>>   * the caller is not privileged
>>>>>
>>>>> However, I don't see this behavior with NFS, I see EACCES is
>>>>> generated instead.
>>>>
>>>> Agreed that it's probably a server bug.  (Have you run across a case
>>>> where this makes a difference?)
>>>
>>> Thank you.
>>>
>>> No, I've not seen such a real-word scenario.
>>>
>>> I have these LTP test cases failing:
>>>
>>> * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utime/utime06.c
>>> * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utimes/utimes01.c
>>>
>>> and it makes me a bit nervous :)
>>>
>>>>
>>>> Looking at nfsd_setattr()....  The main work is done by notify_change(),
>>>> which is probably doing the right thing.  But before that there's an
>>>> fh_verify()--looks like that is expected to fail in your case.  I bet
>>>> that's the cause.
>>
>> Yes, it is.
>>
>> nfsd do the permission checking before notify_change() as,
>>
>> /* This assumes  NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
>> err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));
>>
>> return -EACCES for non-owner user.
>>
>>>
>>> I doubt I can fix it by myself (at least quickly). So I am happy if anyone more experienced will look at it as well :)
>>>
>>> Anyway, if nobody is interested, I'll give it a try, but later.
>>
>> Here is a diff patch for this problem, please try testing.
>> If okay, I will send an official patch.
>>
>> Note: must apply the following patch first in the url,
>> http://git.linux-nfs.org/?p=bfields/linux.git;a=commitdiff;h=cc265089ce1b176dde963c74b53593446ee7f99a
> 
> We could do that if we have to.
> 
> I wonder if we could instead skip the fh_verify's inode_permission call
> entirely?  Most callers of fh_verify don't need the inode_permission
> call at all as far as I can tell, because the following vfs operation
> does permission checking already.
> 
> We still need some nfs-specific checking, I guess (e.g. to make sure we
> aren't allowing setattr on a read-only export of a writeable mount), but
> the inode_permission itself I think we can usually skip.

I have made a draft for fh_verify without inode_permisson as following.
1. rename the check_nfsd_access() to rqst_check_access() that only
   checking the request's flavor.  (Maybe a split patch is better)
2. split a new helper nfsd_check_access() from nfsd_permission()
   for checking nfsd access.
3. nfsd_permission() is not changed by call nfsd_check_access().
4. let fh_verify() call nfsd_check_access(), not nfsd_permission().
5. add nfsd_permission() for do_open_permission().

I just test with pynfs, and the result is same as the old.

> 
> Andreas Gruenbacher has also been complaining that the redundant
> inode_permission calls make the richacl work more difficult, I can't
> remember why exactly.

I will check those patch and discuss of richacl, but maybe later. 

thanks,
Kinglong Mee

-----------------------------------------------------------------------------
diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index f79521a..3199fec 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -935,7 +935,7 @@ static struct svc_export *exp_find(struct cache_detail *cd,
 	return exp;
 }
 
-__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
+__be32 rqst_check_access(struct svc_export *exp, struct svc_rqst *rqstp)
 {
 	struct exp_flavor_info *f;
 	struct exp_flavor_info *end = exp->ex_flavors + exp->ex_nflavors;
diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h
index 1f52bfc..20a50c5 100644
--- a/fs/nfsd/export.h
+++ b/fs/nfsd/export.h
@@ -80,7 +80,7 @@ struct svc_expkey {
 #define EX_WGATHER(exp)		((exp)->ex_flags & NFSEXP_GATHERED_WRITES)
 
 int nfsexp_flags(struct svc_rqst *rqstp, struct svc_export *exp);
-__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp);
+__be32 rqst_check_access(struct svc_export *exp, struct svc_rqst *rqstp);
 
 /*
  * Function declarations
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 864e200..d85d620 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -203,8 +203,11 @@ do_open_permission(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfs
 		accmode |= NFSD_MAY_WRITE;
 
 	status = fh_verify(rqstp, current_fh, S_IFREG, accmode);
+	if (status)
+		return status;
 
-	return status;
+	return nfsd_permission(rqstp, current_fh->fh_export,
+				current_fh->fh_dentry, accmode);
 }
 
 static __be32 nfsd_check_obj_isreg(struct svc_fh *fh)
@@ -1708,7 +1711,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
 				clear_current_stateid(cstate);
 
 			if (need_wrongsec_check(rqstp))
-				op->status = check_nfsd_access(current_fh->fh_export, rqstp);
+				op->status = rqst_check_access(current_fh->fh_export, rqstp);
 		}
 
 encode_op:
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 158badf..33c6c14 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2843,7 +2843,7 @@ nfsd4_encode_dirent_fattr(struct xdr_stream *xdr, struct nfsd4_readdir *cd,
 			nfserr = nfserrno(err);
 			goto out_put;
 		}
-		nfserr = check_nfsd_access(exp, cd->rd_rqstp);
+		nfserr = rqst_check_access(exp, cd->rd_rqstp);
 		if (nfserr)
 			goto out_put;
 
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 350041a..f3b04e5 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -360,13 +360,13 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access)
 			&& exp->ex_path.dentry == dentry)
 		goto skip_pseudoflavor_check;
 
-	error = check_nfsd_access(exp, rqstp);
+	error = rqst_check_access(exp, rqstp);
 	if (error)
 		goto out;
 
 skip_pseudoflavor_check:
 	/* Finally, check access permissions. */
-	error = nfsd_permission(rqstp, exp, dentry, access);
+	error = nfsd_check_access(rqstp, exp, dentry, access);
 
 	if (error) {
 		dprintk("fh_verify: %pd2 permission failure, "
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 84d770b..5a12d2d 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -262,7 +262,7 @@ nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
 	err = nfsd_lookup_dentry(rqstp, fhp, name, len, &exp, &dentry);
 	if (err)
 		return err;
-	err = check_nfsd_access(exp, rqstp);
+	err = rqst_check_access(exp, rqstp);
 	if (err)
 		goto out;
 	/*
@@ -2012,11 +2012,10 @@ static int exp_rdonly(struct svc_rqst *rqstp, struct svc_export *exp)
  * Check for a user's access permissions to this inode.
  */
 __be32
-nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
+nfsd_check_access(struct svc_rqst *rqstp, struct svc_export *exp,
 					struct dentry *dentry, int acc)
 {
 	struct inode	*inode = d_inode(dentry);
-	int		err;
 
 	if ((acc & NFSD_MAY_MASK) == NFSD_MAY_NOP)
 		return 0;
@@ -2052,6 +2051,18 @@ nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
 		}
 	if ((acc & NFSD_MAY_TRUNC) && IS_APPEND(inode))
 		return nfserr_perm;
+	return 0;
+}
+__be32
+nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
+					struct dentry *dentry, int acc)
+{
+	struct inode	*inode = d_inode(dentry);
+	int		err;
+
+	err = nfsd_check_access(rqstp, exp, dentry, acc);
+	if (err)
+		return err;
 
 	if (acc & NFSD_MAY_LOCK) {
 		/* If we cannot rely on authentication in NLM requests,
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 2050cb0..e3e8eed 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -101,6 +101,8 @@ __be32		nfsd_readdir(struct svc_rqst *, struct svc_fh *,
 __be32		nfsd_statfs(struct svc_rqst *, struct svc_fh *,
 				struct kstatfs *, int access);
 
+__be32		nfsd_check_access(struct svc_rqst *, struct svc_export *,
+				struct dentry *, int);
 __be32		nfsd_permission(struct svc_rqst *, struct svc_export *,
 				struct dentry *, int);
 

Re: nfsd: EACCES vs EPERM on utime()/utimes() calls

[Stanislav Kholmanskikh]

Hi.

On 06/04/2015 03:43 PM, Kinglong Mee wrote:
> On 6/3/2015 12:09 AM, Stanislav Kholmanskikh wrote:
>> On 06/02/2015 12:23 AM, bfields@fieldses.org wrote:
>>> On Mon, Jun 01, 2015 at 06:01:02PM +0300, Stanislav Kholmanskikh wrote:
>>>> Hello.
>>>>
>>>> As the man page for utime/utimes states [1], EPERM is returned if
>>>> the second argument of utime/utimes is not NULL and:
>>>>    * the caller's effective user id does not match the owner of the file
>>>>    * the caller does not have write access to the file
>>>>    * the caller is not privileged
>>>>
>>>> However, I don't see this behavior with NFS, I see EACCES is
>>>> generated instead.
>>>
>>> Agreed that it's probably a server bug.  (Have you run across a case
>>> where this makes a difference?)
>>
>> Thank you.
>>
>> No, I've not seen such a real-word scenario.
>>
>> I have these LTP test cases failing:
>>
>> * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utime/utime06.c
>> * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utimes/utimes01.c
>>
>> and it makes me a bit nervous :)
>>
>>>
>>> Looking at nfsd_setattr()....  The main work is done by notify_change(),
>>> which is probably doing the right thing.  But before that there's an
>>> fh_verify()--looks like that is expected to fail in your case.  I bet
>>> that's the cause.
>
> Yes, it is.
>
> nfsd do the permission checking before notify_change() as,
>
> /* This assumes  NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
> err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));
>
> return -EACCES for non-owner user.
>
>>
>> I doubt I can fix it by myself (at least quickly). So I am happy if anyone more experienced will look at it as well :)
>>
>> Anyway, if nobody is interested, I'll give it a try, but later.
>
> Here is a diff patch for this problem, please try testing.
> If okay, I will send an official patch.
>
> Note: must apply the following patch first in the url,
> http://git.linux-nfs.org/?p=bfields/linux.git;a=commitdiff;h=cc265089ce1b176dde963c74b53593446ee7f99a
>
> thanks,
> Kinglong Mee
> -------------------------------------------------------------
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 84d770b..2533088 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -407,10 +371,23 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
>   	bool		get_write_count;
>   	int		size_change = 0;
>
> -	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
> +	if (iap->ia_valid & ATTR_SIZE) {
>   		accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
> -	if (iap->ia_valid & ATTR_SIZE)
>   		ftype = S_IFREG;
> +	}
> +
> +	/*
> +	 * According to utimes_common(),
> +	 *
> +	 * If times is NULL (or both times are UTIME_NOW),
> +	 * then we need to check permissions, because
> +	 * inode_change_ok() won't do it.
> +	 */
> +	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME)) {
> +		accmode |= NFSD_MAY_OWNER_OVERRIDE;
> +		if (!(iap->ia_valid & (ATTR_ATIME_SET | ATTR_MTIME_SET)))
> +			accmode |= NFSD_MAY_WRITE;
> +	}
>
>   	/* Callers that do fh_verify should do the fh_want_write: */
>   	get_write_count = !fhp->fh_dentry;
>

Tested with v4.1-rc6 plust the above two patches.

The issue is fixed.

My utime_test  now reports EPERM. LTP's utime06, utimes01 now pass, 
other LTP syscall test cases don't show any regression either.

Thank you!