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

[David Timber]

On 9/5/26 05:49, Chi Zhiling wrote:
> From: Chi Zhiling <chizhiling@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
>  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.

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).

Davo