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@suse.cz>
Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
---
 fs/ext4/xattr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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);
 	} 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;
 }

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@suse.cz>
> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>

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.

								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@suse.com>
SUSE Labs, CR

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@suse.cz>
>> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
>
> 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@suse.com>
> SUSE Labs, CR