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 X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9E01FC432BE for ; Wed, 11 Aug 2021 14:11:52 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id D94906104F for ; Wed, 11 Aug 2021 14:11:51 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org D94906104F Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=szeredi.hu Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 4A4728D0002; Wed, 11 Aug 2021 10:11:51 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4544E6B0073; Wed, 11 Aug 2021 10:11:51 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 31C368D0002; Wed, 11 Aug 2021 10:11:51 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0216.hostedemail.com [216.40.44.216]) by kanga.kvack.org (Postfix) with ESMTP id 15DAD6B0072 for ; Wed, 11 Aug 2021 10:11:51 -0400 (EDT) Received: from smtpin11.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id B89E8250A9 for ; Wed, 11 Aug 2021 14:11:50 +0000 (UTC) X-FDA: 78462988380.11.98CD30F Received: from mail-ed1-f51.google.com (mail-ed1-f51.google.com [209.85.208.51]) by imf04.hostedemail.com (Postfix) with ESMTP id EF63450056F4 for ; Wed, 11 Aug 2021 14:11:49 +0000 (UTC) Received: by mail-ed1-f51.google.com with SMTP id k9so3929520edr.10 for ; Wed, 11 Aug 2021 07:11:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=szeredi.hu; s=google; h=date:from:to:cc:subject:message-id:mime-version:content-disposition :content-transfer-encoding; bh=h8iKVDtBUyzsr7/V/NiTCr0uqw4kRNEUJDacgRjPpPI=; b=XL181+6FYKSKBUdZaWYWBUY8mgGlgimGrbE6W9t+S/CSDUsMfhMILm5c4fJpe8cGWw y69iS8ZOrpqeOroj49BS7wxsPVlCUcRdfdNbsYlFXhzeHCD4zDd6Y1FjOnfhb8XaTgmn 2UqXLYJFB69t5469hPnoS0iq7QRam7VNhUGBc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:mime-version :content-disposition:content-transfer-encoding; bh=h8iKVDtBUyzsr7/V/NiTCr0uqw4kRNEUJDacgRjPpPI=; b=saBZH+yLPX8GqxDpaJ/1r5IQoLctfAaxXNUh9SEISysg19IsiBMI07U7NHLcSTWHnD VKuptf0Zv4zsNLpKgcCpEQStqYeFDBBcY5K1FpFrHkWMf55I3wT2A+flmVKX4xY5acLo M+NXcYswpvgnxOrZ71H+n9BOhrNduKJe151tCB427Ae1ddL4ykl7mPblGnT6ZQuux3nO ibw1drnI7vfV42CHrOAUFZFBMXwRrX5h9YD0aMdv0vVotIrCK6BohOYbTWoDI7M0MfiF aJ6yff7XQ9RIDSFN2+HxLI34MUqr4JOa3MgPJhpFXeQ2SFfRbQEXGgQkeqJoBsgGeqpD u5HA== X-Gm-Message-State: AOAM530Y3cs9kHdvhxNKIrRSnCI8enP/gMvqaFwyqZ1FZGfhhuozhUwo B8cGYuZTAi+pV1eOsmFiqdTMyQ== X-Google-Smtp-Source: ABdhPJxzcRA6Pnf737/+TIKDMNnxcylKXpNDwUnkPSHJCb5Yk/qPApU4i/Om2je8VrI1JQyJESThkg== X-Received: by 2002:aa7:d296:: with SMTP id w22mr11561530edq.170.1628691108358; Wed, 11 Aug 2021 07:11:48 -0700 (PDT) Received: from miu.piliscsaba.redhat.com (catv-86-101-169-16.catv.broadband.hu. [86.101.169.16]) by smtp.gmail.com with ESMTPSA id s24sm3290589edq.56.2021.08.11.07.11.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Aug 2021 07:11:47 -0700 (PDT) Date: Wed, 11 Aug 2021 16:11:45 +0200 From: Miklos Szeredi To: Linus Torvalds Cc: Christian =?utf-8?B?S8O2bmln?= , Matthew Wilcox , linux-mm , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-unionfs@vger.kernel.org Subject: mmap denywrite mess (Was: [GIT PULL] overlayfs fixes for 5.14-rc6) Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: EF63450056F4 X-Stat-Signature: gqensg5xs7k6g67rrqgr8djkemenjbah Authentication-Results: imf04.hostedemail.com; dkim=none ("invalid DKIM record") header.d=szeredi.hu header.s=google header.b=XL181+6F; dmarc=none; spf=pass (imf04.hostedemail.com: domain of miklos@szeredi.hu designates 209.85.208.51 as permitted sender) smtp.mailfrom=miklos@szeredi.hu X-HE-Tag: 1628691109-697026 Content-Transfer-Encoding: quoted-printable 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 Mon, Aug 09, 2021 at 02:25:17PM -0700, Linus Torvalds wrote: > Ugh. Th edances with denywrite and mapping_unmap_writable are really > really annoying. Attached version has error and success paths separated. Was that your complaint? > I get the feeling that the whole thing with deny_write_access and > mapping_map_writable could possibly be done after-the-fact somehow as > part of actually inserting the vma in the vma tree, rather than done > as the vma is prepared. I don't know if that's doable or not. The final denywrite count is obtai= ned in __vma_link_file(), called after __vma_link(). The questions are: - does the order of those helper calls matter? - if it does, could the __vma_link() be safely undone after an unsuccess= ful __vmal_link_file()? > And most users of vma_set_file() probably really don't want that whole > thing at all (ie the DRM stuff that just switches out a local thing. > They also don't check for the new error cases you've added. Christian K=C3=B6nig wants to follow up with those checks (which should b= e asserts, if the code wasn't buggy in the first place). > So I really think this is quite questionable, and those cases should > probably have been done entirely inside ovlfs rather than polluting > the cases that don't care and don't check. I don't get that. mmap_region() currently drops the deny counts from the original file. That doesn't work for overlayfs since it needs to take ne= w temp counts on the override file. So mmap_region() is changed to drop the counts on vma->vm_file, but then = all callers of vma_set_file() will need to do that switch of temp counts, the= re's no way around that. Thanks, Miklos For reference, here's the previous discussion: https://lore.kernel.org/linux-mm/YNHXzBgzRrZu1MrD@miu.piliscsaba.redhat.c= om/ --- fs/overlayfs/file.c | 4 +++- include/linux/mm.h | 2 +- mm/mmap.c | 2 +- mm/util.c | 31 ++++++++++++++++++++++++++++++- 4 files changed, 35 insertions(+), 4 deletions(-) --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c @@ -475,7 +475,9 @@ static int ovl_mmap(struct file *file, s if (WARN_ON(file !=3D vma->vm_file)) return -EIO; =20 - vma_set_file(vma, realfile); + ret =3D vma_set_file(vma, realfile); + if (ret) + return ret; =20 old_cred =3D ovl_override_creds(file_inode(file)->i_sb); ret =3D call_mmap(vma->vm_file, vma); --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2780,7 +2780,7 @@ static inline void vma_set_page_prot(str } #endif =20 -void vma_set_file(struct vm_area_struct *vma, struct file *file); +int /* __must_check */ vma_set_file(struct vm_area_struct *vma, struct f= ile *file); =20 #ifdef CONFIG_NUMA_BALANCING unsigned long change_prot_numa(struct vm_area_struct *vma, --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1806,6 +1806,7 @@ unsigned long mmap_region(struct file *f */ vma->vm_file =3D get_file(file); error =3D call_mmap(file, vma); + file =3D vma->vm_file; if (error) goto unmap_and_free_vma; =20 @@ -1867,7 +1868,6 @@ unsigned long mmap_region(struct file *f if (vm_flags & VM_DENYWRITE) allow_write_access(file); } - file =3D vma->vm_file; out: perf_event_mmap(vma); =20 --- a/mm/util.c +++ b/mm/util.c @@ -314,12 +314,41 @@ int vma_is_stack_for_current(struct vm_a /* * Change backing file, only valid to use during initial VMA setup. */ -void vma_set_file(struct vm_area_struct *vma, struct file *file) +int vma_set_file(struct vm_area_struct *vma, struct file *file) { + vm_flags_t vm_flags =3D vma->vm_flags; + int err; + + /* Get temporary denial counts on replacement */ + if (vm_flags & VM_DENYWRITE) { + err =3D deny_write_access(file); + if (err) + return err; + } + if (vm_flags & VM_SHARED) { + err =3D mapping_map_writable(file->f_mapping); + if (err) + goto undo_denywrite; + } + /* Changing an anonymous vma with this is illegal */ get_file(file); swap(vma->vm_file, file); + + /* Undo temporary denial counts on replaced */ + if (vm_flags & VM_SHARED) + mapping_unmap_writable(file->f_mapping); + + if (vm_flags & VM_DENYWRITE) + allow_write_access(file); + fput(file); + return 0; + +undo_denywrite: + if (vm_flags & VM_DENYWRITE) + allow_write_access(file); + return err; } EXPORT_SYMBOL(vma_set_file); =20