On Tue, Nov 14, 2023 at 05:33:13PM +0200, Amir Goldstein wrote: > In vfs code, file_start_write() is usually called after the permission > hook in rw_verify_area(). vfs_dedupe_file_range_one() is an exception > to this rule. > > In vfs_dedupe_file_range_one(), move file_start_write() to after the > the rw_verify_area() checks to make them "start-write-safe". > > This is needed for fanotify "pre content" events. > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > --- > fs/remap_range.c | 32 +++++++++++++------------------- > 1 file changed, 13 insertions(+), 19 deletions(-) > > diff --git a/fs/remap_range.c b/fs/remap_range.c > index 42f79cb2b1b1..de4b09d0ba1d 100644 > --- a/fs/remap_range.c > +++ b/fs/remap_range.c > @@ -445,46 +445,40 @@ loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos, > WARN_ON_ONCE(remap_flags & ~(REMAP_FILE_DEDUP | > REMAP_FILE_CAN_SHORTEN)); > > - ret = mnt_want_write_file(dst_file); > - if (ret) > - return ret; > - > /* > * This is redundant if called from vfs_dedupe_file_range(), but other > * callers need it and it's not performance sesitive... > */ > ret = remap_verify_area(src_file, src_pos, len, false); > if (ret) > - goto out_drop_write; > + return ret; > > ret = remap_verify_area(dst_file, dst_pos, len, true); > if (ret) > - goto out_drop_write; > + return ret; > > - ret = -EPERM; > if (!allow_file_dedupe(dst_file)) > - goto out_drop_write; > + return -EPERM; So that check specifically should come after mnt_want_write_file() because it calls inode_permission() which takes the mount's idmapping into account. And before you hold mnt_want_write_file() the idmapping of the mount can still change. Once you've gotten write access though we tell the anyone trying to change the mount's write-relevant properties to go away. With your changes that check might succeed now but fail later. So please move that check below mnt_want_write_file(). That shouldn't be a problem. Fwiw, for security_file_permission() it doesn't matter because the LSMs don't care about DAC permission - at least not the ones that currently implement the hook. I verified that years ago and just rechecked. If they start caring - which I sincerely hope they don't - then we have to do a bunch of rework anyway to make that work reliably. But I doubt that'll happen or we'll let that happen. While at it, please rename allow_file_dedupe() to may_dedupe_file() so it mirrors our helpers in fs/namei.c.
