[PATCH 0/2] ext4: mark FC as ineligible using an handle

[PATCH 0/2] ext4: mark FC as ineligible using an handle

[Luis Henriques (SUSE)]

Hi!

The following patches were suggested by Jan Kara[1] some time ago.  Basically,
they try to address a race that exists in several places, where function
ext4_fc_mark_ineligible() is called with a NULL handle: in these cases, marking
the filesystem as ineligible may effectively happen _after_ the fast-commit is
done.

There are several places where this happen, and the two patches that follow try
to address some of them.  The first patch fixes the calls that exist in
__track_dentry_update().

The second patch _partially_ fixes the call in ext4_xattr_set() -- this is
because it may fail to obtain an handle.  And, to be honest, I'm not sure what
to do in this case: try again to obtain a new handle just for marking the fc,
with a lower value for credits? or leave it as-is, with the NULL?

The other two missing callers are the ioctl for filesystem resize and
ext4_evict_inode().

[1] https://lore.kernel.org/all/20240724101504.e2t4pvgw6td7rrmm@quack3/

Luis Henriques (SUSE) (2):
  ext4: use handle to mark fc as ineligible in __track_dentry_update()
  ext4: mark fc as ineligible using an handle in ext4_xattr_set()

 fs/ext4/fast_commit.c | 19 +++++++++++--------
 fs/ext4/xattr.c       |  5 ++++-
 2 files changed, 15 insertions(+), 9 deletions(-)

[PATCH 1/2] ext4: use handle to mark fc as ineligible in __track_dentry_update()

[Luis Henriques (SUSE)]

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 fixes the calls to this function in
__track_dentry_update() by adding an extra parameter to the callback used in
ext4_fc_track_template().

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
---
 fs/ext4/fast_commit.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index 3926a05eceee..c330efd771d1 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -372,7 +372,7 @@ void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handl
  */
 static int ext4_fc_track_template(
 	handle_t *handle, struct inode *inode,
-	int (*__fc_track_fn)(struct inode *, void *, bool),
+	int (*__fc_track_fn)(handle_t *handle, struct inode *, void *, bool),
 	void *args, int enqueue)
 {
 	bool update = false;
@@ -389,7 +389,7 @@ static int ext4_fc_track_template(
 		ext4_fc_reset_inode(inode);
 		ei->i_sync_tid = tid;
 	}
-	ret = __fc_track_fn(inode, args, update);
+	ret = __fc_track_fn(handle, inode, args, update);
 	mutex_unlock(&ei->i_fc_lock);
 
 	if (!enqueue)
@@ -413,7 +413,8 @@ struct __track_dentry_update_args {
 };
 
 /* __track_fn for directory entry updates. Called with ei->i_fc_lock. */
-static int __track_dentry_update(struct inode *inode, void *arg, bool update)
+static int __track_dentry_update(handle_t *handle, struct inode *inode,
+				 void *arg, bool update)
 {
 	struct ext4_fc_dentry_update *node;
 	struct ext4_inode_info *ei = EXT4_I(inode);
@@ -428,14 +429,14 @@ static int __track_dentry_update(struct inode *inode, void *arg, bool update)
 
 	if (IS_ENCRYPTED(dir)) {
 		ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_ENCRYPTED_FILENAME,
-					NULL);
+					handle);
 		mutex_lock(&ei->i_fc_lock);
 		return -EOPNOTSUPP;
 	}
 
 	node = kmem_cache_alloc(ext4_fc_dentry_cachep, GFP_NOFS);
 	if (!node) {
-		ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, NULL);
+		ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, handle);
 		mutex_lock(&ei->i_fc_lock);
 		return -ENOMEM;
 	}
@@ -447,7 +448,7 @@ static int __track_dentry_update(struct inode *inode, void *arg, bool update)
 		node->fcd_name.name = kmalloc(dentry->d_name.len, GFP_NOFS);
 		if (!node->fcd_name.name) {
 			kmem_cache_free(ext4_fc_dentry_cachep, node);
-			ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, NULL);
+			ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, handle);
 			mutex_lock(&ei->i_fc_lock);
 			return -ENOMEM;
 		}
@@ -569,7 +570,8 @@ void ext4_fc_track_create(handle_t *handle, struct dentry *dentry)
 }
 
 /* __track_fn for inode tracking */
-static int __track_inode(struct inode *inode, void *arg, bool update)
+static int __track_inode(handle_t *handle, struct inode *inode, void *arg,
+			 bool update)
 {
 	if (update)
 		return -EEXIST;
@@ -607,7 +609,8 @@ struct __track_range_args {
 };
 
 /* __track_fn for tracking data updates */
-static int __track_range(struct inode *inode, void *arg, bool update)
+static int __track_range(handle_t *handle, struct inode *inode, void *arg,
+			 bool update)
 {
 	struct ext4_inode_info *ei = EXT4_I(inode);
 	ext4_lblk_t oldstart;

Re: [PATCH 1/2] ext4: use handle to mark fc as ineligible in __track_dentry_update()

[Jan Kara]

On Thu 19-09-24 10:38:47, 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 fixes the calls to this function in
> __track_dentry_update() by adding an extra parameter to the callback used in
> ext4_fc_track_template().
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>

This looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/fast_commit.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 3926a05eceee..c330efd771d1 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -372,7 +372,7 @@ void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handl
>   */
>  static int ext4_fc_track_template(
>  	handle_t *handle, struct inode *inode,
> -	int (*__fc_track_fn)(struct inode *, void *, bool),
> +	int (*__fc_track_fn)(handle_t *handle, struct inode *, void *, bool),
>  	void *args, int enqueue)
>  {
>  	bool update = false;
> @@ -389,7 +389,7 @@ static int ext4_fc_track_template(
>  		ext4_fc_reset_inode(inode);
>  		ei->i_sync_tid = tid;
>  	}
> -	ret = __fc_track_fn(inode, args, update);
> +	ret = __fc_track_fn(handle, inode, args, update);
>  	mutex_unlock(&ei->i_fc_lock);
>  
>  	if (!enqueue)
> @@ -413,7 +413,8 @@ struct __track_dentry_update_args {
>  };
>  
>  /* __track_fn for directory entry updates. Called with ei->i_fc_lock. */
> -static int __track_dentry_update(struct inode *inode, void *arg, bool update)
> +static int __track_dentry_update(handle_t *handle, struct inode *inode,
> +				 void *arg, bool update)
>  {
>  	struct ext4_fc_dentry_update *node;
>  	struct ext4_inode_info *ei = EXT4_I(inode);
> @@ -428,14 +429,14 @@ static int __track_dentry_update(struct inode *inode, void *arg, bool update)
>  
>  	if (IS_ENCRYPTED(dir)) {
>  		ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_ENCRYPTED_FILENAME,
> -					NULL);
> +					handle);
>  		mutex_lock(&ei->i_fc_lock);
>  		return -EOPNOTSUPP;
>  	}
>  
>  	node = kmem_cache_alloc(ext4_fc_dentry_cachep, GFP_NOFS);
>  	if (!node) {
> -		ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, NULL);
> +		ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, handle);
>  		mutex_lock(&ei->i_fc_lock);
>  		return -ENOMEM;
>  	}
> @@ -447,7 +448,7 @@ static int __track_dentry_update(struct inode *inode, void *arg, bool update)
>  		node->fcd_name.name = kmalloc(dentry->d_name.len, GFP_NOFS);
>  		if (!node->fcd_name.name) {
>  			kmem_cache_free(ext4_fc_dentry_cachep, node);
> -			ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, NULL);
> +			ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_NOMEM, handle);
>  			mutex_lock(&ei->i_fc_lock);
>  			return -ENOMEM;
>  		}
> @@ -569,7 +570,8 @@ void ext4_fc_track_create(handle_t *handle, struct dentry *dentry)
>  }
>  
>  /* __track_fn for inode tracking */
> -static int __track_inode(struct inode *inode, void *arg, bool update)
> +static int __track_inode(handle_t *handle, struct inode *inode, void *arg,
> +			 bool update)
>  {
>  	if (update)
>  		return -EEXIST;
> @@ -607,7 +609,8 @@ struct __track_range_args {
>  };
>  
>  /* __track_fn for tracking data updates */
> -static int __track_range(struct inode *inode, void *arg, bool update)
> +static int __track_range(handle_t *handle, struct inode *inode, void *arg,
> +			 bool update)
>  {
>  	struct ext4_inode_info *ei = EXT4_I(inode);
>  	ext4_lblk_t oldstart;
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/2] ext4: mark fc as ineligible using an handle in ext4_xattr_set()

[Luis Henriques (SUSE)]

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

Re: [PATCH 2/2] ext4: mark fc as ineligible using an handle in ext4_xattr_set()

[Jan Kara]

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

Re: [PATCH 2/2] ext4: mark fc as ineligible using an handle in ext4_xattr_set()

[Luis Henriques]

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