From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 854FAC2BA19 for ; Thu, 16 Apr 2020 02:20:46 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 2A01720737 for ; Thu, 16 Apr 2020 02:20:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="kcq+6btH" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2A01720737 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id AF5568E006D; Wed, 15 Apr 2020 22:20:45 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AA6428E0001; Wed, 15 Apr 2020 22:20:45 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9BC288E006D; Wed, 15 Apr 2020 22:20:45 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0043.hostedemail.com [216.40.44.43]) by kanga.kvack.org (Postfix) with ESMTP id 860658E0001 for ; Wed, 15 Apr 2020 22:20:45 -0400 (EDT) Received: from smtpin03.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 4772C180AD802 for ; Thu, 16 Apr 2020 02:20:45 +0000 (UTC) X-FDA: 76712114850.03.smoke23_5dc8218fd4919 X-HE-Tag: smoke23_5dc8218fd4919 X-Filterd-Recvd-Size: 8098 Received: from mail-ej1-f68.google.com (mail-ej1-f68.google.com [209.85.218.68]) by imf05.hostedemail.com (Postfix) with ESMTP for ; Thu, 16 Apr 2020 02:20:44 +0000 (UTC) Received: by mail-ej1-f68.google.com with SMTP id s3so152716eji.6 for ; Wed, 15 Apr 2020 19:20:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Mgz6bbdQfDVz34mF8jJQbTz4t2DA1H3apdU+WrR+R7Y=; b=kcq+6btHmb01cooZ1xUD/y9he2yoQ4t039E5mbUs2BkDlGwudOwJ46xfNOzh91fJrK cnmTtdcKJzViFnX/YSyC9g9bK9fy8Z8mBpig5VPviP3O0Zu3fidj81rq0sr+yioLllzb i0WBaroEoibasQOLasvyihivuvKavssL5fbCXotGXqFAhNSQebmrLYTKDEJhq1tFswer bIju+QaB37x9zzyPDW3ziDpn6Vcjc+yqGwagc5mHDmumsjsCq2LWLOiHIGWCNJK/e9XN HEhNHRDuqbY/gV1KmAvZ4hcdQ6vjgXO+BOPkJzxiUZWd34mkQULMsJZcLrOOQFUb8lj0 mqUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Mgz6bbdQfDVz34mF8jJQbTz4t2DA1H3apdU+WrR+R7Y=; b=FoF19FqClw4Qo+CCnqDfKwPKt91ff5ARQxLSdlqc8/Ko+GH4Xo3Kb1Bz3LAR9sel0W JG6W7G9Le62Z6+3B16k3eXV9KEoJ3swaT4yBsFDdiSGCZkw2vG197bOPxidIMBR30lSo Cs4pdcaFyQnQtG4Q6Nj6vVtTwb1cIjiC7B7OxZ1mqJezIYHn0o4UqyQcrYvzjVllLoqe ohQJZVSW44VtL5ZrMBuDSYzXPfe8Z+WrifMTFIwllJtr9BZJsunc334Bn5yythJUH56m cGIHDvK1lIkPn3Iy0yvgD8wjqqUPXuvMPvhkmAAVxThOR9taoGuVXM+mLi/mxyovICHn Vj9A== X-Gm-Message-State: AGi0PubTkKqi4Qs37TDI5xi4YgLhWdwOCsiUYocIvPBuGc4w/aWE6K2m HV8KivN3ADgTnIw0APZh40kW7rJdEwJNhfmsveE= X-Google-Smtp-Source: APiQypKpfYDVN5HQk19fsgn0mpfBch5zFFu+LfHUbVv1552Asb2QXRW/U8D7v+O1oz3x859kAMzRv1K1elyUAzZ6qy4= X-Received: by 2002:a17:906:374b:: with SMTP id e11mr8047637ejc.283.1587003642849; Wed, 15 Apr 2020 19:20:42 -0700 (PDT) MIME-Version: 1.0 References: <000000000000e5838c05a3152f53@google.com> In-Reply-To: From: Yang Shi Date: Wed, 15 Apr 2020 19:20:30 -0700 Message-ID: Subject: Re: possible deadlock in shmem_uncharge To: Hugh Dickins Cc: syzbot , Andrew Morton , Linux Kernel Mailing List , Linux MM , syzkaller-bugs@googlegroups.com, Linus Torvalds Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, Apr 15, 2020 at 7:04 PM Hugh Dickins wrote: > > On Mon, 13 Apr 2020, Yang Shi wrote: > > On Sun, Apr 12, 2020 at 3:11 AM syzbot > > wrote: > > > > > > Hello, > > > > > > syzbot found the following crash on: > > > > > > HEAD commit: ae46d2aa mm/gup: Let __get_user_pages_locked() return -EIN.. > > > git tree: upstream > > > console output: https://syzkaller.appspot.com/x/log.txt?x=14a30a77e00000 > > > kernel config: https://syzkaller.appspot.com/x/.config?x=ca75979eeebf06c2 > > > dashboard link: https://syzkaller.appspot.com/bug?extid=c8a8197c8852f566b9d9 > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15f5632be00000 > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=132ade57e00000 > > > > > > The bug was bisected to: > > > > > > commit 71725ed10c40696dc6bdccf8e225815dcef24dba > > > Author: Hugh Dickins > > > Date: Tue Apr 7 03:07:57 2020 +0000 > > > > > > mm: huge tmpfs: try to split_huge_page() when punching hole > > > > > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=120a752be00000 > > > final crash: https://syzkaller.appspot.com/x/report.txt?x=110a752be00000 > > > console output: https://syzkaller.appspot.com/x/log.txt?x=160a752be00000 > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > > Reported-by: syzbot+c8a8197c8852f566b9d9@syzkaller.appspotmail.com > > > Fixes: 71725ed10c40 ("mm: huge tmpfs: try to split_huge_page() when punching hole") > > No, that commit just gave syzkaller an easier way to reach old code. > > > > > > > ===================================================== > > > WARNING: SOFTIRQ-safe -> SOFTIRQ-unsafe lock order detected > > > 5.6.0-syzkaller #0 Not tainted > > > ----------------------------------------------------- > > > syz-executor428/8337 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire: > > > ffff8880a851c778 (&info->lock){....}-{2:2}, at: shmem_uncharge+0x24/0x270 mm/shmem.c:341 > > > > > > and this task is already holding: > > > ffff8880a851cac8 (&xa->xa_lock#4){..-.}-{2:2}, at: spin_lock include/linux/spinlock.h:353 [inline] > > > ffff8880a851cac8 (&xa->xa_lock#4){..-.}-{2:2}, at: split_huge_page_to_list+0xad0/0x33b0 mm/huge_memory.c:2864 > > > which would create a new lock dependency: > > > (&xa->xa_lock#4){..-.}-{2:2} -> (&info->lock){....}-{2:2} > > > > > > but this new dependency connects a SOFTIRQ-irq-safe lock: > > > (&xa->xa_lock#4){..-.}-{2:2} > > > > It looks shmem_uncharge() is just called by __split_huge_page() and > > collapse_file(). The collapse_file() has acquired xa_lock with irq > > disabled before acquiring info->lock, so it is safe. > > __split_huge_page() is called with holding xa_lock with irq enabled, > > but lru_lock is acquired with irq disabled before acquiring xa_lock. > > > > So, it is unnecessary to acquire info->lock with irq disabled in > > shmem_uncharge(). Can syzbot try the below patch? > > But I disagree with the patch below. You're right that IRQ-disabling > here is unnecessary, given its two callers; but I'm not sure that we > want it to look different from shmem_charge() and all other info->lock > takers; and, more importantly, I don't see how removing the redundant > IRQ-saving below could make it any less liable to deadlock. Yes, I realized the patch can't suppress the lockdep splat. But, actually I didn't understand how this deadlock could happen because info_lock is acquired with IRQ disabled before acquiring user_shm_lock. So, interrupt can't come in at all if I didn't miss anything. > > The crucial observation comes lower down > > > to a SOFTIRQ-irq-unsafe lock: > > > (shmlock_user_lock){+.+.}-{2:2} > and there's another syzbot report that's come out on shmlock_user_lock, > "possible deadlock in user_shm_lock". > > I believe all that's needed to fix both reports is not to use info->lock > in shmem_lock() - I see now that we saw lockdep reports of this kind > internally, a long time ago, and fixed them in that way. > > (I haven't composed the patch and references yet, and not decided if > I'll add it here or there or separately. I'll put it together now.) > > Hugh > > > > > diff --git a/mm/shmem.c b/mm/shmem.c > > index d722eb8..100117b 100644 > > --- a/mm/shmem.c > > +++ b/mm/shmem.c > > @@ -334,15 +334,14 @@ bool shmem_charge(struct inode *inode, long pages) > > void shmem_uncharge(struct inode *inode, long pages) > > { > > struct shmem_inode_info *info = SHMEM_I(inode); > > - unsigned long flags; > > > > /* nrpages adjustment done by __delete_from_page_cache() or caller */ > > > > - spin_lock_irqsave(&info->lock, flags); > > + spin_lock(&info->lock); > > info->alloced -= pages; > > inode->i_blocks -= pages * BLOCKS_PER_PAGE; > > shmem_recalc_inode(inode); > > - spin_unlock_irqrestore(&info->lock, flags); > > + spin_unlock(&info->lock); > > > > shmem_inode_unacct_blocks(inode, pages); > > }