lkml.org 
[lkml]   [2008]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH] utimensat() non-conformances and fixes -- version 2
    From
    Miklos,

    Thanks for the comments.

    Miklos Szeredi wrote:
    >> This is a revised version of my earlier
    >> (http://article.gmane.org/gmane.linux.man/129/ ) patch to fix
    >> non-conformances in the utimensat() implementation.
    >
    > The patch looks functionally correct. But there are several things I
    > don't really like...

    [..]

    >> - if (!times || (nsec_special(times[0].tv_nsec) &&
    >> - nsec_special(times[1].tv_nsec))) {
    >> + if (!times || (times[0].tv_nsec == UTIME_NOW &&
    >> + times[1].tv_nsec == UTIME_NOW)) {
    >> error = -EACCES;
    >> +
    >
    > How about explicitly turning UTIME_NOW/UTIME_NOW into times = NULL at
    > the beginning of the function? That would both simplify things and
    > also make it absolutely sure that the two cases are handled the same
    > way (which makes sense, and is also what the standard wants).

    Yes, simple, and sensible.

    >> if (IS_IMMUTABLE(inode))
    >> goto mnt_drop_write_and_out;
    >>
    >> @@ -147,7 +145,20 @@
    >> goto mnt_drop_write_and_out;
    >> }
    >> }
    >> + } else if (times && ((times[0].tv_nsec == UTIME_NOW &&
    >> + times[1].tv_nsec == UTIME_OMIT)
    >> + ||
    >> + (times[0].tv_nsec == UTIME_OMIT &&
    >> + times[1].tv_nsec == UTIME_NOW))) {
    >> + error = -EPERM;
    >> +
    >> + if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
    >> + goto mnt_drop_write_and_out;
    >> +
    >> + if (!is_owner_or_cap(inode))
    >> + goto mnt_drop_write_and_out;
    >
    > I don't like adding _more_ owner checks to this function. It would be
    > better if we were removing them: some weird filesystems want to do
    > their own permission checking and so the owner checks should really be
    > moved into inode_change_ok().
    >
    > One way to do that could be to add a pseudo attribute flag,
    > e.g. ATTR_TIMES_UPDATE, that tells the permission checking code to
    > check the owner, even when neither ATTR_[AM]TIME_SET flag is set.
    >
    > Even the check for the owner in the !times case could be removed, by
    > adding ATTR_TIMES_UPDATE only if we don't have write permission on the
    > file. That's a cleanup I'd really be happy with.

    See below.

    > All this may also be done with the ATTR_FORCE flag, but that would
    > mean:
    >
    > - modifying lots of call sites
    > - making it impossible to selectively check the permission if
    > multiple attributes are being modified (don't know if any callers
    > want that though).

    Okay, I won't go that route...

    Regarding your suggestions above, are you meaning something like the
    patch below?

    The patch is a little less pretty than I'd like because of the need to
    return EACCES or EPERM depending on whether (times == NULL). In
    particular, these lines in utimes.c:

    + if (!times && error == -EPERM)
    + error = -EACCES;

    seem a little fragile (but maybe I worry too much).

    Cheers,

    Michael

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

    diff -ru linux-2.6.26-rc2/fs/attr.c linux-2.6.26-rc2-utimensat-fix/fs/attr.c
    --- linux-2.6.26-rc2/fs/attr.c 2008-01-24 23:58:37.000000000 +0100
    +++ linux-2.6.26-rc2-utimensat-fix/fs/attr.c 2008-05-16 21:56:51.000000000 +0200
    @@ -51,7 +51,8 @@
    }

    /* Check for setting the inode time. */
    - if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET)) {
    + if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET |
    + ATTR_TIMES_UPDATE)) {
    if (!is_owner_or_cap(inode))
    goto error;
    }
    diff -ru linux-2.6.26-rc2/fs/utimes.c linux-2.6.26-rc2-utimensat-fix/fs/utimes.c
    --- linux-2.6.26-rc2/fs/utimes.c 2008-05-15 10:33:20.000000000 +0200
    +++ linux-2.6.26-rc2-utimensat-fix/fs/utimes.c 2008-05-16 22:14:31.000000000 +0200
    @@ -40,14 +40,9 @@

    #endif

    -static bool nsec_special(long nsec)
    -{
    - return nsec == UTIME_OMIT || nsec == UTIME_NOW;
    -}
    -
    static bool nsec_valid(long nsec)
    {
    - if (nsec_special(nsec))
    + if (nsec == UTIME_OMIT || nsec == UTIME_NOW)
    return true;

    return nsec >= 0 && nsec <= 999999999;
    @@ -102,11 +97,15 @@
    if (error)
    goto dput_and_out;

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

    if (times[0].tv_nsec == UTIME_OMIT)
    @@ -131,25 +130,39 @@
    * UTIME_NOW, then need to check permissions, because
    * inode_change_ok() won't do it.
    */
    - if (!times || (nsec_special(times[0].tv_nsec) &&
    - nsec_special(times[1].tv_nsec))) {
    + if (!times) {
    error = -EACCES;
    if (IS_IMMUTABLE(inode))
    goto mnt_drop_write_and_out;

    - if (!is_owner_or_cap(inode)) {
    - if (f) {
    - if (!(f->f_mode & FMODE_WRITE))
    - goto mnt_drop_write_and_out;
    - } else {
    - error = vfs_permission(&nd, MAY_WRITE);
    - if (error)
    - goto mnt_drop_write_and_out;
    - }
    + if (f) {
    + if (!(f->f_mode & FMODE_WRITE))
    + newattrs.ia_valid |= ATTR_TIMES_UPDATE;
    + } else {
    + error = vfs_permission(&nd, MAY_WRITE);
    + if (error == -EACCES)
    + newattrs.ia_valid |= ATTR_TIMES_UPDATE;
    + else if (error)
    + goto mnt_drop_write_and_out;
    }
    + } else if (times && ((times[0].tv_nsec == UTIME_NOW &&
    + times[1].tv_nsec == UTIME_OMIT)
    + ||
    + (times[0].tv_nsec == UTIME_OMIT &&
    + times[1].tv_nsec == UTIME_NOW))) {
    + error = -EPERM;
    +
    + if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
    + goto mnt_drop_write_and_out;
    +
    + newattrs.ia_valid |= ATTR_TIMES_UPDATE;
    }
    +
    mutex_lock(&inode->i_mutex);
    error = notify_change(dentry, &newattrs);
    + if (!times && error == -EPERM)
    + error = -EACCES;
    +
    mutex_unlock(&inode->i_mutex);
    mnt_drop_write_and_out:
    mnt_drop_write(mnt);
    diff -ru linux-2.6.26-rc2/include/linux/fs.h linux-2.6.26-rc2-utimensat-fix/include/linux/fs.h
    --- linux-2.6.26-rc2/include/linux/fs.h 2008-05-15 10:33:25.000000000 +0200
    +++ linux-2.6.26-rc2-utimensat-fix/include/linux/fs.h 2008-05-16 21:39:24.000000000 +0200
    @@ -317,22 +317,23 @@
    * Attribute flags. These should be or-ed together to figure out what
    * has been changed!
    */
    -#define ATTR_MODE 1
    -#define ATTR_UID 2
    -#define ATTR_GID 4
    -#define ATTR_SIZE 8
    -#define ATTR_ATIME 16
    -#define ATTR_MTIME 32
    -#define ATTR_CTIME 64
    -#define ATTR_ATIME_SET 128
    -#define ATTR_MTIME_SET 256
    -#define ATTR_FORCE 512 /* Not a change, but a change it */
    -#define ATTR_ATTR_FLAG 1024
    -#define ATTR_KILL_SUID 2048
    -#define ATTR_KILL_SGID 4096
    -#define ATTR_FILE 8192
    -#define ATTR_KILL_PRIV 16384
    -#define ATTR_OPEN 32768 /* Truncating from open(O_TRUNC) */
    +#define ATTR_MODE 1
    +#define ATTR_UID 2
    +#define ATTR_GID 4
    +#define ATTR_SIZE 8
    +#define ATTR_ATIME 16
    +#define ATTR_MTIME 32
    +#define ATTR_CTIME 64
    +#define ATTR_ATIME_SET 128
    +#define ATTR_MTIME_SET 256
    +#define ATTR_FORCE 512 /* Not a change, but a change it */
    +#define ATTR_ATTR_FLAG 1024
    +#define ATTR_KILL_SUID 2048
    +#define ATTR_KILL_SGID 4096
    +#define ATTR_FILE 8192
    +#define ATTR_KILL_PRIV 16384
    +#define ATTR_OPEN 32768 /* Truncating from open(O_TRUNC) */
    +#define ATTR_TIMES_UPDATE 65536

    /*
    * This is the Inode Attributes structure, used for notify_change(). It

    \
     
     \ /
      Last update: 2008-05-17 22:01    [W:0.032 / U:128.916 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site