Jan Kara <[email protected]> wrote:
> 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.
I've been debugging why flatpak was always considering its database
corrupted, and found this commit to be the source of the issue.
$ ostree --repo=repo --mode=bare-user-only init
$ mkdir tree && umask 0 && ln -s target tree/symlink && umask 022
$ ostree --repo=repo commit --branch=foo tree/
c508e0564267b376661889b9016f8438bd6d39412078838f78856383fdd8ac2f
$ ostree --repo=repo fsck
Validating refs...
Validating refs in collections...
Enumerating commits...
Verifying content integrity of 1 commit objects...
fsck objects (1/4) [=== ] 25%
error: In commits c508e0564267b376661889b9016f8438bd6d39412078838f78856383fdd8ac2f: fsck content object a6b40a5400ed082fbe067d2c8397aab54046a089768651c392a36db46d24c1cd: Corrupted file object; checksum expected='a6b40a5400ed082fbe067d2c8397aab54046a089768651c392a36db46d24c1cd'
actual='6bdc88f9722f96dbd51735e381f8a1b0e01363e1d7ee2edbb474c091f83c3987'
$
Turns out that symlinks are inheriting umask on my system (which
has CONFIG_EXT4_FS_POSIX_ACL=n):
$ umask 022
$ ln -s target symlink
$ ls -l symlink
lrwxr-xr-x 1 michael michael 6 Mar 13 13:28 symlink -> target
$
Looking at the referenced functions, posix_acl_create() returns
early before applying umask for symlinks, but ext4_init_acl() now
applies the umask unconditionally.
After reverting this commit, it works correctly. I am also unable
to reproduce the mentioned issue with O_TMPFILE after reverting the
commit. It seems that the bug was fixed properly in ac6800e279a2
('fs: Add missing umask strip in vfs_tmpfile'), and all branches
that have this ext4_init_acl patch already had ac6800e279a2 backported.
So I think this patch should be reverted, since the bug was already
fixed and it breaks symlink modes. If not, it should at least be
changed to not to apply the umask to symlinks.
> > commit 484fd6c1de13b336806a967908a927cc0356e312
> > Author: Max Kellermann <[email protected]>
> > 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" <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Max Kellermann <[email protected]>
> > Link: https://lore.kernel.org/r/[email protected]
> > Signed-off-by: Theodore Ts'o <[email protected]>
> >
> > 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 */
On Wed, Mar 13, 2024 at 9:39 PM Michael Forney <[email protected]> wrote:
> Turns out that symlinks are inheriting umask on my system (which
> has CONFIG_EXT4_FS_POSIX_ACL=n):
>
> $ umask 022
> $ ln -s target symlink
> $ ls -l symlink
> lrwxr-xr-x 1 michael michael 6 Mar 13 13:28 symlink -> target
> $
>
> Looking at the referenced functions, posix_acl_create() returns
> early before applying umask for symlinks, but ext4_init_acl() now
> applies the umask unconditionally.
Indeed, I forgot to exclude symlinks from this - sorry for the breakage.
> After reverting this commit, it works correctly. I am also unable
> to reproduce the mentioned issue with O_TMPFILE after reverting the
> commit. It seems that the bug was fixed properly in ac6800e279a2
> ('fs: Add missing umask strip in vfs_tmpfile'), and all branches
> that have this ext4_init_acl patch already had ac6800e279a2 backported.
I can post a patch that adds the missing check or a revert - what do
the FS maintainers prefer?
(There was a bug with O_TMPFILE ignoring umasks years ago - I first
posted the patch in 2018 or so - but by the time my patch actually got
merged, the bug had already been fixed somewhere else IIRC.)
Max