| Message ID | 1553599.1715262072@warthog.procyon.org.uk |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4: Don't reduce symlink i_mode by umask if no ACL support | expand |
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
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
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
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>
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.
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; }
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(-)