[PATCH] Better handle NULL utimes() in the NFS client

Bruce Evans brde at optusnet.com.au
Wed Jan 16 05:19:17 UTC 2013


On Tue, 15 Jan 2013, Rick Macklem wrote:

> Bruce Evans wrote:

>> I can't see anything that does the different permissions check for
>> the VA_UTIMES_NULL case, and testing shows that this case is just
>> broken,
>> at least for an old version of the old nfs client -- the same
>> permissions
>> are required for all cases, but write permission is supposed to be
>> enough for the VA_UTIMES_NULL case (since write permission is
>> sufficient
>> for setting the mtime to the current time (plus epsilon) using
>> write(2)
>> and truncate(2). Setting the atime to the current time should require
>> no more and no less than read permission, since it can be done using
>> read(2), but utimes(NULL) requires write permission for that too).
>>
> I did a quick test on a -current client/server and it seems to work ok.
> The client uses SET_TIME_TO_SERVER and the server sets VA_UTIMES_NULL
> for this case. At least it works for a UFS exported volume.

It's not working for me with newnfs from 4 Mar 2012:

$ mount | grep /c
besplex:/c on /c (nfs, asynchronous)
$ ls -l /c/tmp/z
-rw-rw-rw-  1 root  wheel  0 Jan 16 15:12 /c/tmp/z
# Not even root owns it, since root on the client is mapped to 0xFFFFFFFFE.
$ touch /c/tmp/z
touch: /c/tmp/z: Operation not permitted
$ touch -r . /c/tmp/z
touch: /c/tmp/z: Operation not permitted
touch: /c/tmp/z: Operation not permitted

The error message from touch are confusing.  For plain touch:
- it fails twice using utimes(), with errno EPERM and no error message
- it then succeeds using read(), write() and truncate()
- it then prints an error message
- it then exits with status 0.
This is with an old version of touch.  It always prints an error message
if it reaches the read()/write()/truncate() step (rw() function):
- if rw() succeeded, then it prints an error message after the rw()
   returns.  rw() fails to preserve errno, so the errno for this step
   is garbage, but it is usually the one from the second failing utimes().
- if rw() fails, it prints an error message internally.  The errno for
   this is now correct.
The current version of touch is even more broken.  Someone removed the
rw() step from it, under the naive assumption that utimes() actually
works.

For touch -r:
- it fails twice using utimes(), with errno EPERM and no error message.
   Now even trying the second time (with utimes(NULL) is a bug.  A
   comment says that there is nothing else that we can do in this case,
   but the code actually falls through and does something wrong (it
   tries to set to the current time instead of to the specified time).
   This bug fixed in the current version.
- since it is not supposed to do anything more, it prints an error message
   after the first utimes() failure.  It also sets rval to 1 to give an
   exit status of 1 later.
- then it continues the same as for the plain touch case:
    - it then "succeeds" using read(), write() and truncate(), but this
      success is in clobbering the timestamps to the current time
    - it then prints an error message despite "succeeding"
- it then exits with status 1.

The nfs error is just for the second utimes() in the plain touch case.
This should succeed (it succeeds on a local ffs file system).  Also, when
it fails, the correct errno is EACCES, not EPERM.  This works correctly
after changing the file mode to readonly and using the buggy touch -r
to reach the second utimes() -- the error is now EACCES for both nfs
and local ffs.  So it seems that the server ffs is being reached
correctly, but the non-error case for utimes(NULL) is being mishandled
somewhere.  This is not due to some maproot magic, since the same error
occurs for the non-error case when the ownership is changed to a mere
user (!= the test user).

>> Oops, on looking at the code I now think it _is_ possible to pass the
>> request to set the current time on the server, since in the
>> NFSV3SATTRTIME_TOSERVER case we just pass this case value and not
>> any time value to the server, so the server has no option but to use
>> its current time. It is not surprising that the permissions checks
>> for this don't work right. I thought that the client was responsible
>> for most permissions checks, but can't find many or the relevant one
>> here. The NFSV3SATTRTIME_TOSERVER code on the server sets
>> VA_UTIMES_NULL, so I would have thought that the permissions check on
>> the server does the right thing.
>>
> As noted above, it seems to work correctly for the new server in -current,
> at least for UFS exports.
>
> Normally a server will do permission checking for NFS RPCs. There is nothing
> stopping a client from doing a check and returning an error, but traditionally
> a server has not trusted a client to do so. (I'm not sure if adding a check
> in the client is what jhb@ was referring to in his reply to this?)

Checking in the client doesn't seem right now.  The bug seems to be a
different one on the server.

>> There are some large timestamping bugs nearby:
>>
>> - the old nfs server code for NFSV3SATTRTIME_TOSERVER uses
>> getnanotime()
>> to read the current time. This violates the system's policy set by
>> the vfs.timestamp precision in most cases, since using getnanotime()
>> is the worst supported policy and is not the defaul.
>> ...
>
>> New nfs code never uses the correct function vfs_timestamp().
> This needs to be fixed. Until now, I would have had no idea what is the
> correct interface. (When I did the port, I just used a call that seemed
> to return what I wanted.;-)
>
> Having said that, after reading what you wrote below, it is not obvious
> to me what the correct fix is? (It seems to be a choice between microtime()
> and vfs_timestamp()?)

Just use vfs_timestamp() whenever generating a file timestamp but not for
other purposes.  Like permissions checking, the client very rarely generates
file timestamps, and even on the server most timestamps are not generated
by nfs directly.  So there are only a few places to check and change.  We
know about fifos and the utimes(NULL) case in the server (the latter is
emulating upper layers in vfs) before calling VOP_SETATTR().  I wonder
how well the fifo code works.  Its timestamps aren't very important, but
they should be synced to the server very occasionally.

Bruce


More information about the freebsd-fs mailing list