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

DISCUSSION Stanislav Kholmanskikh 4 participants 8 messages Jun 1, 2015 – Jun 7, 2015

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Stanislav Kholmanskikh

author

2015-06-01 15:01 UTC #0 lore raw

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.

J. Bruce Fields

2015-06-01 21:23 UTC #1 lore raw

Quoted 10 lines · 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.

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.

Quoted 79 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Stanislav Kholmanskikh

author

2015-06-02 16:09 UTC #2 lore raw

Quoted 9 lines · On 06/02/2015 12:23 AM, [email protected] wrote:

Quoted 8 lines · 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

Quoted 5 lines

Quoted 2 lines
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 :)

Quoted 5 lines

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.

Quoted 82 lines

--b.
Quoted 79 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Kinglong Mee

2015-06-04 12:43 UTC #3 lore raw

Quoted 18 lines · Stanislav Kholmanskikh

Quoted 9 lines · On 06/02/2015 12:23 AM, [email protected] wrote:
Quoted 8 lines · 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
Quoted 5 lines
Quoted 2 lines
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.

Quoted 14 lines

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 :)
Quoted 5 lines
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.

Quoted 4 lines

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
-------------------------------------------------------------

fs/nfsd/vfs.c +15 -2

index 84d770b..2533088 100644
@@ -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;

J. Bruce Fields

2015-06-04 20:27 UTC #4 lore raw

Quoted 46 lines · Kinglong Mee

Quoted 18 lines · Stanislav Kholmanskikh
Quoted 9 lines · On 06/02/2015 12:23 AM, [email protected] wrote:
Quoted 8 lines · 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
Quoted 5 lines
Quoted 2 lines
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.
Quoted 14 lines
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 :)
Quoted 5 lines
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.
Quoted 3 lines
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 :)

Quoted 7 lines

Quoted 1 line
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.

Quoted 34 lines

thanks,
Kinglong Mee
-------------------------------------------------------------

fs/nfsd/vfs.c +15 -2

index 84d770b..2533088 100644
@@ -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;

Al Viro

2015-06-05 00:50 UTC #5 lore raw

Quoted 4 lines · J. Bruce Fields

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?

Kinglong Mee

2015-06-07 08:25 UTC #6 lore raw

In reply to J. Bruce Fields

Quoted 57 lines · J. Bruce Fields

Quoted 46 lines · Kinglong Mee
Quoted 18 lines · Stanislav Kholmanskikh
Quoted 9 lines · On 06/02/2015 12:23 AM, [email protected] wrote:
Quoted 8 lines · 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
Quoted 5 lines
Quoted 2 lines
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.
Quoted 14 lines
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 :)
Quoted 5 lines
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.
Quoted 3 lines
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 :)
Quoted 7 lines
Quoted 1 line
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.

Quoted 8 lines

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.

Quoted 4 lines

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

-----------------------------------------------------------------------------

fs/nfsd/export.c +1 -1

index f79521a..3199fec 100644
@@ -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;

fs/nfsd/export.h +1 -1

index 1f52bfc..20a50c5 100644
@@ -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

fs/nfsd/nfs4proc.c +5 -2

index 864e200..d85d620 100644
@@ -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:

fs/nfsd/nfs4xdr.c +1 -1

index 158badf..33c6c14 100644
@@ -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;

fs/nfsd/nfsfh.c +2 -2

index 350041a..f3b04e5 100644
@@ -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, "

fs/nfsd/vfs.c +14 -3

index 84d770b..5a12d2d 100644
@@ -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,

fs/nfsd/vfs.h +2 -0

index 2050cb0..e3e8eed 100644
@@ -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);

Stanislav Kholmanskikh

author

2015-06-05 15:30 UTC #7 lore raw

In reply to Kinglong Mee

Hi.

Quoted 78 lines · Kinglong Mee

Quoted 18 lines · Stanislav Kholmanskikh
Quoted 9 lines · On 06/02/2015 12:23 AM, [email protected] wrote:
Quoted 8 lines · 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
Quoted 5 lines
Quoted 2 lines
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.
Quoted 14 lines
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 :)
Quoted 5 lines
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.
Quoted 4 lines
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
-------------------------------------------------------------
fs/nfsd/vfs.c +8 -2
index 84d770b..2533088 100644
@@ -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

fs/nfsd/vfs.c nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, · line 393

Quoted 10 lines

fs/nfsd/vfs.c +7 -0

+	 * 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!