| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20260205193349.2227351-1-clm@meta.com>
Date: Thu, 5 Feb 2026 11:32:58 -0800
From: Chris Mason <clm@...a.com>
To: "Darrick J. Wong" <djwong@...nel.org>
CC: <miklos@...redi.hu>, <joannelkoong@...il.com>, <bernd@...ernd.com>,
<neal@...pa.dev>, <linux-ext4@...r.kernel.org>,
<linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH 5/5] fuse: propagate default and file acls on creation
"Darrick J. Wong" <djwong@...nel.org> wrote:
> From: Darrick J. Wong <djwong@...nel.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
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@...nel.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).
> @@ -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/
[ ... ]
Powered by blists - more mailing lists