[f2fs-dev] [PATCH] f2fs: extend stat_lock to avoid potential race in statfs

[f2fs-dev] [PATCH] f2fs: extend stat_lock to avoid potential race in statfs

[Niels Dossche]

The calculation for f_bfree performs a calculation involving
current_reserved_blocks and total_valid_user_blocks. Both can be
modified under stat_lock. As stat_lock is not used to read both these
values in statfs, this can lead to inconsistent results. Extend the
locking to prevent this issue.
Commit c9c8ed50d94c ("f2fs: fix to avoid potential race on sbi->unusable_block_count access/update")
already added the use of sbi->stat_lock in statfs in
order to make the calculation of multiple, different fields atomic so
that results are consistent. This is similar to that patch regarding the
change in statfs.

Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
---

Note:
I am currently working on a static analyser to detect missing locks
using type-based static analysis as my master's thesis
in order to obtain my master's degree.
If you would like to have more details, please let me know.
This was a reported case. I manually verified the report by looking
at the code, so that I do not send wrong information or patches.
After concluding that this seems to be a true positive, I created
this patch. This was compile-tested and runtime-tested on x86_64.
This issue was found on Linux v5.17.4.

 fs/f2fs/super.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index ea939db18f88..ece768869187 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1739,10 +1739,12 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	buf->f_bsize = sbi->blocksize;
 
 	buf->f_blocks = total_count - start_count;
+
+	spin_lock(&sbi->stat_lock);
+
 	buf->f_bfree = user_block_count - valid_user_blocks(sbi) -
 						sbi->current_reserved_blocks;
 
-	spin_lock(&sbi->stat_lock);
 	if (unlikely(buf->f_bfree <= sbi->unusable_block_count))
 		buf->f_bfree = 0;
 	else
-- 
2.35.2



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: extend stat_lock to avoid potential race in statfs

[Jaegeuk Kim]

On 04/21, Niels Dossche wrote:
> The calculation for f_bfree performs a calculation involving
> current_reserved_blocks and total_valid_user_blocks. Both can be
> modified under stat_lock. As stat_lock is not used to read both these
> values in statfs, this can lead to inconsistent results. Extend the
> locking to prevent this issue.
> Commit c9c8ed50d94c ("f2fs: fix to avoid potential race on sbi->unusable_block_count access/update")
> already added the use of sbi->stat_lock in statfs in
> order to make the calculation of multiple, different fields atomic so
> that results are consistent. This is similar to that patch regarding the
> change in statfs.

Is this enough? It seems we also need to cover sbi->user_block_count and
sbi->total_node_count by stat_lock.

> 
> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
> ---
> 
> Note:
> I am currently working on a static analyser to detect missing locks
> using type-based static analysis as my master's thesis
> in order to obtain my master's degree.
> If you would like to have more details, please let me know.
> This was a reported case. I manually verified the report by looking
> at the code, so that I do not send wrong information or patches.
> After concluding that this seems to be a true positive, I created
> this patch. This was compile-tested and runtime-tested on x86_64.
> This issue was found on Linux v5.17.4.
> 
>  fs/f2fs/super.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index ea939db18f88..ece768869187 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1739,10 +1739,12 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
>  	buf->f_bsize = sbi->blocksize;
>  
>  	buf->f_blocks = total_count - start_count;
> +
> +	spin_lock(&sbi->stat_lock);
> +
>  	buf->f_bfree = user_block_count - valid_user_blocks(sbi) -
>  						sbi->current_reserved_blocks;
>  
> -	spin_lock(&sbi->stat_lock);
>  	if (unlikely(buf->f_bfree <= sbi->unusable_block_count))
>  		buf->f_bfree = 0;
>  	else
> -- 
> 2.35.2


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: extend stat_lock to avoid potential race in statfs

[Niels Dossche]

On 4/22/22 19:06, Jaegeuk Kim wrote:
> On 04/21, Niels Dossche wrote:
>> The calculation for f_bfree performs a calculation involving
>> current_reserved_blocks and total_valid_user_blocks. Both can be
>> modified under stat_lock. As stat_lock is not used to read both these
>> values in statfs, this can lead to inconsistent results. Extend the
>> locking to prevent this issue.
>> Commit c9c8ed50d94c ("f2fs: fix to avoid potential race on sbi->unusable_block_count access/update")
>> already added the use of sbi->stat_lock in statfs in
>> order to make the calculation of multiple, different fields atomic so
>> that results are consistent. This is similar to that patch regarding the
>> change in statfs.
> 
> Is this enough? It seems we also need to cover sbi->user_block_count and
> sbi->total_node_count by stat_lock.

You're right, that seems to need locking for atomicity too. I'll send a v2.
Thanks!

> 
>>
>> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
>> ---
>>
>> Note:
>> I am currently working on a static analyser to detect missing locks
>> using type-based static analysis as my master's thesis
>> in order to obtain my master's degree.
>> If you would like to have more details, please let me know.
>> This was a reported case. I manually verified the report by looking
>> at the code, so that I do not send wrong information or patches.
>> After concluding that this seems to be a true positive, I created
>> this patch. This was compile-tested and runtime-tested on x86_64.
>> This issue was found on Linux v5.17.4.
>>
>>  fs/f2fs/super.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>> index ea939db18f88..ece768869187 100644
>> --- a/fs/f2fs/super.c
>> +++ b/fs/f2fs/super.c
>> @@ -1739,10 +1739,12 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
>>  	buf->f_bsize = sbi->blocksize;
>>  
>>  	buf->f_blocks = total_count - start_count;
>> +
>> +	spin_lock(&sbi->stat_lock);
>> +
>>  	buf->f_bfree = user_block_count - valid_user_blocks(sbi) -
>>  						sbi->current_reserved_blocks;
>>  
>> -	spin_lock(&sbi->stat_lock);
>>  	if (unlikely(buf->f_bfree <= sbi->unusable_block_count))
>>  		buf->f_bfree = 0;
>>  	else
>> -- 
>> 2.35.2



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel