Date: Wed, 16 Jan 2013 09:26:46 -0500 (EST) From: Rick Macklem <rmacklem@uoguelph.ca> 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: <1642392672.2036529.1358346406018.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <20130116151051.O1060@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <file> - worked for both local and NFS mount
touch -r <other-file> <file> - 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1642392672.2036529.1358346406018.JavaMail.root>