From owner-freebsd-fs@FreeBSD.ORG Wed Jan 16 14:26:47 2013 Return-Path: Delivered-To: fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id D684FF76; Wed, 16 Jan 2013 14:26:47 +0000 (UTC) (envelope-from rmacklem@uoguelph.ca) Received: from esa-annu.net.uoguelph.ca (esa-annu.mail.uoguelph.ca [131.104.91.36]) by mx1.freebsd.org (Postfix) with ESMTP id 65382C41; Wed, 16 Jan 2013 14:26:47 +0000 (UTC) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: Ap8EAAO49lCDaFvO/2dsb2JhbABFhjq3YnOCHgEBBAEjVgUWDgoCAg0ZAlkGiCYGpmmRKYEji1KDMIETA4hhjSuQSYMTggY X-IronPort-AV: E=Sophos;i="4.84,479,1355115600"; d="scan'208";a="9317566" Received: from erie.cs.uoguelph.ca (HELO zcs3.mail.uoguelph.ca) ([131.104.91.206]) by esa-annu.net.uoguelph.ca with ESMTP; 16 Jan 2013 09:26:46 -0500 Received: from zcs3.mail.uoguelph.ca (localhost.localdomain [127.0.0.1]) by zcs3.mail.uoguelph.ca (Postfix) with ESMTP id 08BA7B3F15; Wed, 16 Jan 2013 09:26:46 -0500 (EST) Date: Wed, 16 Jan 2013 09:26:46 -0500 (EST) From: Rick Macklem To: Bruce Evans Message-ID: <1642392672.2036529.1358346406018.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <20130116151051.O1060@besplex.bde.org> Subject: Re: [PATCH] Better handle NULL utimes() in the NFS client MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [172.17.91.203] X-Mailer: Zimbra 6.0.10_GA_2692 (ZimbraWebClient - FF3.0 (Win)/6.0.10_GA_2692) 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: Wed, 16 Jan 2013 14:26:47 -0000 Bruce Evans wrote: > 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 > Well, I just ran essentially the same test, using the new client patched with jhb@'s patch and an up to date server and I got the same behaviour as when doing the touch locally on the file in the file system. - when not the file owner, but having write permissions touch - worked for both local and NFS mount touch -r - failed with Operation not permitted for both local and NFS mount The test I had done before used a trivial program that just did a utimes(NULL) and it worked as non-owner with write access, as well. The server appears to have been patched for this at r157325 (Apr. 2006). Maybe your server hasn't been patched for this? rick > 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