[PATCH] ext4: Don't reduce symlink i_mode by umask if no ACL support

[PATCH] ext4: Don't reduce symlink i_mode by umask if no ACL support

[David Howells]

If CONFIG_EXT4_FS_POSIX_ACL=n then the fallback version of ext4_init_acl()
will mask off the umask bits from the new inode's i_mode.  This should not
be done if the inode is a symlink.  If CONFIG_EXT4_FS_POSIX_ACL=y, then we
go through posix_acl_create() instead which does the right thing with
symlinks.

Fix this by making the fallback version of ext4_init_acl() do nothing if
inode is a symlink.

Fixes: 484fd6c1de13 ("ext4: apply umask if ACL support is disabled")
Signed-off-by: David Howells <dhowells@redhat.com>
---
 fs/ext4/acl.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/acl.h b/fs/ext4/acl.h
index ef4c19e5f570..566625286442 100644
--- a/fs/ext4/acl.h
+++ b/fs/ext4/acl.h
@@ -71,7 +71,8 @@ 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();
+	if (!S_ISLNK(inode->i_mode))
+		inode->i_mode &= ~current_umask();
 
 	return 0;
 }

Re: [PATCH] ext4: Don't reduce symlink i_mode by umask if no ACL support

[Miklos Szeredi]

On Thu, 9 May 2024 at 15:41, David Howells <dhowells@redhat.com> wrote:

> diff --git a/fs/ext4/acl.h b/fs/ext4/acl.h
> index ef4c19e5f570..566625286442 100644
> --- a/fs/ext4/acl.h
> +++ b/fs/ext4/acl.h
> @@ -71,7 +71,8 @@ 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();
> +       if (!S_ISLNK(inode->i_mode))
> +               inode->i_mode &= ~current_umask();

I think this should just be removed unconditionally, since the VFS now
takes care of mode masking in vfs_prepare_mode().

Thanks,
Miklos

Re: [PATCH] ext4: Don't reduce symlink i_mode by umask if no ACL support

[David Howells]

Miklos Szeredi <miklos@szeredi.hu> wrote:

> I think this should just be removed unconditionally, since the VFS now
> takes care of mode masking in vfs_prepare_mode().

That works for symlinks because the symlink path doesn't call it?

David

Re: [PATCH] ext4: Don't reduce symlink i_mode by umask if no ACL support

[Miklos Szeredi]

On Thu, 9 May 2024 at 16:08, David Howells <dhowells@redhat.com> wrote:
>
> Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> > I think this should just be removed unconditionally, since the VFS now
> > takes care of mode masking in vfs_prepare_mode().
>
> That works for symlinks because the symlink path doesn't call it?

Yep.

Thanks,
Miklos

Re: [PATCH] ext4: Don't reduce symlink i_mode by umask if no ACL support

[Theodore Ts'o]

On Thu, May 09, 2024 at 03:47:27PM +0200, Miklos Szeredi wrote:
> On Thu, 9 May 2024 at 15:41, David Howells <dhowells@redhat.com> wrote:
> 
> > diff --git a/fs/ext4/acl.h b/fs/ext4/acl.h
> > index ef4c19e5f570..566625286442 100644
> > --- a/fs/ext4/acl.h
> > +++ b/fs/ext4/acl.h
> > @@ -71,7 +71,8 @@ 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();
> > +       if (!S_ISLNK(inode->i_mode))
> > +               inode->i_mode &= ~current_umask();
> 
> I think this should just be removed unconditionally, since the VFS now
> takes care of mode masking in vfs_prepare_mode().

The following is in the ext4 tree:

commit c77194965dd0dcc26f9c1671d2e74e4eb1248af5
Author: Max Kellermann <max.kellermann@ionos.com>
Date:   Fri Mar 15 15:29:56 2024 +0100

    Revert "ext4: apply umask if ACL support is disabled"
    
    This reverts commit 484fd6c1de13b336806a967908a927cc0356e312.  The
    commit caused a regression because now the umask was applied to
    symlinks and the fix is unnecessary because the umask/O_TMPFILE bug
    has been fixed somewhere else already.
    
    Fixes: https://lore.kernel.org/lkml/28DSITL9912E1.2LSZUVTGTO52Q@mforney.org/
    Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
    Reviewed-by: Christian Brauner <brauner@kernel.org>
    Tested-by: Michael Forney <mforney@mforney.org>
    Link: https://lore.kernel.org/r/20240315142956.2420360-1-max.kellermann@ionos.com
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Re: [PATCH] ext4: Don't reduce symlink i_mode by umask if no ACL support

[Christian Brauner]

On Thu, May 09, 2024 at 03:07:17PM +0100, David Howells wrote:
> Miklos Szeredi <miklos@szeredi.hu> wrote:
> 
> > I think this should just be removed unconditionally, since the VFS now
> > takes care of mode masking in vfs_prepare_mode().
> 
> That works for symlinks because the symlink path doesn't call it?

All of the mode handling should now be done correctly in the VFS (see
Miklos reply as well). In general the less fs specific mode handling we
have, the better because we've been bitten by this before.