From: Ritesh Harjani <riteshh@linux.ibm.com>
To: viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, wugyuan@cn.ibm.com,
jlayton@kernel.org, hsiangkao@aol.com
Subject: Re: [PATCH RESEND 1/1] vfs: Really check for inode ptr in lookup_fast
Date: Tue, 15 Oct 2019 09:37:28 +0530 [thread overview]
Message-ID: <20191015040730.6A84742047@d06av24.portsmouth.uk.ibm.com> (raw)
In-Reply-To: <20190927044243.18856-1-riteshh@linux.ibm.com>
ping!!
On 9/27/19 10:12 AM, Ritesh Harjani wrote:
> d_is_negative can race with d_instantiate_new()
> -> __d_set_inode_and_type().
> For e.g. in use cases where Thread-1 is creating
> symlink (doing d_instantiate_new()) & Thread-2 is doing
> cat of that symlink while doing lookup_fast (via REF-walk-
> one such case is, when ->permission returns -ECHILD).
>
> During this race if __d_set_and_inode_type() does out-of-order
> execution and set the dentry->d_flags before setting
> dentry->inode, then it can result into following kernel panic.
>
> This change fixes the issue by directly checking for inode.
>
> E.g. kernel panic, since inode was NULL.
> trailing_symlink() -> may_follow_link() -> inode->i_uid.
> Issue signature:-
> [NIP : trailing_symlink+80]
> [LR : trailing_symlink+1092]
> #4 [c00000198069bb70] trailing_symlink at c0000000004bae60 (unreliable)
> #5 [c00000198069bc00] path_openat at c0000000004bdd14
> #6 [c00000198069bc90] do_filp_open at c0000000004c0274
> #7 [c00000198069bdb0] do_sys_open at c00000000049b248
> #8 [c00000198069be30] system_call at c00000000000b388
>
> Sequence of events:-
> Thread-2(Comm: ln) Thread-1(Comm: cat)
>
> dentry = __d_lookup() //nonRCU
>
> __d_set_and_inode_type() (Out-of-order execution)
> flags = READ_ONCE(dentry->d_flags);
> flags &= ~(DCACHE_ENTRY_TYPE | DCACHE_FALLTHRU);
> flags |= type_flags;
> WRITE_ONCE(dentry->d_flags, flags);
>
> if (unlikely(d_is_negative()) // fails
> {}
> // since d_flags is already updated in
> // Thread-2 in parallel but inode
> // not yet set.
> // d_is_negative returns false
>
> *inode = d_backing_inode(path->dentry);
> // means inode is still NULL
>
> dentry->d_inode = inode;
>
> trailing_symlink()
> may_follow_link()
> inode = nd->link_inode;
> // nd->link_inode = NULL
> //Then it crashes while
> //doing inode->i_uid
>
> Reported-by: Guang Yuan Wu <wugyuan@cn.ibm.com>
> Tested-by: Guang Yuan Wu <wugyuan@cn.ibm.com>
> Acked-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> ---
> fs/namei.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 671c3c1a3425..7c5337cddebd 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1617,7 +1617,21 @@ static int lookup_fast(struct nameidata *nd,
> dput(dentry);
> return status;
> }
> - if (unlikely(d_is_negative(dentry))) {
> +
> + /*
> + * Caution: d_is_negative() can race with
> + * __d_set_inode_and_type().
> + * For e.g. in use cases where Thread-1 is creating
> + * symlink (doing d_instantiate_new()) & Thread-2 is doing
> + * cat of that symlink and falling here (via Ref-walk) while
> + * doing lookup_fast (one such case is when ->permission
> + * returns -ECHILD).
> + * Now if __d_set_inode_and_type() does out-of-order execution
> + * i.e. it first sets the dentry->d_flags & then dentry->inode
> + * then it can result into inode being NULL, causing panic later.
> + * Hence directly check if inode is NULL here.
> + */
> + if (unlikely(d_really_is_negative(dentry))) {
> dput(dentry);
> return -ENOENT;
> }
>
next prev parent reply other threads:[~2019-10-15 4:10 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-27 4:42 [PATCH RESEND 1/1] vfs: Really check for inode ptr in lookup_fast Ritesh Harjani
2019-10-15 4:07 ` Ritesh Harjani [this message]
2019-10-22 13:38 ` Ritesh Harjani
2019-10-22 14:37 ` Al Viro
2019-10-22 14:50 ` Al Viro
2019-10-22 20:11 ` Al Viro
2019-10-23 11:05 ` Ritesh Harjani
2019-11-01 23:46 ` Al Viro
2019-11-02 6:17 ` Al Viro
2019-11-02 17:24 ` Paul E. McKenney
2019-11-02 17:22 ` Paul E. McKenney
2019-11-02 18:08 ` Al Viro
2019-11-03 14:41 ` Paul E. McKenney
2019-11-03 16:35 ` [RFC] lookup_one_len_unlocked() lousy calling conventions Al Viro
2019-11-03 18:20 ` Al Viro
2019-11-03 18:51 ` [PATCH][RFC] ecryptfs_lookup_interpose(): lower_dentry->d_inode is not stable Al Viro
2019-11-03 19:03 ` [PATCH][RFC] ecryptfs_lookup_interpose(): lower_dentry->d_parent is not stable either Al Viro
2019-11-13 7:01 ` [PATCH][RFC] ecryptfs_lookup_interpose(): lower_dentry->d_inode is not stable Amir Goldstein
2019-11-13 12:52 ` Al Viro
2019-11-13 16:22 ` Amir Goldstein
2019-11-13 20:18 ` Jean-Louis Biasini
2019-11-03 17:05 ` [PATCH][RFC] ecryptfs unlink/rmdir breakage (similar to caught in ecryptfs rename last year) Al Viro
2019-11-09 3:13 ` [PATCH][RFC] race in exportfs_decode_fh() Al Viro
2019-11-09 16:55 ` Linus Torvalds
2019-11-09 18:26 ` Al Viro
2019-11-11 9:16 ` Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191015040730.6A84742047@d06av24.portsmouth.uk.ibm.com \
--to=riteshh@linux.ibm.com \
--cc=hsiangkao@aol.com \
--cc=jlayton@kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=viro@zeniv.linux.org.uk \
--cc=wugyuan@cn.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).