[PATCH] utimensat() non-conformances and fixes

[PATCH] utimensat() non-conformances and fixes

[Michael Kerrisk]

Ulrich,

While writing a man page for utimensat(2) and futimens(3), I think I've
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 utimensat(int dirfd, const char *pathname,
                     const struct timespec times[2], int flags);

1. The draft POSIX.1-200x specification for utimensat() 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.

2. 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 -- see lines 32400 to 32403 of draft 5 of the spec), either: the
caller's effective user ID must match the owner of the file; or the caller
must have appropriate privileges. If this condition is violated, then the
error EPERM results. However, the current implementation does not generate
EPERM if one tv_nsec field is UTIME_NOW while the other is UTIME_OMIT -- it
should give this error for that case.

3. Traditionally, utime()/utimes() gives the error EACCES for the case
where the timestamp pointer argument is NULL (i.e., set both timestamps to
the current time), and the file is owned by a user other than the effective
UID of the caller, and the file is not writable by the effective UID of the
program.  utimensat() also gives this error in the same case.  However, in
the same circumstances, when utimensat() is given a 'times' array in which
both tv_nsec fields are UTIME_NOW, which provides equivalent functionality
to specifying 'times' as NULL, the call succeeds.  I think that it should fail
with the error EACCES in this case.

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.

The first part of the patch below (made against 2.6.25-rc6, but still
applies against rc8) addresses bugs 2, 3, and 4 and the second part of the
patch addresses bug 1.  Do you agree with my analyses and fixes?

Following the patch, at the end of this mail is a command-line-driven test
program that can be used to test the current implementation, and my patch.
 Some test cases below demonstrate the 4 cases described above, showing how
a vanilla 2.6.25-rcN kernel behaves, and how it behaves with my patch applied.

Finally, with my patch applied, I believe that the following lines could be
removed from the sys_utimensat() routine (I've done some light testing to verify
this, but more testing would be in order):

                /* Nothing to do, we must not even check the path.  */
                if (tstimes[0].tv_nsec == UTIME_OMIT &&
                    tstimes[1].tv_nsec == UTIME_OMIT)
                        return 0;

Cheers,

Michael

Signed-off-by:  Michael Kerrisk <mtk.manpages@gmail.com>

Test cases
==========

All test cases run as user 'mtk'

Bug 1
-----

Behavior with vanilla 2.6.25-rcN kernel:

$ ls -l u
-r-------- 1 mtk users 0 Apr  7 21:39 u
$ ./t_utimensat u 1 n 1 n
dirfd is -100
pathname is u
tsp is 0xbfb41408; struct  = { 1, 1073741823 } { 1, 1073741823 }
flags is 0
utimensat: Invalid argument

Behavior with my patch applied:

$ ls -l u
-r-------- 1 mtk users 0 Apr  7 21:39 u
$ ./t_utimensat u 1 n 1 n
dirfd is -100
pathname is u
tsp is 0xbfab7378; struct  = { 1, 1073741823 } { 1, 1073741823 }
flags is 0
utimensat() succeeded
Last file access:         Mon Apr  7 21:39:36 2008
Last file modification:   Mon Apr  7 21:39:36 2008
Last status change:       Mon Apr  7 21:39:36 2008


Bug 2
-----

Behavior with vanilla 2.6.25-rcN kernel:

$ ls -l p
-rw-r--r-- 1 root root 0 Apr  7 10:34 p
$ ./t_utimensat p 0 n 0 o
dirfd is -100
pathname is p
tsp is 0xbfb7ac38; struct  = { 0, 1073741823 } { 0, 1073741822 }
flags is 0
utimensat() succeeded
Last file access:         Mon Apr  7 22:00:51 2008
Last file modification:   Mon Apr  7 10:34:00 2008
Last status change:       Mon Apr  7 22:00:51 2008


Behavior with my patch applied:

$ ls -l p
-rw-r--r-- 1 root root 0 Apr  7 10:34 p
$ ./t_utimensat p 0 n 0 o
dirfd is -100
pathname is p
tsp is 0xbfc40d08; struct  = { 1, 1073741823 } { 1, 1073741822 }
flags is 0
utimensat failed with EPERM


Bug 3
-----

Behavior with vanilla 2.6.25-rcN kernel:

$ ls -l p
-rw-r--r-- 1 root root 0 Apr  7 22:03 p
$ ./t_utimensat p 0 n 0 n
dirfd is -100
pathname is p
tsp is 0xbfd99658; struct  = { 0, 1073741823 } { 0, 1073741823 }
flags is 0
utimensat() succeeded
Last file access:         Mon Apr  7 22:03:31 2008
Last file modification:   Mon Apr  7 22:03:31 2008
Last status change:       Mon Apr  7 22:03:31 2008
$ ./t_utimensat p
dirfd is -100
pathname is p
tsp is (nil)
flags is 0
utimensat failed with EACCES

Behavior with my patch applied:

$ ls -l p
-rw-r--r-- 1 root root 0 Apr  7 10:34 p
$ ./t_utimensat p 0 n 0 n
dirfd is -100
pathname is p
tsp is 0xbfa98358; struct  = { 0, 1073741823 } { 0, 1073741823 }
flags is 0
utimensat failed with EACCES
$ ./t_utimensat p
dirfd is -100
pathname is p
tsp is (nil)
flags is 0
utimensat failed with EACCES


Bug 4
-----

Behavior with vanilla 2.6.25-rcN kernel:

$ ls -l fa fi
-rw-r--r-- 1 mtk users 0 Apr  7 21:43 fa
-rw-r--r-- 1 mtk users 0 Apr  7 09:11 fi
$ lsattr -l fa fi
fa                           Append_Only
fi                           Immutable
$ ./t_utimensat fa 0 n 0 n
dirfd is -100
pathname is fa
tsp is 0xbfc43508; struct  = { 0, 1073741823 } { 0, 1073741823 }
flags is 0
utimensat failed with EPERM
$ ./t_utimensat fi 0 n 0 n
dirfd is -100
pathname is fi
tsp is 0xbfb54c18; struct  = { 0, 1073741823 } { 0, 1073741823 }
flags is 0
utimensat failed with EPERM


Behavior with my patch applied:

$ ls -l fa fi
-rw-r--r-- 1 mtk users 0 Apr  7 18:24 fa
-rw-r--r-- 1 mtk users 0 Apr  7 09:11 fi
$ lsattr -l fa fi
fa                           Append_Only
fi                           Immutable
$ ./t_utimensat fa 0 n 0 n
dirfd is -100
pathname is fa
tsp is 0xbfa7e338; struct  = { 0, 1073741823 } { 0, 1073741823 }
flags is 0
utimensat() succeeded
Last file access:         Mon Apr  7 21:43:23 2008
Last file modification:   Mon Apr  7 21:43:23 2008
Last status change:       Mon Apr  7 21:43:23 2008
$ ./t_utimensat fi 0 n 0 n
dirfd is -100
pathname is fi
tsp is 0xbfe8d748; struct  = { 0, 1073741823 } { 0, 1073741823 }
flags is 0
utimensat failed with EACCES



==================================================

--- linux-2.6.25-rc6-orig/fs/utimes.c	2008-04-07 22:25:08.000000000 +0200
+++ linux-2.6.25-rc6/fs/utimes.c	2008-04-07 23:57:41.000000000 +0200
@@ -95,27 +95,37 @@

 	/* Don't worry, the checks are done in inode_change_ok() */
 	newattrs.ia_valid = ATTR_CTIME | ATTR_MTIME | ATTR_ATIME;
-	if (times) {
+	if (times && ! ((times[0].tv_nsec == UTIME_NOW &&
+			 times[1].tv_nsec == UTIME_NOW) ||
+			(times[0].tv_nsec == UTIME_OMIT &&
+			 times[1].tv_nsec == UTIME_OMIT))) {
 		error = -EPERM;
                 if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
                         goto dput_and_out;

-		if (times[0].tv_nsec == UTIME_OMIT)
-			newattrs.ia_valid &= ~ATTR_ATIME;
-		else if (times[0].tv_nsec != UTIME_NOW) {
+		if (times[0].tv_nsec == UTIME_OMIT) {
+			newattrs.ia_atime = inode->i_atime;
+			newattrs.ia_valid |= ATTR_ATIME_SET;
+		} else if (times[0].tv_nsec != UTIME_NOW) {
 			newattrs.ia_atime.tv_sec = times[0].tv_sec;
 			newattrs.ia_atime.tv_nsec = times[0].tv_nsec;
 			newattrs.ia_valid |= ATTR_ATIME_SET;
 		}

-		if (times[1].tv_nsec == UTIME_OMIT)
-			newattrs.ia_valid &= ~ATTR_MTIME;
-		else if (times[1].tv_nsec != UTIME_NOW) {
+		if (times[1].tv_nsec == UTIME_OMIT) {
+			newattrs.ia_mtime = inode->i_mtime;
+			newattrs.ia_valid |= ATTR_MTIME_SET;
+		} else if (times[1].tv_nsec != UTIME_NOW) {
 			newattrs.ia_mtime.tv_sec = times[1].tv_sec;
 			newattrs.ia_mtime.tv_nsec = times[1].tv_nsec;
 			newattrs.ia_valid |= ATTR_MTIME_SET;
 		}
+
+	} else if (unlikely(times && times[0].tv_nsec == UTIME_OMIT &&
+			times[1].tv_nsec == UTIME_OMIT)) {
+		newattrs.ia_valid &= ~(ATTR_ATIME | ATTR_MTIME | ATTR_CTIME);
 	} else {
+		/* times is NULL, or both tv_nsec fields are UTIME_NOW */
 		error = -EACCES;
                 if (IS_IMMUTABLE(inode))
                         goto dput_and_out;
@@ -150,14 +160,6 @@
 	if (utimes) {
 		if (copy_from_user(&tstimes, utimes, sizeof(tstimes)))
 			return -EFAULT;
-		if ((tstimes[0].tv_nsec == UTIME_OMIT ||
-		     tstimes[0].tv_nsec == UTIME_NOW) &&
-		    tstimes[0].tv_sec != 0)
-			return -EINVAL;
-		if ((tstimes[1].tv_nsec == UTIME_OMIT ||
-		     tstimes[1].tv_nsec == UTIME_NOW) &&
-		    tstimes[1].tv_sec != 0)
-			return -EINVAL;

 		/* Nothing to do, we must not even check the path.  */
 		if (tstimes[0].tv_nsec == UTIME_OMIT &&


==================================================

/* t_utimensat.c

   Copyright (C) 2008, Michael Kerrisk <mtk.manpages@gmail.com>

   Licensed under the GPLv2 or later.

   A command-line interface for testing the utimensat() system
   call.

   17 Mar 2008  Initial creation
*/
#define _GNU_SOURCE
#define _ATFILE_SOURCE
#include <stdio.h>
#include <time.h>
#include <errno.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/syscall.h>
#include <fcntl.h>
#include <string.h>
#include <sys/stat.h>

#define errExit(msg)    do { perror(msg); exit(EXIT_FAILURE); \
                        } while (0)


#define __NR_utimensat          320     /* x86 syscall number */

# define UTIME_NOW      ((1l << 30) - 1l)
# define UTIME_OMIT     ((1l << 30) - 2l)

static inline int
utimensat(int dirfd, const char *pathname,
          const struct timespec times[2], int flags)
{
    return syscall(__NR_utimensat, dirfd, pathname, times, flags);
}

static void
usageError(char *progName)
{
    fprintf(stderr, "Usage: %s pathname [atime-sec "
            "atime-nsec mtime-sec mtime-nsec]\n\n", progName);
    fprintf(stderr, "Permitted options are:\n");
    fprintf(stderr, "    [-d path] "
            "open a directory file descriptor"
            " (instead of using AT_FDCWD)\n");
    fprintf(stderr, "    -w        Open directory file "
            "descriptor with O_RDWR (instead of O_RDONLY)\n");
    fprintf(stderr, "    -n        Use AT_SYMLINK_NOFOLLOW\n");
    fprintf(stderr, "\n");

    fprintf(stderr, "pathname can be \"NULL\" to use NULL "
            "argument in call\n");
    fprintf(stderr, "\n");

    fprintf(stderr, "Either nsec field can be\n");
    fprintf(stderr, "    'n' for UTIME_NOW\n");
    fprintf(stderr, "    'o' for UTIME_OMIT\n");
    fprintf(stderr, "\n");

    fprintf(stderr, "If the time fields are omitted, "
            "then a NULL 'times' argument is used\n");
    fprintf(stderr, "\n");

    exit(EXIT_FAILURE);
}

int
main(int argc, char *argv[])
{
    int flags, dirfd, opt, oflag;
    struct timespec ts[2];
    struct timespec *tsp;
    char *pathname, *dirfdPath;
    struct stat sb;

    flags = 0;
    dirfd = AT_FDCWD;
    dirfdPath = NULL;
    oflag = O_RDONLY;

    while ((opt = getopt(argc, argv, "d:nw")) != -1) {
        switch (opt) {
        case 'd':
            dirfdPath = optarg;
            break;

        case 'n':
            flags |= AT_SYMLINK_NOFOLLOW;
            printf("Not following symbolic links\n");
            break;

        case 'w':
            oflag = O_RDWR;
            break;

        default:
            usageError(argv[0]);
        }
    }

    if ((optind + 5 != argc) && (optind + 1 != argc))
        usageError(argv[0]);

    if (dirfdPath != NULL) {
        dirfd = open(dirfdPath, oflag);
        if (dirfd == -1) errExit("open");

        printf("Opened dirfd");
        printf(" O_RDWR");
        printf(": %s\n", dirfdPath);
    }

    pathname = (strcmp(argv[optind], "NULL") == 0) ?
                        NULL : argv[optind];

    if (argc == optind + 1) {
        tsp = NULL;

    } else {
        ts[0].tv_sec = atoi(argv[optind + 1]);
        if (argv[optind + 2][0] == 'n') {
            ts[0].tv_nsec = UTIME_NOW;
        } else if (argv[optind + 2][0] == 'o') {
            ts[0].tv_nsec = UTIME_OMIT;
        } else {
            ts[0].tv_nsec = atoi(argv[optind + 2]);
        }

        ts[1].tv_sec = atoi(argv[optind + 3]);
        if (argv[optind + 4][0] == 'n') {
            ts[1].tv_nsec = UTIME_NOW;
        } else if (argv[optind + 4][0] == 'o') {
            ts[1].tv_nsec = UTIME_OMIT;
        } else {
            ts[1].tv_nsec = atoi(argv[optind + 4]);
        }

        tsp = ts;
    }

    /* For testing purposes, it may have been useful to run this
       program as set-user-ID-root so that a directory file
       descriptor could be opened as root.  Now we reset to the
       real UID before making the utimensat() call, so that the
       permission checking is performed under that UID. */

    if (geteuid() == 0) {
        uid_t u;

        u = getuid();

        printf("Resettng UIDs to %ld\n", (long) u);

        if (setresuid(u, u, u) == -1)
            errExit("setresuid");
    }

    printf("dirfd is %d\n", dirfd);
    printf("pathname is %s\n", pathname);
    printf("tsp is %p", tsp);
    if (tsp != NULL) {
        printf("; struct  = { %ld, %ld } { %ld, %ld }",
                (long) tsp[0].tv_sec, (long) tsp[0].tv_nsec,
                (long) tsp[1].tv_sec, (long) tsp[1].tv_nsec);
    }
    printf("\n");
    printf("flags is %d\n", flags);

    if (utimensat(dirfd, pathname, tsp, flags) == -1) {
        if (errno == EPERM)
            printf("utimensat failed with EPERM\n");
        else if (errno == EACCES)
            printf("utimensat failed with EACCES\n");
        else
            perror("utimensat");
        exit(EXIT_FAILURE);
    }

    printf("utimensat() succeeded\n");

    if (stat(pathname, &sb) == -1) errExit("stat");
    printf("Last file access:         %s", ctime(&sb.st_atime));
    printf("Last file modification:   %s", ctime(&sb.st_mtime));
    printf("Last status change:       %s", ctime(&sb.st_ctime));

    exit(EXIT_SUCCESS);
}

Re: [PATCH] utimensat() non-conformances and fixes

[Michael Kerrisk]

Ulrich,

Ping!  Could you please review this patch.

Cheers,

Michael

On Tue, Apr 8, 2008 at 12:18 AM, Michael Kerrisk
<mtk.manpages@googlemail.com> wrote:
> Ulrich,
>
> While writing a man page for utimensat(2) and futimens(3), I think I've
> 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 utimensat(int dirfd, const char *pathname,
>                     const struct timespec times[2], int flags);
>
> 1. The draft POSIX.1-200x specification for utimensat() 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.
>
> 2. 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 -- see lines 32400 to 32403 of draft 5 of the spec), either: the
> caller's effective user ID must match the owner of the file; or the caller
> must have appropriate privileges. If this condition is violated, then the
> error EPERM results. However, the current implementation does not generate
> EPERM if one tv_nsec field is UTIME_NOW while the other is UTIME_OMIT -- it
> should give this error for that case.
>
> 3. Traditionally, utime()/utimes() gives the error EACCES for the case
> where the timestamp pointer argument is NULL (i.e., set both timestamps to
> the current time), and the file is owned by a user other than the effective
> UID of the caller, and the file is not writable by the effective UID of the
> program.  utimensat() also gives this error in the same case.  However, in
> the same circumstances, when utimensat() is given a 'times' array in which
> both tv_nsec fields are UTIME_NOW, which provides equivalent functionality
> to specifying 'times' as NULL, the call succeeds.  I think that it should fail
> with the error EACCES in this case.
>
> 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.
>
> The first part of the patch below (made against 2.6.25-rc6, but still
> applies against rc8) addresses bugs 2, 3, and 4 and the second part of the
> patch addresses bug 1.  Do you agree with my analyses and fixes?
>
> Following the patch, at the end of this mail is a command-line-driven test
> program that can be used to test the current implementation, and my patch.
>  Some test cases below demonstrate the 4 cases described above, showing how
> a vanilla 2.6.25-rcN kernel behaves, and how it behaves with my patch applied.
>
> Finally, with my patch applied, I believe that the following lines could be
> removed from the sys_utimensat() routine (I've done some light testing to verify
> this, but more testing would be in order):
>
>                /* Nothing to do, we must not even check the path.  */
>                if (tstimes[0].tv_nsec == UTIME_OMIT &&
>                    tstimes[1].tv_nsec == UTIME_OMIT)
>                        return 0;
>
> Cheers,
>
> Michael
>
> Signed-off-by:  Michael Kerrisk <mtk.manpages@gmail.com>
>
> Test cases
> ==========
>
> All test cases run as user 'mtk'
>
> Bug 1
> -----
>
> Behavior with vanilla 2.6.25-rcN kernel:
>
> $ ls -l u
> -r-------- 1 mtk users 0 Apr  7 21:39 u
> $ ./t_utimensat u 1 n 1 n
> dirfd is -100
> pathname is u
> tsp is 0xbfb41408; struct  = { 1, 1073741823 } { 1, 1073741823 }
> flags is 0
> utimensat: Invalid argument
>
> Behavior with my patch applied:
>
> $ ls -l u
> -r-------- 1 mtk users 0 Apr  7 21:39 u
> $ ./t_utimensat u 1 n 1 n
> dirfd is -100
> pathname is u
> tsp is 0xbfab7378; struct  = { 1, 1073741823 } { 1, 1073741823 }
> flags is 0
> utimensat() succeeded
> Last file access:         Mon Apr  7 21:39:36 2008
> Last file modification:   Mon Apr  7 21:39:36 2008
> Last status change:       Mon Apr  7 21:39:36 2008
>
>
> Bug 2
> -----
>
> Behavior with vanilla 2.6.25-rcN kernel:
>
> $ ls -l p
> -rw-r--r-- 1 root root 0 Apr  7 10:34 p
> $ ./t_utimensat p 0 n 0 o
> dirfd is -100
> pathname is p
> tsp is 0xbfb7ac38; struct  = { 0, 1073741823 } { 0, 1073741822 }
> flags is 0
> utimensat() succeeded
> Last file access:         Mon Apr  7 22:00:51 2008
> Last file modification:   Mon Apr  7 10:34:00 2008
> Last status change:       Mon Apr  7 22:00:51 2008
>
>
> Behavior with my patch applied:
>
> $ ls -l p
> -rw-r--r-- 1 root root 0 Apr  7 10:34 p
> $ ./t_utimensat p 0 n 0 o
> dirfd is -100
> pathname is p
> tsp is 0xbfc40d08; struct  = { 1, 1073741823 } { 1, 1073741822 }
> flags is 0
> utimensat failed with EPERM
>
>
> Bug 3
> -----
>
> Behavior with vanilla 2.6.25-rcN kernel:
>
> $ ls -l p
> -rw-r--r-- 1 root root 0 Apr  7 22:03 p
> $ ./t_utimensat p 0 n 0 n
> dirfd is -100
> pathname is p
> tsp is 0xbfd99658; struct  = { 0, 1073741823 } { 0, 1073741823 }
> flags is 0
> utimensat() succeeded
> Last file access:         Mon Apr  7 22:03:31 2008
> Last file modification:   Mon Apr  7 22:03:31 2008
> Last status change:       Mon Apr  7 22:03:31 2008
> $ ./t_utimensat p
> dirfd is -100
> pathname is p
> tsp is (nil)
> flags is 0
> utimensat failed with EACCES
>
> Behavior with my patch applied:
>
> $ ls -l p
> -rw-r--r-- 1 root root 0 Apr  7 10:34 p
> $ ./t_utimensat p 0 n 0 n
> dirfd is -100
> pathname is p
> tsp is 0xbfa98358; struct  = { 0, 1073741823 } { 0, 1073741823 }
> flags is 0
> utimensat failed with EACCES
> $ ./t_utimensat p
> dirfd is -100
> pathname is p
> tsp is (nil)
> flags is 0
> utimensat failed with EACCES
>
>
> Bug 4
> -----
>
> Behavior with vanilla 2.6.25-rcN kernel:
>
> $ ls -l fa fi
> -rw-r--r-- 1 mtk users 0 Apr  7 21:43 fa
> -rw-r--r-- 1 mtk users 0 Apr  7 09:11 fi
> $ lsattr -l fa fi
> fa                           Append_Only
> fi                           Immutable
> $ ./t_utimensat fa 0 n 0 n
> dirfd is -100
> pathname is fa
> tsp is 0xbfc43508; struct  = { 0, 1073741823 } { 0, 1073741823 }
> flags is 0
> utimensat failed with EPERM
> $ ./t_utimensat fi 0 n 0 n
> dirfd is -100
> pathname is fi
> tsp is 0xbfb54c18; struct  = { 0, 1073741823 } { 0, 1073741823 }
> flags is 0
> utimensat failed with EPERM
>
>
> Behavior with my patch applied:
>
> $ ls -l fa fi
> -rw-r--r-- 1 mtk users 0 Apr  7 18:24 fa
> -rw-r--r-- 1 mtk users 0 Apr  7 09:11 fi
> $ lsattr -l fa fi
> fa                           Append_Only
> fi                           Immutable
> $ ./t_utimensat fa 0 n 0 n
> dirfd is -100
> pathname is fa
> tsp is 0xbfa7e338; struct  = { 0, 1073741823 } { 0, 1073741823 }
> flags is 0
> utimensat() succeeded
> Last file access:         Mon Apr  7 21:43:23 2008
> Last file modification:   Mon Apr  7 21:43:23 2008
> Last status change:       Mon Apr  7 21:43:23 2008
> $ ./t_utimensat fi 0 n 0 n
> dirfd is -100
> pathname is fi
> tsp is 0xbfe8d748; struct  = { 0, 1073741823 } { 0, 1073741823 }
> flags is 0
> utimensat failed with EACCES
>
>
>
> ==================================================
>
> --- linux-2.6.25-rc6-orig/fs/utimes.c   2008-04-07 22:25:08.000000000 +0200
> +++ linux-2.6.25-rc6/fs/utimes.c        2008-04-07 23:57:41.000000000 +0200
> @@ -95,27 +95,37 @@
>
>        /* Don't worry, the checks are done in inode_change_ok() */
>        newattrs.ia_valid = ATTR_CTIME | ATTR_MTIME | ATTR_ATIME;
> -       if (times) {
> +       if (times && ! ((times[0].tv_nsec == UTIME_NOW &&
> +                        times[1].tv_nsec == UTIME_NOW) ||
> +                       (times[0].tv_nsec == UTIME_OMIT &&
> +                        times[1].tv_nsec == UTIME_OMIT))) {
>                error = -EPERM;
>                 if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
>                         goto dput_and_out;
>
> -               if (times[0].tv_nsec == UTIME_OMIT)
> -                       newattrs.ia_valid &= ~ATTR_ATIME;
> -               else if (times[0].tv_nsec != UTIME_NOW) {
> +               if (times[0].tv_nsec == UTIME_OMIT) {
> +                       newattrs.ia_atime = inode->i_atime;
> +                       newattrs.ia_valid |= ATTR_ATIME_SET;
> +               } else if (times[0].tv_nsec != UTIME_NOW) {
>                        newattrs.ia_atime.tv_sec = times[0].tv_sec;
>                        newattrs.ia_atime.tv_nsec = times[0].tv_nsec;
>                        newattrs.ia_valid |= ATTR_ATIME_SET;
>                }
>
> -               if (times[1].tv_nsec == UTIME_OMIT)
> -                       newattrs.ia_valid &= ~ATTR_MTIME;
> -               else if (times[1].tv_nsec != UTIME_NOW) {
> +               if (times[1].tv_nsec == UTIME_OMIT) {
> +                       newattrs.ia_mtime = inode->i_mtime;
> +                       newattrs.ia_valid |= ATTR_MTIME_SET;
> +               } else if (times[1].tv_nsec != UTIME_NOW) {
>                        newattrs.ia_mtime.tv_sec = times[1].tv_sec;
>                        newattrs.ia_mtime.tv_nsec = times[1].tv_nsec;
>                        newattrs.ia_valid |= ATTR_MTIME_SET;
>                }
> +
> +       } else if (unlikely(times && times[0].tv_nsec == UTIME_OMIT &&
> +                       times[1].tv_nsec == UTIME_OMIT)) {
> +               newattrs.ia_valid &= ~(ATTR_ATIME | ATTR_MTIME | ATTR_CTIME);
>        } else {
> +               /* times is NULL, or both tv_nsec fields are UTIME_NOW */
>                error = -EACCES;
>                 if (IS_IMMUTABLE(inode))
>                         goto dput_and_out;
> @@ -150,14 +160,6 @@
>        if (utimes) {
>                if (copy_from_user(&tstimes, utimes, sizeof(tstimes)))
>                        return -EFAULT;
> -               if ((tstimes[0].tv_nsec == UTIME_OMIT ||
> -                    tstimes[0].tv_nsec == UTIME_NOW) &&
> -                   tstimes[0].tv_sec != 0)
> -                       return -EINVAL;
> -               if ((tstimes[1].tv_nsec == UTIME_OMIT ||
> -                    tstimes[1].tv_nsec == UTIME_NOW) &&
> -                   tstimes[1].tv_sec != 0)
> -                       return -EINVAL;
>
>                /* Nothing to do, we must not even check the path.  */
>                if (tstimes[0].tv_nsec == UTIME_OMIT &&
>
>
> ==================================================
>
> /* t_utimensat.c
>
>   Copyright (C) 2008, Michael Kerrisk <mtk.manpages@gmail.com>
>
>   Licensed under the GPLv2 or later.
>
>   A command-line interface for testing the utimensat() system
>   call.
>
>   17 Mar 2008  Initial creation
> */
> #define _GNU_SOURCE
> #define _ATFILE_SOURCE
> #include <stdio.h>
> #include <time.h>
> #include <errno.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <sys/syscall.h>
> #include <fcntl.h>
> #include <string.h>
> #include <sys/stat.h>
>
> #define errExit(msg)    do { perror(msg); exit(EXIT_FAILURE); \
>                        } while (0)
>
>
> #define __NR_utimensat          320     /* x86 syscall number */
>
> # define UTIME_NOW      ((1l << 30) - 1l)
> # define UTIME_OMIT     ((1l << 30) - 2l)
>
> static inline int
> utimensat(int dirfd, const char *pathname,
>          const struct timespec times[2], int flags)
> {
>    return syscall(__NR_utimensat, dirfd, pathname, times, flags);
> }
>
> static void
> usageError(char *progName)
> {
>    fprintf(stderr, "Usage: %s pathname [atime-sec "
>            "atime-nsec mtime-sec mtime-nsec]\n\n", progName);
>    fprintf(stderr, "Permitted options are:\n");
>    fprintf(stderr, "    [-d path] "
>            "open a directory file descriptor"
>            " (instead of using AT_FDCWD)\n");
>    fprintf(stderr, "    -w        Open directory file "
>            "descriptor with O_RDWR (instead of O_RDONLY)\n");
>    fprintf(stderr, "    -n        Use AT_SYMLINK_NOFOLLOW\n");
>    fprintf(stderr, "\n");
>
>    fprintf(stderr, "pathname can be \"NULL\" to use NULL "
>            "argument in call\n");
>    fprintf(stderr, "\n");
>
>    fprintf(stderr, "Either nsec field can be\n");
>    fprintf(stderr, "    'n' for UTIME_NOW\n");
>    fprintf(stderr, "    'o' for UTIME_OMIT\n");
>    fprintf(stderr, "\n");
>
>    fprintf(stderr, "If the time fields are omitted, "
>            "then a NULL 'times' argument is used\n");
>    fprintf(stderr, "\n");
>
>    exit(EXIT_FAILURE);
> }
>
> int
> main(int argc, char *argv[])
> {
>    int flags, dirfd, opt, oflag;
>    struct timespec ts[2];
>    struct timespec *tsp;
>    char *pathname, *dirfdPath;
>    struct stat sb;
>
>    flags = 0;
>    dirfd = AT_FDCWD;
>    dirfdPath = NULL;
>    oflag = O_RDONLY;
>
>    while ((opt = getopt(argc, argv, "d:nw")) != -1) {
>        switch (opt) {
>        case 'd':
>            dirfdPath = optarg;
>            break;
>
>        case 'n':
>            flags |= AT_SYMLINK_NOFOLLOW;
>            printf("Not following symbolic links\n");
>            break;
>
>        case 'w':
>            oflag = O_RDWR;
>            break;
>
>        default:
>            usageError(argv[0]);
>        }
>    }
>
>    if ((optind + 5 != argc) && (optind + 1 != argc))
>        usageError(argv[0]);
>
>    if (dirfdPath != NULL) {
>        dirfd = open(dirfdPath, oflag);
>        if (dirfd == -1) errExit("open");
>
>        printf("Opened dirfd");
>        printf(" O_RDWR");
>        printf(": %s\n", dirfdPath);
>    }
>
>    pathname = (strcmp(argv[optind], "NULL") == 0) ?
>                        NULL : argv[optind];
>
>    if (argc == optind + 1) {
>        tsp = NULL;
>
>    } else {
>        ts[0].tv_sec = atoi(argv[optind + 1]);
>        if (argv[optind + 2][0] == 'n') {
>            ts[0].tv_nsec = UTIME_NOW;
>        } else if (argv[optind + 2][0] == 'o') {
>            ts[0].tv_nsec = UTIME_OMIT;
>        } else {
>            ts[0].tv_nsec = atoi(argv[optind + 2]);
>        }
>
>        ts[1].tv_sec = atoi(argv[optind + 3]);
>        if (argv[optind + 4][0] == 'n') {
>            ts[1].tv_nsec = UTIME_NOW;
>        } else if (argv[optind + 4][0] == 'o') {
>            ts[1].tv_nsec = UTIME_OMIT;
>        } else {
>            ts[1].tv_nsec = atoi(argv[optind + 4]);
>        }
>
>        tsp = ts;
>    }
>
>    /* For testing purposes, it may have been useful to run this
>       program as set-user-ID-root so that a directory file
>       descriptor could be opened as root.  Now we reset to the
>       real UID before making the utimensat() call, so that the
>       permission checking is performed under that UID. */
>
>    if (geteuid() == 0) {
>        uid_t u;
>
>        u = getuid();
>
>        printf("Resettng UIDs to %ld\n", (long) u);
>
>        if (setresuid(u, u, u) == -1)
>            errExit("setresuid");
>    }
>
>    printf("dirfd is %d\n", dirfd);
>    printf("pathname is %s\n", pathname);
>    printf("tsp is %p", tsp);
>    if (tsp != NULL) {
>        printf("; struct  = { %ld, %ld } { %ld, %ld }",
>                (long) tsp[0].tv_sec, (long) tsp[0].tv_nsec,
>                (long) tsp[1].tv_sec, (long) tsp[1].tv_nsec);
>    }
>    printf("\n");
>    printf("flags is %d\n", flags);
>
>    if (utimensat(dirfd, pathname, tsp, flags) == -1) {
>        if (errno == EPERM)
>            printf("utimensat failed with EPERM\n");
>        else if (errno == EACCES)
>            printf("utimensat failed with EACCES\n");
>        else
>            perror("utimensat");
>        exit(EXIT_FAILURE);
>    }
>
>    printf("utimensat() succeeded\n");
>
>    if (stat(pathname, &sb) == -1) errExit("stat");
>    printf("Last file access:         %s", ctime(&sb.st_atime));
>    printf("Last file modification:   %s", ctime(&sb.st_mtime));
>    printf("Last status change:       %s", ctime(&sb.st_ctime));
>
>    exit(EXIT_SUCCESS);
> }
>



-- 
Michael Kerrisk
Maintainer of the Linux man-pages project
http://www.kernel.org/doc/man-pages/
Want to report a man-pages bug? Look here:
http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [PATCH] utimensat() non-conformances and fixes

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Michael Kerrisk wrote:
> 1. The draft POSIX.1-200x specification for utimensat() 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.

OK, for now.  I think the implemented behavior is better, though.


> However, the current implementation does not generate
> EPERM if one tv_nsec field is UTIME_NOW while the other is UTIME_OMIT -- it
> should give this error for that case.

This is probably a necessary change.  Non-synchronized changes might be
a security problem.


> However, in
> the same circumstances, when utimensat() is given a 'times' array in which
> both tv_nsec fields are UTIME_NOW, which provides equivalent functionality
> to specifying 'times' as NULL, the call succeeds.  I think that it should fail
> with the error EACCES in this case.

I guess so.


> (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.

Is this something I changed?  I doubt I added this.

- --
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iEYEARECAAYFAkgMMjsACgkQ2ijCOnn/RHT9dwCgxhprkeAg86sW11ilKtHaVYtO
Ae0An18utIREI/MnfwPO5HixxZbJz7zD
=hrvK
-----END PGP SIGNATURE-----

Re: [PATCH] utimensat() non-conformances and fixes

[Michael Kerrisk]

On Mon, Apr 21, 2008 at 8:20 AM, Ulrich Drepper <drepper@redhat.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Michael Kerrisk wrote:
> > 1. The draft POSIX.1-200x specification for utimensat() 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.
>
> OK, for now.  I think the implemented behavior is better, though.

My only real objection to the implemented behavior is that it doesn't
follow the spec.  It doesn't seem unreasonable to change the spec
here.  Or is it already too late for that?  (I suppose it may well
be.)

> > However, the current implementation does not generate
> > EPERM if one tv_nsec field is UTIME_NOW while the other is UTIME_OMIT -- it
> > should give this error for that case.
>
> This is probably a necessary change.  Non-synchronized changes might be
> a security problem.

Yes.

> > However, in
> > the same circumstances, when utimensat() is given a 'times' array in which
> > both tv_nsec fields are UTIME_NOW, which provides equivalent functionality
> > to specifying 'times' as NULL, the call succeeds.  I think that it should fail
> > with the error EACCES in this case.
>
> I guess so.

Ok.

> > (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.
>
> Is this something I changed?  I doubt I added this.

Well, I just went away and tested yet again, and utime[s]() with a
NULL second argument gives the behavior I describe (and always did) --
and utimensat() should behave the same way for the
(times == NULL && times[0].tv_nsec == UTIME_NOW && times[1].tv_nsec ==
UTIME_NOW)
case, but does not.

Cheers,

Michael

-- 
Michael Kerrisk
Maintainer of the Linux man-pages project
http://www.kernel.org/doc/man-pages/
Want to report a man-pages bug? Look here:
http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [PATCH] utimensat() non-conformances and fixes

[Miklos Szeredi]

[Please, please, please always CC linux-fsdevel on VFS related work.
It's even in MAINTAINERS now]

> --- linux-2.6.25-rc6-orig/fs/utimes.c	2008-04-07 22:25:08.000000000 +0200
> +++ linux-2.6.25-rc6/fs/utimes.c	2008-04-07 23:57:41.000000000 +0200
> @@ -95,27 +95,37 @@
> 
>  	/* Don't worry, the checks are done in inode_change_ok() */
>  	newattrs.ia_valid = ATTR_CTIME | ATTR_MTIME | ATTR_ATIME;
> -	if (times) {
> +	if (times && ! ((times[0].tv_nsec == UTIME_NOW &&
> +			 times[1].tv_nsec == UTIME_NOW) ||
> +			(times[0].tv_nsec == UTIME_OMIT &&
> +			 times[1].tv_nsec == UTIME_OMIT))) {
>  		error = -EPERM;
>                  if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
>                          goto dput_and_out;
> 
> -		if (times[0].tv_nsec == UTIME_OMIT)
> -			newattrs.ia_valid &= ~ATTR_ATIME;
> -		else if (times[0].tv_nsec != UTIME_NOW) {
> +		if (times[0].tv_nsec == UTIME_OMIT) {
> +			newattrs.ia_atime = inode->i_atime;
> +			newattrs.ia_valid |= ATTR_ATIME_SET;

This seems wrong.  Why exactly was this change made?

Setting the time to inode->i_atime is *not* the same as not setting
the time.  For some filesystems i_atime is just a cached value, that
may or may not match the actual last access time.  For those
filesystems this could have very strange effects.

> +		} else if (times[0].tv_nsec != UTIME_NOW) {
>  			newattrs.ia_atime.tv_sec = times[0].tv_sec;
>  			newattrs.ia_atime.tv_nsec = times[0].tv_nsec;
>  			newattrs.ia_valid |= ATTR_ATIME_SET;
>  		}
> 
> -		if (times[1].tv_nsec == UTIME_OMIT)
> -			newattrs.ia_valid &= ~ATTR_MTIME;
> -		else if (times[1].tv_nsec != UTIME_NOW) {
> +		if (times[1].tv_nsec == UTIME_OMIT) {
> +			newattrs.ia_mtime = inode->i_mtime;
> +			newattrs.ia_valid |= ATTR_MTIME_SET;

Ditto.

> +		} else if (times[1].tv_nsec != UTIME_NOW) {
>  			newattrs.ia_mtime.tv_sec = times[1].tv_sec;
>  			newattrs.ia_mtime.tv_nsec = times[1].tv_nsec;
>  			newattrs.ia_valid |= ATTR_MTIME_SET;
>  		}
> +
> +	} else if (unlikely(times && times[0].tv_nsec == UTIME_OMIT &&
> +			times[1].tv_nsec == UTIME_OMIT)) {
> +		newattrs.ia_valid &= ~(ATTR_ATIME | ATTR_MTIME | ATTR_CTIME);

So this is a no-op?  In which case this check could be moved to the
top of the function to just return zero.

Miklos

>  	} else {
> +		/* times is NULL, or both tv_nsec fields are UTIME_NOW */
>  		error = -EACCES;
>                  if (IS_IMMUTABLE(inode))
>                          goto dput_and_out;
> @@ -150,14 +160,6 @@
>  	if (utimes) {
>  		if (copy_from_user(&tstimes, utimes, sizeof(tstimes)))
>  			return -EFAULT;
> -		if ((tstimes[0].tv_nsec == UTIME_OMIT ||
> -		     tstimes[0].tv_nsec == UTIME_NOW) &&
> -		    tstimes[0].tv_sec != 0)
> -			return -EINVAL;
> -		if ((tstimes[1].tv_nsec == UTIME_OMIT ||
> -		     tstimes[1].tv_nsec == UTIME_NOW) &&
> -		    tstimes[1].tv_sec != 0)
> -			return -EINVAL;
> 
>  		/* Nothing to do, we must not even check the path.  */
>  		if (tstimes[0].tv_nsec == UTIME_OMIT &&
> 
> 
> ==================================================