Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
MIME-Version: 1.0
References: <69dda7be-d7c8-401f-89f3-7a5ca5550e2f@oracle.com>
 <20231009144340.418904-1-max.kellermann@ionos.com> <20231010131125.3uyfkqbcetfcqsve@quack3>
 <CAKPOu+-nC2bQTZYL0XTzJL6Tx4Pi1gLfNWCjU2Qz1f_5CbJc1w@mail.gmail.com> <20231011100541.sfn3prgtmp7hk2oj@quack3>
In-Reply-To: <20231011100541.sfn3prgtmp7hk2oj@quack3>
From:   Max Kellermann <max.kellermann@ionos.com>
Date:   Wed, 11 Oct 2023 12:51:12 +0200
Message-ID: <CAKPOu+_xdFALt9sgdd5w66Ab6KTqiy8+Z0Yd3Ss4+92jh8nCwg@mail.gmail.com>
Subject: Re: [PATCH v2] fs/{posix_acl,ext2,jfs,ceph}: apply umask if ACL
 support is disabled
To:     Jan Kara <jack@suse.cz>
Cc:     Xiubo Li <xiubli@redhat.com>, Ilya Dryomov <idryomov@gmail.com>,
        Jeff Layton <jlayton@kernel.org>, Jan Kara <jack@suse.com>,
        Dave Kleikamp <shaggy@kernel.org>, ceph-devel@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-ext4@vger.kernel.org,
        jfs-discussion@lists.sourceforge.net,
        Christian Brauner <brauner@kernel.org>,
        Yang Xu <xuyang2018.jy@fujitsu.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Oct 11, 2023 at 12:05=E2=80=AFPM Jan Kara <jack@suse.cz> wrote:
> So I've checked some more and the kernel doc comments before
> mode_strip_umask() and vfs_prepare_mode() make it pretty obvious - all
> paths creating new inodes must be calling vfs_prepare_mode(). As a result
> mode_strip_umask() which handles umask stripping for filesystems not
> supporting posix ACLs. For filesystems that do support ACLs,
> posix_acl_create() must be call and that handles umask stripping. So your
> fix should not be needed. CCed some relevant people for confirmation.

Thanks, Jan. Do you think the documentation is obvious enough, or
shall I look around and try to improve the documentation? I'm not a FS
expert, so it may be just my fault that it confused me.... I just
analyzed the O_TMPFILE vulnerability four years ago (because it was
reported to me as the maintainer of a userspace software).

Apart from my doubts that this API contract is too error prone, I'm
not quite sure if all filesystems really implement it properly.

For example, overlayfs unconditionally sets SB_POSIXACL, even if the
kernel has no ACL support. Would this ignore the umask? I'm not sure,
overlayfs is a special beast.
Then there's orangefs which allows setting the "acl" mount option (and
thus SB_POSIXACL) even if the kernel has no ACL support. Same for gfs2
and maybe cifs, maybe more, I didn't check them all.

The "mainstream" filesystems like ext4 seem to be implemented
properly, though this is still too fragile for my taste... ext4 has
the SB_POSIXACL code even if there's no kernel ACL support, but it is
not reachable because EXT4_MOUNT_POSIX_ACL cannot be set from
userspace in that case. The code looks suspicious, but is okay in the
end - still not my taste.

I see so much redundant code regarding the "acl" mount option in all
filesystems. I believe the API should be designed in a way that it is
safe-by-default, and shouldn't need very careful considerations in
each and every filesystem, or else all filesystems repeat the same
mistakes until the last one gets fixed.

Max