On Thu, Oct 12, 2023 at 6:37 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> In commit f61b9bb3f838 ("fs: add a new SB_I_NOUMASK flag") we added a
> new SB_I_NOUMASK flag that is used by filesystems like NFS to indicate
> that umask stripping is never supposed to be done in the vfs independent
> of whether or not POSIX ACLs are supported.
>
> Overlayfs falls into the same category as it raises SB_POSIXACL
> unconditionally to defer umask application to the upper filesystem.
>
> Now that we have SB_I_NOUMASK use that and make SB_POSIXACL properly
> conditional on whether or not the kernel does have support for it. This
> will enable use to turn IS_POSIXACL() into nop on kernels that don't
> have POSIX ACL support avoding bugs from missed umask stripping.
>
> Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
> ---
> Hey Amir & Miklos,
>
> This depends on the aforementioned patch in vfs.misc. So if you're fine
> with this change I'd take this through vfs.misc.
Generally, I'm fine with a version of this going through the vfs tree.
>
> Christian
> ---
> fs/overlayfs/super.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 9f43f0d303ad..361189b676b0 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1489,8 +1489,16 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
> sb->s_xattr = ofs->config.userxattr ? ovl_user_xattr_handlers :
> ovl_trusted_xattr_handlers;
> sb->s_fs_info = ofs;
> +#ifdef CONFIG_FS_POSIX_ACL
> sb->s_flags |= SB_POSIXACL;
> +#endif
IDGI, if IS_POSIXACL() is going to turn into noop
why do we need this ifdef?
To me the flag SB_POSIXACL means that any given inode
MAY have a custom posix acl.
> sb->s_iflags |= SB_I_SKIP_SYNC | SB_I_IMA_UNVERIFIABLE_SIGNATURE;
> + /*
> + * Ensure that umask handling is done by the filesystems used
> + * for the the upper layer instead of overlayfs as that would
> + * lead to unexpected results.
> + */
> + sb->s_iflags |= SB_I_NOUMASK;
>
Looks like FUSE has a similar pattern, although the testing and then
setting of SB_POSIXACL is perplexing to me:
/* Handle umasking inside the fuse code */
if (sb->s_flags & SB_POSIXACL)
fc->dont_mask = 1;
sb->s_flags |= SB_POSIXACL;
And for NFS, why was SB_I_NOUMASK set for v4 and not for v3
when v3 clearly states:
case 3:
/*
* The VFS shouldn't apply the umask to mode bits.
* We will do so ourselves when necessary.
*/
sb->s_flags |= SB_POSIXACL;
Feels like I am missing parts of the big picture?
Thanks,
Amir.
