Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Gabriel Krisman Bertazi <krisman@suse.de>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     linux-ext4@vger.kernel.org, Theodore Ts'o <tytso@mit.edu>,
        linux-fsdevel@vger.kernel.org,
        linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [PATCH 1/3] ext4: reject casefold inode flag without casefold
 feature
In-Reply-To: <20230814192406.GD1171@sol.localdomain> (Eric Biggers's message
        of "Mon, 14 Aug 2023 12:24:06 -0700")
Organization: SUSE
References: <20230814182903.37267-1-ebiggers@kernel.org>
        <20230814182903.37267-2-ebiggers@kernel.org> <87jztx5tle.fsf@suse.de>
        <20230814192406.GD1171@sol.localdomain>
Date:   Mon, 14 Aug 2023 15:52:54 -0400
Message-ID: <877cpx5rl5.fsf@suse.de>
User-Agent: Gnus/5.13 (Gnus v5.13)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Eric Biggers <ebiggers@kernel.org> writes:

> On Mon, Aug 14, 2023 at 03:09:33PM -0400, Gabriel Krisman Bertazi wrote:
>> Eric Biggers <ebiggers@kernel.org> writes:
>> 
>> > From: Eric Biggers <ebiggers@google.com>
>> >
>> > It is invalid for the casefold inode flag to be set without the casefold
>> > superblock feature flag also being set.  e2fsck already considers this
>> > case to be invalid and handles it by offering to clear the casefold flag
>> > on the inode.  __ext4_iget() also already considered this to be invalid,
>> > sort of, but it only got so far as logging an error message; it didn't
>> > actually reject the inode.  Make it reject the inode so that other code
>> > doesn't have to handle this case.  This matches what f2fs does.
>> >
>> > Note: we could check 's_encoding != NULL' instead of
>> > ext4_has_feature_casefold().  This would make the check robust against
>> > the casefold feature being enabled by userspace writing to the page
>> > cache of the mounted block device.  However, it's unsolvable in general
>> > for filesystems to be robust against concurrent writes to the page cache
>> > of the mounted block device.  Though this very particular scenario
>> > involving the casefold feature is solvable, we should not pretend that
>> > we can support this model, so let's just check the casefold feature.
>> > tune2fs already forbids enabling casefold on a mounted filesystem.
>> 
>> just because we can't fix the general issue for the entire filesystem
>> doesn't mean this case *must not* ever be addressed. What is the
>> advantage of making the code less robust against the syzbot code?  Just
>> check sb->s_encoding and be safe later knowing the unicode map is
>> available.
>> 
>
> Just to make sure, it sounds like you agree that the late checks of ->s_encoding
> are not needed and only __ext4_iget() should handle it, right?  That simplifies
> the code so it is obviously beneficial if we can do it.

Yes.  After we get the inode from __ext4_iget, I think it doesn't matter
if the user went behind our back straight to the block device and
changed the superblock to remove the feature bit. If we already loaded
->s_encoding, it won't be unloaded, so only checking at ext4_iget should
be enough, as far as I can tell.


-- 
Gabriel Krisman Bertazi