From patchwork Wed Aug 11 14:11:45 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Miklos Szeredi X-Patchwork-Id: 12431295 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-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 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 obtained 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 unsuccessful __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önig wants to follow up with those checks (which should be 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 new 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, there's no way around that. Thanks, Miklos For reference, here's the previous discussion: https://lore.kernel.org/linux-mm/YNHXzBgzRrZu1MrD@miu.piliscsaba.redhat.com/ --- 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 != vma->vm_file)) return -EIO; - vma_set_file(vma, realfile); + ret = vma_set_file(vma, realfile); + if (ret) + return ret; old_cred = ovl_override_creds(file_inode(file)->i_sb); ret = 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 -void vma_set_file(struct vm_area_struct *vma, struct file *file); +int /* __must_check */ vma_set_file(struct vm_area_struct *vma, struct file *file); #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 = get_file(file); error = call_mmap(file, vma); + file = vma->vm_file; if (error) goto unmap_and_free_vma; @@ -1867,7 +1868,6 @@ unsigned long mmap_region(struct file *f if (vm_flags & VM_DENYWRITE) allow_write_access(file); } - file = vma->vm_file; out: perf_event_mmap(vma); --- 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 = vma->vm_flags; + int err; + + /* Get temporary denial counts on replacement */ + if (vm_flags & VM_DENYWRITE) { + err = deny_write_access(file); + if (err) + return err; + } + if (vm_flags & VM_SHARED) { + err = 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);