On Thu, Sep 19 2024, Jan Kara wrote:
> On Thu 19-09-24 10:38:48, Luis Henriques (SUSE) wrote:
>> Calling ext4_fc_mark_ineligible() with a NULL handle is racy and may result
>> in a fast-commit being done before the filesystem is effectively marked as
>> ineligible. This patch reduces the risk of this happening in function
>> ext4_xattr_set() by using an handle if one is available.
>>
>> Suggested-by: Jan Kara <jack@xxxxxxx>
>> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@xxxxxxxxx>
>
> One comment below:
>
>> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
>> index 46ce2f21fef9..dbe4d11cd332 100644
>> --- a/fs/ext4/xattr.c
>> +++ b/fs/ext4/xattr.c
>> @@ -2554,11 +2554,15 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
>> handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits);
>> if (IS_ERR(handle)) {
>> error = PTR_ERR(handle);
>> + ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_XATTR,
>> + NULL);
>
> So when starting a transaction fails:
>
> a) We have a big problem, the journal is aborted so marking fs ineligible
> is moot.
>
> b) We don't set anything and bail with error to userspace so again marking
> fs as ineligible is pointless.
>
> So there's no need to do anything in this case.
Ah! I spent a good amount of time trying to understand if there was a
point marking it as ineligible in that case, but couldn't reach a clear
conclusion. That's why I decided to leave it there. And, hoping to get
some early feedback, that's also why I decided to send these 2 patches
first, because fixing ext4_evict_inode() will require a bit more re-work.
The fix will have to deal with more error paths, and probably the call to
ext4_journal_start() will need to be moved upper in the function.
And, as always, thanks a lot your review, Jan.
Cheers,
--
Luís
> Honza
>
>> } else {
>> int error2;
>>
>> error = ext4_xattr_set_handle(handle, inode, name_index, name,
>> value, value_len, flags);
>> + ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_XATTR,
>> + handle);
>> error2 = ext4_journal_stop(handle);
>> if (error == -ENOSPC &&
>> ext4_should_retry_alloc(sb, &retries))
>> @@ -2566,7 +2570,6 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
>> if (error == 0)
>> error = error2;
>> }
>> - ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_XATTR, NULL);
>>
>> return error;
>> }
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
