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 8CE8DCDB474 for ; Wed, 11 Oct 2023 18:14:28 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id EB9FA8D00EB; Wed, 11 Oct 2023 14:14:27 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E6A048D0002; Wed, 11 Oct 2023 14:14:27 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D58EA8D00EB; Wed, 11 Oct 2023 14:14:27 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id C4FE58D0002 for ; Wed, 11 Oct 2023 14:14:27 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 86B388034E for ; Wed, 11 Oct 2023 18:14:27 +0000 (UTC) X-FDA: 81333980574.04.4B0D392 Received: from mail-wr1-f46.google.com (mail-wr1-f46.google.com [209.85.221.46]) by imf26.hostedemail.com (Postfix) with ESMTP id 917CB14001F for ; Wed, 11 Oct 2023 18:14:13 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=gcl6M2UH; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf26.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.46 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1697048053; 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=F7TS+RaEJbB7+7d5gDLjd+koKDndSt/GnmuFIJ1o+Jg=; b=S3NgLWAQ59cKk226ZHH7gvodrIgnqpeiBFWsWGE/XSIBqc3ouOcmwqdZPs0cScDtwK1qd8 y4l6y2dRehtCQEZ4UUrbfzz6V5l4cBUIBhZbGLz4GtQYnYsR4Yc2MzF/pFSRQux0MGip9E Qpi5PvgdKKMvRGp2tzVfhBittiVw3s8= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=gcl6M2UH; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf26.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.46 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1697048053; a=rsa-sha256; cv=none; b=Kn4X+DTDrBW5FK8cEz/NXKERyUeinjpiFWvHRSKKzLURdDFjoLEvvjQZmBozPY4QcakauG fbM3/X3IP5nlLzIMlBPZ5R0rz6kbAtdb4CnBCbEuEdXuY2tjSTnVHn3G4pdNrW00SknvvM BYALuc8gG/Oh6lmaTE0H/rShhhXxn4o= Received: by mail-wr1-f46.google.com with SMTP id ffacd0b85a97d-32d81864e3fso107038f8f.2 for ; Wed, 11 Oct 2023 11:14:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1697048052; x=1697652852; darn=kvack.org; 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=F7TS+RaEJbB7+7d5gDLjd+koKDndSt/GnmuFIJ1o+Jg=; b=gcl6M2UHR7FbN0bww56SV98ycwuPJkhoFzfy0HyFgyCmzjr4UGTw/0+RxoJYXOQyeB nPhR/cG3Fy1Ky1JYNw1MGHCF9OyaMO7Lc1uu4zRfeAN5cSPZXScSVAyx9wb1Gha7b54T 5LwOWzQO2CeQVEpdFiIqmLiMbcCZOHODcW1jg4pElg8SKZLLHygxAgH2r66l7gPlGkbd KMtrDLa3VOzeIW5bq6skUwrNJOHo4PDinL2oi+fZfh+jdnc76wu+bCmZ4jpfXGIVofJd jpwjuP8e8DxkuF+ltZ63OJ2+8cAcYPf3oikbve/yzB2FPVv+fvnWbsFat/1l7hfcbs25 UkjQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697048052; x=1697652852; 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=F7TS+RaEJbB7+7d5gDLjd+koKDndSt/GnmuFIJ1o+Jg=; b=nWxzM2Cgi2imYEInxbUS+ArFMxUdwkHZp9UTbeMMKGF6I0cigcHiFyIPMAEHp1EWen D2CLjzKJxJTcjA9QyEBNRu79pujHsRH4Ou+t8UTIS8toZDqVm6SXFN3uNbVtTNR+F19n DTwj1D6m2UTvgJPjzl27NtQCQMeWbKH1pB67S79BJBR7/OA9gSg/RTl+8Dh7bw8FAMmP 4f6dHnNJi0nGRiakrqWGYm0+PrdbKYx32MK8h3dFj7k8EyVJD24ttcWL/n52ELIz6hPQ ci1EkE6DeVhtYYWjOwnV1oKT2UBVMCMS6upIBvyUSvXCU6H8b1GC+/ziIct9kbzKDsKg Tl1w== X-Gm-Message-State: AOJu0YwEpLVCaEHWVMBJ/z9ko+ytxCmW2wOPyN/KOxFcu7BMV42s721v AaSBChWiBcI3GOavsadMN4o= X-Google-Smtp-Source: AGHT+IFv4S4WUlVw96jvYX9WVjLTJEQQiPuTwG+QUFDRmpY26gHHcgPdhYQ+AOKeY4se6H/9yrLzZw== X-Received: by 2002:a5d:6c69:0:b0:32c:eeee:d438 with SMTP id r9-20020a5d6c69000000b0032ceeeed438mr5956021wrz.54.1697048051663; Wed, 11 Oct 2023 11:14:11 -0700 (PDT) Received: from localhost ([2a00:23c5:dc8c:8701:1663:9a35:5a7b:1d76]) by smtp.gmail.com with ESMTPSA id 9-20020a05600c020900b003feea62440bsm17359617wmi.43.2023.10.11.11.14.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Oct 2023 11:14:10 -0700 (PDT) Date: Wed, 11 Oct 2023 19:14:10 +0100 From: Lorenzo Stoakes To: Jan Kara Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , Mike Kravetz , Muchun Song , Alexander Viro , Christian Brauner , Matthew Wilcox , Hugh Dickins , Andy Lutomirski , linux-fsdevel@vger.kernel.org, bpf@vger.kernel.org Subject: Re: [PATCH v3 3/3] mm: enforce the mapping_map_writable() check after call_mmap() Message-ID: <512d8089-759c-47b7-864d-f4a38a9eacf3@lucifer.local> References: <20231011094627.3xohlpe4gm2idszm@quack3> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20231011094627.3xohlpe4gm2idszm@quack3> X-Rspam-User: X-Stat-Signature: q5t7hh8twwpt14q6cawpbr87cuqxu7rc X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 917CB14001F X-HE-Tag: 1697048053-875771 X-HE-Meta: U2FsdGVkX1/+OVIGLz0/wqvY4zXAtq+8Paw0ihbZYJHtGD2ZECno6DI26LobYmAAfpwWVYYvD5X4XUVwQwgvbYZfUPtgejP/GpKbz++7KuwBjv4jXPI7bQWgGEBDuv+jsIPlTypMDBg1ljcnafL8vJrlXg3NZADJ0kWixoHpbxE+Tk3dm4fRZz9Y+to4lxR0PJQmcCRqZ8m6qTkcxaItfTQ+09R8QL84t3/o2DlVYxx0QuJ6hBi0TFsyHXk7Y0BOC1m06ApiVozOEWfUY3M+Gqccf6eKuHIBHItmRaXuzjzKoPXqeh3Vmxl5nNOi26yayQJKEiDCREOtqzexMre4nWPjUuwwvShARhlucCHX/BdghtzSXUcjj33M0l1G8inlt3u2I5ZNMjUMalyG1BwAFGKj42tSCGWrjfOYHqhRfPo0IDEMfHaRDMLzz1chnyTSTm5nIFPf7KR2AzCDKLlOl2hywhVbLaZutNx/7XlgCGpR2F4KHxJ7IOL6tlxFtR8+PqobKQpv4xPyIvpkknoorIOe4AEUiBXVpH5Z3B1D4cR9HJmEWjrnu2CFBuHmYTT+Au+hae9GHG1uspk5rYR/OY21sSxNiSBFpuS79Kcwtoq2shVB6abHASRVCMBtbBWm8cYVa8lxdmDFioD0Nq2Uh0UQ6OmwaRP0pzQqWXhvYprUax66BXoPv0kScG66pJOpCznpMp1cxH61x3LmnsaPXCLBO6lwvRC7kHJewcKQLBu6BILAKM2zsArFi9tXyleVWFjaGY2NNjUCLKtyztF8nywzu4wARfruZSR0b4LxHXcdattxdZN1giGxVOXb066/0lRcBk6gHc8UjsOegQVi5sOVkWBjIk2eT5DS+Nn4+0P1XfSHtgTlbwSp7sTC2cg059iw7UqPH5KDZyWkGSU5EHTfkI3OzuJ+RV5Uw3HHncOxDuZ14EKEswLv8fb6tlshvzxzmYVR23x9QGGSDtZ wzcpdcf1 WG3M2WJm5ofr3XgRjjrYSTmzCZuezesDAEhIVk6cN8T5xGQ3Kg0n6pw4w9P+sWR8ap+1funBDHno1homHxOJ6dOIvEBGNjmegk0OQTuLZid1Cx+ueloKa0x7rKQGuBV+89YYgdgyn53W6AGeweLNAazZpKXswQZJbSyXkNPnrvzJH6wB/ITKrlI8AwikLJxHv704c+6lMTwboIfI7v7TPAeZUvmmyxg+ZlszsP2Z2NYe91C8dImqdWMAYYAECTgTOI5/fT5w0uN7/idbtQS4aBRoaZutHCEvXcVMcewsiev5PFd+sM99A0ww1Az1G38YuL102vi3LiihfaIre8V2Ubk+dTOcdu3D6eL0sJVJGB6T4HM4f4iEHwtPXfoqHjkAJe8xXXB1Ya6TZpmEzXdX5NNckqy4RQGg75dxgRm1h0eMHEb8ZPQ0mOC6SmH0QvYmXGkUGXUKmO/2aob7CCNQTNwdh4UtHJEOmq8qWn/MGQLnuHeqEf3b6WwlzjqEI4PnnqBLvnF79REYGM+IhQ07jRKGhfQXNtlZAwGGNa4zFbyMU1i6px6/JvF3lHCnpWAGiJl2A 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, Oct 11, 2023 at 11:46:27AM +0200, Jan Kara wrote: > On Sat 07-10-23 21:51:01, Lorenzo Stoakes wrote: > > In order for an F_SEAL_WRITE sealed memfd mapping to have an opportunity to > > clear VM_MAYWRITE in seal_check_write() we must be able to invoke either > > the shmem_mmap() or hugetlbfs_file_mmap() f_ops->mmap() handler to do so. > > > > We would otherwise fail the mapping_map_writable() check before we had > > the opportunity to clear VM_MAYWRITE. > > > > However, the existing logic in mmap_region() performs this check BEFORE > > calling call_mmap() (which invokes file->f_ops->mmap()). We must enforce > > this check AFTER the function call. > > > > In order to avoid any risk of breaking call_mmap() handlers which assume > > this will have been done first, we continue to mark the file writable > > first, simply deferring enforcement of it failing until afterwards. > > > > This enables mmap(..., PROT_READ, MAP_SHARED, fd, 0) mappings for memfd's > > sealed via F_SEAL_WRITE to succeed, whereas previously they were not > > permitted. > > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217238 > > Signed-off-by: Lorenzo Stoakes > > ... > > > diff --git a/mm/mmap.c b/mm/mmap.c > > index 6f6856b3267a..9fbee92aaaee 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -2767,17 +2767,25 @@ 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; > > - } > > + int writable_error = 0; > > + > > + if (vma_is_shared_maywrite(vma)) > > + writable_error = mapping_map_writable(file->f_mapping); > > > > vma->vm_file = get_file(file); > > error = call_mmap(file, vma); > > if (error) > > goto unmap_and_free_vma; > > > > + /* > > + * call_mmap() may have changed VMA flags, so retry this check > > + * if it failed before. > > + */ > > + if (writable_error && vma_is_shared_maywrite(vma)) { > > + error = writable_error; > > + goto close_and_free_vma; > > + } > > Hum, this doesn't quite give me a peace of mind ;). One bug I can see is > that if call_mmap() drops the VM_MAYWRITE flag, we seem to forget to drop > i_mmap_writeable counter here? This wouldn't be applicable in the F_SEAL_WRITE case, as the i_mmap_writable counter would already have been decremented, and thus an error would arise causing no further decrement, and everything would work fine. It'd be very odd for something to be writable here but the driver to make it not writable. But we do need to account for this. > > I've checked why your v2 version broke i915 and I think the reason maybe > has nothing to do with i915. Just in case call_mmap() failed, it ended up > jumping to unmap_and_free_vma which calls mapping_unmap_writable() but we > didn't call mapping_map_writable() yet so the counter became imbalanced. yeah that must be the cause, I thought perhaps somehow __remove_shared_vm_struct() got invoked by i915_gem_mmap() but I didn't trace it through to see if it was possible. Looking at it again, i don't think that is possible, as we hold a mmap/vma write lock, and the only operations that can cause __remove_shared_vm_struct() to run are things that would not be able to do so with this lock held. > > So I'd be for returning to v2 version, just fix up the error handling > paths... So in conclusion, I agree, this is the better approach. Will respin in v4. > > Honza > > > > + > > /* > > * Expansion is handled above, merging is handled below. > > * Drivers should not alter the address of the VMA. > > -- > > 2.42.0 > > > -- > Jan Kara > SUSE Labs, CR