Re: [PATCH 5/5] fuse: propagate default and file acls on creation

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Feb 05, 2026 at 11:32:58AM -0800, Chris Mason wrote:
> "Darrick J. Wong" <djwong@kernel.org> wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > For local filesystems, propagate the default and file access ACLs to new
> > children when creating them, just like the other in-kernel local
> > filesystems.
> > 
> 
> Hi everyone,
> 
> I'm trying out my AI review prompts on a few more trees, and I ran it
> on the fuse-iomap-cache branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=fuse-iomap-cache

Yessss some detailed review!  For the first time in my Linux career I'm
getting patch feedback before the patches are even posted!

So, thank you! :D

> This was the latest email lore could find, apologies if I grabbed the wrong one.
> 
> Hopefully the review below is useful, please let me know if there are places it
> has gone wildly wrong.  I kept the header to make it obvious exactly which
> commit I'd run through:
> 
> commit 72f7e38b8ab1d52b7e44818ba7db9335bc499476
> Author: Darrick J. Wong <djwong@kernel.org>
> 
> fuse: propagate default and file acls on creation
> 
> For local filesystems, propagate the default and file access ACLs to new
> children when creating them, just like the other in-kernel local
> filesystems.
> 
> > diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
> > index 72bb4c94079b7..f2cb7e8db4cad 100644
> > --- a/fs/fuse/acl.c
> > +++ b/fs/fuse/acl.c
> 
> [ ... ]
> 
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index b2dd3b189246e..3af81a063cc50 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -821,26 +821,28 @@ static int fuse_create_open(struct mnt_idmap *idmap, struct inode *dir,
> >  	struct fuse_entry_out outentry;
> >  	struct fuse_inode *fi;
> >  	struct fuse_file *ff;
> > +	struct posix_acl *default_acl = NULL, *acl = NULL;
> >  	int epoch, err;
> >  	bool trunc = flags & O_TRUNC;
> >
> >  	/* Userspace expects S_IFREG in create mode */
> >  	BUG_ON((mode & S_IFMT) != S_IFREG);
> >
> > +	err = fuse_acl_create(dir, &mode, &default_acl, &acl);
> > +	if (err)
> > +		return err;
> > +
> >  	epoch = atomic_read(&fm->fc->epoch);
> >  	forget = fuse_alloc_forget();
> >  	err = -ENOMEM;
> >  	if (!forget)
> > -		goto out_err;
> > +		goto out_acl_release;
> >
> >  	err = -ENOMEM;
> >  	ff = fuse_file_alloc(fm, true);
> >  	if (!ff)
> >  		goto out_put_forget_req;
> >
> > -	if (!fm->fc->dont_mask)
> > -		mode &= ~current_umask();
> > -
> >  	flags &= ~O_NOCTTY;
> >  	memset(&inarg, 0, sizeof(inarg));
> >  	memset(&outentry, 0, sizeof(outentry));
> > @@ -892,12 +894,16 @@ static int fuse_create_open(struct mnt_idmap *idmap, struct inode *dir,
> >  		fuse_sync_release(NULL, ff, flags);
> >  		fuse_queue_forget(fm->fc, forget, outentry.nodeid, 1);
> >  		err = -ENOMEM;
> > -		goto out_err;
> > +		goto out_acl_release;
> >  	}
> >  	kfree(forget);
> >  	d_instantiate(entry, inode);
> >  	entry->d_time = epoch;
> >  	fuse_change_entry_timeout(entry, &outentry);
> > +
> > +	err = fuse_init_acls(inode, default_acl, acl);
> > +	if (err)
> > +		goto out_acl_release;
> >  	fuse_dir_changed(dir);
> >  	err = generic_file_open(inode, file);
> >  	if (!err) {
> > @@ -919,7 +925,9 @@ static int fuse_create_open(struct mnt_idmap *idmap, struct inode *dir,
> >  	fuse_file_free(ff);
> >  out_put_forget_req:
> >  	kfree(forget);
> > -out_err:
> > +out_acl_release:
> > +	posix_acl_release(default_acl);
> > +	posix_acl_release(acl);
> >  	return err;
> >  }
> 
> Does this leak the ACL pointers on the success path? When fuse_acl_create()
> allocates non-NULL ACLs (occurs when IS_POSIXACL(dir) &&
> fuse_inode_has_local_acls(dir) and the parent directory has default ACLs),
> and the file creation succeeds, the function returns directly at 'return
> err;' without releasing the ACLs via posix_acl_release().
> 
> The cleanup at out_acl_release is only reached on error paths. This would
> leak the posix_acl structures on every successful file creation when local
> ACLs are enabled.
> 
> Compare with create_new_entry() which correctly calls posix_acl_release()
> on both success and error paths (lines 1056-1057 and 1065-1066).

Oh, yeah, that is a pretty stupid memory leak, thanks for catching it.

> > @@ -979,14 +989,18 @@ static struct dentry *create_new_entry(struct mnt_idmap *idmap, struct fuse_moun
> >  	struct fuse_forget_link *forget;
> >  	int epoch, err;
> >
> > -	if (fuse_is_bad(dir))
> > -		return ERR_PTR(-EIO);
> > +	if (fuse_is_bad(dir)) {
> > +		err = -EIO;
> > +		goto out_acl_release;
> > +	}
> >
> >  	epoch = atomic_read(&fm->fc->epoch);
> >
> >  	forget = fuse_alloc_forget();
> > -	if (!forget)
> > -		return ERR_PTR(-ENOMEM);
> > +	if (!forget) {
> > +		err = -ENOMEM;
> > +		goto out_acl_release;
> > +	}
> >
> >  	memset(&outarg, 0, sizeof(outarg));
> >  	args->nodeid = get_node_id(dir);
> > @@ -1016,7 +1030,8 @@ static struct dentry *create_new_entry(struct mnt_idmap *idmap, struct fuse_moun
> >  			  &outarg.attr, ATTR_TIMEOUT(&outarg), 0, 0);
> >  	if (!inode) {
> >  		fuse_queue_forget(fm->fc, forget, outentry.nodeid, 1);
> > -		return ERR_PTR(-ENOMEM);
> > +		err = -ENOMEM;
> > +		goto out_acl_release;
> >  	}
> >  	kfree(forget);
> >
> > @@ -1032,19 +1047,31 @@ static struct dentry *create_new_entry(struct mnt_idmap *idmap, struct fuse_moun
> >  		entry->d_time = epoch;
> >  		fuse_change_entry_timeout(entry, &outarg);
> >  	}
> > +
> > +	err = fuse_init_acls(inode, default_acl, acl);
> > +	if (err)
> > +		goto out_acl_release;
> >  	fuse_dir_changed(dir);
> > +
> > +	posix_acl_release(default_acl);
> > +	posix_acl_release(acl);
> >  	return d;
> >
> >  out_put_forget_req:
> >  	if (err == -EEXIST)
> >  		fuse_invalidate_entry(entry);
> >  	kfree(forget);
> > + out_acl_release:
> > +	posix_acl_release(default_acl);
> > +	posix_acl_release(acl);
> >  	return ERR_PTR(err);
> >  }
> 
> Chen Linxuan raised a concern in v5 about pairing fuse_acl_create and
> posix_acl_release together. While the author's response explained the
> design choice to have create_new_entry consume the ACL arguments, there is
> a memory leak when d_splice_alias returns an error:
> 
> >  		d = d_splice_alias(inode, entry);
> >  		if (IS_ERR(d))
> >  			return d;
> 
> The function returns ERR_PTR without releasing the ACL pointers. The
> normal path (lines 1056-1057) and error path (lines 1065-1066) both
> release ACLs, but the IS_ERR(d) path at line 1041 does not.
> 
> https://lore.kernel.org/all/CAC1kPDOv4sy3NPexFtdoROFi18b98W+PbP+9t8y4Jd5fQqCxCg@mail.gmail.com/

Whoops.  Also a good catch; I'll change that to:

	d = d_splice_alias(inode, entry);
	if (IS_ERR(d)) {
		err = PTR_ERR(d);
		goto out_acl_release;
	}

--D

> [ ... ]
> 
>