Messages in this thread | | | Subject | Re: [PATCH RESEND 1/1] vfs: Really check for inode ptr in lookup_fast | From | Ritesh Harjani <> | Date | Tue, 22 Oct 2019 19:08:54 +0530 |
| |
I think we have still not taken this patch. Al?
On 10/15/19 9:37 AM, Ritesh Harjani wrote: > 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; >> } >> >
| |