linux-ext4 - Re: [PATCH 03/14] mm: Protect operations adding pages to page cache with invalidate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210713123548.GA24271@quack2.suse.cz>
Date:   Tue, 13 Jul 2021 14:35:48 +0200
From:   Jan Kara <jack@...e.cz>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     Jan Kara <jack@...e.cz>, linux-fsdevel@...r.kernel.org,
        linux-ext4@...r.kernel.org, "Darrick J. Wong" <djwong@...nel.org>,
        Ted Tso <tytso@....edu>, Dave Chinner <david@...morbit.com>,
        Matthew Wilcox <willy@...radead.org>, linux-mm@...ck.org,
        linux-xfs@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net,
        linux-cifs@...r.kernel.org, ceph-devel@...r.kernel.org,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH 03/14] mm: Protect operations adding pages to page cache
 with invalidate_lock

On Tue 13-07-21 07:25:05, Christoph Hellwig wrote:
> Still looks good.  That being said the additional conditional locking in
> filemap_fault makes it fall over the readbility cliff for me.  Something
> like this on top of your series would help:

Yeah, that's better. Applied, thanks. 

								Honza

> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index fd3f94d36c49..0fad08331cf4 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3040,21 +3040,23 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
>  	 * Do we have something in the page cache already?
>  	 */
>  	page = find_get_page(mapping, offset);
> -	if (likely(page) && !(vmf->flags & FAULT_FLAG_TRIED)) {
> +	if (likely(page)) {
>  		/*
> -		 * We found the page, so try async readahead before
> -		 * waiting for the lock.
> +		 * We found the page, so try async readahead before waiting for
> +		 * the lock.
>  		 */
> -		fpin = do_async_mmap_readahead(vmf, page);
> -	} else if (!page) {
> +		if (!(vmf->flags & FAULT_FLAG_TRIED))
> +			fpin = do_async_mmap_readahead(vmf, page);
> +		if (unlikely(!PageUptodate(page))) {
> +			filemap_invalidate_lock_shared(mapping);
> +			mapping_locked = true;
> +		}
> +	} else {
>  		/* No page in the page cache at all */
>  		count_vm_event(PGMAJFAULT);
>  		count_memcg_event_mm(vmf->vma->vm_mm, PGMAJFAULT);
>  		ret = VM_FAULT_MAJOR;
>  		fpin = do_sync_mmap_readahead(vmf);
> -	}
> -
> -	if (!page) {
>  retry_find:
>  		/*
>  		 * See comment in filemap_create_page() why we need
> @@ -3073,9 +3075,6 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
>  			filemap_invalidate_unlock_shared(mapping);
>  			return VM_FAULT_OOM;
>  		}
> -	} else if (unlikely(!PageUptodate(page))) {
> -		filemap_invalidate_lock_shared(mapping);
> -		mapping_locked = true;
>  	}
>  
>  	if (!lock_page_maybe_drop_mmap(vmf, page, &fpin))
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR