erofs: Fix state inconsistency when updating fsid/domain_id

[PATCH v1] erofs: Fix state inconsistency when updating fsid/domain_id

Posted by Baolin Liu 1 month ago

From: Baolin Liu <liubaolin@kylinos.cn>

When updating fsid or domain_id, the code frees the old pointer before
allocating a new one. If allocation fails, the pointer becomes NULL
while the old value is already freed, causing state inconsistency.

Fix by allocating the new value first, and only freeing the old value
on success.

Signed-off-by: Baolin Liu <liubaolin@kylinos.cn>
---
 fs/erofs/super.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 937a215f626c..6e083d7e634c 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -509,16 +509,22 @@ static int erofs_fc_parse_param(struct fs_context *fc,
 		break;
 #ifdef CONFIG_EROFS_FS_ONDEMAND
 	case Opt_fsid:
-		kfree(sbi->fsid);
-		sbi->fsid = kstrdup(param->string, GFP_KERNEL);
-		if (!sbi->fsid)
+		char *new_fsid;
+
+		new_fsid = kstrdup(param->string, GFP_KERNEL);
+		if (!new_fsid)
 			return -ENOMEM;
+		kfree(sbi->fsid);
+		sbi->fsid = new_fsid;
 		break;
 	case Opt_domain_id:
-		kfree(sbi->domain_id);
-		sbi->domain_id = kstrdup(param->string, GFP_KERNEL);
-		if (!sbi->domain_id)
+		char *new_domain_id;
+
+		new_domain_id = kstrdup(param->string, GFP_KERNEL);
+		if (!new_domain_id)
 			return -ENOMEM;
+		kfree(sbi->domain_id);
+		sbi->domain_id = new_domain_id;
 		break;
 #else
 	case Opt_fsid:
-- 
2.39.2

Re: [PATCH v1] erofs: Fix state inconsistency when updating fsid/domain_id

Posted by Hongbo Li 1 month ago

Hi,

On 2026/1/6 10:55, Baolin Liu wrote:
> From: Baolin Liu <liubaolin@kylinos.cn>
> 
> When updating fsid or domain_id, the code frees the old pointer before
> allocating a new one. If allocation fails, the pointer becomes NULL
> while the old value is already freed, causing state inconsistency.
> 
> Fix by allocating the new value first, and only freeing the old value
> on success.
> 
> Signed-off-by: Baolin Liu <liubaolin@kylinos.cn>
> ---
>   fs/erofs/super.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index 937a215f626c..6e083d7e634c 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -509,16 +509,22 @@ static int erofs_fc_parse_param(struct fs_context *fc,
>   		break;
>   #ifdef CONFIG_EROFS_FS_ONDEMAND
>   	case Opt_fsid:
> -		kfree(sbi->fsid);
> -		sbi->fsid = kstrdup(param->string, GFP_KERNEL);
> -		if (!sbi->fsid)
> +		char *new_fsid;
> +
> +		new_fsid = kstrdup(param->string, GFP_KERNEL);

May be there is no need to keep the old pointer. Because
1) The fsid/domain_id is ignored in reconfiguration.
2) Even if memory allocation fails when the user first mounts with multi 
fsid/domain_id options (like -o fsid=xxx1,fsid=xxx2), the old fsid 
pointer would also need to be released in cleanup procedure.

so am I right?

Thanks,
Hongbo

> +		if (!new_fsid)
>   			return -ENOMEM;
> +		kfree(sbi->fsid);
> +		sbi->fsid = new_fsid;
>   		break;
>   	case Opt_domain_id:
> -		kfree(sbi->domain_id);
> -		sbi->domain_id = kstrdup(param->string, GFP_KERNEL);
> -		if (!sbi->domain_id)
> +		char *new_domain_id;
> +
> +		new_domain_id = kstrdup(param->string, GFP_KERNEL);
> +		if (!new_domain_id)
>   			return -ENOMEM;
> +		kfree(sbi->domain_id);
> +		sbi->domain_id = new_domain_id;
>   		break;
>   #else
>   	case Opt_fsid:

Re: [PATCH v1] erofs: Fix state inconsistency when updating fsid/domain_id

Posted by liubaolin 1 month ago

> Dear Hongbo Li,
> 
> I have reviewed this carefully, and I agree with your point. The old value will eventually be freed in erofs_sb_free(), and keeping it here does not appear to be necessary. Therefore, this patch does not need to be considered further.
> 
> Thank you for your review.
> 
> Dear Gao Xiang,
> 
> Thank you for your review as well.
> 
> Best regards,
> Baolin Liu
>
> 

在 2026/1/6 11:30, Hongbo Li 写道:
> Hi,
> 
> On 2026/1/6 10:55, Baolin Liu wrote:
>> From: Baolin Liu <liubaolin@kylinos.cn>
>>
>> When updating fsid or domain_id, the code frees the old pointer before
>> allocating a new one. If allocation fails, the pointer becomes NULL
>> while the old value is already freed, causing state inconsistency.
>>
>> Fix by allocating the new value first, and only freeing the old value
>> on success.
>>
>> Signed-off-by: Baolin Liu <liubaolin@kylinos.cn>
>> ---
>>   fs/erofs/super.c | 18 ++++++++++++------
>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index 937a215f626c..6e083d7e634c 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -509,16 +509,22 @@ static int erofs_fc_parse_param(struct 
>> fs_context *fc,
>>           break;
>>   #ifdef CONFIG_EROFS_FS_ONDEMAND
>>       case Opt_fsid:
>> -        kfree(sbi->fsid);
>> -        sbi->fsid = kstrdup(param->string, GFP_KERNEL);
>> -        if (!sbi->fsid)
>> +        char *new_fsid;
>> +
>> +        new_fsid = kstrdup(param->string, GFP_KERNEL);
> 
> May be there is no need to keep the old pointer. Because
> 1) The fsid/domain_id is ignored in reconfiguration.
> 2) Even if memory allocation fails when the user first mounts with multi 
> fsid/domain_id options (like -o fsid=xxx1,fsid=xxx2), the old fsid 
> pointer would also need to be released in cleanup procedure.
> 
> so am I right?
> 
> Thanks,
> Hongbo
> 
>> +        if (!new_fsid)
>>               return -ENOMEM;
>> +        kfree(sbi->fsid);
>> +        sbi->fsid = new_fsid;
>>           break;
>>       case Opt_domain_id:
>> -        kfree(sbi->domain_id);
>> -        sbi->domain_id = kstrdup(param->string, GFP_KERNEL);
>> -        if (!sbi->domain_id)
>> +        char *new_domain_id;
>> +
>> +        new_domain_id = kstrdup(param->string, GFP_KERNEL);
>> +        if (!new_domain_id)
>>               return -ENOMEM;
>> +        kfree(sbi->domain_id);
>> +        sbi->domain_id = new_domain_id;
>>           break;
>>   #else
>>       case Opt_fsid:

Re: [PATCH v1] erofs: Fix state inconsistency when updating fsid/domain_id

Posted by Gao Xiang 1 month ago

Hi Baolin,

On 2026/1/6 10:55, Baolin Liu wrote:
> From: Baolin Liu <liubaolin@kylinos.cn>
> 
> When updating fsid or domain_id, the code frees the old pointer before
> allocating a new one. If allocation fails, the pointer becomes NULL
> while the old value is already freed, causing state inconsistency.
> 
> Fix by allocating the new value first, and only freeing the old value
> on success.
> 
> Signed-off-by: Baolin Liu <liubaolin@kylinos.cn>
> ---
>   fs/erofs/super.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index 937a215f626c..6e083d7e634c 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -509,16 +509,22 @@ static int erofs_fc_parse_param(struct fs_context *fc,
>   		break;
>   #ifdef CONFIG_EROFS_FS_ONDEMAND
>   	case Opt_fsid:
> -		kfree(sbi->fsid);
> -		sbi->fsid = kstrdup(param->string, GFP_KERNEL);
> -		if (!sbi->fsid)
> +		char *new_fsid;

could you move

char *new_fsid, *new_domain_id;

to the beginning of `erofs_fc_parse_param()`
to avoid variable definitions in the switch statement?

or maybe just call it as:

char *newstr;

Thanks,
Gao Xiang

> +
> +		new_fsid = kstrdup(param->string, GFP_KERNEL);
> +		if (!new_fsid)
>   			return -ENOMEM;
> +		kfree(sbi->fsid);
> +		sbi->fsid = new_fsid;
>   		break;
>   	case Opt_domain_id:
> -		kfree(sbi->domain_id);
> -		sbi->domain_id = kstrdup(param->string, GFP_KERNEL);
> -		if (!sbi->domain_id)
> +		char *new_domain_id;
> +
> +		new_domain_id = kstrdup(param->string, GFP_KERNEL);
> +		if (!new_domain_id)
>   			return -ENOMEM;
> +		kfree(sbi->domain_id);
> +		sbi->domain_id = new_domain_id;
>   		break;
>   #else
>   	case Opt_fsid: