| Message ID | 20240313130708.2915988-1-mmakassikis@freebox.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ksmbd: clear RENAME_NOREPLACE before calling vfs_rename | expand |
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 ?
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!
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
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
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;
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(+)