Re: [PATCH 13/26] vfs: create generic_remap_file_range_touch to update inode metadata

[Christoph Hellwig <hch@infradead.org>]

On Mon, Oct 15, 2018 at 08:19:26PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Create a new VFS helper to handle inode metadata updates when remapping
> into a file.  If the operation can possibly alter the file contents, we
> must update the ctime and mtime and remove security privileges, just
> like we do for regular file writes.  Wire up ocfs2 to ensure consistent
> behavior.

Subject line doesn't match the actual function name..

> +/* Update inode timestamps and remove security privileges when remapping. */
> +static int generic_remap_file_range_target(struct file *file,
> +					   unsigned int remap_flags)
> +{
> +	int ret;
> +
> +	/* If can't alter the file contents, we're done. */
> +	if (remap_flags & REMAP_FILE_DEDUP)
> +		return 0;
> +
> +	/* Update the timestamps, since we can alter file contents. */
> +	if (!(file->f_mode & FMODE_NOCMTIME)) {
> +		ret = file_update_time(file);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Clear the security bits if the process is not being run by root.
> +	 * This keeps people from modifying setuid and setgid binaries.
> +	 */
> +	return file_remove_privs(file);
> +}
> +
>  /*
>   * Check that the two inodes are eligible for cloning, the ranges make
>   * sense, and then flush all dirty data.  Caller must ensure that the
> @@ -1820,6 +1844,10 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
>  	if (ret)
>  		return ret;
>  
> +	ret = generic_remap_file_range_target(file_out, remap_flags);
> +	if (ret)
> +		return ret;
> +

Also I find the name still somewhat odd.  Why don't we side-step that
issue by moving the code directly into generic_remap_file_range_prep?

Something like this folded in:

diff --git a/fs/read_write.c b/fs/read_write.c
index 37a7d3fe35d8..6de813cf9e63 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1752,30 +1752,6 @@ static int generic_remap_check_len(struct inode *inode_in,
 	return (remap_flags & REMAP_FILE_DEDUP) ? -EBADE : -EINVAL;
 }
 
-/* Update inode timestamps and remove security privileges when remapping. */
-static int generic_remap_file_range_target(struct file *file,
-					   unsigned int remap_flags)
-{
-	int ret;
-
-	/* If can't alter the file contents, we're done. */
-	if (remap_flags & REMAP_FILE_DEDUP)
-		return 0;
-
-	/* Update the timestamps, since we can alter file contents. */
-	if (!(file->f_mode & FMODE_NOCMTIME)) {
-		ret = file_update_time(file);
-		if (ret)
-			return ret;
-	}
-
-	/*
-	 * Clear the security bits if the process is not being run by root.
-	 * This keeps people from modifying setuid and setgid binaries.
-	 */
-	return file_remove_privs(file);
-}
-
 /*
  * Read a page's worth of file data into the page cache.  Return the page
  * locked.
@@ -1950,9 +1926,25 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
 	if (ret)
 		return ret;
 
-	ret = generic_remap_file_range_target(file_out, remap_flags);
-	if (ret)
-		return ret;
+	if (!(remap_flags & REMAP_FILE_DEDUP)) {
+		/*
+		 * Update the timestamps, since we can alter file contents.
+		 */
+		if (!(file_out->f_mode & FMODE_NOCMTIME)) {
+			ret = file_update_time(file_out);
+			if (ret)
+				return ret;
+		}
+
+		/*
+		 * Clear the security bits if the process is not being run by
+		 * root.  This keeps people from modifying setuid and setgid
+		 * binaries.
+		 */
+		ret = file_remove_privs(file_out);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }