vfs: fix utimensat() non-conformances to spec

Andrew,

This patch series is a revised version of the patch series that I sent yesterday to fix various utimensat() non-conformances. Patches 1 and 2 are unchanged (and were Acked by Miklos); patches 3 and 4 are revised following comments by Miklos.

As requested I've split the original patch ("utimensat() non-conformances and fixes [v4] (patch)";
http://article.gmane.org/gmane.linux.file-systems/24325 ) into four parts. Ideally, these should be applied for 2.6.26-rc, for the reasons outlined in my earlier mail "utimensat() non-conformances and fixes [v4] (test results)",
http://article.gmane.org/gmane.linux.kernel/689411/ .

The four patches can be applied independently, except that patch 3 needs patch 2 to be applied first.

I've run my test suite
(http://article.gmane.org/gmane.linux.file-systems/24327 ) against this version of the patches, and all tests pass.

Cheers,

Michael

PS Just for reference, I'll include my earlier "overview" message here:

-------- Original Message --------
Subject: utimensat() non-conformances and fixes [v4] (overview)
Date: Tue, 03 Jun 2008 22:13:32 +0200
From: Michael Kerrisk <[email protected]>
To: Andrew Morton <[email protected]>
CC: lkml <[email protected]>, Christoph Hellwig <[email protected]>, Miklos Szeredi <[email protected]>, Al Viro <[email protected]>, [email protected], Ulrich Drepper <[email protected]>, [email protected], Subrata Modak <[email protected]>

Andrew,

A follow-on to this mail is a patch (against 2.6.26-rc4) for -mm. The patch fixes several problems in the utimensat() system call.

I would like to see this patch get into mainline ASAP. If you don't want to push it for .26, I can understand that, since we are late in the cycle.
Nevertheless, I will provide some arguments in favor of doing so, in a
follow-on mail. (I'll also provide a fairly comprehensive test suite, and test results, which may make you feel fairly confident of the patch.)

==

In an earlier mail I described four problems with the utimensat() implementation, and attempted a clumsy fix,
http://thread.gmane.org/gmane.linux.man/129

Miklos pointed out a number of problems in my attempted fix, and pushed a fix into .26-rc for one of the problems (number 3 in the mail), which was a security-related issue (http://thread.gmane.org/gmane.linux.kernel/678130).
He also gave me a couple of clues along the way about how to fix the
remaining problems.

In this mail, I will explain the remaining problems from the beginning. (Although Miklos pushed a fix for one of the problems, there still remain four problems, because I found another one in the meantime.)

==

While writing a man page for utimensat(2) and futimens(3), I found a few bugs in the utimensat() system call (i.e., non-conformances with either the specification in the draft POSIX.1-200x revision or traditional Linux behavior).

       int futimens(inf fd, const struct timespec times[2]);

       int utimensat(int dirfd, const char *pathname,
                     const struct timespec times[2], int flags);

1. The POSIX.1 draft says that to do anything other than setting both timestamps to a time other than the current time (i.e., times is not NULL, and both tv_nsec fields are not UTIME_NOW and both tv_nsec fields are not UTIME_OMIT), either:
a) the caller's effective user ID must match the file owner; or
b) the caller must have appropriate privileges.
If this condition is violated, then the error EPERM should result. However, the current implementation does not generate EPERM if one tv_nsec field is UTIME_NOW while the other is TIME_OMIT -- it should give this error for that case.

2. The POSIX.1 draft says:

Only a process with the effective user ID equal to the user ID of the file, *or with write access to the file*, or with appropriate privileges may use futimens() or utimensat() with a null pointer as the times argument or with both tv_nsec fields set to the special value UTIME_NOW.

The important piece here is "with write access to the file", and this matters for futimens(), which deals with an argument that is a file descriptor referring to the file whose timestamps are being updated, The standard is saying that the "writability" check is based on the file permissions, not the access mode with which the file is opened. (This behavior is consistent with the semantics of FreeBSD's futimes().) However, Linux is currently doing the latter -- futimens(fd, times) is implemented as

utimensat(fd, NULL, times, 0)

and within the utimensat() implementation we have the code:

                f = fget(dfd);  // dfd is 'fd'
                ...
                if (f) {
                        if (!(f->f_mode & FMODE_WRITE))
                                goto mnt_drop_write_and_out;

The check should instead be based on the file permissions.

3. The POSIX.1 draft says that if a times[n].tv_nsec field is UTIME_OMIT or UTIME_NOW, then the value in the corresponding tv_sec field is ignored. However the current Linux implementation requires the tv_sec value to be zero (or the EINVAL error results). This requirement should be removed.

4. A further bug relates to traditional Linux behavior. Traditionally (i.e., utime(2) and utimes(2)), the EPERM error could also occur if the 'times' argument was non-NULL (i.e., we are setting the timestamps to a value other than the current time) and the file is marked as immutable or append-only. The current implementation also returns this error if 'times' is non-NULL and the tv_nsec fields are both UTIME_NOW. For consistency, the

   (times != NULL && times[0].tv_nsec == UTIME_NOW &&
                     times[1].tv_nsec == UTIME_NOW)

case should be treated like the traditional utimes() case where 'times' is NULL. That is, the call should succeed for a file marked append-only and should give the error EACCES if the file is marked as immutable.

Cheers,

Michael

PS For completeness, here are the expected results from various cases of calls to utime() and utimensat().

                |   utime[s]() |     utimensat() 'times' argument
                | NULL   non-  | NULL   .       non-NULL arg
                | arg    NULL  | arg    . 2*NOW  2*OMIT  OMIT   {x,y}
                |        arg   |        .                +NOW
----------------+--------------+-------------------------------------
file owned by/  |              |        .
permissions     |              |        .
                |              |        .
caller     400  | succ   succ  | succ   . succ   succ    succ   succ
                |              |        .
not-caller 644  | EACCES EPERM | EACCES . EACCES succ    EPERM  EPERM
                |              |        .
not-caller 666  | succ   EPERM | succ   . succ   succ    EPERM  EPERM
----------------+--------------+-------------------------------------
ext2 attributes |              |        .
append-only     | succ   EPERM | succ   . succ   succ    EPERM  EPERM
                |              |        .
immutable       | EACCES EPERM | EACCES . EACCES succ    EPERM  EPERM

D'oh! I left the "v2" markers off the resent patchs 1 and 3, so they got the same subject lines as yesterday's versions. Sorry about that!

ACK for all four patches.

In patch 3/4 the indentation of that big "if" looks really horribly broken, but it will be cleaned out completely, so I won't complain...

Miklos

Aargh -- true, I messed up the indentation!

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html