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

[Bruce Evans]

On Tue, 15 Jan 2013, John Baldwin wrote:

> On Monday, January 14, 2013 11:51:23 pm 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).
>
> Correct.  All the other uses of VA_UTIMES_NULL in the tree are to
> provide the permissions check you describe and there is a large
> comment about it in ufs_setattr().  Other filesystems have comments
> that reference ufs_setattr().  I think these checks should be done
> in nfs_setattr() rather than in the routine to build an NFS attribute
> object however.

Perhaps it can be done in vfs.  There are some technical problems with
this, but perhaps they are small.  One is that file systems might not
even have any timestamps.  (I forgot to mention a related problem with
the error handling.  The permissions checks for utimes() are usually
too strict for file systems that only have fake ownerships, like
msdosfs.  OTOH, msdosfs also doesn't have atimes for most variants of
the file system.  Since utimes() is supposed to set both the mtime and
the atime (especially in the non-NULL case), it strictly cannot work
on msdosfs.  But msdosfs is not strict about this.  It silently ignores
the atimes when it can't set them.)

> Fixing NFS to properly use vfs_timestamp() seems to be a larger
> project.

I think it is smaller.  For the new nfs code, it is not as simple as
changing the NFSGETTIME() macro, since nfs wants extra precision in
most cases (for things like comparing cache times), and very rarely
wants the semantics of vfs_timestamp().

I somehow missed seeing seeing even more confusion in this area:
- the new nfs code also has a macro NFSGETNANOTIME() which reduces to
   getnanotime().
- monotonic times should be used if possible, but the new nfs code only
   uses them for NFSD_MONOSEC (which is used a lot) and in 2 places in
   nfs_commonkrpc.c where a hard-coded getmicrouptime() is used.
- NFSGETNANOTIME() is used 4 times.  But there are 4 hard-coded uses of
   getnanotime().  The latter are mostly in places where vfs_timestamp()
   is correct, for things like n_atim for fifos.  nfs almost never needs
   to set file timestamps, since most file timestamps are set by leaf
   (non-nfs) file systems on the server.  The only exceptions that I know
   about are the ones already noted (utimes(NULL) on the server, something
   in create() (?), and n_atim for fifos on the client (?).  n_atim for
   special files on the client were an exception when special files were
   supported.
In the old nfs code:
- there are no NFSGET*TIME() macros, and there don't seem to be any
   get*time() calls instead either.  The get*time() calls used are
   almost exactly the same ones as in the nfs nfs code, with the only
   exception that I noticed being the ones in the server for utimes(NULL).
- monotonic times are only used in the same 2 places in nfs_commonkrpc.c.
   The non-monotonic time_second is used a lot (hard-coded), I think in
   much the same places where the new nfs server code time_uptime via
   NFSD_MONOSEC.

I don't like obfuscating standard time calls using macros.  Others that
I don't like:
- both the old and the new nfs client use NFS_TIMESPEC_COMPARE() instead
   of the standard and better timespeccmp().  NFS_TIMESSPEC_COMPARE()
   is more verbose and only compares for equality.  Its implementation
   is home made and not based on timespeccmp().
- the new nfs client also has a macro NFS_CMPTIME().  This gives the
   same result as NFS_TIMESPEC_COMPARE() (or timespeccmp(..., =)..  Iti
   works accidentally for both timespecs and timevals, due to the POSIX
   bug that the struct members for timespecs abuse the prefix for timevals
   in their spelling.  Code derived from the old nfs client has not been
   translated to use the new macro (the "new" macro is probably actually
   older and may even be older or more likely just from a different code
   base than FreeBSD's timespeccmp()).
- the new nfs client also has a macro NFS_SETTIME().  This doesn't actually
   set the time, but converts from the global timeval `time' to a timespec
   in the same way as the standard macro TIMEVAL_TO_TIMESPEC() would if it
   is passed a pointer to the global timeval.  Accessing the global `time'
   like this would give races.  Fortunately, the global `time' doesn't
   exist in FreeBSD, so of course this macro is never used.

I like NFSD_MONOSEC, however.  Global variables give a much more fragile
and harder to translate API than function calls and function-like macros.

Bruce