Re: [PATCH v2] fs/{posix_acl,ext2,jfs,ceph}: apply umask if ACL support is disabled

[Jan Kara]

On Thu 12-10-23 10:29:18, Theodore Ts'o wrote:
> On Wed, Oct 11, 2023 at 07:26:06PM +0200, Jan Kara wrote:
> > I don't think this is accurate. posix_acl_create() needs unmasked 'mode'
> > because instead of using current_umask() for masking it wants to use
> > whatever is stored in the ACLs as an umask.
> > 
> > So I still think we need to keep umask handling in both posix_acl_create()
> > and vfs_prepare_mode(). But filesystem's only obligation would be to call
> > posix_acl_create() if the inode is IS_POSIXACL. No more caring about when
> > to apply umask and when not based on config or mount options.
> 
> Ah, right, thanks for the clarification.  I *think* the following
> patch in the ext4 dev branch (not yet in Linus's tree, but it should
> be in linux-next) should be harmless, though, right?  And once we get
> the changes in vfs_prepare_mode() we can revert in ext4 --- or do
> folks I think I should just drop it from the ext4 dev branch now?

It definitely does no harm. As you say, you can revert it once the VFS
changes land if you want.

								Honza

> commit 484fd6c1de13b336806a967908a927cc0356e312
> Author: Max Kellermann <max.kellermann@xxxxxxxxx>
> Date:   Tue Sep 19 10:18:23 2023 +0200
> 
>     ext4: apply umask if ACL support is disabled
>     
>     The function ext4_init_acl() calls posix_acl_create() which is
>     responsible for applying the umask.  But without
>     CONFIG_EXT4_FS_POSIX_ACL, ext4_init_acl() is an empty inline function,
>     and nobody applies the umask.
>     
>     This fixes a bug which causes the umask to be ignored with O_TMPFILE
>     on ext4:
>     
>      https://github.com/MusicPlayerDaemon/MPD/issues/558
>      https://bugs.gentoo.org/show_bug.cgi?id=686142#c3
>      https://bugzilla.kernel.org/show_bug.cgi?id=203625
>     
>     Reviewed-by: "J. Bruce Fields" <bfields@xxxxxxxxxx>
>     Cc: stable@xxxxxxxxxxxxxxx
>     Signed-off-by: Max Kellermann <max.kellermann@xxxxxxxxx>
>     Link: https://lore.kernel.org/r/20230919081824.1096619-1-max.kellermann@xxxxxxxxx
>     Signed-off-by: Theodore Ts'o <tytso@xxxxxxx>
> 
> diff --git a/fs/ext4/acl.h b/fs/ext4/acl.h
> index 0c5a79c3b5d4..ef4c19e5f570 100644
> --- a/fs/ext4/acl.h
> +++ b/fs/ext4/acl.h
> @@ -68,6 +68,11 @@ extern int ext4_init_acl(handle_t *, struct inode *, struct inode *);
>  static inline int
>  ext4_init_acl(handle_t *handle, struct inode *inode, struct inode *dir)
>  {
> +	/* usually, the umask is applied by posix_acl_create(), but if
> +	   ext4 ACL support is disabled at compile time, we need to do
> +	   it here, because posix_acl_create() will never be called */
> +	inode->i_mode &= ~current_umask();
> +
>  	return 0;
>  }
>  #endif  /* CONFIG_EXT4_FS_POSIX_ACL */
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR