From patchwork Tue Jul 17 12:46:08 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Miklos Szeredi X-Patchwork-Id: 10529357 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 68BF4600F4 for ; Tue, 17 Jul 2018 12:46:14 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4C1C928F40 for ; Tue, 17 Jul 2018 12:46:14 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4075228FFD; Tue, 17 Jul 2018 12:46:14 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI, T_DKIM_INVALID autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B2F0728FF0 for ; Tue, 17 Jul 2018 12:46:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731646AbeGQNSl (ORCPT ); Tue, 17 Jul 2018 09:18:41 -0400 Received: from mail-oi0-f68.google.com ([209.85.218.68]:41424 "EHLO mail-oi0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731541AbeGQNSk (ORCPT ); Tue, 17 Jul 2018 09:18:40 -0400 Received: by mail-oi0-f68.google.com with SMTP id k12-v6so1720809oiw.8 for ; Tue, 17 Jul 2018 05:46:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=szeredi.hu; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=/YNGtiLOrNgDJhJG7ECSwj3yCPMs9gQpx8/SEdMmmIs=; b=FGJVZihvoeIhxEV9SspksfvVfU/0StuLi1KTyXQYekny5liWzzgT9ENmkbjzXw3AIJ IIni+8fOpJZbKZUeATBRt4m+LzbLcVcSvqxKa5LbUlPa6llBNAYEExxXwryqomoCrG9u G+viMW/oSkFyzGZrWoSbN5J+nzTepAT3Y9kGk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=/YNGtiLOrNgDJhJG7ECSwj3yCPMs9gQpx8/SEdMmmIs=; b=NhmX7RCoxjyBymSU8q0Qt9K8H05dsBSAXMn88LWqif7yT4pn1EeqIww/29B+U7tw4F nY+gHF2aEdnsSfGJE2rgtZIgmdXq6Da1/NXL0m+U3addiC99Gt5vZJcA1U4QpOnNoJem JmRegVAZVZSqY1Hh7owV4Cq2OilHS2pd7DBOLSn26Kj4qnwbARAzpl4p4e1jqf6hfnsd 2DZXFRy02IeAV9cJHPGWrFym2SGtla6wMG+Xw53lx7Y2fQrk479YNpfFnCaJGZq0k4sW YPXaTbtq0oZiwJCnLdsqCM6xxuV/GXvbAMynhnyrcfzZk+uimyMSSJuRLyQArDDCYOL5 v7OA== X-Gm-Message-State: AOUpUlHKnL0WBkdeSTiaU9aqk6XIEAx4lXaAO5SFpvdhokt5YZ+0kFyJ LupXfgkoRl1gxVOPtJR5k70EpGeRPzh09+XuvufWkQ== X-Google-Smtp-Source: AAOMgpeDt7rqDcgcRjgcQPKwAO/FuCo9jUh8mr7QMhWbNSyc/1bUyZQ11TLM6r75WKTyUNBratgazvZidxYrJogMZJk= X-Received: by 2002:aca:2ec9:: with SMTP id u192-v6mr1386111oiu.17.1531831569697; Tue, 17 Jul 2018 05:46:09 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a9d:113c:0:0:0:0:0 with HTTP; Tue, 17 Jul 2018 05:46:08 -0700 (PDT) X-Originating-IP: [212.96.48.140] In-Reply-To: References: <0000000000006a10de0570cf4d66@google.com> From: Miklos Szeredi Date: Tue, 17 Jul 2018 14:46:08 +0200 Message-ID: Subject: Re: WARNING: lock held when returning to user space in fuse_lock_inode To: Dmitry Vyukov Cc: syzbot , linux-fsdevel , LKML , syzkaller-bugs , astrachan@google.com Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Jul 17, 2018 at 1:36 PM, Dmitry Vyukov wrote: > On Tue, Jul 17, 2018 at 1:14 PM, Miklos Szeredi wrote: >> On Thu, Jul 12, 2018 at 5:49 PM, syzbot >> wrote: >>> Hello, >>> >>> syzbot found the following crash on: >>> >>> HEAD commit: c25c74b7476e Merge tag 'trace-v4.18-rc3-2' of git://git.ke.. >>> git tree: upstream >>> console output: https://syzkaller.appspot.com/x/log.txt?x=177bcec2400000 >>> kernel config: https://syzkaller.appspot.com/x/.config?x=25856fac4e580aa7 >>> dashboard link: https://syzkaller.appspot.com/bug?extid=3f7b29af1baa9d0a55be >>> compiler: gcc (GCC) 8.0.1 20180413 (experimental) >>> syzkaller repro:https://syzkaller.appspot.com/x/repro.syz?x=13aa7678400000 >>> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17492678400000 >>> >>> IMPORTANT: if you fix the bug, please add the following tag to the commit: >>> Reported-by: syzbot+3f7b29af1baa9d0a55be@syzkaller.appspotmail.com >>> >>> random: sshd: uninitialized urandom read (32 bytes read) >>> random: sshd: uninitialized urandom read (32 bytes read) >>> random: sshd: uninitialized urandom read (32 bytes read) >>> >>> ================================================ >>> WARNING: lock held when returning to user space! >>> 4.18.0-rc4+ #143 Not tainted >>> ------------------------------------------------ >>> syz-executor012/4539 is leaving the kernel with locks still held! >>> 1 lock held by syz-executor012/4539: >>> #0: (____ptrval____) (&fi->mutex){+.+.}, at: fuse_lock_inode+0xaf/0xe0 >>> fs/fuse/inode.c:363 >> >> False positive. >> >> fi->mutex is definitely not held by the acquiring task when returning >> to userspace. Maybe syzkaller is confused by the fact that there are >> several interdependent tasks involved with fuse: the one calling into >> fuse by doing something (looking up ./file0/file0) and the one that >> reads the fuse device (returning with the LOOKUP request for "file0"). >> The second one will return with that lock held, but it's not the one >> that acquired it, so there's no bug at all here. > > Hi Miklos, > > syzkaller is unrelated here. That's what kernel self-detects and > prints. So either way there is something to fix in kernel here: either > fuse or lockdep. > > +Alistair did some analysis offline, hope you don't mind if I repost > your description: > === > Just from reading the code, I think I can see how this happens. Fuse > is wrapping its inode mutex with a check for "parallel_dirops", which > is set up in process_init_reply(). The FUSE_PARALLEL_DIROPS appears to > always be set, in fuse_send_init(), but its initial state is to be > disabled. So if the mutex gets taken, and it'll never be unlocked if > the initial command is flushed by fuse_readdir()'s use of > fuse_lock_inode(). > === Ah, indeed. Fix attached. Thanks, Miklos Tested-by: Alistair Strachan From: Miklos Szeredi Subject: fuse: fix inital parallel dirops If parallel dirops are enabled in FUSE_INIT reply, then first operation may leave fi->mutex held. Reported-by: syzbot+3f7b29af1baa9d0a55be@syzkaller.appspotmail.com Signed-off-by: Miklos Szeredi --- fs/fuse/dir.c | 10 ++++++---- fs/fuse/fuse_i.h | 4 ++-- fs/fuse/inode.c | 14 ++++++++++---- 3 files changed, 18 insertions(+), 10 deletions(-) --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -355,11 +355,12 @@ static struct dentry *fuse_lookup(struct struct inode *inode; struct dentry *newent; bool outarg_valid = true; + bool locked; - fuse_lock_inode(dir); + locked = fuse_lock_inode(dir); err = fuse_lookup_name(dir->i_sb, get_node_id(dir), &entry->d_name, &outarg, &inode); - fuse_unlock_inode(dir); + fuse_unlock_inode(dir, locked); if (err == -ENOENT) { outarg_valid = false; err = 0; @@ -1340,6 +1341,7 @@ static int fuse_readdir(struct file *fil struct fuse_conn *fc = get_fuse_conn(inode); struct fuse_req *req; u64 attr_version = 0; + bool locked; if (is_bad_inode(inode)) return -EIO; @@ -1367,9 +1369,9 @@ static int fuse_readdir(struct file *fil fuse_read_fill(req, file, ctx->pos, PAGE_SIZE, FUSE_READDIR); } - fuse_lock_inode(inode); + locked = fuse_lock_inode(inode); fuse_request_send(fc, req); - fuse_unlock_inode(inode); + fuse_unlock_inode(inode, locked); nbytes = req->out.args[0].size; err = req->out.h.error; fuse_put_request(fc, req); --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -974,8 +974,8 @@ int fuse_do_setattr(struct dentry *dentr void fuse_set_initialized(struct fuse_conn *fc); -void fuse_unlock_inode(struct inode *inode); -void fuse_lock_inode(struct inode *inode); +void fuse_unlock_inode(struct inode *inode, bool locked); +bool fuse_lock_inode(struct inode *inode); int fuse_setxattr(struct inode *inode, const char *name, const void *value, size_t size, int flags); --- a/fs/fuse/inode.c +++ b/fs/fuse/inode.c @@ -357,15 +357,21 @@ int fuse_reverse_inval_inode(struct supe return 0; } -void fuse_lock_inode(struct inode *inode) +bool fuse_lock_inode(struct inode *inode) { - if (!get_fuse_conn(inode)->parallel_dirops) + bool locked = false; + + if (!get_fuse_conn(inode)->parallel_dirops) { mutex_lock(&get_fuse_inode(inode)->mutex); + locked = true; + } + + return locked; } -void fuse_unlock_inode(struct inode *inode) +void fuse_unlock_inode(struct inode *inode, bool locked) { - if (!get_fuse_conn(inode)->parallel_dirops) + if (locked) mutex_unlock(&get_fuse_inode(inode)->mutex); }