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 4A43BC7618E for ; Wed, 26 Apr 2023 07:01:01 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B3FEC6B0085; Wed, 26 Apr 2023 03:01:00 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AF0036B0087; Wed, 26 Apr 2023 03:01:00 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 969E66B0088; Wed, 26 Apr 2023 03:01:00 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 83A1D6B0085 for ; Wed, 26 Apr 2023 03:01:00 -0400 (EDT) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 29A27801C3 for ; Wed, 26 Apr 2023 07:01:00 +0000 (UTC) X-FDA: 80722645080.03.DC605FB Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) by imf06.hostedemail.com (Postfix) with ESMTP id BCC80180011 for ; Wed, 26 Apr 2023 07:00:57 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=Y0C7ltP0; spf=pass (imf06.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.50 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1682492457; 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=7/YWmwgvXwDCheOphPVsxeQwjXr2X0sXbZUbiV8/qiY=; b=z+M2NUMba4uvOPyI1UB76bvfzajTYBqLkiZbjYj41TTevSHHplA8/GuOgnz/CG9Byiodc5 jLQDFfW62+aFpRNIajI4mQdm0xYaY5IIzeRDnKQX4exSh3SukXZCfCFdj3QZ0zpQx7NM7A d7Zp+P9JdfAYJcxywOyqghezIBlGyFg= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=Y0C7ltP0; spf=pass (imf06.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.50 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1682492457; a=rsa-sha256; cv=none; b=FQTORCz9oMWaE1y0CAMN5A0TpNNcPesSmNWbKuMiWgDACkrCcGLFg+9A1ac+XzzKg8plm+ Z6Ad6TdpOxstmk9SVXvaMb4GFpAsqjSLNUj2oOQFaX6Z9IdyjS9JqdkUdtfXMCQFfvo6Fb zBZtrqbsSE7j8iCJUbczVnmEvUwWQVA= Received: by mail-wm1-f50.google.com with SMTP id 5b1f17b1804b1-3f1e2555b5aso25385785e9.0 for ; Wed, 26 Apr 2023 00:00:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1682492456; x=1685084456; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=7/YWmwgvXwDCheOphPVsxeQwjXr2X0sXbZUbiV8/qiY=; b=Y0C7ltP0MmwYGVOj6/3WhOxUn/325ozQ8Ule72oKYnXxtERDk5+GyiV+7wTDdc5Xvf spsIpcZ2d8vhPriSlMN0AFd5ayiPbKuv9ZgcaMlhmtguNx//Dj1XH5BtP3GfDqQroHfv T3l3kqKPhJtkg68e8GYONnIDkoXY1AUeZbbPTdfg2QreGEnlWHjJ+cgFhwbSyuP5dYPP FiUh64wygVHIG77LmDu+JjGv339aib6vMOSkF7RurhMQRgOdTUgQ0GLH9HdrG65PLScq x/5hoas/Gehy/WEV+GYqMfaIul+dfsFAnz4tOh5I5+iMehGf3Ovk8TAciNgu4ZRiABop Fq2A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682492456; x=1685084456; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=7/YWmwgvXwDCheOphPVsxeQwjXr2X0sXbZUbiV8/qiY=; b=kQpmX9+1D19G6QMzBrZzN4kxi6cFxspw8MC3o9z1OOk5BokQxErh82JJDg8eC09vdt eGZEMGIuv0/zd7MaYDZZOGr17RU7nc5hmP6D9/Kh1BGL49gQMhrLinN/PfzGQoagxnTD hK+y7boFPipGNuIHz0gexO+h3crfpyORDjxa4RbOd3zpdcMtcdRAsQpEFoqw1lqbS0TU kATeOBLPZtRHqYbNo6vTMuXEw5DvUmAPGYRa6QUciqWw64kNQt2d9qAvsH7W4uKohBPx h6v/BDu6bbwIExa9IhhUyQOc74DXVSUk/Tmde1TYFPHh8fubxBw83M70ueU5qsuBH7sn myUA== X-Gm-Message-State: AAQBX9ehk+QAwvydDxuKjsiU6J+SZ0mgftj5gmSAQF8UMfNqrvhSd3jn 1XJmz2ecGVdFBXUpRrOswXs= X-Google-Smtp-Source: AKy350bY4MEOjK1YDo3fYvDL+vTKok3nznnxqs8hfDObulF1FU+17WKszasxGr5S5JXMNPiLy4hyFw== X-Received: by 2002:adf:f7c5:0:b0:2fb:87f7:3812 with SMTP id a5-20020adff7c5000000b002fb87f73812mr13025211wrq.1.1682492455885; Wed, 26 Apr 2023 00:00:55 -0700 (PDT) Received: from localhost ([2a00:23c5:dc8c:8701:1663:9a35:5a7b:1d76]) by smtp.gmail.com with ESMTPSA id m7-20020adfe0c7000000b003048477729asm4738407wri.81.2023.04.26.00.00.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 26 Apr 2023 00:00:54 -0700 (PDT) Date: Wed, 26 Apr 2023 08:00:53 +0100 From: Lorenzo Stoakes To: Mika =?iso-8859-1?Q?Penttil=E4?= Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , Jason Gunthorpe , Jens Axboe , Matthew Wilcox , Dennis Dalessandro , Leon Romanovsky , Christian Benvenuti , Nelson Escobar , Bernard Metzler , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Ian Rogers , Adrian Hunter , Bjorn Topel , Magnus Karlsson , Maciej Fijalkowski , Jonathan Lemon , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Christian Brauner , Richard Cochran , Alexei Starovoitov , Daniel Borkmann , Jesper Dangaard Brouer , John Fastabend , linux-fsdevel@vger.kernel.org, linux-perf-users@vger.kernel.org, netdev@vger.kernel.org, bpf@vger.kernel.org, Oleg Nesterov , Jason Gunthorpe , John Hubbard , Jan Kara , "Kirill A . Shutemov" , Pavel Begunkov , David Hildenbrand Subject: Re: [PATCH v4] mm/gup: disallow GUP writing to file-backed mappings by default Message-ID: <5ffd7f32-d236-4da4-93f7-c2fe39a6e035@lucifer.local> References: <3b92d56f55671a0389252379237703df6e86ea48.1682464032.git.lstoakes@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: BCC80180011 X-Stat-Signature: jd1cssfh7gf4cjuja1rkoctwkyr87nmo X-HE-Tag: 1682492457-482392 X-HE-Meta: U2FsdGVkX1/oVOdJatHGzTywRkZbq6ETTbVPlOlFky2iYGYvNOPaDlCgPEel8bSEVt5zeomEvRsDecWDBMNpv9vRJkjdWBvNAb647akxeGO9n19sJAY6b0exA/ppxV56NLxhQG3dDtiZ00IWTxDxsn3SR93tC7451MZHwMyfmNj2nfAQ3BeTMNsnDylQrmZt/aa8GYn2UP/Q2MUV40b3TeQrLzmM8Ks5WPVG93b7/0aZw30njEs0tFVG+PocFXch9xkoYFl4psY7s5TIp1ET+OuoOLgniyza2m+lrkJWSomoF4GEgsyAvVpUe00NDV9eQBgwtVG/vNyhilSQHzJS+Dca5V21bLjQ6j8VdBtdSITAwQ/FzUJr+XCQRyME0Gpt4bxzkYNH7Kb9ZOWfF8PfHj+GsnIRCqF3y+rDm0eqIavzwPYZ9ZdDxLP1a8fPCvXQC+DlqBnOTFfhHVRBUSMkNRGvzRGWoZAjMY1rAjuIERPScPUuyjiMUJ/H/jEUlAo2UQkKV4TfsInWqmpvE1vE00nyhaTPZw9nLwWc/Tz2CsQjpKp6hGGqNlOgzkp83y+PeBf2FHnM1CZVourq9eDvW4eF7KtbvQbyABwOkcnKI2jpDbgnrRl5vqBMsZ5QLZsVnd9caNoNuwjyKR6OMYyhFCUC+2lNup6J2PAr/IMWqaNIJw92dh5S6sIDQQz72aaHPzyA5t0vY+JR5WoENSaC5fJFjlB3K5VeeOUPXyZB/8R8sFhhFCsCMtxIUFsEMPRq9UhlNnYu+5/T5w1to8dBRdjZnQH7eAxZMgVKEKWClkAqhZAyM1KayGLZCcugZgoJNbA50f1i6RxjiukGZs086saRU7fld4iJzCtR3LYSZX7lLTB3xXvMkHd1jBXkN57Z03TWtXcmuASDJaOropTi4RRstU0edB7QMt4geucQ4N8dejx10yPyzPV8zDmGYvBgFrCSl0nA1X2UhRwGHUT UoW+jGE9 wWGH0vxzt4UkHkiqm77UscC+niQEYcHLCOnjDShU3uKwf0VQSm4DQ5uTFgee7NKPo0ma5caYU7Iprxnd8/AWFZHObHYDCZfCWrI+788cQY9ZgBU0Tf5ToDPlKd4g7usyEC0xcYyygxwT5nP6TNC2s5/8fQSTGw46eUOx8vT/YYW7VZIJ41YvE26gUdq14Us35MZnruwu9/M+mWyako46cxT95ReTnpdWFSQdILfpF/DajYsuC97MlK9UY1h49zP/gSES76ivTREWJ3WIffF5UDyMhi58kFiBxjEcd+JA66hU9uAJ6f/AYgRJ48piJSG8cUVQV7Npr2NOWU1eHCZ5AUXfQkVnFVYZ6Cgf+HNXEKpfz9zyOEmCdAM9cQRBw86yvUsSWh4zVwXT25KMWW/XuLpv8936zK2V/PxTIxEU7tWhZr/UIuzFSB4JXO2+NQ/kfNay9yWp5UCQAlStbfBJrjgxL3bZ1F5eEhKNWIUxcdludc5GBasF+EILj+j0qijd0bG7tLBT7WFq2b7OywzA7IELBOUYjFf+hOJur 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: On Wed, Apr 26, 2023 at 06:18:38AM +0300, Mika Penttilä wrote: > Hi, > > > On 26.4.2023 2.15, Lorenzo Stoakes wrote: > > GUP does not correctly implement write-notify semantics, nor does it > > guarantee that the underlying pages are correctly dirtied, which could lead > > to a kernel oops or data corruption when writing to file-backed mappings. > > > > This is only relevant when the mappings are file-backed and the underlying > > file system requires folio dirty tracking. File systems which do not, such > > as shmem or hugetlb, are not at risk and therefore can be written to > > without issue. > > > > Unfortunately this limitation of GUP has been present for some time and > > requires future rework of the GUP API in order to provide correct write > > access to such mappings. > > > > In the meantime, we add a check for the most broken GUP case - > > FOLL_LONGTERM - which really under no circumstances can safely access > > dirty-tracked file mappings. > > > > As part of this change we separate out vma_needs_dirty_tracking() as a > > helper function to determine this, which is distinct from > > vma_wants_writenotify() which is specific to determining which PTE flags to > > set. > > > > Suggested-by: Jason Gunthorpe > > Signed-off-by: Lorenzo Stoakes > > --- > > v4: > > - Split out vma_needs_dirty_tracking() from vma_wants_writenotify() to reduce > > duplication and update to use this in the GUP check. Note that both separately > > check vm_ops_needs_writenotify() as the latter needs to test this before the > > vm_pgprot_modify() test, resulting in vma_wants_writenotify() checking this > > twice, however it is such a small check this should not be egregious. > > > > v3: > > - Rebased on latest mm-unstable as of 24th April 2023. > > - Explicitly check whether file system requires folio dirtying. Note that > > vma_wants_writenotify() could not be used directly as it is very much focused > > on determining if the PTE r/w should be set (e.g. assuming private mapping > > does not require it as already set, soft dirty considerations). > > - Tested code against shmem and hugetlb mappings - confirmed that these are not > > disallowed by the check. > > - Eliminate FOLL_ALLOW_BROKEN_FILE_MAPPING flag and instead perform check only > > for FOLL_LONGTERM pins. > > - As a result, limit check to internal GUP code. > > https://lore.kernel.org/all/23c19e27ef0745f6d3125976e047ee0da62569d4.1682406295.git.lstoakes@gmail.com/ > > > > v2: > > - Add accidentally excluded ptrace_access_vm() use of > > FOLL_ALLOW_BROKEN_FILE_MAPPING. > > - Tweak commit message. > > https://lore.kernel.org/all/c8ee7e02d3d4f50bb3e40855c53bda39eec85b7d.1682321768.git.lstoakes@gmail.com/ > > > > v1: > > https://lore.kernel.org/all/f86dc089b460c80805e321747b0898fd1efe93d7.1682168199.git.lstoakes@gmail.com/ > > > > include/linux/mm.h | 1 + > > mm/gup.c | 26 +++++++++++++++++++++++++- > > mm/mmap.c | 37 ++++++++++++++++++++++++++++--------- > > 3 files changed, 54 insertions(+), 10 deletions(-) > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index 37554b08bb28..f7da02fc89c6 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -2433,6 +2433,7 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma, > > #define MM_CP_UFFD_WP_ALL (MM_CP_UFFD_WP | \ > > MM_CP_UFFD_WP_RESOLVE) > > +bool vma_needs_dirty_tracking(struct vm_area_struct *vma); > > int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot); > > static inline bool vma_wants_manual_pte_write_upgrade(struct vm_area_struct *vma) > > { > > diff --git a/mm/gup.c b/mm/gup.c > > index 1f72a717232b..53652453037c 100644 > > --- a/mm/gup.c > > +++ b/mm/gup.c > > @@ -959,16 +959,37 @@ static int faultin_page(struct vm_area_struct *vma, > > return 0; > > } > > +/* > > + * Writing to file-backed mappings which require folio dirty tracking using GUP > > + * is a fundamentally broken operation as kernel write access to GUP mappings > > + * may not adhere to the semantics expected by a file system. > > + */ > > +static inline bool can_write_file_mapping(struct vm_area_struct *vma, > > + unsigned long gup_flags) > > +{ > > + /* If we aren't pinning then no problematic write can occur. */ > > + if (!(gup_flags & (FOLL_GET | FOLL_PIN))) > > + return true; > > + > > + /* We limit this check to the most egregious case - a long term pin. */ > > + if (!(gup_flags & FOLL_LONGTERM)) > > + return true; > > + > > + /* If the VMA requires dirty tracking then GUP will be problematic. */ > > + return vma_needs_dirty_tracking(vma); > > +} > > + > > static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) > > { > > vm_flags_t vm_flags = vma->vm_flags; > > int write = (gup_flags & FOLL_WRITE); > > int foreign = (gup_flags & FOLL_REMOTE); > > + bool vma_anon = vma_is_anonymous(vma); > > if (vm_flags & (VM_IO | VM_PFNMAP)) > > return -EFAULT; > > - if (gup_flags & FOLL_ANON && !vma_is_anonymous(vma)) > > + if ((gup_flags & FOLL_ANON) && !vma_anon) > > return -EFAULT; > > if ((gup_flags & FOLL_LONGTERM) && vma_is_fsdax(vma)) > > @@ -978,6 +999,9 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) > > return -EFAULT; > > if (write) { > > + if (!vma_anon && !can_write_file_mapping(vma, gup_flags)) > > + return -EFAULT; > > + > > if (!(vm_flags & VM_WRITE)) { > > if (!(gup_flags & FOLL_FORCE)) > > return -EFAULT; > > diff --git a/mm/mmap.c b/mm/mmap.c > > index 536bbb8fa0ae..aac638dd22cf 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -1475,6 +1475,32 @@ SYSCALL_DEFINE1(old_mmap, struct mmap_arg_struct __user *, arg) > > } > > #endif /* __ARCH_WANT_SYS_OLD_MMAP */ > > +/* Do VMA operations imply write notify is required? */ > > +static inline bool vm_ops_needs_writenotify( > > + const struct vm_operations_struct *vm_ops) > > +{ > > + return vm_ops && (vm_ops->page_mkwrite || vm_ops->pfn_mkwrite); > > +} > > + > > +/* > > + * Does this VMA require the underlying folios to have their dirty state > > + * tracked? > > + */ > > +bool vma_needs_dirty_tracking(struct vm_area_struct *vma) > > +{ > > + /* Does the filesystem need to be notified? */ > > + if (vm_ops_needs_writenotify(vma->vm_ops)) > > + return true; > > + > > + /* Specialty mapping? */ > > + if (vma->vm_flags & VM_PFNMAP) > > + return false; > > + > > + /* Can the mapping track the dirty pages? */ > > + return vma->vm_file && vma->vm_file->f_mapping && > > + mapping_can_writeback(vma->vm_file->f_mapping); > > +} > > + > > What would be the exact reproducer of the problem? AFAIK writenotify is > handled (by handle_mm_fault()) for non cow mappings (shared), where it only > matters. The issue is reproduced simply by page_to_virt(pinned_page)[0] = 'x' :) The problem is that no faulting actually occurs, so no writenotify, and no PG_dirty tracking does either. Unexpected page dirtying can occur even after they are cleaned in folio_clear_dirty_for_io(), because the caller might manually mark the page dirty at an unexpected time as with the unpin_*dirty*() helpers. I think the long-term solution is to provide a different interface where pages are passed back briefly with locks held and with a manual invocation of writeprotect, or perhaps some kthread_use_mm() thing so we actually trigger the faulting logic, but in the meantime this change helps restore some sanity. > > GUP will only allow FOLL_FORCE without faulting for PageAnonExclusive pages. > So if you want something beyond normal cow semantics you have custom vm_ops > (and mmap() and fault()) This has nothing to do with FOLL_FORCE. > > Also for longterm pinning gups vs fork vs swap there has been fixes by david > recently. I don't think these are relevant in any way to this issue. > > > > > /* > > * Some shared mappings will want the pages marked read-only > > * to track write events. If so, we'll downgrade vm_page_prot > > @@ -1484,14 +1510,13 @@ SYSCALL_DEFINE1(old_mmap, struct mmap_arg_struct __user *, arg) > > int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot) > > { > > vm_flags_t vm_flags = vma->vm_flags; > > - const struct vm_operations_struct *vm_ops = vma->vm_ops; > > /* If it was private or non-writable, the write bit is already clear */ > > if ((vm_flags & (VM_WRITE|VM_SHARED)) != ((VM_WRITE|VM_SHARED))) > > return 0; > > /* The backer wishes to know when pages are first written to? */ > > - if (vm_ops && (vm_ops->page_mkwrite || vm_ops->pfn_mkwrite)) > > + if (vm_ops_needs_writenotify(vma->vm_ops)) > > return 1; > > /* The open routine did something to the protections that pgprot_modify > > @@ -1511,13 +1536,7 @@ int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot) > > if (userfaultfd_wp(vma)) > > return 1; > > - /* Specialty mapping? */ > > - if (vm_flags & VM_PFNMAP) > > - return 0; > > - > > - /* Can the mapping track the dirty pages? */ > > - return vma->vm_file && vma->vm_file->f_mapping && > > - mapping_can_writeback(vma->vm_file->f_mapping); > > + return vma_needs_dirty_tracking(vma); > > } > > /* > > > --Mika >