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 20D4DD3A661 for ; Tue, 29 Oct 2024 15:43:21 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 935A06B008A; Tue, 29 Oct 2024 11:43:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8BE426B008C; Tue, 29 Oct 2024 11:43:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7118D6B0092; Tue, 29 Oct 2024 11:43:20 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 4F8E96B008A for ; Tue, 29 Oct 2024 11:43:20 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id B180B81C6B for ; Tue, 29 Oct 2024 15:43:19 +0000 (UTC) X-FDA: 82727058666.28.FB48EC2 Received: from mail-ed1-f46.google.com (mail-ed1-f46.google.com [209.85.208.46]) by imf14.hostedemail.com (Postfix) with ESMTP id 34262100015 for ; Tue, 29 Oct 2024 15:42:49 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=fZEPZXz6; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf14.hostedemail.com: domain of shy828301@gmail.com designates 209.85.208.46 as permitted sender) smtp.mailfrom=shy828301@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1730216422; 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=WhcIQpJ1IaeKRXa1HnFGErQ2YwEx8j6m+/AxbcPsv0o=; b=64HUmJP494iCMw71ZERHP/aqDtyvFTbfBcxJB1I9uG8VQGTMCCHK4jalNf/RgjEwvLjP21 +L+cp6TVzpJ8BjrW2XNz5sF9O6ixy4lF71UV0VeXl6O/dpNViRfx18lL4TNQZpOyiu3C+n CBQuuNeBLiv8YCsXjHQV+0fE12RXREs= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1730216422; a=rsa-sha256; cv=none; b=4UtpYCM+Ur5ARuWojtzeDtq/pp0sDzjzhpLzDlroYfvZw7/mHpvrW2CmkWNJf1YSimK45Y /lU2SeA1+oAEarTCIW2f4EqjyocjilMhFjg+B20IiwAW6QIqMhW36obcLkGB88c23bi29t LjjHqJxjTC00pkK6n5Qp5Ht0VBBwnOc= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=fZEPZXz6; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf14.hostedemail.com: domain of shy828301@gmail.com designates 209.85.208.46 as permitted sender) smtp.mailfrom=shy828301@gmail.com Received: by mail-ed1-f46.google.com with SMTP id 4fb4d7f45d1cf-5c957d8bce2so2754703a12.2 for ; Tue, 29 Oct 2024 08:43:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1730216596; x=1730821396; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=WhcIQpJ1IaeKRXa1HnFGErQ2YwEx8j6m+/AxbcPsv0o=; b=fZEPZXz6M294SB8CGJkjAcMhBx4bGj3IwKA2Mlr45uqC7fzP257N6oKxvpkQlo3tTY QIPdnqFFAWBo2aibow9eImHaGGauxi754+YCtRhNfLLjDeNLct6/6JWLtF0v+iH1pQc1 rGDyc2AWy9SMa8moIe+AD1azheAwPgfNYz3bQhY8+7OPQWyoe0pKWBEVhXPv/XUhieBh g2RstWC6SfgsKsVYhVVzObVGoj5PlfNwoVymYA6s6oRbyN4mDLEAQwbQUs/5QcJWl1t5 Zhpj6jrqw9xcK7OQWdgUFbqWrxY3lOwlGeeYLSCBjabA68dLqKQ5vslH+xiOCgU3Pv+w bOaQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1730216596; x=1730821396; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=WhcIQpJ1IaeKRXa1HnFGErQ2YwEx8j6m+/AxbcPsv0o=; b=YxFCdwF0sz/DDgJDzlQ4dUg55s2F2mMAMS7395BSdMxIjAFNR64M9HB2p6uDB1RjUD yffb94L3GxXig04dbBxV5mOAI1ZwlLZkzmIzNtgzFusmDHkCJx0NSn61Q765RzzoR8dx abwtJpbuGZdOgfM5sWqPRaqfEqSNABDqPU8IiKSz+9IrbCHsHhQeGqH47knEhG0sJsgD rDOahpYz4LVq7P4q/zb9UMbDQlhbz14b2Kn4Q3fvT6VybaOhcOyU+3DhF9ZMqe2a6Xwz nuMpJ3t6LYWDN5B1JS3/DZjCkGc5wR+bjtav49CTeMeEDNmp88nuw/8DdjVtMWxRWliw BMJA== X-Forwarded-Encrypted: i=1; AJvYcCVs33KfT/Uomb0ViCxcvuUM6FvCVWoDq4mrAh2U3lh/h6DdfiXH/YZRPRWeoOFfiyIwqnwMN5Kavw==@kvack.org X-Gm-Message-State: AOJu0Yx0NzB3FuTZZaZ1XK/JNlbAM4PJ7sgkWihsV+g7KCH3ykI9JW/v rWRaUsZTSlhhaM2YCAYEQEY0Ql0/L3X53wyrc5RYcYYm+VtEHDdsGmc6DGbdoBC5e775QABDgiw ZH23j8OHIr9Z1Hjq/a3RgAc90KC8= X-Google-Smtp-Source: AGHT+IHb5QK6poBquQTDAvxMF22pm9uNsacla4ytTbbmj5HwMECDKQOCfJeTdzlHxsToe9BWwGmFq9L4dtaIpMl/oDY= X-Received: by 2002:a05:6402:348f:b0:5c9:34b4:69a8 with SMTP id 4fb4d7f45d1cf-5cbbf889850mr14400108a12.6.1730216595849; Tue, 29 Oct 2024 08:43:15 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Yang Shi Date: Tue, 29 Oct 2024 08:43:04 -0700 Message-ID: Subject: Re: [PATCH hotfix 6.12 v3 4/5] mm: refactor arch_validate_flags() and arm64 MTE handling To: Lorenzo Stoakes Cc: Andrew Morton , "Liam R . Howlett" , Vlastimil Babka , Jann Horn , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Linus Torvalds , Peter Xu , Catalin Marinas , Will Deacon , Mark Brown , "David S . Miller" , Andreas Larsson Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 34262100015 X-Stat-Signature: ydqo9yj6gbadpfxghayts84kno14589k X-Rspam-User: X-HE-Tag: 1730216569-937088 X-HE-Meta: U2FsdGVkX1+Lu1+2dDcCJqmNsIrBHoZlEmJ5vbMLIZrIn3vR2EhK2nz8ZI90FpMt4fp+q1DmewWjQBtNNWQnnFygVYigZp2/7dzPxo44N5hgAwXYZz2AI3WeYas/04Q/ByuvozryqGxukdxlnQ9jlsw3C7nuuh85iIH6KyMaDOq7ggL5UgkDf06rcEZ4Qczp8Meske0yDBQEHVD/N003PT0OwDkac5jG7UQfX3DOsKu/oxl1J1UgfNl5JPAXPpfQyAAxMDnLIalLrJu7Z4vEI6gp9T59OYznZ352bchqg0IrFoCWY6XMnbSEA9bI7Z5ivv7QhjwLfVZDv7WVDybN6Fz9JCPs6r+8UpeGsMRDgw9Z+QShzx1ATfBms2UcOBiojo4FzTuRbPEAyc6mZiQJac7wEOtnJrmMDGmFtxXVKyI2fFVULw5m0nib+alxH5HKpcPnhxaO6nGxPHrUXrt5xAvqoZFERzcX6WZhbzvzChw7X0jStJ5hmzORyawMaHNahRxRW8iG2Yn3yVGBSOgj0F16EtupcTvRwuwmRis+tzQA/D0KrR0BVuw0r0Hi6E/5JRuLKBPEQULEijeP5t8paIN/Gak6xk3LqBr2FrTrKvqLQMvQ8noWWpM/wI0FvdeksysZjna1QRadKz6CmOa6Da7raN+lYVF8SutueTkDdWnpa8qZZi6A3Lz9Tp8aQ0eJbP2GcRXjd4j9STaY/ak8CI4uXkW1BIw1lO3SdktC5ZCZeo7Z95euAPcpALI7VpDOCO3JmPxSAKEgWKX45UF3kfgBs+7zystHoE3y8mVZ/kzMt5eAPSAWVbnNTM0Zvg3aMnkyM2adZUozHfHDMDCB6caHevbOX8PMb0fwveKJVQRfdhJ5iBmX7WjiFsdiqrZDmkqUcEWz/BKzXmPxOJnr3y2mtGcy84ZzkIuKrYSTRxDMDbFcDduJPgnI5nLX7PtF6Dai58EiNj5PK+sBw2Y QLL4Vu4m wC8L79+5NXap1I8SI9X/Z9ziKFde/HLBfpdjStLXTHhD8stG17Ojqvf+8cj+onjy7CRrwG8PGSBIDA4txp7CVXG2bubZdEq+zCa9ZnoZm7o/hZT5gmIAncakLtUdXFOuB+NRvejJxBLurjWyUjBtg15oZKhsDyaNErNGPJfAx4cz/8qPxLuDXFFGilbJVojAi2Yl0M9DAP9wdgMqxRrBAXo7kbZjzOx43jJLUlVP/yS6uNHT/uVBXR3TyxMNvNVjKkM4baumd6ptE44LfZlC/mAkEbIJT26ATLV0Ni1lUAK7D2lntn9N/RhVxTYS0nA5rdWB6cePb68skr1V3rTdf5YeDrSy/TF1IWkmSYajHH8kQWrLrzUhT/9utOlvqIngoH6FNoccWNkhqT0xZ/PgsYpSBii15kLht9xS3hV4OC1A/M9fw/s1YVYK42w== 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 Tue, Oct 29, 2024 at 6:04=E2=80=AFAM Lorenzo Stoakes wrote: > > Currently MTE is permitted in two circumstances (desiring to use MTE havi= ng > been specified by the VM_MTE flag) - where MAP_ANONYMOUS is specified, as > checked by arch_calc_vm_flag_bits() and actualised by setting the > VM_MTE_ALLOWED flag, or if the file backing the mapping is shmem, in whic= h > case we set VM_MTE_ALLOWED in shmem_mmap() when the mmap hook is activate= d > in mmap_region(). > > The function that checks that, if VM_MTE is set, VM_MTE_ALLOWED is also s= et > is the arm64 implementation of arch_validate_flags(). > > Unfortunately, we intend to refactor mmap_region() to perform this check > earlier, meaning that in the case of a shmem backing we will not have > invoked shmem_mmap() yet, causing the mapping to fail spuriously. > > It is inappropriate to set this architecture-specific flag in general mm > code anyway, so a sensible resolution of this issue is to instead move th= e > check to arch_validate_flags() itself. > > This requires a modification to the arch_validate_flags() signature to pa= ss > in a pointer to the struct file associated with the mapping, however this > is not too egregious as this is only used by two architectures anyway - > arm64 and sparc. > > So this patch performs this adjustment and moves the check to > arch_validate_flags() which resolves the issue. > > We'd ideally like to have eliminated the arch_calc_vm_flag_bits() case, b= ut > we risk inadvertently changing behaviour as we do not have mmap() flags > available at the point of the arch_validate_flags() check and a MAP_ANON = | > MAP_HUGETLB case would be accepted for MTE currently, but a MAP_SHARED | > MAP_HUGETLB would not. > > This is likely an oversight but we want to try to keep behaviour identica= l > to before in this patch. > > So continue to check VM_MTE_ALLOWED which arch_calc_vm_flag_bits() sets i= f > MAP_ANON. > > Reported-by: Jann Horn > Fixes: deb0f6562884 ("mm/mmap: undo ->mmap() when arch_validate_flags() f= ails") > Cc: stable > Acked-by: Vlastimil Babka > Tested-by: Mark Brown > Signed-off-by: Lorenzo Stoakes > --- > arch/arm64/include/asm/mman.h | 29 +++++++++++++++++++++++++---- > arch/sparc/include/asm/mman.h | 5 +++-- > include/linux/mman.h | 2 +- > mm/mmap.c | 2 +- > mm/mprotect.c | 2 +- > mm/shmem.c | 3 --- > 6 files changed, 31 insertions(+), 12 deletions(-) > > diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.= h > index 9e39217b4afb..94925a0482e3 100644 > --- a/arch/arm64/include/asm/mman.h > +++ b/arch/arm64/include/asm/mman.h > @@ -6,7 +6,9 @@ > > #ifndef BUILD_VDSO > #include > +#include > #include > +#include > > static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot, > unsigned long pkey) > @@ -60,15 +62,34 @@ static inline bool arch_validate_prot(unsigned long p= rot, > } > #define arch_validate_prot(prot, addr) arch_validate_prot(prot, addr) > > -static inline bool arch_validate_flags(unsigned long vm_flags) > +static inline bool arch_validate_flags(struct file *file, unsigned long = vm_flags) > { > if (!system_supports_mte()) > return true; > > - /* only allow VM_MTE if VM_MTE_ALLOWED has been set previously */ > - return !(vm_flags & VM_MTE) || (vm_flags & VM_MTE_ALLOWED); > + if (!(vm_flags & VM_MTE)) > + return true; > + > + /* > + * We explicitly permit MAP_ANONYMOUS and shmem backed mappings t= o use > + * MTE. We check for shmem explicitly here. > + * > + * The MAP_ANONYMOUS case is handled by arch_calc_vm_flag_bits() = which > + * explicitly sets VM_MTE_ALLOWED which we check for here also. > + * > + * Ideally we'd perform both checks here but existing semantics s= upport > + * hugetlb in MAP_ANONYMOUS mode but not a MAP_SHARED mapping, wh= ich is > + * likely unintended but we maintain identical behaviour for > + * consistency. > + */ > + if (vm_flags & VM_MTE_ALLOWED) > + return true; > + if (shmem_file(file)) > + return true; Sorry for chiming in late. This looks problematic IIUC. The patch 5/5 moved arch_validate_flags() before call_mmap() as the commit log said. But shmem_file() does this check: mapping->a_ops =3D=3D &shmem_aops But mapping->a_ops is not initialized until shmem_mmap(), which is called by call_mmap(). So shmem_file() should always return false here. Did I miss something? > + > + return false; > } > -#define arch_validate_flags(vm_flags) arch_validate_flags(vm_flags) > +#define arch_validate_flags(file, vm_flags) arch_validate_flags(file, vm= _flags) > > #endif /* !BUILD_VDSO */ > > diff --git a/arch/sparc/include/asm/mman.h b/arch/sparc/include/asm/mman.= h > index af9c10c83dc5..d426e1f7c2c1 100644 > --- a/arch/sparc/include/asm/mman.h > +++ b/arch/sparc/include/asm/mman.h > @@ -10,6 +10,7 @@ int sparc_mmap_check(unsigned long addr, unsigned long = len); > > #ifdef CONFIG_SPARC64 > #include > +#include > > static inline void ipi_set_tstate_mcde(void *arg) > { > @@ -54,11 +55,11 @@ static inline int sparc_validate_prot(unsigned long p= rot, unsigned long addr) > return 1; > } > > -#define arch_validate_flags(vm_flags) arch_validate_flags(vm_flags) > +#define arch_validate_flags(file, vm_flags) arch_validate_flags(file, vm= _flags) > /* arch_validate_flags() - Ensure combination of flags is valid for a > * VMA. > */ > -static inline bool arch_validate_flags(unsigned long vm_flags) > +static inline bool arch_validate_flags(struct file *file, unsigned long = vm_flags) > { > /* If ADI is being enabled on this VMA, check for ADI > * capability on the platform and ensure VMA is suitable > diff --git a/include/linux/mman.h b/include/linux/mman.h > index 8ddca62d6460..82e6488026b7 100644 > --- a/include/linux/mman.h > +++ b/include/linux/mman.h > @@ -117,7 +117,7 @@ static inline bool arch_validate_prot(unsigned long p= rot, unsigned long addr) > * > * Returns true if the VM_* flags are valid. > */ > -static inline bool arch_validate_flags(unsigned long flags) > +static inline bool arch_validate_flags(struct file *file, unsigned long = flags) > { > return true; > } > diff --git a/mm/mmap.c b/mm/mmap.c > index ab71d4c3464c..40b5858ae875 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1511,7 +1511,7 @@ unsigned long mmap_region(struct file *file, unsign= ed long addr, > } > > /* Allow architectures to sanity-check the vm_flags */ > - if (!arch_validate_flags(vma->vm_flags)) { > + if (!arch_validate_flags(file, vma->vm_flags)) { > error =3D -EINVAL; > goto close_and_free_vma; > } > diff --git a/mm/mprotect.c b/mm/mprotect.c > index 6f450af3252e..c6db98b893fc 100644 > --- a/mm/mprotect.c > +++ b/mm/mprotect.c > @@ -816,7 +816,7 @@ static int do_mprotect_pkey(unsigned long start, size= _t len, > } > > /* Allow architectures to sanity-check the new flags */ > - if (!arch_validate_flags(newflags)) { > + if (!arch_validate_flags(vma->vm_file, newflags)) { > error =3D -EINVAL; > break; > } > diff --git a/mm/shmem.c b/mm/shmem.c > index 4ba1d00fabda..e87f5d6799a7 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -2733,9 +2733,6 @@ static int shmem_mmap(struct file *file, struct vm_= area_struct *vma) > if (ret) > return ret; > > - /* arm64 - allow memory tagging on RAM-based files */ > - vm_flags_set(vma, VM_MTE_ALLOWED); > - > file_accessed(file); > /* This is anonymous shared memory if it is unlinked at the time = of mmap */ > if (inode->i_nlink) > -- > 2.47.0 >