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 040FCC04FFE for ; Mon, 29 Apr 2024 09:29:14 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2ACB76B008A; Mon, 29 Apr 2024 05:29:14 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 25BF56B008C; Mon, 29 Apr 2024 05:29:14 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 14BAD6B0092; Mon, 29 Apr 2024 05:29:14 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id EC5FF6B008A for ; Mon, 29 Apr 2024 05:29:13 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 4499F121192 for ; Mon, 29 Apr 2024 09:29:13 +0000 (UTC) X-FDA: 82062045786.11.4014C69 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf26.hostedemail.com (Postfix) with ESMTP id 3B8D9140018 for ; Mon, 29 Apr 2024 09:29:11 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf26.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1714382951; 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=lOpGu6VH+MAP5tanjsOW/mt/qTQ+zajNGErPBLiTXeY=; b=U8SiRgQ0p0TZcztTJwBzifrA9DDucGDQdudjYAYqGX2HUi4/vuXxrepNiEne41j2MDRyWT oTfKkKu2vB+gj6HKQyVaXvC71fACHUvIrx9eqVSCS8awTqdbw9bbsuJFl7HMljwI1NDL2d GhE1eEWf/sD+CUaq9xM9iv9yWiwOvsA= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf26.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714382951; a=rsa-sha256; cv=none; b=BAC8zUDynAfMgeYI3q5mfHi4PWlj7rzH0n3Sz9ZSGV0nqebhGCNrV6dD4AsWMPIER5mbjh ZSz/aFBrh1PBNMbxRHUr4Q18K3VmdTheMnQ0xfBbs9W7gLHcLVwWqVLtVqvH9NtUBrwzff A3ZOfjPYriYBW9RXmqixMjKeRj5onfc= Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 820C92F4; Mon, 29 Apr 2024 02:29:37 -0700 (PDT) Received: from [10.57.65.53] (unknown [10.57.65.53]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 3BB4D3F73F; Mon, 29 Apr 2024 02:29:09 -0700 (PDT) Message-ID: <23fbca83-a175-4d5a-8cf7-ed54c1830d37@arm.com> Date: Mon, 29 Apr 2024 10:29:07 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1] mm: Fix race between __split_huge_pmd_locked() and GUP-fast Content-Language: en-GB To: John Hubbard , Zi Yan Cc: Andrew Morton , Zi Yan , "Aneesh Kumar K.V" , linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20240425170704.3379492-1-ryan.roberts@arm.com> <17956e0f-1101-42d7-9cba-87e196312484@nvidia.com> <2C9D897B-0E96-42F0-B3C4-9926E6DF5F4B@nvidia.com> <328b2b69-e4ab-4a00-9137-1f7c3dc2d195@nvidia.com> From: Ryan Roberts In-Reply-To: <328b2b69-e4ab-4a00-9137-1f7c3dc2d195@nvidia.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Stat-Signature: j1swjmrr9wwqsyoioasfpjpcgmf1u7hu X-Rspamd-Queue-Id: 3B8D9140018 X-Rspam-User: X-Rspamd-Server: rspam12 X-HE-Tag: 1714382951-606483 X-HE-Meta: U2FsdGVkX1+UEg7gRWkguPxkwwGExT3vtrMzsMje0giez+au0CleRwnOUyRjcfTvNbZQrEVgpUZKWR4TxKAYoybw3p0Emh0qwhC1jPtSQOwCTwxWyikhQ4bdf21K97eLGZnBqUwCqIe/zoDCjLFHWt7fFvIsfb8vfexD5OvC6cWIh6bwqKjqe8XFXwgI0sdxbHlrR+U2NsDSO1cDTgyyzZNpyMHgldNPxeoF05cuNrstvkt0dfeb4knfc91w9BNRe5weVJoP3PI5pXvq9Ek9gr+vlPooi493owBZxmZFn9o5rlHo1IgvX1OHdg6DukgKPz3+Ywpipr8YNIcAkgqYCk+Kv07uNfG4zZtseEdIoGzQmHWse648X8TSFrtvlIw9lhcT2ARaRcIr+5/fs8NgoGuGIVBE1YTZ8WLTNhOIErOKrE3rfRle1isugHd+X/EkdtLYXTXgOG08DheJNKp3SVMKd//dc1MSZW2t9nm3TYlkQkxj8EY1EOWplMnQhlgc8SGMakeBwwT9nBtfNmzQI0pYLkvSBnqA8OHKLz5/E5KF8gVblJnUP/I+QeZcNCf3mNVYv3wkk4zpRD0CWRzB7EwvXIjMV1Tioo2reuX99fRFouh8dy6XQ2dltfPpokH8r3w8ExFmfjlJQPHzXPTcVgsES1GAjJln+cvxlY5vax8O+gpeghkss+rNBTIucyAf+J0yM8WgwPvDJ/kT61zRUhA5vTlNiSNRRhDC0wb5lY4d/+nSbbSQd02SyFjWNm1coWGXWBZnLQdjeu0B83M3hOTJXTZY9js1Qg0YiwdEWkX/0Tgck1qjnSehJAnkVlSA3TMGUNfq/+TBR7r8otVmV9a7JjwfglKNUtK83hba6wiadYH0VYbwyyfmrQF8LJpSWiNnNUDZgT4= 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 27/04/2024 20:11, John Hubbard wrote: > On 4/27/24 8:14 AM, Zi Yan wrote: >> On 27 Apr 2024, at 0:41, John Hubbard wrote: >>> On 4/25/24 10:07 AM, Ryan Roberts wrote: >>>> __split_huge_pmd_locked() can be called for a present THP, devmap or >>>> (non-present) migration entry. It calls pmdp_invalidate() >>>> unconditionally on the pmdp and only determines if it is present or not >>>> based on the returned old pmd. This is a problem for the migration entry >>>> case because pmd_mkinvalid(), called by pmdp_invalidate() must only be >>>> called for a present pmd. >>>> >>>> On arm64 at least, pmd_mkinvalid() will mark the pmd such that any >>>> future call to pmd_present() will return true. And therefore any >>>> lockless pgtable walker could see the migration entry pmd in this state >>>> and start interpretting the fields as if it were present, leading to >>>> BadThings (TM). GUP-fast appears to be one such lockless pgtable walker. >>>> I suspect the same is possible on other architectures. >>>> >>>> Fix this by only calling pmdp_invalidate() for a present pmd. And for >>> >>> Yes, this seems like a good design decision (after reading through the >>> discussion that you all had in the other threads). >> >> This will only be good for arm64 and does not prevent other arch developers >> to write code breaking arm64, since only arm64's pmd_mkinvalid() can turn >> a swap entry to a pmd_present() entry. > > Well, let's characterize it in a bit more detail, then: Hi All, Thanks for all the feedback! I had thought that this patch would be entirely uncontraversial - obviously I was wrong :) I've read all the emails, and trying to summarize a way forward here... > > 1) This patch will make things better for arm64. That's important! > > 2) Equally important, this patch does not make anything worse for >    other CPU arches. > > 3) This patch represents a new design constraint on the CPU arch >    layer, and thus requires documentation and whatever enforcement >    we can provide, in order to keep future code out of trouble. I know its only semantics, but I don't view this as a new design constraint. I see it as an existing constraint that was previously being violated, and this patch aims to fix that. The generic version of pmdp_invalidate() unconditionally does a tlb invalidation on the address range covered by the pmd. That makes no sense unless the pmd was previously present. So my conclusion is that the function only expects to be called for present pmds. Additionally Documentation/mm/arch_pgtable_helpers.rst already says this: " | pmd_mkinvalid | Invalidates a mapped PMD [1] | " I read "mapped" to be a synonym for "present". So I think its already documented. Happy to explcitly change "mapped" to "present" though, if it helps? Finally, [1] which is linked from Documentation/mm/arch_pgtable_helpers.rst, also implies this constraint, although it doesn't explicitly say it. [1] https://lore.kernel.org/linux-mm/20181017020930.GN30832@redhat.com/ > > 3.a) See the VM_WARN_ON() hunks below. It sounds like everybody would be happy if I sprinkle these into the arches that override pmdp_invalidate[_ad]()? There are 3 arches that have their own version of pmdp_invalidate(); powerpc, s390 and sparc. And 1 that has its own version of pmdp_invalidate_ad(); x86. I'll add them in all of those. I'll use VM_WARN_ON_ONCE() as suggested by John. I'd rather not put it directly into pmd_mkinvalid() since that would set a precedent for adding them absolutely everywhere. (e.g. pte_mkdirty(), ...). > > 3.b) I like the new design constraint, because it is reasonable and >      clearly understandable: don't invalidate a non-present page >      table entry. > > I do wonder if there is somewhere else that this should be documented? If I change: " | pmd_mkinvalid | Invalidates a mapped PMD [1] | " To: " | pmd_mkinvalid | Invalidates a present PMD; do not call for | | non-present pmd [1] | " Is that sufficient? (I'll do the same for pud_mkinvalid() too. > > > thanks,