On Tue, Nov 21, 2023 at 5:10 PM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > 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. > Right. Good catch! Thanks, Amir.
