Date: Tue, 15 Jan 2013 14:58:42 -0500 From: John Baldwin <jhb@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: Rick Macklem <rmacklem@freebsd.org>, fs@freebsd.org Subject: Re: [PATCH] Better handle NULL utimes() in the NFS client Message-ID: <201301151458.42874.jhb@freebsd.org> In-Reply-To: <20130115141019.H1444@besplex.bde.org> References: <162405990.1985479.1358212854967.JavaMail.root@erie.cs.uoguelph.ca> <20130115141019.H1444@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Monday, January 14, 2013 11:51:23 pm Bruce Evans wrote:
> On Mon, 14 Jan 2013, Rick Macklem wrote:
>
> > John Baldwin wrote:
> >> The NFS client tries to infer when an application has passed NULL to
> >> utimes()
> >> so that it can let the server set the timestamp rather than using a
> >> client-
> >> supplied timestamp. It does this by checking to see if the desired
> >> timestamp's second matches the current second. However, this breaks
> >> applications that are intentionally trying to set a specific timestamp
> >> within
> >> the current second. In addition, utimes() sets a flag to indicate if
> >> NULL was
> >> passed to utimes(). The patch below changes the NFS client to check
> >> this flag
> >> and only use the server-supplied time in that case:
>
> It is certainly an error to not check VA_UTIMES_NULL at all. I think
> the flag (or the NULL pointer) cannot be passed to the server, so the
> best we can do for the VA_UTIMES_NULL case is read the current time on
> the client and pass it to the server. Upper layers have already read
> the current time, but have passed us VA_UTIMES_NULL so that we can tell
> that the pointer was originally null so that we can do the different
> permissions checks for this case.
>
> >> Index: fs/nfsclient/nfs_clport.c
> >> ===================================================================
> >> --- fs/nfsclient/nfs_clport.c (revision 225511)
> >> +++ fs/nfsclient/nfs_clport.c (working copy)
> >> @@ -762,7 +762,7 @@
> >> *tl = newnfs_false;
> >> }
> >> if (vap->va_atime.tv_sec != VNOVAL) {
> >> - if (vap->va_atime.tv_sec != curtime.tv_sec) {
> >> + if (!(vap->va_vaflags & VA_UTIMES_NULL)) {
> >> NFSM_BUILD(tl, u_int32_t *, 3 * NFSX_UNSIGNED);
> >> *tl++ = txdr_unsigned(NFSV3SATTRTIME_TOCLIENT);
> >> txdr_nfsv3time(&vap->va_atime, tl);
> >> @@ -775,7 +775,7 @@
> >> *tl = txdr_unsigned(NFSV3SATTRTIME_DONTCHANGE);
> >> ...
>
> Something mangled the patch so that it is hard to see what it does. It
> just uses the flag instead of guessing.
>
> 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.
Fixing NFS to properly use vfs_timestamp() seems to be a larger
project.
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201301151458.42874.jhb>