[PATCH] ksmbd: clear RENAME_NOREPLACE before calling vfs_rename

[PATCH] ksmbd: clear RENAME_NOREPLACE before calling vfs_rename

[Marios Makassikis]

File overwrite case is explicitly handled, so it is not necessary to
pass RENAME_NOREPLACE to vfs_rename.

Clearing the flag fixes rename operations when the share is a ntfs-3g
mount. The latter uses an older version of fuse with no support for
flags in the ->rename op.

Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
---
 fs/smb/server/vfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
index c487e834331a..d7fbea2ed0bf 100644
--- a/fs/smb/server/vfs.c
+++ b/fs/smb/server/vfs.c
@@ -754,10 +754,13 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
 		goto out4;
 	}
 
+	/* explicitly handle file overwrite case, for compatibility with
+	 * filesystems that may not support rename flags (e.g: fuse) */
 	if ((flags & RENAME_NOREPLACE) && d_is_positive(new_dentry)) {
 		err = -EEXIST;
 		goto out4;
 	}
+	flags &= ~(RENAME_NOREPLACE);
 
 	if (old_child == trap) {
 		err = -EINVAL;
-- 
2.34.1

Re: [PATCH] ksmbd: clear RENAME_NOREPLACE before calling vfs_rename

[Marios Makassikis]

On Wed, Mar 13, 2024 at 2:07 PM Marios Makassikis
<mmakassikis@freebox.fr> wrote:
>
> File overwrite case is explicitly handled, so it is not necessary to
> pass RENAME_NOREPLACE to vfs_rename.
>
> Clearing the flag fixes rename operations when the share is a ntfs-3g
> mount. The latter uses an older version of fuse with no support for
> flags in the ->rename op.
>
> Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
> ---
>  fs/smb/server/vfs.c | 3 +++
>  1 file changed, 3 insertions(+)
>

Bumping this as I haven't received any feedback.
Are there any issues with the patch ?

Re: [PATCH] ksmbd: clear RENAME_NOREPLACE before calling vfs_rename

[Namjae Jeon]

2024년 4월 15일 (월) 오후 6:01, Marios Makassikis <mmakassikis@freebox.fr>님이 작성:
>
> On Wed, Mar 13, 2024 at 2:07 PM Marios Makassikis
> <mmakassikis@freebox.fr> wrote:
> >
> > File overwrite case is explicitly handled, so it is not necessary to
> > pass RENAME_NOREPLACE to vfs_rename.
> >
> > Clearing the flag fixes rename operations when the share is a ntfs-3g
> > mount. The latter uses an older version of fuse with no support for
> > flags in the ->rename op.
> >
> > Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
> > ---
> >  fs/smb/server/vfs.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
>
> Bumping this as I haven't received any feedback.
> Are there any issues with the patch ?
Sorry for missing this patch. Please cc me when submitting the patch
to the list next time.
I didn't understand why it is a problem with ntfs-3g yet.
Is it just clean-up patch ? or this flags cause some issue with ntfs-3g ?
Could you please elaborate more ?

Thanks!

Re: [PATCH] ksmbd: clear RENAME_NOREPLACE before calling vfs_rename

[Marios Makassikis]

On Mon, Apr 15, 2024 at 12:51 PM Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> 2024년 4월 15일 (월) 오후 6:01, Marios Makassikis <mmakassikis@freebox.fr>님이 작성:
> >
> > On Wed, Mar 13, 2024 at 2:07 PM Marios Makassikis
> > <mmakassikis@freebox.fr> wrote:
> > >
> > > File overwrite case is explicitly handled, so it is not necessary to
> > > pass RENAME_NOREPLACE to vfs_rename.
> > >
> > > Clearing the flag fixes rename operations when the share is a ntfs-3g
> > > mount. The latter uses an older version of fuse with no support for
> > > flags in the ->rename op.
> > >
> > > Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
> > > ---
> > >  fs/smb/server/vfs.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> >
> > Bumping this as I haven't received any feedback.
> > Are there any issues with the patch ?
> Sorry for missing this patch. Please cc me when submitting the patch
> to the list next time.
> I didn't understand why it is a problem with ntfs-3g yet.
> Is it just clean-up patch ? or this flags cause some issue with ntfs-3g ?
> Could you please elaborate more ?
>
> Thanks!

Until commit 74d7970febf ("ksmbd: fix racy issue from using ->d_parent
and ->d_name"),
the logic to overwrite a file or fail depending on the ReplaceIfExists
flag was open-coded.
This is the same as calling vfs_rename() with the RENAME_NOREPLACE flag, so it
makes sense to use that instead.

When using FUSE, the behaviour depends on the userland application implementing
the fs. On the kernel side, this is the function that ends up being called:

fs/fuse/dir.c:
static int fuse_rename2(struct mnt_idmap *idmap, struct inode *olddir,
                        struct dentry *oldent, struct inode *newdir,
                        struct dentry *newent, unsigned int flags)
{
        struct fuse_conn *fc = get_fuse_conn(olddir);
        int err;

        if (fuse_is_bad(olddir))
                return -EIO;

        if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE | RENAME_WHITEOUT))
                return -EINVAL;

        if (flags) {
                if (fc->no_rename2 || fc->minor < 23)
                        return -EINVAL;

                err = fuse_rename_common(olddir, oldent, newdir, newent, flags,
                                         FUSE_RENAME2,
                                         sizeof(struct fuse_rename2_in));
                if (err == -ENOSYS) {
                        fc->no_rename2 = 1;
                        err = -EINVAL;
                }
        } else {
                err = fuse_rename_common(olddir, oldent, newdir, newent, 0,
                                         FUSE_RENAME,
                                         sizeof(struct fuse_rename_in));
        }

        return err;
}

Because ntfs-3g uses an older version of the FUSE API and flags are
passed by ksmbd,
rename attempts fail because of this bit:

        if (flags) {
                if (fc->no_rename2 || fc->minor < 23)
                        return -EINVAL;

ksmbd already handles the overwrite case before even calling
vfs_rename(). So passing
the flag doesn't add much.

--
Marios

Re: [PATCH] ksmbd: clear RENAME_NOREPLACE before calling vfs_rename

[Namjae Jeon]

2024년 4월 15일 (월) 오후 9:36, Marios Makassikis <mmakassikis@freebox.fr>님이 작성:
>
> On Mon, Apr 15, 2024 at 12:51 PM Namjae Jeon <linkinjeon@kernel.org> wrote:
> >
> > 2024년 4월 15일 (월) 오후 6:01, Marios Makassikis <mmakassikis@freebox.fr>님이 작성:
> > >
> > > On Wed, Mar 13, 2024 at 2:07 PM Marios Makassikis
> > > <mmakassikis@freebox.fr> wrote:
> > > >
> > > > File overwrite case is explicitly handled, so it is not necessary to
> > > > pass RENAME_NOREPLACE to vfs_rename.
> > > >
> > > > Clearing the flag fixes rename operations when the share is a ntfs-3g
> > > > mount. The latter uses an older version of fuse with no support for
> > > > flags in the ->rename op.
> > > >
> > > > Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
> > > > ---
> > > >  fs/smb/server/vfs.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > >
> > > Bumping this as I haven't received any feedback.
> > > Are there any issues with the patch ?
> > Sorry for missing this patch. Please cc me when submitting the patch
> > to the list next time.
> > I didn't understand why it is a problem with ntfs-3g yet.
> > Is it just clean-up patch ? or this flags cause some issue with ntfs-3g ?
> > Could you please elaborate more ?
> >
> > Thanks!
>
> Until commit 74d7970febf ("ksmbd: fix racy issue from using ->d_parent
> and ->d_name"),
> the logic to overwrite a file or fail depending on the ReplaceIfExists
> flag was open-coded.
> This is the same as calling vfs_rename() with the RENAME_NOREPLACE flag, so it
> makes sense to use that instead.
>
> When using FUSE, the behaviour depends on the userland application implementing
> the fs. On the kernel side, this is the function that ends up being called:
>
> fs/fuse/dir.c:
> static int fuse_rename2(struct mnt_idmap *idmap, struct inode *olddir,
>                         struct dentry *oldent, struct inode *newdir,
>                         struct dentry *newent, unsigned int flags)
> {
>         struct fuse_conn *fc = get_fuse_conn(olddir);
>         int err;
>
>         if (fuse_is_bad(olddir))
>                 return -EIO;
>
>         if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE | RENAME_WHITEOUT))
>                 return -EINVAL;
>
>         if (flags) {
>                 if (fc->no_rename2 || fc->minor < 23)
>                         return -EINVAL;
>
>                 err = fuse_rename_common(olddir, oldent, newdir, newent, flags,
>                                          FUSE_RENAME2,
>                                          sizeof(struct fuse_rename2_in));
>                 if (err == -ENOSYS) {
>                         fc->no_rename2 = 1;
>                         err = -EINVAL;
>                 }
>         } else {
>                 err = fuse_rename_common(olddir, oldent, newdir, newent, 0,
>                                          FUSE_RENAME,
>                                          sizeof(struct fuse_rename_in));
>         }
>
>         return err;
> }
>
> Because ntfs-3g uses an older version of the FUSE API and flags are
> passed by ksmbd,
> rename attempts fail because of this bit:
>
>         if (flags) {
>                 if (fc->no_rename2 || fc->minor < 23)
>                         return -EINVAL;
>
> ksmbd already handles the overwrite case before even calling
> vfs_rename(). So passing
> the flag doesn't add much.
Okay, Thanks for your detailed explanation:)

Can you fix a warning from checkpatch.pl ?

WARNING: Block comments use a trailing */ on a separate line
#123: FILE: fs/smb/server/vfs.c:758:
+ * filesystems that may not support rename flags (e.g: fuse) */

total: 0 errors, 1 warnings, 13 lines checked

Thanks.

>
> --
> Marios

[PATCH v2] ksmbd: clear RENAME_NOREPLACE before calling vfs_rename

[Marios Makassikis]

File overwrite case is explicitly handled, so it is not necessary to
pass RENAME_NOREPLACE to vfs_rename.

Clearing the flag fixes rename operations when the share is a ntfs-3g
mount. The latter uses an older version of fuse with no support for
flags in the ->rename op.

Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
---
v2 change:

fix checkpatch warning:
  WARNING: Block comments use a trailing */ on a separate line

 fs/smb/server/vfs.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
index 22f0f3db3ac9..51b1b0bed616 100644
--- a/fs/smb/server/vfs.c
+++ b/fs/smb/server/vfs.c
@@ -754,10 +754,15 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
 		goto out4;
 	}
 
+	/*
+	 * explicitly handle file overwrite case, for compatibility with
+	 * filesystems that may not support rename flags (e.g: fuse)
+	 */
 	if ((flags & RENAME_NOREPLACE) && d_is_positive(new_dentry)) {
 		err = -EEXIST;
 		goto out4;
 	}
+	flags &= ~(RENAME_NOREPLACE);
 
 	if (old_child == trap) {
 		err = -EINVAL;
-- 
2.34.1

Re: [PATCH v2] ksmbd: clear RENAME_NOREPLACE before calling vfs_rename

[Namjae Jeon]

2024년 4월 15일 (월) 오후 10:13, Marios Makassikis <mmakassikis@freebox.fr>님이 작성:
>
> File overwrite case is explicitly handled, so it is not necessary to
> pass RENAME_NOREPLACE to vfs_rename.
>
> Clearing the flag fixes rename operations when the share is a ntfs-3g
> mount. The latter uses an older version of fuse with no support for
> flags in the ->rename op.
>
> Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
Applied it to #ksmbd-for-next-next.
Thanks for your patch!