From owner-freebsd-fs@FreeBSD.ORG Wed Jan 16 05:19:17 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 921C69C9; Wed, 16 Jan 2013 05:19:17 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx07.syd.optusnet.com.au (fallbackmx07.syd.optusnet.com.au [211.29.132.9]) by mx1.freebsd.org (Postfix) with ESMTP id D282B8B2; Wed, 16 Jan 2013 05:19:16 +0000 (UTC) Received: from mail27.syd.optusnet.com.au (mail27.syd.optusnet.com.au [211.29.133.168]) by fallbackmx07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r0G5JBQC009796; Wed, 16 Jan 2013 16:19:11 +1100 Received: from c211-30-173-106.carlnfd1.nsw.optusnet.com.au (c211-30-173-106.carlnfd1.nsw.optusnet.com.au [211.30.173.106]) by mail27.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r0G5J1mS028275 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 16 Jan 2013 16:19:02 +1100 Date: Wed, 16 Jan 2013 16:19:01 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Rick Macklem Subject: Re: [PATCH] Better handle NULL utimes() in the NFS client In-Reply-To: <1149390778.2023367.1358290140175.JavaMail.root@erie.cs.uoguelph.ca> Message-ID: <20130116151051.O1060@besplex.bde.org> References: <1149390778.2023367.1358290140175.JavaMail.root@erie.cs.uoguelph.ca> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=Or8XUFDt c=1 sm=1 a=S8Qr1IbAvFsA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=U1Z5fgpPGSMA:10 a=0YgfEWQ0QP7ufM_Kn4MA:9 a=CjuIK1q_8ugA:10 a=TEtd8y5WR3g2ypngnwZWYw==:117 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 05:19:17 -0000 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