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

[Dave Chinner <david@fromorbit.com>]

On Fri, Aug 09, 2024 at 02:44:21PM -0400, 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.
> 
> Export a simple helper that file systems that have their own ->fault()
> will use, and have a more complicated helper to be do fancy things with
> in filemap_fault.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

....
> +/**
> + * filemap_maybe_emit_fsnotify_event - maybe emit a pre-content event.
> + * @vmf:	struct vm_fault containing details of the fault.
> + *
> + * If we have a pre-content watch on this file we will emit an event for this
> + * range.  If we return anything the fault caller should return immediately, we
> + * will return VM_FAULT_RETRY if we had to emit an event, which will trigger the
> + * fault again and then the fault handler will run the second time through.
> + *
> + * Return: a bitwise-OR of %VM_FAULT_ codes, 0 if nothing happened.
> + */
> +vm_fault_t filemap_maybe_emit_fsnotify_event(struct vm_fault *vmf)
> +{
> +	struct file *fpin = NULL;
> +	vm_fault_t ret;
> +
> +	ret = __filemap_maybe_emit_fsnotify_event(vmf, &fpin);
> +	if (ret) {
> +		if (fpin)
> +			fput(fpin);
> +		return ret;
> +	} else if (fpin) {
> +		fput(fpin);
> +		return VM_FAULT_RETRY;
> +	}

Logic is back to front.  Both paths have to check for fpin,
only one fpin path needs to modify ret:

	ret = __filemap_maybe_emit_fsnotify_event(vmf, &fpin);
	if (fpin) {
		fput(fpin);
		if (!ret)
			ret = VM_FAULT_RETRY;
	}
	return ret;

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(filemap_maybe_emit_fsnotify_event);
> +
>  /**
>   * filemap_fault - read in file data for page fault handling
>   * @vmf:	struct vm_fault containing details of the fault
> @@ -3299,6 +3391,19 @@ 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.
> +	 */
> +	ret = __filemap_maybe_emit_fsnotify_event(vmf, &fpin);
> +	if (unlikely(ret)) {
> +		if (fpin) {
> +			fput(fpin);
> +			ret |= VM_FAULT_RETRY;
> +		}
> +		return ret;
> +	}

Why do we or in VM_FAULT_RETRY here where as the previous case we
simply return VM_FAULT_RETRY? Which one of these is wrong? If both
are correct, then a comment explaining this is in order...

-Dave.
-- 
Dave Chinner
david@fromorbit.com