lkml.org 
[lkml]   [2020]   [Jan]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
    On Fri, Jan 03, 2020 at 01:49:01AM +0000, Al Viro wrote:
    > On Thu, Jan 02, 2020 at 02:59:20PM +1100, Aleksa Sarai wrote:
    > > On 2020-01-01, Al Viro <viro@zeniv.linux.org.uk> wrote:
    > > > On Thu, Jan 02, 2020 at 01:44:07AM +1100, Aleksa Sarai wrote:
    > > >
    > > > > Thanks, this fixes the issue for me (and also fixes another reproducer I
    > > > > found -- mounting a symlink on top of itself then trying to umount it).
    > > > >
    > > > > Reported-by: Aleksa Sarai <cyphar@cyphar.com>
    > > > > Tested-by: Aleksa Sarai <cyphar@cyphar.com>
    > > >
    > > > Pushed into #fixes.
    > >
    > > Thanks. One other thing I noticed is that umount applies to the
    > > underlying symlink rather than the mountpoint on top. So, for example
    > > (using the same scripts I posted in the thread):
    > >
    > > # ln -s /tmp/foo link
    > > # ./mount_to_symlink /etc/passwd link
    > > # umount -l link # will attempt to unmount "/tmp/foo"
    > >
    > > Is that intentional?
    >
    > It's a mess, again in mountpoint_last(). FWIW, at some point I proposed
    > to have nd_jump_link() to fail with -ELOOP if the target was a symlink;
    > Linus asked for reasons deeper than my dislike of the semantics, I looked
    > around and hadn't spotted anything. And there hadn't been at the time,
    > but when four months later umount_lookup_last() went in I failed to look
    > for that source of potential problems in it ;-/

    FWIW, since Ian appears to agree that we want ->d_manage() on the mount
    crossing at the end of umount(2) lookup, here's a much simpler solution -
    kill mountpoint_last() and switch to using lookup_last(). As a side
    benefit, LOOKUP_NO_REVAL also goes away. It's possible to trim the
    things even more (path_mountpoint() is very similar to path_lookupat()
    at that point, and it's not hard to make the differences conditional on
    something like LOOKUP_UMOUNT); I would rather do that part in the
    cleanups series - the one below is easier to backport.

    Aleksa, Ian - could you see if the patch below works for you?

    commit e56b43b971a7c08762fceab330a52b7245041dbc
    Author: Al Viro <viro@zeniv.linux.org.uk>
    Date: Fri Jan 10 17:17:19 2020 -0500

    reimplement path_mountpoint() with less magic

    ... and get rid of a bunch of bugs in it. Background:
    the reason for path_mountpoint() is that umount() really doesn't
    want attempts to revalidate the root of what it's trying to umount.
    The thing we want to avoid actually happen from complete_walk();
    solution was to do something parallel to normal path_lookupat()
    and it both went overboard and got the boilerplate subtly
    (and not so subtly) wrong.

    A better solution is to do pretty much what the normal path_lookupat()
    does, but instead of complete_walk() do unlazy_walk(). All it takes
    to avoid that ->d_weak_revalidate() call... mountpoint_last() goes
    away, along with everything it got wrong, and so does the magic around
    LOOKUP_NO_REVAL.

    Another source of bugs is that when we traverse mounts at the final
    location (and we need to do that - umount . expects to get whatever's
    overmounting ., if any, out of the lookup) we really ought to take
    care of ->d_manage() - as it is, manual umount of autofs automount
    in progress can lead to unpleasant surprises for the daemon. Easily
    solved by using handle_lookup_down() instead of follow_mount().

    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

    diff --git a/fs/namei.c b/fs/namei.c
    index d6c91d1e88cb..1793661c3342 100644
    --- a/fs/namei.c
    +++ b/fs/namei.c
    @@ -1649,17 +1649,15 @@ static struct dentry *__lookup_slow(const struct qstr *name,
    if (IS_ERR(dentry))
    return dentry;
    if (unlikely(!d_in_lookup(dentry))) {
    - if (!(flags & LOOKUP_NO_REVAL)) {
    - int error = d_revalidate(dentry, flags);
    - if (unlikely(error <= 0)) {
    - if (!error) {
    - d_invalidate(dentry);
    - dput(dentry);
    - goto again;
    - }
    + int error = d_revalidate(dentry, flags);
    + if (unlikely(error <= 0)) {
    + if (!error) {
    + d_invalidate(dentry);
    dput(dentry);
    - dentry = ERR_PTR(error);
    + goto again;
    }
    + dput(dentry);
    + dentry = ERR_PTR(error);
    }
    } else {
    old = inode->i_op->lookup(inode, dentry, flags);
    @@ -2618,72 +2616,6 @@ int user_path_at_empty(int dfd, const char __user *name, unsigned flags,
    EXPORT_SYMBOL(user_path_at_empty);

    /**
    - * mountpoint_last - look up last component for umount
    - * @nd: pathwalk nameidata - currently pointing at parent directory of "last"
    - *
    - * This is a special lookup_last function just for umount. In this case, we
    - * need to resolve the path without doing any revalidation.
    - *
    - * The nameidata should be the result of doing a LOOKUP_PARENT pathwalk. Since
    - * mountpoints are always pinned in the dcache, their ancestors are too. Thus,
    - * in almost all cases, this lookup will be served out of the dcache. The only
    - * cases where it won't are if nd->last refers to a symlink or the path is
    - * bogus and it doesn't exist.
    - *
    - * Returns:
    - * -error: if there was an error during lookup. This includes -ENOENT if the
    - * lookup found a negative dentry.
    - *
    - * 0: if we successfully resolved nd->last and found it to not to be a
    - * symlink that needs to be followed.
    - *
    - * 1: if we successfully resolved nd->last and found it to be a symlink
    - * that needs to be followed.
    - */
    -static int
    -mountpoint_last(struct nameidata *nd)
    -{
    - int error = 0;
    - struct dentry *dir = nd->path.dentry;
    - struct path path;
    -
    - /* If we're in rcuwalk, drop out of it to handle last component */
    - if (nd->flags & LOOKUP_RCU) {
    - if (unlazy_walk(nd))
    - return -ECHILD;
    - }
    -
    - nd->flags &= ~LOOKUP_PARENT;
    -
    - if (unlikely(nd->last_type != LAST_NORM)) {
    - error = handle_dots(nd, nd->last_type);
    - if (error)
    - return error;
    - path.dentry = dget(nd->path.dentry);
    - } else {
    - path.dentry = d_lookup(dir, &nd->last);
    - if (!path.dentry) {
    - /*
    - * No cached dentry. Mounted dentries are pinned in the
    - * cache, so that means that this dentry is probably
    - * a symlink or the path doesn't actually point
    - * to a mounted dentry.
    - */
    - path.dentry = lookup_slow(&nd->last, dir,
    - nd->flags | LOOKUP_NO_REVAL);
    - if (IS_ERR(path.dentry))
    - return PTR_ERR(path.dentry);
    - }
    - }
    - if (d_flags_negative(smp_load_acquire(&path.dentry->d_flags))) {
    - dput(path.dentry);
    - return -ENOENT;
    - }
    - path.mnt = nd->path.mnt;
    - return step_into(nd, &path, 0, d_backing_inode(path.dentry), 0);
    -}
    -
    -/**
    * path_mountpoint - look up a path to be umounted
    * @nd: lookup context
    * @flags: lookup flags
    @@ -2699,14 +2631,17 @@ path_mountpoint(struct nameidata *nd, unsigned flags, struct path *path)
    int err;

    while (!(err = link_path_walk(s, nd)) &&
    - (err = mountpoint_last(nd)) > 0) {
    + (err = lookup_last(nd)) > 0) {
    s = trailing_symlink(nd);
    }
    + if (!err)
    + err = unlazy_walk(nd);
    + if (!err)
    + err = handle_lookup_down(nd);
    if (!err) {
    *path = nd->path;
    nd->path.mnt = NULL;
    nd->path.dentry = NULL;
    - follow_mount(path);
    }
    terminate_walk(nd);
    return err;
    diff --git a/fs/nfs/nfstrace.h b/fs/nfs/nfstrace.h
    index f64a33d2a1d1..2a82dcce5fc1 100644
    --- a/fs/nfs/nfstrace.h
    +++ b/fs/nfs/nfstrace.h
    @@ -206,7 +206,6 @@ TRACE_DEFINE_ENUM(LOOKUP_AUTOMOUNT);
    TRACE_DEFINE_ENUM(LOOKUP_PARENT);
    TRACE_DEFINE_ENUM(LOOKUP_REVAL);
    TRACE_DEFINE_ENUM(LOOKUP_RCU);
    -TRACE_DEFINE_ENUM(LOOKUP_NO_REVAL);
    TRACE_DEFINE_ENUM(LOOKUP_OPEN);
    TRACE_DEFINE_ENUM(LOOKUP_CREATE);
    TRACE_DEFINE_ENUM(LOOKUP_EXCL);
    @@ -224,7 +223,6 @@ TRACE_DEFINE_ENUM(LOOKUP_DOWN);
    { LOOKUP_PARENT, "PARENT" }, \
    { LOOKUP_REVAL, "REVAL" }, \
    { LOOKUP_RCU, "RCU" }, \
    - { LOOKUP_NO_REVAL, "NO_REVAL" }, \
    { LOOKUP_OPEN, "OPEN" }, \
    { LOOKUP_CREATE, "CREATE" }, \
    { LOOKUP_EXCL, "EXCL" }, \
    diff --git a/include/linux/namei.h b/include/linux/namei.h
    index 7fe7b87a3ded..07bfb0874033 100644
    --- a/include/linux/namei.h
    +++ b/include/linux/namei.h
    @@ -34,7 +34,6 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};

    /* internal use only */
    #define LOOKUP_PARENT 0x0010
    -#define LOOKUP_NO_REVAL 0x0080
    #define LOOKUP_JUMPED 0x1000
    #define LOOKUP_ROOT 0x2000
    #define LOOKUP_ROOT_GRABBED 0x0008
    \
     
     \ /
      Last update: 2020-01-11 00:20    [W:3.857 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site