From owner-freebsd-fs@FreeBSD.ORG Tue Jan 15 20:28:00 2013 Return-Path: Delivered-To: fs@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id AFA488DA; Tue, 15 Jan 2013 20:28:00 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id 88634647; Tue, 15 Jan 2013 20:28:00 +0000 (UTC) Received: from pakbsde14.localnet (unknown [38.105.238.108]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id C4AEBB9AD; Tue, 15 Jan 2013 15:27:59 -0500 (EST) From: John Baldwin To: Bruce Evans Subject: Re: [PATCH] Better handle NULL utimes() in the NFS client Date: Tue, 15 Jan 2013 14:58:42 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p22; KDE/4.5.5; amd64; ; ) References: <162405990.1985479.1358212854967.JavaMail.root@erie.cs.uoguelph.ca> <20130115141019.H1444@besplex.bde.org> In-Reply-To: <20130115141019.H1444@besplex.bde.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201301151458.42874.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Tue, 15 Jan 2013 15:27:59 -0500 (EST) Cc: Rick Macklem , fs@freebsd.org X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 Jan 2013 20:28:00 -0000 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