fuse already uses d_splice_alias() to ensure an appropriate dentry is
found for a newly created dentry. Now that ->mkdir can return that
dentry we do so.
This requires changing create_new_entry() to return a dentry and
handling that change in all callers.
Signed-off-by: NeilBrown <neilb@suse.de>
---
fs/fuse/dir.c | 55 +++++++++++++++++++++++++++++++--------------------
1 file changed, 34 insertions(+), 21 deletions(-)
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 5bb65f38bfb8..8c44c9c73c38 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -781,9 +781,9 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
/*
* Code shared between mknod, mkdir, symlink and link
*/
-static int create_new_entry(struct mnt_idmap *idmap, struct fuse_mount *fm,
- struct fuse_args *args, struct inode *dir,
- struct dentry *entry, umode_t mode)
+static struct dentry *create_new_entry(struct mnt_idmap *idmap, struct fuse_mount *fm,
+ struct fuse_args *args, struct inode *dir,
+ struct dentry *entry, umode_t mode)
{
struct fuse_entry_out outarg;
struct inode *inode;
@@ -792,11 +792,11 @@ static int create_new_entry(struct mnt_idmap *idmap, struct fuse_mount *fm,
struct fuse_forget_link *forget;
if (fuse_is_bad(dir))
- return -EIO;
+ return ERR_PTR(-EIO);
forget = fuse_alloc_forget();
if (!forget)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
memset(&outarg, 0, sizeof(outarg));
args->nodeid = get_node_id(dir);
@@ -826,29 +826,27 @@ static int create_new_entry(struct mnt_idmap *idmap, struct fuse_mount *fm,
&outarg.attr, ATTR_TIMEOUT(&outarg), 0, 0);
if (!inode) {
fuse_queue_forget(fm->fc, forget, outarg.nodeid, 1);
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
}
kfree(forget);
d_drop(entry);
d = d_splice_alias(inode, entry);
if (IS_ERR(d))
- return PTR_ERR(d);
+ return d;
- if (d) {
+ if (d)
fuse_change_entry_timeout(d, &outarg);
- dput(d);
- } else {
+ else
fuse_change_entry_timeout(entry, &outarg);
- }
fuse_dir_changed(dir);
- return 0;
+ return d;
out_put_forget_req:
if (err == -EEXIST)
fuse_invalidate_entry(entry);
kfree(forget);
- return err;
+ return ERR_PTR(err);
}
static int fuse_mknod(struct mnt_idmap *idmap, struct inode *dir,
@@ -856,6 +854,7 @@ static int fuse_mknod(struct mnt_idmap *idmap, struct inode *dir,
{
struct fuse_mknod_in inarg;
struct fuse_mount *fm = get_fuse_mount(dir);
+ struct dentry *de;
FUSE_ARGS(args);
if (!fm->fc->dont_mask)
@@ -871,7 +870,12 @@ static int fuse_mknod(struct mnt_idmap *idmap, struct inode *dir,
args.in_args[0].value = &inarg;
args.in_args[1].size = entry->d_name.len + 1;
args.in_args[1].value = entry->d_name.name;
- return create_new_entry(idmap, fm, &args, dir, entry, mode);
+ de = create_new_entry(idmap, fm, &args, dir, entry, mode);
+ if (IS_ERR(de))
+ return PTR_ERR(de);
+ if (de)
+ dput(de);
+ return 0;
}
static int fuse_create(struct mnt_idmap *idmap, struct inode *dir,
@@ -917,7 +921,7 @@ static struct dentry *fuse_mkdir(struct mnt_idmap *idmap, struct inode *dir,
args.in_args[0].value = &inarg;
args.in_args[1].size = entry->d_name.len + 1;
args.in_args[1].value = entry->d_name.name;
- return ERR_PTR(create_new_entry(idmap, fm, &args, dir, entry, S_IFDIR));
+ return create_new_entry(idmap, fm, &args, dir, entry, S_IFDIR);
}
static int fuse_symlink(struct mnt_idmap *idmap, struct inode *dir,
@@ -925,6 +929,7 @@ static int fuse_symlink(struct mnt_idmap *idmap, struct inode *dir,
{
struct fuse_mount *fm = get_fuse_mount(dir);
unsigned len = strlen(link) + 1;
+ struct dentry *de;
FUSE_ARGS(args);
args.opcode = FUSE_SYMLINK;
@@ -934,7 +939,12 @@ static int fuse_symlink(struct mnt_idmap *idmap, struct inode *dir,
args.in_args[1].value = entry->d_name.name;
args.in_args[2].size = len;
args.in_args[2].value = link;
- return create_new_entry(idmap, fm, &args, dir, entry, S_IFLNK);
+ de = create_new_entry(idmap, fm, &args, dir, entry, S_IFLNK);
+ if (IS_ERR(de))
+ return PTR_ERR(de);
+ if (de)
+ dput(de);
+ return 0;
}
void fuse_flush_time_update(struct inode *inode)
@@ -1117,7 +1127,7 @@ static int fuse_rename2(struct mnt_idmap *idmap, struct inode *olddir,
static int fuse_link(struct dentry *entry, struct inode *newdir,
struct dentry *newent)
{
- int err;
+ struct dentry *de;
struct fuse_link_in inarg;
struct inode *inode = d_inode(entry);
struct fuse_mount *fm = get_fuse_mount(inode);
@@ -1131,13 +1141,16 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
args.in_args[0].value = &inarg;
args.in_args[1].size = newent->d_name.len + 1;
args.in_args[1].value = newent->d_name.name;
- err = create_new_entry(&invalid_mnt_idmap, fm, &args, newdir, newent, inode->i_mode);
- if (!err)
+ de = create_new_entry(&invalid_mnt_idmap, fm, &args, newdir, newent, inode->i_mode);
+ if (!IS_ERR(de)) {
+ if (de)
+ dput(de);
+ de = NULL;
fuse_update_ctime_in_cache(inode);
- else if (err == -EINTR)
+ } else if (PTR_ERR(de) == -EINTR)
fuse_invalidate_attr(inode);
- return err;
+ return PTR_ERR(de);
}
static void fuse_fillattr(struct mnt_idmap *idmap, struct inode *inode,
--
2.47.1On Fri, Feb 21, 2025 at 10:36:33AM +1100, NeilBrown wrote:
> @@ -871,7 +870,12 @@ static int fuse_mknod(struct mnt_idmap *idmap, struct inode *dir,
> args.in_args[0].value = &inarg;
> args.in_args[1].size = entry->d_name.len + 1;
> args.in_args[1].value = entry->d_name.name;
> - return create_new_entry(idmap, fm, &args, dir, entry, mode);
> + de = create_new_entry(idmap, fm, &args, dir, entry, mode);
> + if (IS_ERR(de))
> + return PTR_ERR(de);
> + if (de)
> + dput(de);
> + return 0;
Can that really happen?
> @@ -934,7 +939,12 @@ static int fuse_symlink(struct mnt_idmap *idmap, struct inode *dir,
> args.in_args[1].value = entry->d_name.name;
> args.in_args[2].size = len;
> args.in_args[2].value = link;
> - return create_new_entry(idmap, fm, &args, dir, entry, S_IFLNK);
> + de = create_new_entry(idmap, fm, &args, dir, entry, S_IFLNK);
> + if (IS_ERR(de))
> + return PTR_ERR(de);
> + if (de)
> + dput(de);
> + return 0;
Same question.
> + de = create_new_entry(&invalid_mnt_idmap, fm, &args, newdir, newent, inode->i_mode);
> + if (!IS_ERR(de)) {
> + if (de)
> + dput(de);
> + de = NULL;
Whoa... Details, please. What's going on here?
On Sat, 22 Feb 2025, Al Viro wrote:
> On Fri, Feb 21, 2025 at 10:36:33AM +1100, NeilBrown wrote:
>
> > @@ -871,7 +870,12 @@ static int fuse_mknod(struct mnt_idmap *idmap, struct inode *dir,
> > args.in_args[0].value = &inarg;
> > args.in_args[1].size = entry->d_name.len + 1;
> > args.in_args[1].value = entry->d_name.name;
> > - return create_new_entry(idmap, fm, &args, dir, entry, mode);
> > + de = create_new_entry(idmap, fm, &args, dir, entry, mode);
> > + if (IS_ERR(de))
> > + return PTR_ERR(de);
> > + if (de)
> > + dput(de);
> > + return 0;
>
> Can that really happen?
Probably now. It would require S_IFDIR to be passed in the mode to
vfs_mknod(). I don't think any current callers do that, but I don't see
any code in vfs_mknod() to prevent it.
>
> > @@ -934,7 +939,12 @@ static int fuse_symlink(struct mnt_idmap *idmap, struct inode *dir,
> > args.in_args[1].value = entry->d_name.name;
> > args.in_args[2].size = len;
> > args.in_args[2].value = link;
> > - return create_new_entry(idmap, fm, &args, dir, entry, S_IFLNK);
> > + de = create_new_entry(idmap, fm, &args, dir, entry, S_IFLNK);
> > + if (IS_ERR(de))
> > + return PTR_ERR(de);
> > + if (de)
> > + dput(de);
> > + return 0;
>
> Same question.
That definitely cannot happen. - because we *know* that d_splice_alias()
never returns a dentry for any but an S_IFDIR inode (how might we
explain that to the rust type system I wonder :-).
I was going for "obviously correct" without try to optimise, but you are
correct that testing for a non-NULL non-ERR dentry should be optimsed
away as impossible in all cases except mkdir.
Thanks,
NeilBrown
>
> > + de = create_new_entry(&invalid_mnt_idmap, fm, &args, newdir, newent, inode->i_mode);
> > + if (!IS_ERR(de)) {
> > + if (de)
> > + dput(de);
> > + de = NULL;
>
> Whoa... Details, please. What's going on here?
>
On Mon, Feb 24, 2025 at 01:26:18PM +1100, NeilBrown wrote: > Probably now. It would require S_IFDIR to be passed in the mode to > vfs_mknod(). I don't think any current callers do that, but I don't see > any code in vfs_mknod() to prevent it. Not allowed (and that's caller's responsibility to enforce). Local filesystems would break horribly if that ever happened. ->mknod() instance _may_ be a convenient helper for ->mkdir() et.al. to call, but even for ramfs it won't coincide with ->mkdir() (wrong i_nlink, for one thing). If that's not documented, it really should be. vfs_mknod() may be called for block devices, character devices, FIFOs and sockets. Nothing else is allowed.
On Fri, 2025-02-21 at 10:36 +1100, NeilBrown wrote:
> fuse already uses d_splice_alias() to ensure an appropriate dentry is
> found for a newly created dentry. Now that ->mkdir can return that
> dentry we do so.
>
> This requires changing create_new_entry() to return a dentry and
> handling that change in all callers.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> fs/fuse/dir.c | 55 +++++++++++++++++++++++++++++++--------------------
> 1 file changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 5bb65f38bfb8..8c44c9c73c38 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -781,9 +781,9 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> /*
> * Code shared between mknod, mkdir, symlink and link
> */
> -static int create_new_entry(struct mnt_idmap *idmap, struct fuse_mount *fm,
> - struct fuse_args *args, struct inode *dir,
> - struct dentry *entry, umode_t mode)
> +static struct dentry *create_new_entry(struct mnt_idmap *idmap, struct fuse_mount *fm,
> + struct fuse_args *args, struct inode *dir,
> + struct dentry *entry, umode_t mode)
> {
> struct fuse_entry_out outarg;
> struct inode *inode;
> @@ -792,11 +792,11 @@ static int create_new_entry(struct mnt_idmap *idmap, struct fuse_mount *fm,
> struct fuse_forget_link *forget;
>
> if (fuse_is_bad(dir))
> - return -EIO;
> + return ERR_PTR(-EIO);
>
> forget = fuse_alloc_forget();
> if (!forget)
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
>
> memset(&outarg, 0, sizeof(outarg));
> args->nodeid = get_node_id(dir);
> @@ -826,29 +826,27 @@ static int create_new_entry(struct mnt_idmap *idmap, struct fuse_mount *fm,
> &outarg.attr, ATTR_TIMEOUT(&outarg), 0, 0);
> if (!inode) {
> fuse_queue_forget(fm->fc, forget, outarg.nodeid, 1);
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
> }
> kfree(forget);
>
> d_drop(entry);
> d = d_splice_alias(inode, entry);
> if (IS_ERR(d))
> - return PTR_ERR(d);
> + return d;
>
> - if (d) {
> + if (d)
> fuse_change_entry_timeout(d, &outarg);
> - dput(d);
> - } else {
> + else
> fuse_change_entry_timeout(entry, &outarg);
> - }
> fuse_dir_changed(dir);
> - return 0;
> + return d;
>
> out_put_forget_req:
> if (err == -EEXIST)
> fuse_invalidate_entry(entry);
> kfree(forget);
> - return err;
> + return ERR_PTR(err);
> }
>
> static int fuse_mknod(struct mnt_idmap *idmap, struct inode *dir,
> @@ -856,6 +854,7 @@ static int fuse_mknod(struct mnt_idmap *idmap, struct inode *dir,
> {
> struct fuse_mknod_in inarg;
> struct fuse_mount *fm = get_fuse_mount(dir);
> + struct dentry *de;
> FUSE_ARGS(args);
>
> if (!fm->fc->dont_mask)
> @@ -871,7 +870,12 @@ static int fuse_mknod(struct mnt_idmap *idmap, struct inode *dir,
> args.in_args[0].value = &inarg;
> args.in_args[1].size = entry->d_name.len + 1;
> args.in_args[1].value = entry->d_name.name;
> - return create_new_entry(idmap, fm, &args, dir, entry, mode);
> + de = create_new_entry(idmap, fm, &args, dir, entry, mode);
> + if (IS_ERR(de))
> + return PTR_ERR(de);
> + if (de)
> + dput(de);
> + return 0;
> }
>
> static int fuse_create(struct mnt_idmap *idmap, struct inode *dir,
> @@ -917,7 +921,7 @@ static struct dentry *fuse_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> args.in_args[0].value = &inarg;
> args.in_args[1].size = entry->d_name.len + 1;
> args.in_args[1].value = entry->d_name.name;
> - return ERR_PTR(create_new_entry(idmap, fm, &args, dir, entry, S_IFDIR));
> + return create_new_entry(idmap, fm, &args, dir, entry, S_IFDIR);
> }
>
> static int fuse_symlink(struct mnt_idmap *idmap, struct inode *dir,
> @@ -925,6 +929,7 @@ static int fuse_symlink(struct mnt_idmap *idmap, struct inode *dir,
> {
> struct fuse_mount *fm = get_fuse_mount(dir);
> unsigned len = strlen(link) + 1;
> + struct dentry *de;
> FUSE_ARGS(args);
>
> args.opcode = FUSE_SYMLINK;
> @@ -934,7 +939,12 @@ static int fuse_symlink(struct mnt_idmap *idmap, struct inode *dir,
> args.in_args[1].value = entry->d_name.name;
> args.in_args[2].size = len;
> args.in_args[2].value = link;
> - return create_new_entry(idmap, fm, &args, dir, entry, S_IFLNK);
> + de = create_new_entry(idmap, fm, &args, dir, entry, S_IFLNK);
> + if (IS_ERR(de))
> + return PTR_ERR(de);
> + if (de)
> + dput(de);
> + return 0;
> }
>
> void fuse_flush_time_update(struct inode *inode)
> @@ -1117,7 +1127,7 @@ static int fuse_rename2(struct mnt_idmap *idmap, struct inode *olddir,
> static int fuse_link(struct dentry *entry, struct inode *newdir,
> struct dentry *newent)
> {
> - int err;
> + struct dentry *de;
> struct fuse_link_in inarg;
> struct inode *inode = d_inode(entry);
> struct fuse_mount *fm = get_fuse_mount(inode);
> @@ -1131,13 +1141,16 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
> args.in_args[0].value = &inarg;
> args.in_args[1].size = newent->d_name.len + 1;
> args.in_args[1].value = newent->d_name.name;
> - err = create_new_entry(&invalid_mnt_idmap, fm, &args, newdir, newent, inode->i_mode);
> - if (!err)
> + de = create_new_entry(&invalid_mnt_idmap, fm, &args, newdir, newent, inode->i_mode);
> + if (!IS_ERR(de)) {
> + if (de)
> + dput(de);
> + de = NULL;
> fuse_update_ctime_in_cache(inode);
> - else if (err == -EINTR)
> + } else if (PTR_ERR(de) == -EINTR)
> fuse_invalidate_attr(inode);
>
> - return err;
> + return PTR_ERR(de);
> }
>
> static void fuse_fillattr(struct mnt_idmap *idmap, struct inode *inode,
Pretty straightforward.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
© 2016 - 2025 Red Hat, Inc.