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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 51C43C0015E for ; Mon, 24 Jul 2023 12:00:25 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E60836B0074; Mon, 24 Jul 2023 08:00:24 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id DE9556B0075; Mon, 24 Jul 2023 08:00:24 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C89A36B0078; Mon, 24 Jul 2023 08:00:24 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id B75116B0074 for ; Mon, 24 Jul 2023 08:00:24 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 76C18140701 for ; Mon, 24 Jul 2023 12:00:24 +0000 (UTC) X-FDA: 81046362768.29.826548F Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf19.hostedemail.com (Postfix) with ESMTP id 4D48E1A0024 for ; Mon, 24 Jul 2023 12:00:20 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=ME7kBWQ3; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf19.hostedemail.com: domain of cem@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=cem@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1690200021; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=BuyrWAN4lkOi/ZXx/56phQ1PZWchXKLjLEqwVGJ3Ou0=; b=umAkBgtUoTbqIm/BperJOu2pwWWjKnFwPSvDuYldUk6r1o3+JRLcj8mnL9r9QGc2Wh8/SJ Vdaxk2B1eZNz82/QX/eY+PD/ZXqxJuDrjGjAxAVSmjoCF6wHrZAyPzdTpCBWwjXKz9iW0I /ujyG6Dh0Ku6lqSPxeJanIgZo8OHrsI= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=ME7kBWQ3; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf19.hostedemail.com: domain of cem@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=cem@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1690200021; a=rsa-sha256; cv=none; b=oZ0evTafAoqISNqq6Bt2Pk3urwF1iDaui2qgopuirihbf7Cy0CtKEnkrJe1Qml/f6tPUT+ eysMlQqRI+TcSAPJh/MWU2HvPLCUnV5T1iDAVSTfK6oHGLNGAz2LzHLC7r4YzknNI2H9sh rq3dARcssUJ1XSuhVyapvdtGHw2oX2w= Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id EBEF461113; Mon, 24 Jul 2023 12:00:19 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 78BF7C433C9; Mon, 24 Jul 2023 12:00:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1690200019; bh=tepsi5dNQgg9zr7r19uxSgYo1Xl6I8FiXX1BYmkjSWE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ME7kBWQ3xDEO1dRvrJSaunAq7BVvpRMsFdPg11Yk008DEp6Qc00X0XNc3k7wY7W3X JdWWYkq9Y8HCMwxU2Hjz0RYjVnNgiw/MtGoK9umv2RaHGpMypBvfX48SgnGd1ZWti6 NSPQAjjloeTcloESxfsaMjDQV4CWs+QRTdFKEGWzzdVjqsdiOzqbqlHgYU9PlWv5jO WdtQmvkxcE0IStNWLUjEo8AjkZmjWgGbUmGQyBh4sCmUz0KwYo/qz6CRbH53Mex8u6 pVN424X/WfqyyBc3EQoEHxl2t9Pera0yOIU01Mc1nuCXYOMm9XQfVWADuysw0J2tRN Zll1qyD5BWBsg== Date: Mon, 24 Jul 2023 14:00:14 +0200 From: Carlos Maiolino To: Hugh Dickins Cc: Carlos Maiolino , syzbot , akpm@linux-foundation.org, jack@suse.cz, linux-kernel@vger.kernel.org, linux-mm@kvack.org, syzkaller-bugs@googlegroups.com Subject: Re: [syzbot] [mm?] possible deadlock in shmem_uncharge (2) Message-ID: <20230724120014.k3agnopb76ngdcab@andromeda> References: <0000000000008e62f40600bfe080@google.com> <20230718192920.qmktf3vnt67bb6rq@andromeda> <6d29786a-ebc3-591-d3eb-bd7b125a33f1@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6d29786a-ebc3-591-d3eb-bd7b125a33f1@google.com> X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 4D48E1A0024 X-Stat-Signature: k5n7b77sxoh1fa7cpor5hacjntkfigm4 X-HE-Tag: 1690200020-412216 X-HE-Meta: U2FsdGVkX1/AxRkxz/XfD4dH+NS8bUfhPBuI/Y/7A1qbVRMqBXtbT3UVZBaIORhDqiVWQy82LfcALZgfA5i2H9pxAe7o8AsQLxFXwWfUID9w4zL4uzxdA+j/9m2CnnCO8xFysUsFWLtl4YeoljSWERANi98XGIqSGUxm4Xk0VOXP6atfb42usA87J35ZXNj+NnNG2FtA+w9ejpGPT6E+AAp7IYAvB+gp36nmnSST1aIkfKlbtkYiNhw3yyKiA3mF5dmPHK45vss4meMCAZs4Y3WT41Up0Fhn4H8WxCdXaQ9dLVYHvKKzHH4Nd6wEsPhTWJKzR469YH64jEa0rJSFJxHC+T4CS6q22iGD+/V2WfhYYiVixj51WnLSmPTGjaTdmwyJUbuGPdIGByXZN7wlIa3j9leW8AKwVk6owuhOaqyjRjRD5BTcubU+lNiq86G2t+e++LsiaAzyMSegqNvi4JFl+YndTYLQl6E+tk81jEyvyXOJwCjlIZL+wSnntw+LbZlAlFuRPnYi/dMfHoNJHdScM1e31C/wyZrzWkdOxaNQO56FkQ32+gwn788eQvjLzNaeUNIjnazEm/H+0i77bebR1w/EHeHIKe+xR0H6QjZI0oqiDoXIyHecXjA6A9B0OfyPv8D5j6xwLZGV9360FMQI1KGSO4vPbpj73Z5B26EFI606+k50ikXGA5lhS40LkETxabPzznOn7f5ISerpUvPRV9URFUHU5mOpnO5hPnQwaTZSkLWXXe7ZOvWbf5cIv5q4465Ubmj6VlczMS9CjUhmuOn6ohJT7oQk/Pd+jrxVRfq9FkqF+36USknyTYFn74RQ5ywLamWc58N7FFLLx97WpMc8UWQhhmLpFoeQsc+gl5KX7Gy9GfIqPfYkA0Ju5yjHBmZpMs/X3t+knrzumqojsI/DO90NbAvaWTtcu6UAygAhr67BWRcvmIGVnpkXNByCCPhjP7pr+9NmpnK hvinEQrt RwV8TcFSmgw5VIAdOHCu7OrAMB3Mf16I1k35aaJRVLuRTJGj/dZlPkAEXWufTHyPJgimRWqTCggnNhzcrNxM1DRqFmMe8sDvxB/lYfnGl2R21j6IGm6BmKgxaemN9FV3JSLvSgJ4roPTi5rvd259BqJcQA0HP4MmiZkkMhrgAXuXSXCXPlMck3QhhUPbZWXqXqQoWXrk7whWjPu4Su99ld5NYXoqMT1ZdfF1QEFnjbh6vJh/fatdL/Rn7Ygz274nTwRrmLcpoGgd3zijft4ju+SdnHEnMcd/lhuingVL14XU09qniAMYey63JrRw57NB4q6mrVlU6DdhDBcc68BtkPmfSAjZa79N/0rw9UOtxQsOR0GclNrcAmmURquoFgJ/a04qvmnG3OHu+QVIr8J2hpjGa5jYdCP9opKYbIjdFiUAainjiWT1kpAIgs14UWubA90X5Kx/a3gCo4MawYBdp1UFBjCau1AKx+tvyPasrsy/I+qGwJPmGwMLQIRVpbSuV0TPJ/nG9aZhlD2tNqpizg72NfHrsnJlpv9OvTMDxYVJUwn/lk94hn/OGgx0ZcYuxmzDk8fy9vzYJOF+qhH3u7WM7+v5/a8abhski+Qfib4c4ZPU= 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: Hi Hugh. On Sat, Jul 22, 2023 at 09:37:35PM -0700, Hugh Dickins wrote: > On Tue, 18 Jul 2023, Carlos Maiolino wrote: > > On Tue, Jul 18, 2023 at 11:22:59AM -0700, Hugh Dickins wrote: > > > On Tue, 18 Jul 2023, syzbot wrote: > > > > > > > Hello, > > > > > > > > syzbot found the following issue on: > > > > > > Yes, this doesn't require any syzbot trickery, it showed up as soon as > > > I tried a shmem quota linux-next with lockdep and shmem huge last week. > > > > > > There's some other things wrong with the accounting there (in the non- > > > quota case anyway): I have been working up a patch to fix them, but need > > > to consider what must go in quickly, and what should wait until later. > > > > > > Carlos, in brief: don't worry about this syzbot report, I'm on it (but > > > there's a risk that any patch I send may turn out to break your quotas). > > > > Ok, thanks Hugh, I have the next version ready to go when I saw those syzkaller > > reports. > > > > I will send the new series tomorrow morning then. > > Could you please keep me in the loop, I'm interested to see what's going on > > there. > > I never saw a new series on Wednesday, just a new series on Monday which > I had ignored, knowing that another was coming. I was waiting for your > new series before sending my patch to that, but perhaps you were waiting > for my patch before sending your new series? TL;DR, I spoke with Andrew past week, and we agreed it would be better to wait for you to submit the patches before I send the new version, I thought I had cc'ed in the email thread, but seems like I didn't, my apologies. > > Then when I went to check my patch on next-20230721, found that your > series has been dropped from mm-unstable meanwhile. Maybe Andrew > dropped it because of the lockdep reports (I see now there was another), > or maybe he dropped it because of new series coming too thick and fast, > or maybe he dropped it because of the awkward merge commit which Stephen > Rothwell had to write, to reconcile the vfs and mm trees. (But I was > surprised not to have seen any mm-commits mail on the dropping.) > > So the patch below is against next-20230714, and will hopefully still > apply in your next posting: please include it there (since it modifies > THP source, I think it's best kept as a separate patch in your series). > > But when you repost, if you are still going for the next release, the > more you can do to minimize that merge commit the better. In particular, > the reindentations of shmem_get_inode() and other functions in > "shmem: make shmem_get_inode() return ERR_PTR instead of NULL" > don't help review, and could well be left until some later cleanup - > but I don't know how much that would actually eliminate the clashes. I'm not @work today, so I can't take a deep look into it by now, but, I plan to rebase the series on top of linux-next and I'll include your patch in my series, if that's what I understood from a quick read on your email. I'll try to work on it tomorrow first thing, thanks! Carlos > > (And "Add default quota limit mount options" needs to say "shmem: ..."; > I'd have preferred "tmpfs: ...", but the others say shmem, so okay.) > > [PATCH] shmem: fix quota lock nesting in huge hole handling > > i_pages lock nests inside i_lock, but shmem_charge() and shmem_uncharge() > were being called from THP splitting or collapsing while i_pages lock was > held, and now go on to call dquot_alloc_block_nodirty() which takes > i_lock to update i_blocks. > > We may well want to take i_lock out of this path later, in the non-quota > case even if it's left in the quota case (or perhaps use i_lock instead > of shmem's info->lock throughout); but don't get into that at this time. > > Move the shmem_charge() and shmem_uncharge() calls out from under i_pages > lock, accounting the full batch of holes in a single call. > > Still pass the pages argument to shmem_uncharge(), but it happens now to > be unused: shmem_recalc_inode() is designed to account for clean pages > freed behind shmem's back, so it gets the accounting right by itself; > then the later call to shmem_inode_unacct_blocks() led to imbalance > (that WARN_ON(inode->i_blocks) in shmem_evict_inode()). > > Reported-by: syzbot+38ca19393fb3344f57e6@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/lkml/0000000000008e62f40600bfe080@google.com/ > Reported-by: syzbot+440ff8cca06ee7a1d4db@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/lkml/00000000000076a7840600bfb6e8@google.com/ > Signed-off-by: Hugh Dickins > --- > mm/huge_memory.c | 6 ++++-- > mm/khugepaged.c | 13 +++++++------ > mm/shmem.c | 19 +++++++++---------- > 3 files changed, 20 insertions(+), 18 deletions(-) > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index 762be2f4244c..01f6838329a1 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -2521,7 +2521,7 @@ static void __split_huge_page(struct page *page, struct list_head *list, > struct address_space *swap_cache = NULL; > unsigned long offset = 0; > unsigned int nr = thp_nr_pages(head); > - int i; > + int i, nr_dropped = 0; > > /* complete memcg works before add pages to LRU */ > split_page_memcg(head, nr); > @@ -2546,7 +2546,7 @@ static void __split_huge_page(struct page *page, struct list_head *list, > struct folio *tail = page_folio(head + i); > > if (shmem_mapping(head->mapping)) > - shmem_uncharge(head->mapping->host, 1); > + nr_dropped++; > else if (folio_test_clear_dirty(tail)) > folio_account_cleaned(tail, > inode_to_wb(folio->mapping->host)); > @@ -2583,6 +2583,8 @@ static void __split_huge_page(struct page *page, struct list_head *list, > } > local_irq_enable(); > > + if (nr_dropped) > + shmem_uncharge(head->mapping->host, nr_dropped); > remap_page(folio, nr); > > if (PageSwapCache(head)) { > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > index 6bad69c0e4bd..366ee467fb83 100644 > --- a/mm/khugepaged.c > +++ b/mm/khugepaged.c > @@ -1828,10 +1828,6 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr, > goto xa_locked; > } > } > - if (!shmem_charge(mapping->host, 1)) { > - result = SCAN_FAIL; > - goto xa_locked; > - } > nr_none++; > continue; > } > @@ -2017,8 +2013,13 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr, > */ > try_to_unmap_flush(); > > - if (result != SCAN_SUCCEED) > + if (result == SCAN_SUCCEED && nr_none && > + !shmem_charge(mapping->host, nr_none)) > + result = SCAN_FAIL; > + if (result != SCAN_SUCCEED) { > + nr_none = 0; > goto rollback; > + } > > /* > * The old pages are locked, so they won't change anymore. > @@ -2157,8 +2158,8 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr, > if (nr_none) { > xas_lock_irq(&xas); > mapping->nrpages -= nr_none; > - shmem_uncharge(mapping->host, nr_none); > xas_unlock_irq(&xas); > + shmem_uncharge(mapping->host, nr_none); > } > > list_for_each_entry_safe(page, tmp, &pagelist, lru) { > diff --git a/mm/shmem.c b/mm/shmem.c > index 161592a8d3fe..521a6302dc37 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -424,18 +424,20 @@ static void shmem_recalc_inode(struct inode *inode) > bool shmem_charge(struct inode *inode, long pages) > { > struct shmem_inode_info *info = SHMEM_I(inode); > - unsigned long flags; > + struct address_space *mapping = inode->i_mapping; > > if (shmem_inode_acct_block(inode, pages)) > return false; > > /* nrpages adjustment first, then shmem_recalc_inode() when balanced */ > - inode->i_mapping->nrpages += pages; > + xa_lock_irq(&mapping->i_pages); > + mapping->nrpages += pages; > + xa_unlock_irq(&mapping->i_pages); > > - spin_lock_irqsave(&info->lock, flags); > + spin_lock_irq(&info->lock); > info->alloced += pages; > shmem_recalc_inode(inode); > - spin_unlock_irqrestore(&info->lock, flags); > + spin_unlock_irq(&info->lock); > > return true; > } > @@ -443,16 +445,13 @@ 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 __filemap_remove_folio() or caller */ > > - spin_lock_irqsave(&info->lock, flags); > - info->alloced -= pages; > + spin_lock_irq(&info->lock); > shmem_recalc_inode(inode); > - spin_unlock_irqrestore(&info->lock, flags); > - > - shmem_inode_unacct_blocks(inode, pages); > + /* which has called shmem_inode_unacct_blocks() if necessary */ > + spin_unlock_irq(&info->lock); > } > > /* > -- > 2.35.3 >