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 544B0C77B73 for ; Tue, 2 May 2023 15:09:46 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E371C6B0078; Tue, 2 May 2023 11:09:45 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id DE7F76B007B; Tue, 2 May 2023 11:09:45 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CD6746B007D; Tue, 2 May 2023 11:09:45 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from mail-lf1-f41.google.com (mail-lf1-f41.google.com [209.85.167.41]) by kanga.kvack.org (Postfix) with ESMTP id 672B26B0078 for ; Tue, 2 May 2023 11:09:45 -0400 (EDT) Received: by mail-lf1-f41.google.com with SMTP id 2adb3069b0e04-4efe8991bafso5145190e87.0 for ; Tue, 02 May 2023 08:09:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1683040184; x=1685632184; 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=pACv5sNREpkMwZWTexAWOB8u6qIuWyv5q9pXglKMxz8=; b=WfWdYg1KjbAtjFc8o5wq8jvz29diGtMy+Ep0b9UHcUVfsgbzfCSQgu5tHiWjTA1/mg BrA7hgwmMsD96SZsL9FS2iMO15z4PasGnUd9+LTA2rNIY6vZjJHA2+j6wJgEFxXB3PfL r86aOoaZolQrjNOsIHQPxbIMd7a4knTR7L+RQvrsyTDwWpOld4hjChfxZe/gkmDCD/JQ ay6vg//9rx2g/T5AifWgB9cpKwb1dlnFli1sYVE/1d4e7A1ox6Wz8tNWW2/XpZKj8XAN 4GKd8MieXoXqYr03mwhARmnXx/j4VaCK3ywavepWJku3pH6K58rlwPIihiZPtX9Y/g7V gong== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683040184; x=1685632184; 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=pACv5sNREpkMwZWTexAWOB8u6qIuWyv5q9pXglKMxz8=; b=TSyoIkaPVKdhguDWof8Sk+mIs5sycyKuITC250u8BGQnNuhA54hBHSUkwNgRHBhZum 22SsQgDVz85WAbA1pz4HIkky8qkfcfMCtcgsjk8Gl0A7x8zWSAo2oxQBlCuGZ2i5+SAs LNzXQsVa3uS8wPGNN2EGt9Zm0icvtx71jQmxNEreLElQcrdLUIR6+MZpG6Mk6k0CDWmP Yv5erIyaJL3lubQzAJQ2ucY0grtqnxK/Vt3Fz4wxtnb320gjqNzgQkn4ykeak+hEZ4tH m56h8bGwpQn49NN9MoS9VQOTt4U5YX276eZyanJwDwrMdMGv3gkujSAG7lZ6R6RgQ2p2 OUfQ== X-Gm-Message-State: AC+VfDwCRoznOWBnS/8sQ+avoXBRZyPJpdQXr0oxMmPQkc6zEwMMUVr2 JFovclxqndzlEvWwutHYURdB4m2GcGEa5w== X-Google-Smtp-Source: ACHHUZ4XIbT+qDvJvtiu8jQaj1lCqwwpgK5NNUQ0OCNjgiigaO8IN4kPBYAixWknGzhXZ1y7hUunLA== X-Received: by 2002:adf:f1d2:0:b0:2fa:88f2:b04c with SMTP id z18-20020adff1d2000000b002fa88f2b04cmr9959466wro.20.1683014228992; Tue, 02 May 2023 00:57:08 -0700 (PDT) Received: from localhost (host86-156-84-164.range86-156.btcentralplus.com. [86.156.84.164]) by smtp.gmail.com with ESMTPSA id j6-20020a5d6186000000b003063772a55bsm1082093wru.61.2023.05.02.00.57.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 May 2023 00:57:07 -0700 (PDT) Date: Tue, 2 May 2023 08:57:07 +0100 From: Lorenzo Stoakes To: Andy Lutomirski Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , Matthew Wilcox , Mike Kravetz , Muchun Song , Alexander Viro , Christian Brauner , linux-fsdevel@vger.kernel.org, Jan Kara , Hugh Dickins Subject: Re: [PATCH v2 3/3] mm: perform the mapping_map_writable() check after call_mmap() Message-ID: <7565426e-1080-4521-afdd-4dfbfbc63c9b@lucifer.local> References: <6f3aea05c9cc46094b029cbd1138d163c1ae7f9d.1682890156.git.lstoakes@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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, May 01, 2023 at 12:02:00PM -0700, Andy Lutomirski wrote: > On Sun, Apr 30, 2023 at 3:26 PM Lorenzo Stoakes wrote: > > > > In order for a F_SEAL_WRITE sealed memfd mapping to have an opportunity to > > clear VM_MAYWRITE, we must be able to invoke the appropriate vm_ops->mmap() > > handler to do so. We would otherwise fail the mapping_map_writable() check > > before we had the opportunity to avoid it. > > Is there any reason this can't go before patch 3? I don't quite understand what you mean by this? I mean sure, we could, but intent was to build to this point and leave the most controversial change for last :) > > If I'm understanding correctly, a comment like the following might > make this a lot more comprehensible: > > > > > This patch moves this check after the call_mmap() invocation. Only memfd > > actively denies write access causing a potential failure here (in > > memfd_add_seals()), so there should be no impact on non-memfd cases. > > > > This patch makes the userland-visible change that MAP_SHARED, PROT_READ > > mappings of an F_SEAL_WRITE sealed memfd mapping will now succeed. > > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217238 > > Signed-off-by: Lorenzo Stoakes > > --- > > mm/mmap.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > index 646e34e95a37..1608d7f5a293 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -2642,17 +2642,17 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > > vma->vm_pgoff = pgoff; > > > > if (file) { > > - if (is_shared_maywrite(vm_flags)) { > > - error = mapping_map_writable(file->f_mapping); > > - if (error) > > - goto free_vma; > > - } > > - > > vma->vm_file = get_file(file); > > error = call_mmap(file, vma); > > if (error) > > goto unmap_and_free_vma; > > > > /* vm_ops->mmap() may have changed vma->flags. Check for writability now. */ > Ack, will add on next spin. > > + if (vma_is_shared_maywrite(vma)) { > > + error = mapping_map_writable(file->f_mapping); > > + if (error) > > + goto close_and_free_vma; > > + } > > + > > Alternatively, if anyone is nervous about the change in ordering here, > there could be a whole new vm_op like adjust_vma_flags() that happens > before any of this. Agreed, clearly this change is the most controversial thing here. I did look around and couldn't find any instance where this could cause an issue, since it is purely the mapping_map_writable() that gets run at a different point, but this is certainly an alterative. I have a feeling people might find adding a new op there possibly _more_ nerve-inducing :) but it's an option. > > --Andy