Re: [PATCH 10/10] fsnotify: generate pre-content permission event on page fault

[Jan Kara <jack@suse.cz>]

On Thu 25-07-24 14:19:47, Josef Bacik wrote:
> FS_PRE_ACCESS or FS_PRE_MODIFY will be generated on page fault depending
> on the faulting method.
> 
> This pre-content event is meant to be used by hierarchical storage
> managers that want to fill in the file content on first read access.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
...
> @@ -3287,6 +3288,35 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
>  	if (unlikely(index >= max_idx))
>  		return VM_FAULT_SIGBUS;
>  
> +	/*
> +	 * If we have pre-content watchers then we need to generate events on
> +	 * page fault so that we can populate any data before the fault.
> +	 *
> +	 * We only do this on the first pass through, otherwise the populating
> +	 * application could potentially deadlock on the mmap lock if it tries
> +	 * to populate it with mmap.
> +	 */
> +	if (fault_flag_allow_retry_first(vmf->flags) &&
> +	    fsnotify_file_has_content_watches(file)) {

I'm somewhat nervous that if ALLOW_RETRY isn't set, we'd silently jump into
readpage code without ever sending pre-content event and thus we'd possibly
expose invalid content to userspace? I think we should fail the fault if
fsnotify_file_has_content_watches(file) && !(vmf->flags &
FAULT_FLAG_ALLOW_RETRY).

> +		int mask = (vmf->flags & FAULT_FLAG_WRITE) ? MAY_WRITE : MAY_READ;
> +		loff_t pos = vmf->pgoff << PAGE_SHIFT;
> +
> +		fpin = maybe_unlock_mmap_for_io(vmf, fpin);
> +
> +		/*
> +		 * We can only emit the event if we did actually release the
> +		 * mmap lock.
> +		 */
> +		if (fpin) {
> +			error = fsnotify_file_area_perm(fpin, mask, &pos,
> +							PAGE_SIZE);
> +			if (error) {
> +				fput(fpin);
> +				return VM_FAULT_ERROR;
> +			}
> +		}
> +	}
> +
>  	/*
>  	 * Do we have something in the page cache already?
>  	 */
...
> @@ -3612,6 +3643,13 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,
>  	unsigned long rss = 0;
>  	unsigned int nr_pages = 0, mmap_miss = 0, mmap_miss_saved, folio_type;
>  
> +	/*
> +	 * We are under RCU, we can't emit events here, we need to force a
> +	 * normal fault to make sure the events get sent.
> +	 */
> +	if (fsnotify_file_has_content_watches(file))
> +		return ret;
> +

I don't think we need to do anything for filemap_map_pages(). The call just
inserts page cache content into page tables and whatever is in the page
cache and has folio_uptodate() set should be already valid file content,
shouldn't it?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR