From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f42.google.com (mail-pj1-f42.google.com [209.85.216.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7C5FB3264C8 for ; Mon, 10 Aug 2026 15:50:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.216.42 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786377024; cv=none; b=E9mRrMhX0Yo7UFMzpkFkH2Gohgp1WbbNIVATnfpT/jGRreZPFuTqSWBrL7dV7OnVcek+Z9SMEssz6f2MuUheLKUY65Xq0xZPAd2rUcQ/8xwKJpDdCDIKsL32no4zRKIf6fiu87itaz/s2FkHi86An3sX1RVtdO9VPCS2roookfw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786377024; c=relaxed/simple; bh=sk0U75hdx8tHc1dGHKWD7F3pj3j62UrLT0zigEd+pKI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=bVSjnx3g2pKuqs8UGj88Ko/LkWsKW8dX5DJ/HinUY91sHgagOUbcv1ECB+87sam69DZuQn/wG4DOwYDoSFfQnEW+PIc6I1sUxemw0GWRlV8F0M1MThRpxAgmZKv/VgPiF7MkRi3YYgtSJEfy9kDf+K2JYpD1+uHonKHAaPQdYIM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=G6Ojw1eG; arc=none smtp.client-ip=209.85.216.42 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="G6Ojw1eG" Received: by mail-pj1-f42.google.com with SMTP id 98e67ed59e1d1-38511175ad3so1928268a91.2 for ; Mon, 10 Aug 2026 08:50:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1786377022; x=1786981822; darn=vger.kernel.org; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:from:to:cc:subject :date:message-id:reply-to:content-type; bh=D5RAXal2rZRUWpw76MNjDYAuWkbIPwuFzOtfzJ/aSVc=; b=G6Ojw1eGcQ2iF57p54M5N74Msa/ou4/laly++SUPD9DvaTwnvEgtnXFUgOxSLQ0LAw z9NRcr/wJZ6cMU07AiTbc9QGqJFrWuaL17M1fDfunWVOcbgk5ooTSdqmHftaNkcrZVJ0 Y+x4AxXTCMzJQDL0BYfpXzVW2qEm+RsSthCEw5B7w4Klqq3RxN4hf3iQuYJQqrsR0rXB EeMcwMK84dem2ASeNEvLYRuDiDmbg/1hG1ZFvmluJLAKmjLS9Y2FWB46kud2XMk0f3w1 BwBiTeuE5urpp0xEIGiOHdun5YX5gJ7PVy8QxtZ+MHTF98ETIItxvdmSlKxaPpkWXnBJ YCpw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1786377022; x=1786981822; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=D5RAXal2rZRUWpw76MNjDYAuWkbIPwuFzOtfzJ/aSVc=; b=TJD0DLFrs95dWJykYCY4p8556F7op5WT9YQCZ8Pbd2yeRyNd7yHY/6vmBockRMA+B1 Pr97MT9N/VUODrQrfl+d7AbIb/TljvDBom1aCk9D6o+HS8kUE9YKXfCk9ulBmVR4Rd/2 UBc083KU3eEJDvk/O0/69DVNeWsyROJuUyoy75lnLWT3evnBg8Rh5nVZbiAo85Hx8MYE zWzi45YQS1wjRTAR45AdS/3oT+n0X/qSqQO4wT0qkm+hZ1MMQlcyi5veqG7O1yTnrkTS KsbX2AYS8FsAwONRaJ9WQvlUT63djQk9Vhahc/4dWu1swX6hgWBcKUkxxFXs3bEJJ09G xVOw== X-Forwarded-Encrypted: i=1; AHgh+RqwCA7t8BEh0lxRc2NPYZTII2lh/ZSB2DpZauzG4yK6xuXaoovwXmn1veALjlYXzDiBLEzLZmTCVib7ewo=@vger.kernel.org X-Gm-Message-State: AOJu0YyKP9VmjxT/9gIhV9FsILnmXa21rogUwwirxar8pCr2N+/43bi0 +Pp0Ta2v1y7l+IRY4t1onlo82xPrjt93zRcWyOlRhqtw7XeKiJR2+LP5 X-Gm-Gg: AR+sD10t6gAISgZT0mvBd5RQiqmmO0PxCNT1Rnxuvtv+bqjiM5K50R01utGLtahcFjM 3vmilWjKn15UNMwcfbEKxhaajiRofIyNogFU8oyW8stngG80s5wWo152HzG9d3dYnVCyiI58any IEb8bSOBAoWnPo/cjgqz4np27nqMhA7/sGbvr6SPfBueULxc93CKf3WqZbruPhFIHTbv122Hfqo HuBMov2aFLTgOZoGw7n7stnm+nBsVgzGLC/m2l2WL7QlWTnTGzLy4KSlnzeLiDkrAFECI+LhQBP Vk+yvKr86EYcNcQzDZ6VqlYImgYXYq5iVLKEDgysGWm+A/pum7BYFWkx8YXKwZ23usYUv6elew/ /OBAF4jg5gsH+iN+rP0bJ8hnc4PpEM/qhmoWj8oaPSSBFe3O5PkQJkPfRjVoMS/aJtnJX5oJB3K K7SikegggrWyytwY7JQQhREeg7lLjyCQRhUcNLB++HgK6jRbLAnfXpW1FxqTva0pABTeX95SGuR hYxHw+qyQ== X-Received: by 2002:a17:90b:4a51:b0:37f:e1af:df22 with SMTP id 98e67ed59e1d1-392cca91c03mr2243236a91.17.1786377021604; Mon, 10 Aug 2026 08:50:21 -0700 (PDT) Received: from v4bel ([58.123.110.97]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-390b30d465csm7004937a91.2.2026.08.10.08.50.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 10 Aug 2026 08:50:21 -0700 (PDT) Date: Tue, 11 Aug 2026 00:50:17 +0900 From: Hyunwoo Kim To: "Lorenzo Stoakes (ARM)" Cc: akpm@linux-foundation.org, david@kernel.org, liam@infradead.org, vbabka@kernel.org, rppt@kernel.org, surenb@google.com, mhocko@suse.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org, imv4bel@gmail.com Subject: Re: [PATCH] mm/pagewalk: fix stale walk->action escaping walk_pmd_range() Message-ID: References: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Mon, Aug 10, 2026 at 12:19:54PM +0100, Lorenzo Stoakes (ARM) wrote: > Since it's 2026 + this is your first patch in mm AFAICT, Yeah, my home town is netdev :) > and it's also a > very fiddly and specific issue, I do have to ask - was there was any AI > involved in making this? > > If so you should add an Assisted-by: tag as per > https://docs.kernel.org/process/coding-assistants.html please :) > > On Mon, Aug 10, 2026 at 06:45:17PM +0900, Hyunwoo Kim wrote: > > walk_pmd_range() resets walk->action in only one place in its loop body, > > and that place is after the pmd_none() branch. For a walker with no > > Newline after full stop please, this is too many words in one big block. > > What is a 'reset'? This doesn't really mean anything. You mean sets > walk->action = ACTION_SUBTREE, which is the default action. > > Your commit message doesn't mention ACTION_SUBTREE anywhere, it should. > > Also what you're saying here is just untrue if you consider reset to be > assignment to walk->action (which is a reasonable interpretation) - > walk_pmd_range() changes walk->action in _two_ places. > > Just be clear that you by reset you mean assigning walk->action = > ACTION_SUBTREE. > > > ->install_pte, that branch continues to the next entry without passing the > > reset. So if ->pmd_entry() sets ACTION_AGAIN and returns 0, and the PMD has > > Not passing what reset? This is so unclear. > > > become none by the time the loop restarts at the again label, the reset is > > skipped. If the remaining entries are all none too, the loop returns 0 with > > ACTION_AGAIN still set. The ACTION_AGAIN that walk_pte_range() sets when > > pte_offset_map_lock() fails escapes the same way. > > OK so what you mean to say is: > > if (ops->pmd_entry) > err = ops->pmd_entry(pmd, addr, next, walk); <- 1. sets walk->action = > ACTION_AGAIN > if (err) > break; > > if (walk->action == ACTION_AGAIN) > goto again; > > Then above that code: > > again: > next = pmd_addr_end(addr, end); > if (pmd_none(*pmd)) { <- 2. This triggers because PMD became empty > if (has_install) > err = __pte_alloc(walk->mm, pmd); > else if (ops->pte_hole) > err = ops->pte_hole(addr, next, depth, walk); > if (err) > break; > if (!has_install) > continue; <- 3. Loop around to the next entry, with > walk->action erroneously set to > ACTION_AGAIN still. > } > > walk->action = ACTION_SUBTREE; <- 4. This would reset it EXCEPT if > you are at the end of the range. > > Then in walk_pud_range(): > > err = walk_pmd_range(pud, addr, next, walk); > if (err) > break; > > if (walk->action == ACTION_AGAIN) <- 5. Uh oh doing a retry for no > reason. > goto again; > > So it's only if you're at the end of the PMD range that it's a problem > right? Yes, or more precisely when nothing after it sets walk->action = ACTION_SUBTREE. > You should say that. > > The user-visible problem here is that you can end up calling the callbacks > too many times which explicitly breaks mincore. > > Given I had to decode this it does make me wonder if you fully understand > this or if it's just unclear language. This sort of word salad is very > LLM-ish :) The commit message was written with help from the lovely Claude. Not the most readable, admittedly :) > > Anyway overall maybe rewrite the commit message to something like: > > If ->pmd_entry() sets walk->action = ACTION_AGAIN, the pmd_none() > check is retried. The PMD entry may be cleared at the point of retry. > > In this case, if walk->ops->install_pte is not specified, the code > continues to the next PMD entry in the range without resetting > walk->action to ACTION_SUBTREE. > > This leaves walk->action erroneously set to ACTION_AGAIN, which is > incorrect. > > This was incorrect but not problematic up until commit 3b89863c3fa4 > ("mm/pagewalk: fix race between concurrent split and refault") > which updated walk_pud_range() to check for walk->action == > ACTION_AGAIN upon walk_pmd_range()'s return, causing the PUD walk > to be retried. > > In this case this results in duplicate walk callbacks being > invoked, which is erroneous and will break any caller that is not > idempotent with respect to this (and waste time for those which > are). > > A specific example of this breaking things is mincore which walks > an internal cursor data structure a byte at a time on assumption > that page table entry callbacks are called only once for each > entry. > > Fix the problem by resetting walk->action to ACTION_SUBTREE prior > to the none check. > > The pattern also exists in walk_pud_entry() so fix it there too. > > > > > > Nothing looked at that value after walk_pmd_range() returned until commit > > 3b89863c3fa4 ("mm/pagewalk: fix race between concurrent split and refault") > > turned that into a problem. It added both the PUD check that sets > > ACTION_AGAIN before the loop is entered and the test in walk_pud_range() > > that picks the value up right after walk_pmd_range() returns and walks > > [addr, pud_addr_end(addr, end)) again. That is fine for the PUD check, > > since none of walk_pmd_range()'s own callbacks have run at that point, but > > a value that escaped as described above arrives after those callbacks have > > already covered the range. > > > > For mincore(2) this becomes an out-of-bounds write. ->pmd_entry() and > > ->pte_hole() advance the walk->private cursor by one byte per page, the > > buffer is a single page from __get_free_page(), and mincore(2) asks for at > > most PAGE_SIZE entries at a time, so there is no room to spare. Walking > > the range a second time pushes the cursor past the end of the buffer, and > > it does so again every time the race is hit. Reproducing this needs no > > privileges: run mincore(2) over a 16 MiB anonymous mapping marked > > MADV_NOHUGEPAGE while another thread repeatedly faults in a PMD-aligned > > 2 MiB range inside it and then drops it with madvise(MADV_DONTNEED). > > Obviouisly see above on commit message. > > Is it possible to add a self test that does something like this or is it too > racey to be practical? It looks quite race dependent. Triggering it deterministically would need some of the race window widening tricks from exploit work, which I don't think belongs in a selftest. I can still write one if you'd like. > > > > > Move the reset to the first statement of the loop body. walk_pud_range() > > has the same shape and gets the same change; walk_p4d_range() never looks > > at walk->action, so that hunk keeps the two functions in sync rather than > > fixing a second bug. > > Your commit message doesn't mention how you discovered this. If it was AI > suggesting it (whether locally or sashiko in reply to some review) you > should reference it. > > If it was simply hardcore code inspection then you should say so too :) It was found by accident while fuzzing a different subsystem. Either way, the fuzzer itself was written by AI, so, Assisted-by: Claude:claude-opus-5 > > > > > Fixes: 3b89863c3fa4 ("mm/pagewalk: fix race between concurrent split and refault") > > Cc: stable@vger.kernel.org > > This all seems right. > > > Signed-off-by: Hyunwoo Kim > > The fix itself looks right, but you need to address the other feedback and > respin (also please send a respin with at least 1 day's delay). OK, will do. Best regards, Hyunwoo Kim