Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Date:   Wed, 11 Oct 2023 11:46:27 +0200
From:   Jan Kara <jack@suse.cz>
To:     Lorenzo Stoakes <lstoakes@gmail.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Muchun Song <muchun.song@linux.dev>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Christian Brauner <brauner@kernel.org>,
        Matthew Wilcox <willy@infradead.org>,
        Hugh Dickins <hughd@google.com>,
        Andy Lutomirski <luto@kernel.org>, Jan Kara <jack@suse.cz>,
        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: <20231011094627.3xohlpe4gm2idszm@quack3>
References: <cover.1696709413.git.lstoakes@gmail.com>
 <d2748bc4077b53c60bcb06fccaf976cb2afee345.1696709413.git.lstoakes@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d2748bc4077b53c60bcb06fccaf976cb2afee345.1696709413.git.lstoakes@gmail.com>
Precedence: bulk

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 <lstoakes@gmail.com>

...

> 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?

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.

So I'd be for returning to v2 version, just fix up the error handling
paths...

								Honza


> +
>  		/*
>  		 * Expansion is handled above, merging is handled below.
>  		 * Drivers should not alter the address of the VMA.
> -- 
> 2.42.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR