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 E0EB3C369B2 for ; Thu, 17 Apr 2025 12:38:52 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4C8326B029F; Thu, 17 Apr 2025 08:38:51 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 475C36B02A0; Thu, 17 Apr 2025 08:38:51 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 364A46B02A1; Thu, 17 Apr 2025 08:38:51 -0400 (EDT) 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 170186B029F for ; Thu, 17 Apr 2025 08:38:51 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id B963E1CE3AE for ; Thu, 17 Apr 2025 12:38:51 +0000 (UTC) X-FDA: 83343490062.06.CDAC038 Received: from fanzine2.igalia.com (fanzine.igalia.com [178.60.130.6]) by imf19.hostedemail.com (Postfix) with ESMTP id A0ADE1A0006 for ; Thu, 17 Apr 2025 12:38:48 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=igalia.com header.s=20170329 header.b=ND4O5eDd; dmarc=pass (policy=none) header.from=igalia.com; spf=pass (imf19.hostedemail.com: domain of gavinguo@igalia.com designates 178.60.130.6 as permitted sender) smtp.mailfrom=gavinguo@igalia.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1744893530; a=rsa-sha256; cv=none; b=qcyV7dJgl328BxCC8FsV0nyhz8q2SiGHVf9vVzn7WIjvTp3S9Oc8P00tQgaIZcCCmPsA5g 3AXiuNQoQxaLzp90uf36JJCclcvgd4tIijMVx1Y8EY0zFXNVFhHgNdaXDKGzUV0NiD08+0 4dOUEOsIlGEV/Ekcmhsc0rq4SxsGtp0= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=igalia.com header.s=20170329 header.b=ND4O5eDd; dmarc=pass (policy=none) header.from=igalia.com; spf=pass (imf19.hostedemail.com: domain of gavinguo@igalia.com designates 178.60.130.6 as permitted sender) smtp.mailfrom=gavinguo@igalia.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1744893530; 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:dkim-signature; bh=DhN1xrm3btmQ9MDmtBh97AmEqa32qb4yE0YewSaFB6g=; b=cYtKblEotbUQKbkERxKYAMlhUnX+pghMhBVE6aD7WAhRfdao9kg4RQCABlxPjAVIdrZ7sL Uv041SFOpxPGVI7EUIVq9OwHVaH6gfXWh78NhLQjjdcxv++rQ2LM7Oi2YLk8DsqOQqkQDs WbOU4jHLOqv191HF/tnnLqC2iVsNTcI= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com; s=20170329; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:From: References:Cc:To:Subject:MIME-Version:Date:Message-ID:Sender:Reply-To: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=DhN1xrm3btmQ9MDmtBh97AmEqa32qb4yE0YewSaFB6g=; b=ND4O5eDdmzLluNp/684ZQtG+Lx k7pYohrLoorYBxM5hH15IqgHs0F6o4kkw47ciUNyXg0w+3t8mYgHA/Mw3kTqFUeQy2T0+D3McYIkn kmO1sXggweT8hvyisJPWttTp2zqnOfzSK/TbUPDVR8w9eltd7FF5WPZiF+RWs++/5Pt3GmZoB7rys 42DuIV3zAJvplq8mpjGEze1Y9VbgXRO05FCGJyXRqLaY/nAUf15eS0EzzDFohUj6w8+FPGwe9XFBu mHM/8dyhYpqnOhYbv9zS3u4iLg3H2fqkbGb6ApvDJGim+JLUwqPj2GQAOIW4RtQTVDMiPkXAI3Mxi DjurjBMA==; Received: from 39-14-49-133.adsl.fetnet.net ([39.14.49.133] helo=[192.168.220.43]) by fanzine2.igalia.com with esmtpsa (Cipher TLS1.3:ECDHE_X25519__RSA_PSS_RSAE_SHA256__AES_128_GCM:128) (Exim) id 1u5OVr-000nvR-4H; Thu, 17 Apr 2025 14:38:39 +0200 Message-ID: <492b58a8-ff4a-4afe-b317-6fd1bafc874e@igalia.com> Date: Thu, 17 Apr 2025 20:38:30 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] mm/huge_memory: fix dereferencing invalid pmd migration entry To: Zi Yan Cc: David Hildenbrand , Hugh Dickins , linux-mm@kvack.org, akpm@linux-foundation.org, willy@infradead.org, linmiaohe@huawei.com, revest@google.com, kernel-dev@igalia.com, linux-kernel@vger.kernel.org References: <20250414072737.1698513-1-gavinguo@igalia.com> <27d13454-280f-4966-b694-d7e58d991547@redhat.com> <6787d0ea-a1b9-08cf-1f48-e361058eec20@google.com> <83f17b85-c9fa-43a0-bec1-22c8565b67ad@redhat.com> <98d1d195-7821-4627-b518-83103ade56c0@redhat.com> <7d0ef7b5-043b-beca-72a9-6ae98b0d55fb@google.com> <05a7d51e-f065-445a-af0e-481f3461a76e@redhat.com> <412E70A4-4775-4AF7-A878-7FEBF9A122D8@nvidia.com> <667354f3-7076-4e64-9506-56e81e7d9234@igalia.com> Content-Language: en-US From: Gavin Guo In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Queue-Id: A0ADE1A0006 X-Rspamd-Server: rspam04 X-Stat-Signature: esamk18p8w1np8mf1i4rmqmt6my7sayo X-HE-Tag: 1744893528-65397 X-HE-Meta: U2FsdGVkX1/oSXA5oS+jI1lp3/u6uSfEx7KqPS5JZGGAHkAo2h3oreo3p7pmWvETE1L3ao+z3EPczzuqYi3SuLBKhc4yKoaDjLT7JoN9ohOMlcoMiWwfbHKmn4pscmGfKpvyu6egwtjwXxKf1IlmKgey7aoBjz8gupGSSk+QUSbN5iXgHuaDi+GtUU4ovUihbu8arSfVnV97/31BU9grXWctdaDQ35bX2pdfcnwGLngKcxBWf5ixZrrayJSJqAtragNVIrnES6iQ5zNPsAPtx0jpfijnY+fpwu34csP/HHSD9RZnpkiNSBbH7gw9Ov32rf7hV+5ykwInPl7gnFq3orc6wKkX064sW+94P/bhypkp1m+xhXtEpuOC8/A2OZA+MGrErGsycSc/RU1cRlYiw77D1bXJT8xA+krMZR2HlPrq7R9MhPnkP4uYJXWYf5IdyEbqavmMk3UdlMUSn6EXXrBVsj2X1YEAvo3WMbcQ0nJhOMZZG4Q0v8P2hPQSYmNtYV2TfJFiYLixzdj95Sr5812ji5MOvEJpONIySF8w0wWB6O1D01WKjirCqH5wX9Ofsw7oKj63JiljWk9YB4EEajjMZYHcED36GEVOX4WT5XUOGz1QiZkBwIGM6jlRRDRY3ML29swrT9kO8vUeWIHKSBAp2jlb1qlju0V5lGjvg/MPVIXSkq3X/hCbdezPEtZX4TRDJbKAKqfSFG2NSXnhqmuKq1AuSZh3v1lM6bgjVP4YxnGNDi9RBdWs+l9BfLwq86NwJe0AVGb1CGLayBoc+xmbtz5ifhcmBZV4aUIXbmQ0wWC3CM1T6ny7TBdsF6h+bITqVs9cEAMwJLvgOAn4RTk6K0VzRfKqz9kZaCKYRE+hXhzj8ViLzjmskX/U96m3tRveAe6LNk0vQ00pay92QZgxXVw6RcAvDyIiLetWe1i2RB2Q7tPi+wp4LAtI3q6T2old5rMJmYAkjH4/dvv 3zakYVCr fnyeKmaQC7fYklMNxVFUy66xDDbh2uenFSeVOE+oG9F0WOiBA5Dzi1ZFXsWl7WzVGCGCaTdGHLdP7OPLbZ+ActoZmsPmzMJIXOVcpGBFTGIbv7mVFWvikV5qgExzeGj096astsn8JYVkkxMUj2jMnGKv6X7f26kcMgRvuDjkww++IsdhijAkDgURLBOIyiYihE5tBbWJ+Q95egJApwIToYya3sWccallQht/5ZKhIe6+ISIFaxmPg/BKPy8HB6lrUbX/WE/UVKSwK32cpCdxOGZK3UMKqf+ixbYYFz91sDX5g1l1FnZpIlAHYA4xCkgBTsIMBAUfBzfWT9O052K+2xLU1fSL9tyLjoYI2ozJwU55uaHLsXMDnvk9jjw01YK2ya+WWaV5PFM9U68WB67XbAkHF7A== 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 4/17/25 20:10, Zi Yan wrote: > On 17 Apr 2025, at 8:02, Gavin Guo wrote: > >> On 4/17/25 19:32, Zi Yan wrote: >>> On 17 Apr 2025, at 7:21, Gavin Guo wrote: >>> >>>> On 4/17/25 17:04, David Hildenbrand wrote: >>>>> On 17.04.25 10:55, Hugh Dickins wrote: >>>>>> On Thu, 17 Apr 2025, David Hildenbrand wrote: >>>>>>> On 17.04.25 09:18, David Hildenbrand wrote: >>>>>>>> On 17.04.25 07:36, Hugh Dickins wrote: >>>>>>>>> On Wed, 16 Apr 2025, David Hildenbrand wrote: >>>>>>>>>> >>>>>>>>>> Why not something like >>>>>>>>>> >>>>>>>>>> struct folio *entry_folio; >>>>>>>>>> >>>>>>>>>> if (folio) { >>>>>>>>>>   if (is_pmd_migration_entry(*pmd)) >>>>>>>>>>       entry_folio = pfn_swap_entry_folio(pmd_to_swp_entry(*pmd))); >>>>>>>>>>   else >>>>>>>>>>    entry_folio = pmd_folio(*pmd)); >>>>>>>>>> >>>>>>>>>>   if (folio != entry_folio) >>>>>>>>>>         return; >>>>>>>>>> } >>>>>>>>> >>>>>>>>> My own preference is to not add unnecessary code: >>>>>>>>> if folio and pmd_migration entry, we're not interested in entry_folio. >>>>>>>>> But yes it could be written in lots of other ways. >>>>>>>> >>>>>>>> While I don't disagree about "not adding unnecessary code" in general, >>>>>>>> in this particular case just looking the folio up properly might be the >>>>>>>> better alternative to reasoning about locking rules with conditional >>>>>>>> input parameters :) >>>>>>>> >>>>>>> >>>>>>> FWIW, I was wondering if we can rework that code, letting the caller to the >>>>>>> checking and getting rid of the folio parameter. Something like this >>>>>>> (incomplete, just to >>>>>>> discuss if we could move the TTU_SPLIT_HUGE_PMD handling). >>>>>> >>>>>> Yes, I too dislike the folio parameter used for a single case, and agree >>>>>> it's better for the caller who chose pmd to check that *pmd fits the folio. >>>>>> >>>>>> I haven't checked your code below, but it looks like a much better way >>>>>> to proceed, using the page_vma_mapped_walk() to get pmd lock and check; >>>>>> and cutting out two or more layers of split_huge_pmd obscurity. >>>>>> >>>>>> Way to go.  However... what we want right now is a fix that can easily >>>>>> go to stable: the rearrangements here in 6.15-rc mean, I think, that >>>>>> whatever goes into the current tree will have to be placed differently >>>>>> for stable, no seamless backports; but Gavin's patch (reworked if you >>>>>> insist) can be adapted to stable (differently for different releases) >>>>>> more more easily than the future direction you're proposing here. >>>>> >>>>> I'm fine with going with the current patch and looking into cleaning it up properly (if possible). >>>>> >>>>> So for this patch >>>>> >>>>> Acked-by: David Hildenbrand >>>>> >>>>> @Gavin, can you look into cleaning that up? >>>> >>>> Thank you for your review. Before I begin the cleanup, could you please >>>> confirm the following action items: >>>> >>>> Zi Yan's suggestions for the patch are: >>>> 1. Replace the page fault with an invalid address access in the commit >>>> description. >>>> >>>> 2. Simplify the nested if-statements into a single if-statement to >>>> reduce indentation. >>> >>> 3. Can you please add Huge’s explanation below in the commit log? >>> That clarifies the issue. Thank you for the fix. >> >> Sure, will send out another patch for your review. Thank you for the review. >> > Thanks. Do you mind sharing the syzkaller reproducer if that is > possible and easy? I am trying to understand more about the issue. Sure, this is the reproducer: https://drive.google.com/file/d/1eDBV6VfIzyqD9SeYGQBah-BJXO32Piy8/view Reproducing steps 1). gcc -o repro -lpthread -static ./repro.c 2). ./repro 3). Find the group number and replace 2539 in the following sudo cat /sys/kernel/debug/shrinker/thp-deferred_split-12/count 4). Run the following command in multiple sessions for i in $(seq 10000); do echo "2539 0 100" | sudo tee /sys/kernel/debug/shrinker/thp-deferred_split-12/scan ; done Generally, the bug will be triggered within 5 minutes. > >>> >>> “ >>> an anon_vma lookup points to a >>> location which may contain the folio of interest, but might instead >>> contain another folio: and weeding out those other folios is precisely >>> what the "folio != pmd_folio((*pmd)" check (and the "risk of replacing >>> the wrong folio" comment a few lines above it) is for. >>> ” >>> >>> With that, Acked-by: Zi Yan >>> >>>> >>>> David, based on your comment, I understand that you are recommending the >>>> entry_folio implementation. Also, from your discussion with Hugh, it >>>> appears you agreed with my original approach of returning early when >>>> encountering a PMD migration entry, thereby avoiding unnecessary checks. >>>> Is that correct? If so, I will keep the current logic. Do you have any >>>> additional cleanup suggestions? >>>> >>>> I will start the cleanup work after confirmation. >>>> >>>>> >>>>>> >>>>>> (Hmm, that may be another reason for preferring the reasoning by >>>>>> folio lock: forgive me if I'm misremembering, but didn't those >>>>>> page migration swapops get renamed, some time around 5.11?) >>>>> >>>>> I remember that we did something to PTE handling stuff in the context of PTE markers. But things keep changing all of the time .. :) >>>>> >>> >>> >>> Best Regards, >>> Yan, Zi > > > Best Regards, > Yan, Zi