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 F1968C10DC3 for ; Sat, 2 Dec 2023 09:47:54 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 604216B04DD; Sat, 2 Dec 2023 04:47:54 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 58C956B04DE; Sat, 2 Dec 2023 04:47:54 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 42C4B6B04DF; Sat, 2 Dec 2023 04:47:54 -0500 (EST) 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 2D3F46B04DD for ; Sat, 2 Dec 2023 04:47:54 -0500 (EST) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id F236DC0134 for ; Sat, 2 Dec 2023 09:47:53 +0000 (UTC) X-FDA: 81521401626.05.DD0BB8B Received: from out-186.mta1.migadu.com (out-186.mta1.migadu.com [95.215.58.186]) by imf30.hostedemail.com (Postfix) with ESMTP id D77308000D for ; Sat, 2 Dec 2023 09:47:51 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=t17SwvtP; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf30.hostedemail.com: domain of muchun.song@linux.dev designates 95.215.58.186 as permitted sender) smtp.mailfrom=muchun.song@linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1701510472; 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=kWg0EL3RaVOmoE3sMtlw7YEVcWUVCDJ1DGg90IqvGCU=; b=6aO713Fxqx5jdJn+31ct5eAzDcNyMPvQ6IL+xrx8GMfyM1051UDtJx8U4lUYNRTTtfOYCz w+qEGJyZBxDhJxAaoX9mEpvgbenaKaS9bDiQdMWwMNkkM0Qiz/KPrukdGgP35DOK1xZ/Xi UdyXR6pvYHj2CnbC4gejPClgoRYDSNA= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=t17SwvtP; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf30.hostedemail.com: domain of muchun.song@linux.dev designates 95.215.58.186 as permitted sender) smtp.mailfrom=muchun.song@linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1701510472; a=rsa-sha256; cv=none; b=vmJ6X/C6Cmr4GoIKq7638NRZ2huenygZKvs0WTVIYM/NFT0VPKNYXKLy44p6ftIhr09+d2 /OHGYR4WioOgHrlOYcDSbJhf+haJyz7BelVYbXMATEOLbtESptDamcxvqTxRrWwthmg5JV 8S3D3Xp4VsF6+yac/ajLjOp1agtkzAs= Content-Type: text/plain; charset=utf-8 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1701510469; h=from:from: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=kWg0EL3RaVOmoE3sMtlw7YEVcWUVCDJ1DGg90IqvGCU=; b=t17SwvtPvSBpQHzsL7annwyRE/ek5+7JyWpHeAunX2V05pfdxiCyjCPubq0hiFQjG1U5vE dAQAciU0FmjcGOH3h+1AvMn5R6jmJCjPRGTYqVrQoiGgnYiC5cF4YSy0tHJ8quZ6n3mwFF F9yGO4WDWlwsVSlNeJfUpimQUbkHCr8= Content-Transfer-Encoding: quoted-printable X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Muchun Song Mime-Version: 1.0 Subject: Re: [PATCH 1/4] mm: pagewalk: assert write mmap lock only for walking the user page tables Date: Sat, 2 Dec 2023 17:47:07 +0800 Message-Id: <6D1D5629-56C0-457E-A1BD-A09255AB9A3D@linux.dev> References: Cc: Muchun Song , Mike Kravetz , Andrew Morton , Linux-MM , linux-kernel@vger.kernel.org In-Reply-To: To: Kefeng Wang X-Migadu-Flow: FLOW_OUT X-Rspamd-Queue-Id: D77308000D X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: sun3j4zxnt1tpaxjyteigahqyxyobus1 X-HE-Tag: 1701510471-551885 X-HE-Meta: U2FsdGVkX18l58fMdxM+/cBZvdq7g4z4KJNHah6V7hiKLP6lg2STbI/f9Sbbz1dBIKyv36dwdvTAerCqFPuDpiB9zMVth2VPaY3okiTA9JY0pdck8sLol7/qBiQC825eRoCyip3zkfV98j3UFtVIavwWv+yodIbOvCXoJoErQ82eTKn0lD6GHPsPKYnI6o/VeMGlAHFXdISDi1ffuwvwGAs57Pnf/TQLu2lobDNhmylyIqONcRbPA+BmIi//ZqxywyCWPfBVd2lCyn2Skzi8/tLVY0TK0XR1IAQTyznD0XZYFQRT4bsSaF1sHZSMQrjEMswfDDVUeCouEZ0Gpbj0Zk2mvj8CQcqnqTI90tdTKE8Xw4u1L4a0YtjjZG9U5zXQ+NQVsiZZUgGrVdKQvz5SJdGlRYqmE/vqzcfkQONyhJnC51UKrSRFlhXaBoV41ii1IgRz7O5H4D+HSitaE0NThmObNuHTAnFFW+noejzsGfBa4iMGVL2gnmJ18wcFXdv432zn1tIzFSDxJc08FwWQZLD8ATtTdqD4iHMDFqrBfe7vll/S/cEUAQkbewkzyxDPC11zwApTQTC+LmK5hToXWzj5iECj2VNmgxyY5JaM753A4hP8g//BHEcGvmOP9Bf+O8xADn04oQsBUkDKNvj5EXZc943cPfITopZseynp+kPe+MH+4pCxwrYvolH5qZODDhi7yz4+TxYbrxZUyEGaXsqsQP/Sek6O+CEnh69Hl7D/WfuoVSKnbs3O0fCw9b2I9DHU/JCEeDDeq0bnKLF4cJJrXy9uIKXPPCwRMLOYY1jqlVFwaknyAcGW+cuKba2gK8VOFE3pl9cMAWfQrL/aRjlfsH6qfMdIL6mChubnKmGFc9sTyZbCbFfKZ7MOVdCWE581KjK0yqattusz/HNHbuc1+W5HSue1n9zDexBPU7loCyFPgm49Zi/TKiYL3Uq69b4vN/aZAi9i0cqqdtL AQir7B9d fr0V4ngRL4TqO4WoQiU9SP98daNaPOhU/DIkyefBn8c4dsMBZ4Tscmy4kuzTmQ3sZFHfQSqSBaRDwkljiWq8n+mWT40DRvgxGYkbZ8Ai+gAboK0W/KKsPZpqe6V5b6a/pB2KXNvPGEsDn0T4lhjTTl6ibNa9CBcy0PB8i5/t6RaZ8GzsEaoFgSSXbBrPtBp4mBUm6 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 Dec 2, 2023, at 17:25, Kefeng Wang wrote: >=20 > =EF=BB=BF >=20 > On 2023/12/2 16:08, Muchun Song wrote: >>>> On Dec 1, 2023, at 19:09, Kefeng Wang wrot= e: >>>=20 >>>=20 >>>=20 >>> On 2023/11/27 16:46, Muchun Song wrote: >>>> The 8782fb61cc848 ("mm: pagewalk: Fix race between unmap and page walke= r") >>>> introduces an assertion to walk_page_range_novma() to make all the user= s >>>> of page table walker is safe. However, the race only exists for walking= the >>>> user page tables. And it is ridiculous to hold a particular user mmap w= rite >>>> lock against the changes of the kernel page tables. So only assert at l= east >>>> mmap read lock when walking the kernel page tables. And some users matc= hing >>>> this case could downgrade to a mmap read lock to relief the contention o= f >>>> mmap lock of init_mm, it will be nicer in hugetlb (only holding mmap re= ad >>>> lock) in the next patch. >>>> Signed-off-by: Muchun Song >>>> --- >>>> mm/pagewalk.c | 29 ++++++++++++++++++++++++++++- >>>> 1 file changed, 28 insertions(+), 1 deletion(-) >>>> diff --git a/mm/pagewalk.c b/mm/pagewalk.c >>>> index b7d7e4fcfad7a..f46c80b18ce4f 100644 >>>> --- a/mm/pagewalk.c >>>> +++ b/mm/pagewalk.c >>>> @@ -539,6 +539,11 @@ int walk_page_range(struct mm_struct *mm, unsigned= long start, >>>> * not backed by VMAs. Because 'unusual' entries may be walked this fu= nction >>>> * will also not lock the PTEs for the pte_entry() callback. This is u= seful for >>>> * walking the kernel pages tables or page tables for firmware. >>>> + * >>>> + * Note: Be careful to walk the kernel pages tables, the caller may be= need to >>>> + * take other effective approache (mmap lock may be insufficient) to p= revent >>>> + * the intermediate kernel page tables belonging to the specified addr= ess range >>>> + * from being freed (e.g. memory hot-remove). >>>> */ >>>> int walk_page_range_novma(struct mm_struct *mm, unsigned long start, >>>> unsigned long end, const struct mm_walk_ops *ops, >>>> @@ -556,7 +561,29 @@ int walk_page_range_novma(struct mm_struct *mm, un= signed long start, >>>> if (start >=3D end || !walk.mm) >>>> return -EINVAL; >>>> - mmap_assert_write_locked(walk.mm); >>>> + /* >>>> + * 1) For walking the user virtual address space: >>>> + * >>>> + * The mmap lock protects the page walker from changes to the page >>>> + * tables during the walk. However a read lock is insufficient to >>>> + * protect those areas which don't have a VMA as munmap() detaches >>>> + * the VMAs before downgrading to a read lock and actually tearing >>>> + * down PTEs/page tables. In which case, the mmap write lock should >>>> + * be hold. >>>> + * >>>> + * 2) For walking the kernel virtual address space: >>>> + * >>>> + * The kernel intermediate page tables usually do not be freed, so >>>> + * the mmap map read lock is sufficient. But there are some exceptions= . >>>> + * E.g. memory hot-remove. In which case, the mmap lock is insufficien= t >>>> + * to prevent the intermediate kernel pages tables belonging to the >>>> + * specified address range from being freed. The caller should take >>>> + * other actions to prevent this race. >>>> + */ >>>> + if (mm =3D=3D &init_mm) >>>> + mmap_assert_locked(walk.mm); >>>> + else >>>> + mmap_assert_write_locked(walk.mm); >>>=20 >>> Maybe just use process_mm_walk_lock() and set correct page_walk_lock in s= truct mm_walk_ops? >> No. You also need to make sure the users do not pass the wrong >> walk_lock, so you also need to add something like following: >=20 > But all other walk_page_XX has been converted=EF=BC=8Csee more from commit= > 49b0638502da "mm: enable page walking API to lock vmas during the walk"), > there's nothing special about this one, the calls must pass the right > page_walk_lock to mm_walk_ops, If you think this one is not special, why it is not converted by that commit= at that time?=20 >=20 >> if (mm =3D=3D &init_mm) >> VM_BUG_ON(walk_lock !=3D PGWALK_RDLOCK); >> else >> VM_BUG_ON(walk_lock =3D=3D PGWALK_RDLOCK); >> I do not think the code will be simple. >=20 > or adding the above lock check into process_mm_walk_lock too. No. it=E2=80=99s wrong. walk_page_range_novma is special compared with other= variants, the check is only applicable for walk_page_range_novma, not for i= ts variants.=