missing mnt_drop_write() on open error - Kernel

This is a discussion on missing mnt_drop_write() on open error - Kernel ; I get this at umount, if there was a failed open(): WARNING: at fs/namespace.c:586 __mntput() I think the problem is that may_open() calls mnt_want_write(), but if open doesn't succeed, mnt_drop_write() will not be called. Miklos - To unsubscribe from this ...

+ Reply to Thread
Results 1 to 10 of 10

Thread: missing mnt_drop_write() on open error

  1. missing mnt_drop_write() on open error

    I get this at umount, if there was a failed open():

    WARNING: at fs/namespace.c:586 __mntput()

    I think the problem is that may_open() calls mnt_want_write(), but if
    open doesn't succeed, mnt_drop_write() will not be called.

    Miklos
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: missing mnt_drop_write() on open error

    On Wed, 2007-09-26 at 01:14 +0200, Miklos Szeredi wrote:
    > I get this at umount, if there was a failed open():
    >
    > WARNING: at fs/namespace.c:586 __mntput()
    >
    > I think the problem is that may_open() calls mnt_want_write(), but if
    > open doesn't succeed, mnt_drop_write() will not be called.


    I see what you're talking about, and it does look like a bug to me. I'm
    working on establishing where the 'struct file' gets created in relation
    to all of this, and where best we should back out the write count.

    I should have a patch in a few.

    -- Dave

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: missing mnt_drop_write() on open error

    On Wed, 2007-09-26 at 01:14 +0200, Miklos Szeredi wrote:
    > I get this at umount, if there was a failed open():
    >
    > WARNING: at fs/namespace.c:586 __mntput()
    >
    > I think the problem is that may_open() calls mnt_want_write(), but if
    > open doesn't succeed, mnt_drop_write() will not be called.


    Does this help? I'm also thinking that we should change the open_namei*
    functions to simply return 'struct file *'. Those are the only users
    other than NFS, and forcing the return of a file like that will force
    users to do the fput() on it if they don't want it any more. We'd just
    need to make sure no new may_open() users pop up. Any thoughts on that?

    --

    The r/o bind mount patches have an error in them:
    they unconditionally take a mnt_want_write() on
    all may_open() requests for writeable files. They
    mistakenly do not mnt_drop_write() if may_open()
    encounters an error.

    ---

    lxc-dave/fs/namei.c | 39 ++++++++++++++++++++++++++++++---------
    1 file changed, 30 insertions(+), 9 deletions(-)

    diff -puN fs/namei.c~drop-write-if-may_open-fails fs/namei.c
    --- lxc/fs/namei.c~drop-write-if-may_open-fails 2007-09-25 18:16:59.000000000 -0700
    +++ lxc-dave/fs/namei.c 2007-09-25 18:16:59.000000000 -0700
    @@ -1594,11 +1594,23 @@ int vfs_create(struct inode *dir, struct
    return error;
    }

    +/*
    + * If you call this with FMODE_WRITE set in flag,
    + * and get a successful return, you will have
    + * acquired a write on nd->mnt.
    + *
    + * Basically, if you use this function, you either
    + * need to fix up that write count manually, or just
    + * return a 'struct file' from your function. When
    + * you __fput() that 'struct file' it will get fixed
    + * up automatically.
    + */
    int may_open(struct nameidata *nd, int acc_mode, int flag)
    {
    struct dentry *dentry = nd->dentry;
    struct inode *inode = dentry->d_inode;
    int error;
    + int took_mnt_write = 0;

    if (!inode)
    return -ENOENT;
    @@ -1624,42 +1636,48 @@ int may_open(struct nameidata *nd, int a
    } else if (flag & FMODE_WRITE) {
    /*
    * effectively: !special_file()
    - * balanced by __fput()
    + * Balanced by __fput() and
    + * released on error at the end
    + * of this function.
    */
    error = mnt_want_write(nd->mnt);
    if (error)
    return error;
    + took_mnt_write = 1;
    }

    error = vfs_permission(nd, acc_mode);
    if (error)
    - return error;
    + goto out_err;
    /*
    * An append-only file must be opened in append mode for writing.
    */
    if (IS_APPEND(inode)) {
    + error = -EPERM;
    if ((flag & FMODE_WRITE) && !(flag & O_APPEND))
    - return -EPERM;
    + goto out_err;
    + error = -EPERM;
    if (flag & O_TRUNC)
    - return -EPERM;
    + goto out_err;
    }

    /* O_NOATIME can only be set by the owner or superuser */
    + error = -EPERM;
    if (flag & O_NOATIME)
    if (!is_owner_or_cap(inode))
    - return -EPERM;
    + goto out_err;

    /*
    * Ensure there are no outstanding leases on the file.
    */
    error = break_lease(inode, flag);
    if (error)
    - return error;
    + goto out_err;

    if (flag & O_TRUNC) {
    error = get_write_access(inode);
    if (error)
    - return error;
    + goto out_err;

    /*
    * Refuse to truncate files with mandatory locks held on them.
    @@ -1672,12 +1690,15 @@ int may_open(struct nameidata *nd, int a
    }
    put_write_access(inode);
    if (error)
    - return error;
    + goto out_err;
    } else
    if (flag & FMODE_WRITE)
    DQUOT_INIT(inode);

    - return 0;
    +out_err:
    + if (error && took_mnt_write)
    + mnt_drop_write(nd->mnt);
    + return error;
    }

    static int open_namei_create(struct nameidata *nd, struct path *path,

    -- Dave

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: missing mnt_drop_write() on open error

    > On Wed, 2007-09-26 at 01:14 +0200, Miklos Szeredi wrote:
    > > I get this at umount, if there was a failed open():
    > >
    > > WARNING: at fs/namespace.c:586 __mntput()
    > >
    > > I think the problem is that may_open() calls mnt_want_write(), but if
    > > open doesn't succeed, mnt_drop_write() will not be called.

    >
    > Does this help?


    It didn't fix it for me, but the patch looks OK.

    In __dentry_open() there's still a few places where fput() won't be
    called, notably when ->open fails, which is what I'm triggering I
    think.

    Also even more horrible things can happen because of the
    nd->intent.open.file thing. For example if the lookup routine calls
    lookup_instantiate_filp(), and after this, but before may_open() some
    error happens, then release_open_intent() will call fput() on the
    file, which will cause mnt_drop_write() to be called, even though a
    matching mnt_want_write() hasn't yet been called. Ugly, eh?

    > I'm also thinking that we should change the open_namei*
    > functions to simply return 'struct file *'. Those are the only users
    > other than NFS, and forcing the return of a file like that will force
    > users to do the fput() on it if they don't want it any more. We'd just
    > need to make sure no new may_open() users pop up. Any thoughts on that?


    Yeah, something needs to be done with open, because currently it's way
    too convoluted.

    Miklos
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: missing mnt_drop_write() on open error

    On Wed, Sep 26, 2007 at 10:38:22AM +0200, Miklos Szeredi wrote:
    > Also even more horrible things can happen because of the
    > nd->intent.open.file thing. For example if the lookup routine calls
    > lookup_instantiate_filp(), and after this, but before may_open() some
    > error happens, then release_open_intent() will call fput() on the
    > file, which will cause mnt_drop_write() to be called, even though a
    > matching mnt_want_write() hasn't yet been called. Ugly, eh?


    It's more than horrible. I wouldn't mind using the word utter crap
    for it. It's defintively on my todo list to get rid of this junk
    again. It managed to get in without proper review unfortunately.

    >
    > > I'm also thinking that we should change the open_namei*
    > > functions to simply return 'struct file *'. Those are the only users
    > > other than NFS, and forcing the return of a file like that will force
    > > users to do the fput() on it if they don't want it any more. We'd just
    > > need to make sure no new may_open() users pop up. Any thoughts on that?

    >
    > Yeah, something needs to be done with open, because currently it's way
    > too convoluted.


    I have some changes for open_namei pending and I'll see if incorporating
    this makes sense.
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: missing mnt_drop_write() on open error

    On Wed, 2007-09-26 at 10:38 +0200, Miklos Szeredi wrote:
    > > On Wed, 2007-09-26 at 01:14 +0200, Miklos Szeredi wrote:
    > > > I get this at umount, if there was a failed open():
    > > >
    > > > WARNING: at fs/namespace.c:586 __mntput()
    > > >
    > > > I think the problem is that may_open() calls mnt_want_write(), but if
    > > > open doesn't succeed, mnt_drop_write() will not be called.

    > >
    > > Does this help?

    >
    > It didn't fix it for me, but the patch looks OK.
    >
    > In __dentry_open() there's still a few places where fput() won't be
    > called, notably when ->open fails, which is what I'm triggering I
    > think.
    >
    > Also even more horrible things can happen because of the
    > nd->intent.open.file thing. For example if the lookup routine calls
    > lookup_instantiate_filp(), and after this, but before may_open() some
    > error happens, then release_open_intent() will call fput() on the
    > file, which will cause mnt_drop_write() to be called, even though a
    > matching mnt_want_write() hasn't yet been called. Ugly, eh?


    I used to have a patch that didn't completely trust that all files with
    FMODE_WRITE set to have taken a write on the mnt. I think I used a flag
    to indicate whether or not a particular file had a mnt_want_write() done
    on its behalf. It somewhat artificially keeps the mnt write count
    balanced, but I think it will let us detect when things like this go on.

    > > I'm also thinking that we should change the open_namei*
    > > functions to simply return 'struct file *'. Those are the only users
    > > other than NFS, and forcing the return of a file like that will force
    > > users to do the fput() on it if they don't want it any more. We'd just
    > > need to make sure no new may_open() users pop up. Any thoughts on that?

    >
    > Yeah, something needs to be done with open, because currently it's way
    > too convoluted.


    Sounds like Christoph has some ideas...

    -- Dave

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: missing mnt_drop_write() on open error

    On Wed, 2007-09-26 at 10:38 +0200, Miklos Szeredi wrote:
    > In __dentry_open() there's still a few places where fput() won't be
    > called, notably when ->open fails, which is what I'm triggering I
    > think.
    >
    > Also even more horrible things can happen because of the
    > nd->intent.open.file thing. For example if the lookup routine calls
    > lookup_instantiate_filp(), and after this, but before may_open() some
    > error happens, then release_open_intent() will call fput() on the
    > file, which will cause mnt_drop_write() to be called, even though a
    > matching mnt_want_write() hasn't yet been called. Ugly, eh?


    I'm not sure it is _that_ horrible.

    Do you see any reason we can't just shadow the
    get/put_write_access(inode) calls with mnt_want/drop_write() calls? I
    think they're always matched.

    -- Dave

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: missing mnt_drop_write() on open error

    > On Wed, 2007-09-26 at 10:38 +0200, Miklos Szeredi wrote:
    > > In __dentry_open() there's still a few places where fput() won't be
    > > called, notably when ->open fails, which is what I'm triggering I
    > > think.
    > >
    > > Also even more horrible things can happen because of the
    > > nd->intent.open.file thing. For example if the lookup routine calls
    > > lookup_instantiate_filp(), and after this, but before may_open() some
    > > error happens, then release_open_intent() will call fput() on the
    > > file, which will cause mnt_drop_write() to be called, even though a
    > > matching mnt_want_write() hasn't yet been called. Ugly, eh?

    >
    > I'm not sure it is _that_ horrible.
    >
    > Do you see any reason we can't just shadow the
    > get/put_write_access(inode) calls with mnt_want/drop_write() calls? I
    > think they're always matched.


    Maybe. Can we do the mnt_want_write() from __dentry_open(), instead
    of may_open()? That would be a lot cleaner.

    Btw, may_open() doesn't do mnt_want_write() around the truncation if
    file is opened with O_TRUNC | O_RDONLY.

    Miklos
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: missing mnt_drop_write() on open error

    On Wed, 2007-09-26 at 19:50 +0200, Miklos Szeredi wrote:
    > Maybe. Can we do the mnt_want_write() from __dentry_open(), instead
    > of may_open()? That would be a lot cleaner.


    I'll explore that. It may make very good sense.

    > Btw, may_open() doesn't do mnt_want_write() around the truncation if
    > file is opened with O_TRUNC | O_RDONLY.


    What's the path to may_open() in that case? open_namei() should wrap
    all callers other than nfs, and it does:

    /* O_TRUNC implies we need access checks for write permissions */
    if (flag & O_TRUNC)
    acc_mode |= MAY_WRITE;

    Which should trigger the may_open() code.

    later in open_namei():
    ....
    ok:
    error = may_open(nd, acc_mode, flag);
    if (error)
    goto exit;
    return 0;


    -- Dave

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  10. Re: missing mnt_drop_write() on open error

    > > Btw, may_open() doesn't do mnt_want_write() around the truncation if
    > > file is opened with O_TRUNC | O_RDONLY.

    >
    > What's the path to may_open() in that case? open_namei() should wrap
    > all callers other than nfs, and it does:
    >
    > /* O_TRUNC implies we need access checks for write permissions */
    > if (flag & O_TRUNC)
    > acc_mode |= MAY_WRITE;
    >
    > Which should trigger the may_open() code.


    Ah, I missed that.

    Thanks,
    Miklos
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread