Re: [PATCH 2/2] exfat: take bitmap_lock at the start of exfat_alloc_cluster()

[Chi Zhiling <chizhiling@163.com>]

Hi, David


On 9/7/26 11:21, David Timber wrote:
> On 9/5/26 05:49, Chi Zhiling wrote:
>> From: Chi Zhiling <chizhiling@kylinos.cn>
>>
>> exfat_alloc_cluster() checks sbi->used_clusters against the total
>> number of data clusters before acquiring sbi->bitmap_lock. A concurrent
>> allocation can update sbi->used_clusters after the check but before
>> the lock is acquired, making the check stale. This can allow the
>> allocation to proceed even though there are not enough free clusters,
>> causing it to fail partway through.
>>
>> Acquire sbi->bitmap_lock before checking sbi->used_clusters so that
>> the free-space check and subsequent cluster allocation are serialized
>> with concurrent allocations.
>>
>> Signed-off-by: Chi Zhiling <chizhiling@kylinos.cn>
>> ---
>>   fs/exfat/fatent.c | 17 ++++++++++-------
>>   1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/exfat/fatent.c b/fs/exfat/fatent.c
>> index a6728c361289..3c8bdc131f6f 100644
>> --- a/fs/exfat/fatent.c
>> +++ b/fs/exfat/fatent.c
>> @@ -427,19 +427,22 @@ int exfat_alloc_cluster(struct inode *inode, unsigned int num_alloc,
>>   	struct super_block *sb = inode->i_sb;
>>   	struct exfat_sb_info *sbi = EXFAT_SB(sb);
>>   
>> +	mutex_lock(&sbi->bitmap_lock);
> Speaking of which, I think we should do this as well:
> 
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
> index 217d150652cf..238533982831 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -62,7 +62,9 @@ static int exfat_statfs(struct dentry *dentry, struct kstatfs *buf)
>          buf->f_type = sb->s_magic;
>          buf->f_bsize = sbi->cluster_size;
>          buf->f_blocks = sbi->num_clusters - 2; /* clu 0 & 1 */
> +       mutex_lock(&sbi->bitmap_lock);
>          buf->f_bfree = buf->f_blocks - sbi->used_clusters;
> +       mutex_unlock(&sbi->bitmap_lock);
>          buf->f_bavail = buf->f_bfree;
>          buf->f_fsid = u64_to_fsid(id);
>          /* Unicode utf16 255 characters */
> 
> 
> Because there's a short window of chance that stale data is returned to
> userspace on NUMA systems. For example, if a shell script or a
> multi-threaded process makes changes to the fs and pulls statfs() in
> rapid succession, the kernel might give userspace a wrong impression
> that the fs has been chnaged by other users when it's really just a
> cache coherency issue.

You mean one user has changed the fs, and others see the stale value of 
used_clusters?

For SMP (including NUMA), if a certain CPU changes the value of 
used_clusters, then the caches of all other CPUs will be invalidated and 
need to be refilled. This process is completed by the hardware, see MESI 
protocol.

> 
> Well, this happens all the time with CoW-based fs like btrfs and zfs.
> But this is a traditional fs and people would expect generally the same
> behaviour as FAT(which does the right thing by placing a lock before
> counting clusters).

Perhaps I didn't fully understand your meaning. What is the FAT's behavior?


Thanks,