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 1E61EC4725D for ; Mon, 22 Jan 2024 07:17:27 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 98C7A8D0007; Mon, 22 Jan 2024 02:17:26 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 93C638D0006; Mon, 22 Jan 2024 02:17:26 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 804D28D0007; Mon, 22 Jan 2024 02:17:26 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 6FFEC8D0006 for ; Mon, 22 Jan 2024 02:17:26 -0500 (EST) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id B7F5014069E for ; Mon, 22 Jan 2024 07:17:25 +0000 (UTC) X-FDA: 81706091250.13.F12EDB3 Received: from out30-132.freemail.mail.aliyun.com (out30-132.freemail.mail.aliyun.com [115.124.30.132]) by imf10.hostedemail.com (Postfix) with ESMTP id 9F84BC0011 for ; Mon, 22 Jan 2024 07:17:22 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=alibaba.com; spf=pass (imf10.hostedemail.com: domain of baolin.wang@linux.alibaba.com designates 115.124.30.132 as permitted sender) smtp.mailfrom=baolin.wang@linux.alibaba.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1705907844; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=oM+ov8O/VT2jMkgaNlYbbbNhgQjX/qMu7LArYrKqN0Q=; b=nOQiXjODil1mQcSGjSVoR5kGqqpiFHEO5TyrE+0rWOOOaIykITR28jEQVwNYDM20kKe+gW M0Hz4KQWVu0d3NJ9KJhpiSWMYOLemQLsUxt9D//ry7a+EusSKA4uRJtZDu2KOuUvJAZCsE Pw2zzJkeRedVUAFCJnjkcDe+Z70Si9k= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=alibaba.com; spf=pass (imf10.hostedemail.com: domain of baolin.wang@linux.alibaba.com designates 115.124.30.132 as permitted sender) smtp.mailfrom=baolin.wang@linux.alibaba.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1705907844; a=rsa-sha256; cv=none; b=t23we3VssGQXuuFNnEfSi4uNtRbuxJgO9L3RLHmPM+AyXGv5AHyZKHrbVgFKyuUDG3KFTq Y/lrXImHLaEja7ptTdTyQn6QYFPSuHe/9JqGDfKiwcGJJnfT6RmjHEPc3gs3wvp0BdQCnu Xg2e/iLt+FZleC5gWPmzk71S5kHobOw= X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R441e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=ay29a033018045192;MF=baolin.wang@linux.alibaba.com;NM=1;PH=DS;RN=9;SR=0;TI=SMTPD_---0W.2pzwi_1705907838; Received: from 30.97.48.66(mailfrom:baolin.wang@linux.alibaba.com fp:SMTPD_---0W.2pzwi_1705907838) by smtp.aliyun-inc.com; Mon, 22 Jan 2024 15:17:18 +0800 Message-ID: <8f52414c-e0f2-4931-9b32-5c22f1d581f0@linux.alibaba.com> Date: Mon, 22 Jan 2024 15:17:50 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] fs: improve dump_mapping() robustness To: Charan Teja Kalla , Al Viro Cc: akpm@linux-foundation.org, willy@infradead.org, brauner@kernel.org, jack@suse.cz, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org References: <937ab1f87328516821d39be672b6bc18861d9d3e.1705391420.git.baolin.wang@linux.alibaba.com> <20240118013857.GO1674809@ZenIV> From: Baolin Wang In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 9F84BC0011 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: tq54r51hp3xczyfqbprrseee9a3o3zj9 X-HE-Tag: 1705907842-73002 X-HE-Meta: U2FsdGVkX18ODaEMEbbif26pTZsqYkJvu++tFjuyZJqTHlnitKut2mBscLacFSPmsOVoWpf9bxCbN/HGqGibs+VlR1KyS+XI1UV6ZTBE7foJbPjKbom4ht0HtlPfK+ndYvb1fRI31zno9xQ05ZcQ2Xpc/yDVn31mmYDWvdFEt0r4O6pArqw2OhmfSl+sevOkJ9GB+2qBDJWKc62YGo7QC2imKTRMuBD+aYHbq0cE3nNxORH7h+5TXrl1PdyxsjhLaL94se3Q4NES4f5Jp0luIuKCIMp4cJ81YA97NNa0XP2wRPd/ZHaAHj3rnMSkQQTP1zB74zebkVmDMXNzOuCOCTQZOU+F7nLCF2D0EQyObqzqkJPOaSpS8YOt9zmP5/jvigMk2tM1PQ/WRWV+D+GY66pCXQM3qFG/iHdCDehrQpn6J8aPCWIKLsGAZQIxEg5yMPKA8mETCDCG2w4f6xdBZs6S2baEDGIUKfG50dcKsNpyWVJpcdm+eBJwyomSysTxtaqvvqJ0oMK7jAvtawyBkjPGuAL1Jl4L4QtbS6Dql/hlAAIgfAsjxYjk28KeAjtn8q/9VPR8NwZar4s6ZCoqLig+3rMR5TFdrFp5HDGY5o+yjjWVdXvcNZ4TQxzNTZerdugIjAlpMmKU0ORfbL+oOKVHSOSCaUXc0ifzXj88tc+RYi8+puN94U01VyQAQwS5fzY/N8ncksokrgz7rN9Z3fKSHOPLuxu/zbVg5SjCRSuXok9wHRoxVD3fbEwjQ9eoEr59krylTKGmzE9x1lS9GeXBOoC+8Gvc9g052qr4kDOjQNHtU2T4edNTKDwtMkXkO4OzMHXiP0ib7yEluMp553Ve4CcJ1wA33rTX7uAxpEHZnRqcjoENxzWF3tNbAX2lYUg9cORsxlv8YgfTW35x42c3CS01BMap4uzp+XalYLywIE6Rs/vCm2hoen9d5hqJGKd2olqTCyF4lJ3qcJh nqaSsYTp 9VR6Mw6AKoCJH6fWFIwq5A2DyCmhbrnaawoeO+dqxTJce3zcMoC9gcFukHOwO3/bUU7QQngUqJaubYYQG3Wemm0CULkSMlsvIDEFUY+7rOLXEyNYrTJ6m3GHF571TrfS1zTkDcz5uxPDiuuqWD82IAP1vQqr/ZThQ8UaM 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: List-Subscribe: List-Unsubscribe: On 1/19/2024 11:48 PM, Charan Teja Kalla wrote: > Hi Matthew/Baolin, > > On 1/18/2024 8:13 AM, Baolin Wang wrote: >> >> >> On 1/18/2024 9:38 AM, Al Viro wrote: >>> On Tue, Jan 16, 2024 at 03:53:35PM +0800, Baolin Wang wrote: >>> >>>> With checking the 'dentry.parent' and 'dentry.d_name.name' used by >>>> dentry_name(), I can see dump_mapping() will output the invalid dentry >>>> instead of crashing the system when this issue is reproduced again. >>> >>>>       dentry_ptr = container_of(dentry_first, struct dentry, >>>> d_u.d_alias); >>>> -    if (get_kernel_nofault(dentry, dentry_ptr)) { >>>> +    if (get_kernel_nofault(dentry, dentry_ptr) || >>>> +        !dentry.d_parent || !dentry.d_name.name) { >>>>           pr_warn("aops:%ps ino:%lx invalid dentry:%px\n", >>>>                   a_ops, ino, dentry_ptr); >>>>           return; >>> >>> That's nowhere near enough.  Your ->d_name.name can bloody well be >>> pointing >>> to an external name that gets freed right under you.  Legitimately so. >>> >>> Think what happens if dentry has a long name (longer than would fit into >>> the embedded array) and gets renamed name just after you copy it into >>> a local variable.  Old name will get freed.  Yes, freeing is RCU-delayed, >>> but I don't see anything that would prevent your thread losing CPU >>> and not getting it back until after the sucker's been freed. >> >> Yes, that's possible. And this appears to be a use-after-free issue in >> the existing code, which is different from the issue that my patch >> addressed. >> >> So how about adding a rcu_read_lock() before copying the dentry to a >> local variable in case the old name is freed? >> > > We too seen the below crash while printing the dentry name. > > aops:shmem_aops ino:5e029 dentry name:"dev/zero" > flags: > 0x8000000000080006(referenced|uptodate|swapbacked|zone=2|kasantag=0x0) > raw: 8000000000080006 ffffffc033b1bb60 ffffffc033b1bb60 ffffff8862537600 > raw: 0000000000000001 0000000000000000 00000003ffffffff ffffff807fe64000 > page dumped because: migration failure > migrating pfn aef223 failed ret:1 > page:000000009e72a120 refcount:3 mapcount:0 mapping:000000003325dda1 > index:0x1 pfn:0xaef223 > memcg:ffffff807fe64000 > Unable to handle kernel NULL pointer dereference at virtual address > 0000000000000000 > Mem abort info: > ESR = 0x0000000096000005 > EC = 0x25: DABT (current EL), IL = 32 bits > SET = 0, FnV = 0 > EA = 0, S1PTW = 0 > FSC = 0x05: level 1 translation fault > Data abort info: > ISV = 0, ISS = 0x00000005 > CM = 0, WnR = 0 > user pgtable: 4k pages, 39-bit VAs, pgdp=000000090c12d000 > [0000000000000000] pgd=0000000000000000, p4d=0000000000000000, > pud=0000000000000000 > Internal error: Oops: 0000000096000005 [#1] PREEMPT SMP > > dentry_name+0x1f8/0x3a8 > pointer+0x3b0/0x6b8 > vsnprintf+0x4a4/0x65c > vprintk_store+0x168/0x4a8 > vprintk_emit+0x98/0x218 > vprintk_default+0x44/0x70 > vprintk+0xf0/0x138 > _printk+0x54/0x80 > dump_mapping+0x17c/0x188 > dump_page+0x1d0/0x2e8 > offline_pages+0x67c/0x898 > > > > Not much comfortable with block layer internals, TMK, the below is what > happening in the my case: > memoffline dput() > (offline_pages) (as part of closing of the shmem file) > ------------ -------------------------------------- > ....... > 1) dentry_unlink_inode() > hlist_del_init(&dentry->d_u.d_alias); > > 2) iput(): > a) inode->i_state |= I_FREEING > ..... > b) evict_inode()->..->shmem_undo_range > 1) get the folios with elevated refcount > 3) do_migrate_range(): > a) Because of the elevated > refcount in 2.b.1, the > migration of this page will > be failed. > > 2) truncate_inode_folio() -> > filemap_remove_folio(): > (deletes from the page cache, > set page->mapping=NULL, > decrement the refcount on folio) > b) Call dump_page(): > 1) mapping = page_mapping(page); > 2) dump_mapping(mapping) > a) We unlinked the dentry in 1) > thus dentry_ptr from host->i_dentry.first > is not a proper one. > > b) dentry name print with %pd is resulting into > the mentioned crash. > > > At least in this case, I think __this patchset in its current form can > help us__. This looks another case of NULL pointer access. Thanks for the detailed analysis. Could you provide a Tested-by or Reviewed-by tag if it can solve your problem?