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 3F4BDC77B7E for ; Fri, 28 Apr 2023 15:24:29 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8BFCF6B0072; Fri, 28 Apr 2023 11:24:28 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 86EE76B007B; Fri, 28 Apr 2023 11:24:28 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7375F6B007D; Fri, 28 Apr 2023 11:24:28 -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 646F76B0072 for ; Fri, 28 Apr 2023 11:24:28 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id E98611601F2 for ; Fri, 28 Apr 2023 15:24:27 +0000 (UTC) X-FDA: 80731171374.06.00A597D Received: from mail-wr1-f52.google.com (mail-wr1-f52.google.com [209.85.221.52]) by imf06.hostedemail.com (Postfix) with ESMTP id EED35180015 for ; Fri, 28 Apr 2023 15:24:25 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=KaTij+70; spf=pass (imf06.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.52 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=1682695466; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=vFxBFzzCkgMJp5wwZwbTmANgljskVv8ZGWWfh/YUqMw=; b=yj6E0uDSlXtIOvd91miXF4ZNRvGG6QsYFGjVR7GV47+/BlPuQVhXMfQdtUDxa7Fykv5jQx fwpEa11DNHAdxishEMkMjgbyAim5wZH64gUUKE2hoHCPUgH+sOTaK06BjudNolBtmmTvuf 7d+cR6vgFYHcWBsy6Qxneyjd6ADeCiE= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1682695466; a=rsa-sha256; cv=none; b=WyeF0gb1c78bWaA2r1+y8MyRyvtie9CvgDDTMokiu5gAItwoDdeLvaIS0jKb2S2fhbwB9S vPYmfLp9pDc1XOMDXbvMm1ozgshQ43tjiBm/gL5Cuwv/nTbWUWPDFi7d6mCriY5C28EZG1 2yon6pyZyJwMMMDmir4TcJZ2pHKBjk8= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=KaTij+70; spf=pass (imf06.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.52 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-wr1-f52.google.com with SMTP id ffacd0b85a97d-2fa36231b1cso6280614f8f.2 for ; Fri, 28 Apr 2023 08:24:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1682695464; x=1685287464; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=vFxBFzzCkgMJp5wwZwbTmANgljskVv8ZGWWfh/YUqMw=; b=KaTij+70zbQI8GHYvi242m+Bo30QjzZy6AXy+3OqxSmifn+26MDDXfWHbJcvXnUhcs 43NiqMNu1kC7BScNlCSgOKIX9nEoPt5N+UhB2O2mB//gVceRJ0GOg3Afz/tLy4xST9u/ wtmZiCencwq3etaNFQCgNY3Mws+dZjuvXOaBLyQB+B6BrA5JKp/gmT8LeX6kxddP+7Ay 8RdQ/RmOR8Frs+gJ73V/04u8Js/v/jllF7urG1nx8tyzPL7zX1rmq0d/3DJS2KtfufGN rGKXYhKuu+6A1pv7uSJxB4FxXPXfvbkx0eqvlscrc4VTnajvBMLc/DvHdjvIzJ5KkRmO RY8g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682695464; x=1685287464; h=in-reply-to: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=vFxBFzzCkgMJp5wwZwbTmANgljskVv8ZGWWfh/YUqMw=; b=AFw2KSBBVrmAOGDKJG6SZcE2DABZyWTFDVTeJZbTjmgTf6iQaa2JsQZljy3BRUAHoe I/YCSKNwAJEJNiUix1b8paCXqV00pGtYuesaDt3Jt+h1lkfAWsJBoII6yIpMoiLmQvXP m729p1ylfzWx1ZhOAWDSRVplkWz38WwI4mK7Ezjt7YqG1kUKVSOZHrRsBMyHt1n0RmHR ixx2kMyuklBjSipxxJSXWeEDsnH4wqgoAxcx+SA5yDTRoP1fXmHH2Yv2/afHFmDImscZ OP1O6wntXl8iL4w/O4SOC5dIjr1m4thWow+u7BovWwv4SoJVj+5VU7VrOs/MptDrNNvD bMkA== X-Gm-Message-State: AC+VfDzv+ttLQFRBP88o45zAU7sfSam+9r3YBkPMdZBA1NfYmXonHgXF ce7twQngJ4cuRmy/N56QqGY= X-Google-Smtp-Source: ACHHUZ6ezHv3tx9rmeRSfxfjUqDo7s7uCIkTkU9KBWVitZvprpU4aJm9/SQIKGWI1IBZgzK4ZXHN0Q== X-Received: by 2002:a5d:4090:0:b0:2c5:3cd2:b8e with SMTP id o16-20020a5d4090000000b002c53cd20b8emr3731220wrp.1.1682695464169; Fri, 28 Apr 2023 08:24:24 -0700 (PDT) Received: from localhost (host86-156-84-164.range86-156.btcentralplus.com. [86.156.84.164]) by smtp.gmail.com with ESMTPSA id i11-20020adfe48b000000b002c3f81c51b6sm21336588wrm.90.2023.04.28.08.24.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 28 Apr 2023 08:24:23 -0700 (PDT) Date: Fri, 28 Apr 2023 16:24:22 +0100 From: Lorenzo Stoakes To: David Hildenbrand 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 , Mika Penttila , David Howells , Christoph Hellwig Subject: Re: [PATCH v5] mm/gup: disallow GUP writing to file-backed mappings by default Message-ID: <49ebb100-afd2-4810-b901-1a0f51f45cfc@lucifer.local> References: <6b73e692c2929dc4613af711bdf92e2ec1956a66.1682638385.git.lstoakes@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspam-User: X-Rspamd-Queue-Id: EED35180015 X-Rspamd-Server: rspam09 X-Stat-Signature: wdts1jxwfufhbfwaqtkudbgwbgwhzjdb X-HE-Tag: 1682695465-613350 X-HE-Meta: U2FsdGVkX1+ZTMEb/KAjto/F44PLWAK2hbePA43zt4rRGczxUxJbXpMcbl78hajTWau4iRI4UdPbt6BJjOmldB/clEupuCPRCUshI9hj26QmQ1h5XQUoWS9i35HzgyPzO1EOPulp5BZ741np2oNyALipAkjXoOW6obLsucARnf5j9GRn0F2MJQu11Yj3rbyWaT0buomzTCEjZiW6TK0Gmk2HZor6PFTIJ9TYPYD7Z4fHwnBe7/2f5z07ZIW9goZ9H8OlRUu0oJLk8rTfmUAFLl2XWysjaEDQXOq/hPvBMsnAjJGGD41slAFjrsjqbwz+E4dRSOZjnKdpZs0hG2JcSluKM5C7EtgZAWCnoWdE3Q1WZ3C2Sj47k30l5SZAYaW0bKh/JKZKdkvzdrSuelagx3h/NwwQY+DZ7+Dmc4n1d/iff+edv4wvlNjFszjlLwbAXzI7tuvJdBPtNnxf+FrkNfv4aeLxJqmrz7OVBK63DVkcq4ZHaM9UtLcTcLIMklk5hRh/ghblNuUL1weLkXs1dHDZ0HAp3Sjzt4g//Iq11M5+FnhVdMYAfS9PRvKIiwryeLUtSrMm8GHs+SUlcyR4BvitTJ+0ivstrSTmk0Z47VOyKBEzUNyU5XKy4DjWJWqGvVE30nd1nVFbTJC8T5iwHyrPYEMZRssDZTCjVh/0JW+5L/Ewhj6U4mshLyYF+VLGv8Kt3w04MHTa9eXIx9OkgjfsZ/iYiGyYNGHA/A4fx8n8GeyX1Qy06sjM6HgDmv2i2/jQOjBmP4jeuZP2hywcvk3Ypxi0mZHPBpuZAjfAu+3GvTh5K9HoA6CFgu5TdWv7ip6XJz7u7QCuScgOJ0luI2hpbT9+QbrQQwG0znNZjOHAqrZrk4p5Ic+izubVolh7R1qIFLOVcy9Co/YymrkLwOSQbfgghwcSQPhRXXdSlKlOFHN8xL4hZn2YXi8TMr1KPiFyXW+ZaVvmZL3avnL 5pRigdNJ t9zcjw3Qd8sATCwTl56QVwHj69rXAo7HHBnZBGtN4kRKUnBZ1PtV3CZvy1VHmLHBncQYNRN2ShvBpO2kUhCfju0i4hA/I9cjuF7yBqpLc/VWmM08P0sVjdVbkkKWfYSXVI7OqjinXUcwPEhGtP+yLmyX8gly8GgQKlxL1qiffc8yAra8kd/+l+UCfVuLuE6m4vsja0AcMhCGOuQqlejxL8VUd2WNUck5dXtIDMmQCX86hvku10lBqkEiKCJf3q72IAZQEtgZLl8vltmlNxjCEXqN/y3Vdw26lLb6aTLigGECtnsyjPuSY8UNdZZUAGZmqPZI3GAUtIuOWWAVAbVprPnZfzGSpSorrqc/HEEVBiFjPeN3fXBs74P1n+hK2cfNlD/ncDJzOK51W/A69ThQn9uvyd1HZ+lt0t6hShVy3tCeJt0gSm5oprh/3rB/ospAjrI6n/QeO8fTLnvcXb/yVWgcXUhpN/7dYirDaGkYDw1+rJpH04SxF/xADdvE0IPKyTgF0 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 Fri, Apr 28, 2023 at 05:13:07PM +0200, David Hildenbrand wrote: > [...] > > > > This change has the potential to break existing setups. Simple example: > > > libvirt domains configured for file-backed VM memory that also has a vfio > > > device configured. It can easily be configured by users (evolving VM > > > configuration, copy-paste etc.). And it works from a VM perspective, because > > > the guest memory is essentially stale once the VM is shutdown and the pages > > > were unpinned. At least we're not concerned about stale data on disk. > > > > > > With your changes, such VMs would no longer start, breaking existing user > > > setups with a kernel update. > > > > Which vfio vm_ops are we talking about? vfio_pci_mmap_ops for example > > doesn't specify page_mkwrite or pfn_mkwrite. Unless you mean some arbitrary > > file system in the guest? > > Sorry, you define a VM to have its memory backed by VM memory and, at the > same time, define a vfio-pci device for your VM, which will end up long-term > pinning the VM memory. Ah ack. Jason seemed concerned that this was already a broken case, I guess that's one for you two to hash out... > > > > > I may well be missing context on this so forgive me if I'm being a little > > dumb here, but it'd be good to get a specific example. > > I was giving to little details ;) > > [...] > > > > > > > I know, Jason und John will disagree, but I don't think we want to be very > > > careful with changing the default. > > > > > > Sure, we could warn, or convert individual users using a flag (io_uring). > > > But maybe we should invest more energy on a fix? > > > > This is proactively blocking a cleanup (eliminating vmas) that I believe > > will be useful in moving things forward. I am not against an opt-in option > > (I have been responding to community feedback in adapting my approach), > > which is the way I implemented it all the way back then :) > > There are alternatives: just use a flag as Jason initially suggested and use > that in io_uring code. Then, you can also bail out on the GUP-fast path as > "cannot support it right now, never do GUP-fast". I already implemented the alternatives (look back through revisions to see :) and there were objections for various reasons. Personally my preference is to provide a FOLL_SAFE_FILE_WRITE flag or such and replace the FOLL_LONGTERM check with this (that'll automatically get rejected for GUP-fast so the GUP-fast conundrum wouldn't be a thing either). GUP-fast is a problem as you say,, but it feels like a fundamental issue with GUP-fast as a whole since you don't get to look at a VMA since you can't take the mmap_lock. You could just look at the folio->mapping once you've walked the page tables and say 'I'm out' if FOLL_WRITE and it's non-anon if that's what you're suggesting? I'm not against that change but this being incremental, I think that would be a further increment. > > IMHO, this patch is not a prereq. > > > > > But given we know this is both entirely broken and a potential security > > issue, and FOLL_LONGTERM is about as egregious as you can get (user > > explicitly saying they'll hold write access indefinitely) I feel it is an > > important improvement and makes clear that this is not an acceptable usage. > > > > I see Jason has said more on this also :) > > > > > > > > > > > > > > > > > > Suggested-by: Jason Gunthorpe > > > > Signed-off-by: Lorenzo Stoakes > > > > --- > > > > include/linux/mm.h | 1 + > > > > mm/gup.c | 41 ++++++++++++++++++++++++++++++++++++++++- > > > > mm/mmap.c | 36 +++++++++++++++++++++++++++--------- > > > > 3 files changed, 68 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..d36a5db9feb1 100644 > > > > --- a/mm/gup.c > > > > +++ b/mm/gup.c > > > > @@ -959,16 +959,51 @@ 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 > > > > + * do not adhere to the semantics expected by a file system. > > > > + * > > > > + * Consider the following scenario:- > > > > + * > > > > + * 1. A folio is written to via GUP which write-faults the memory, notifying > > > > + * the file system and dirtying the folio. > > > > + * 2. Later, writeback is triggered, resulting in the folio being cleaned and > > > > + * the PTE being marked read-only. > > > > + * 3. The GUP caller writes to the folio, as it is mapped read/write via the > > > > + * direct mapping. > > > > + * 4. The GUP caller, now done with the page, unpins it and sets it dirty > > > > + * (though it does not have to). > > > > + * > > > > + * This results in both data being written to a folio without writenotify, and > > > > + * the folio being dirtied unexpectedly (if the caller decides to do so). > > > > + */ > > > > +static bool writeable_file_mapping_allowed(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; > > > > > > FOLL_LONGTERM only applies to FOLL_PIN. This check can be dropped. > > > > I understand that of course (well maybe not of course, but I mean I do, I > > have oodles of diagrams referencing this int he book :) This is intended to > > document the fact that the check isn't relevant if we don't pin at all, > > e.g. reading this you see:- > > > > - (implicit) if not writing or anon we're good > > - if not pin we're good > > - ok we are only currently checking one especially egregious case > > - finally, perform the dirty tracking check. > > > > So this is intentional. > > > > > > > > > + > > > > + /* 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 +1013,10 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) > > > > return -EFAULT; > > > > > > > > if (write) { > > > > + if (!vma_anon && > > > > + !writeable_file_mapping_allowed(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..7b6344d1832a 100644 > > > > --- a/mm/mmap.c > > > > > > > > > I'm probably missing something, why don't we have to handle GUP-fast (having > > > said that, it's hard to handle ;) )? The sequence you describe above should > > > apply to GUP-fast as well, no? > > > > > > 1) Pin writable mapped page using GUP-fast > > > 2) Trigger writeback > > > 3) Write to page via pin > > > 4) Unpin and set dirty > > > > You're right, and this is an excellent point. I worry about other GUP use > > cases too, but we're a bit out of luck there because we don't get to check > > the VMA _at all_ (which opens yet another Pandora's box about how safe it > > is to do unlocked pinning :) > > > > But again, this comes down to the fact we're trying to make things > > _incrementally__ better rather than throwing our hands up and saying one > > day my ship will come in... > > That's not how security fixes are supposed to work IMHO, sorry. Sure, but I don't think the 'let things continue to be terribly broken for X more years' is also a great approach. Personally I come at this from the 'I just want my vmas patch series' unblocked perspective :) and feel there's a functional aspect here too. > > -- > Thanks, > > David / dhildenb > >