linux-kernel - Re: [PATCH 2/2] ext4: mark fc as ineligible using an handle in ext4_xattr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87msk23bwu.fsf@linux.dev>
Date: Fri, 20 Sep 2024 10:35:45 +0100
From: Luis Henriques <luis.henriques@...ux.dev>
To: Jan Kara <jack@...e.cz>
Cc: Theodore Ts'o <tytso@....edu>,  Andreas Dilger <adilger@...ger.ca>,
  Harshad Shirwadkar <harshadshirwadkar@...il.com>,
  linux-ext4@...r.kernel.org,  linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] ext4: mark fc as ineligible using an handle in
 ext4_xattr_set()

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