[PATCH 0/2] exfat: fix cluster allocation accounting and locking

[PATCH 0/2] exfat: fix cluster allocation accounting and locking

[Chi Zhiling]

From: Chi Zhiling <chizhiling@kylinos.cn>

Patch 1 fixes sbi->used_clusters accounting when cluster allocation
fails partway through. This can cause the counter to underflow.
The issue is caught by generic/476.

Patch 2 takes bitmap_lock before checking and allocating clusters.


Chi Zhiling (2):
  exfat: fix used_clusters accounting during cluster allocation
  exfat: take bitmap_lock at the start of exfat_alloc_cluster()

 fs/exfat/fatent.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

-- 
2.53.0

[PATCH 1/2] exfat: fix used_clusters accounting during cluster allocation

[Chi Zhiling]

From: Chi Zhiling <chizhiling@kylinos.cn>

In the current exfat_alloc_cluster(), p_chain->size is incremented as
each cluster is allocated so that the partially allocated chain can
be freed by __exfat_free_cluster() on the error path.

However, sbi->used_clusters is still updated only after all clusters
have been allocated, while __exfat_free_cluster() unconditionally
subtracts the number of freed clusters from sbi->used_clusters.
Therefore, when allocation fails partway through, the partially
allocated clusters are subtracted from sbi->used_clusters even
though they were never added to it. This corrupts the free-space
accounting and can cause sbi->used_clusters to underflow.

Increment sbi->used_clusters together with p_chain->size as each
cluster is allocated. This keeps the accounting consistent with
__exfat_free_cluster() on both the success and error paths.

This issue is caught by generic/476 with 256k cluster size:

    Ran: generic/476
    Failures: generic/476
    Failed 1 of 1 tests

    *** fsck.exfat output ***
    ERROR: <path>: more clusters are allocated. truncate to N bytes
    ...
    /dev/vdc: corrupted. directories 331, files 526
    /dev/vdc: files corrupted 9, files fixed 0
    *** end fsck.exfat output ***

Fixes: d5c514b6a0c0 ("exfat: fix the newly allocated clusters are not freed in error handling")
Signed-off-by: Chi Zhiling <chizhiling@kylinos.cn>
---
 fs/exfat/fatent.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/exfat/fatent.c b/fs/exfat/fatent.c
index a8b11e2ce43f..a6728c361289 100644
--- a/fs/exfat/fatent.c
+++ b/fs/exfat/fatent.c
@@ -509,13 +509,13 @@ int exfat_alloc_cluster(struct inode *inode, unsigned int num_alloc,
 			}
 		}
 		p_chain->size++;
+		sbi->used_clusters++;
 
 		last_clu = new_clu;
 
 		if (p_chain->size == num_alloc) {
 done:
 			sbi->clu_srch_ptr = hint_clu;
-			sbi->used_clusters += p_chain->size;
 			mutex_unlock(&sbi->bitmap_lock);
 			return 0;
 		}
-- 
2.53.0

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

[Chi Zhiling]

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);
+
 	total_cnt = EXFAT_DATA_CLUSTER_COUNT(sbi);
 
 	if (unlikely(total_cnt < sbi->used_clusters)) {
 		exfat_fs_error_ratelimit(sb,
 			"%s: invalid used clusters(t:%u,u:%u)\n",
 			__func__, total_cnt, sbi->used_clusters);
-		return -EIO;
+		ret = -EIO;
+		goto unlock;
 	}
 
-	if (num_alloc > total_cnt - sbi->used_clusters)
-		return -ENOSPC;
-
-	mutex_lock(&sbi->bitmap_lock);
+	if (num_alloc > total_cnt - sbi->used_clusters) {
+		ret = -ENOSPC;
+		goto unlock;
+	}
 
 	hint_clu = p_chain->dir;
 	/* find new cluster */
@@ -516,8 +519,8 @@ int exfat_alloc_cluster(struct inode *inode, unsigned int num_alloc,
 		if (p_chain->size == num_alloc) {
 done:
 			sbi->clu_srch_ptr = hint_clu;
-			mutex_unlock(&sbi->bitmap_lock);
-			return 0;
+			ret = 0;
+			goto unlock;
 		}
 
 		hint_clu = new_clu + 1;
-- 
2.53.0

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

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

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

[Chi Zhiling]

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,

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

[David Timber]

On 9/7/26 08:04, Chi Zhiling wrote:
[...]
>
> 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.
My bad for taking NUMA as an example. I was just curious as to what you
might have to say on this since the fix is kinda related to your patch.

I was just pointing out that there's a minor race condition in
exfat_statfs(). I used NUMA as an example because the delay through the
interconnect would make the cache coherency problem worse. btw, I think
the MESI protocol only covers cache. It has no control over the
execution flow of processors.

I might have an access to a real NUMA hardware that I can test my
reproducer on. I'll report back with a patch if I can actually reproduce
it. Thanks anyways.

Davo

Re: [PATCH 0/2] exfat: fix cluster allocation accounting and locking

[Namjae Jeon]

On Sat, Sep 5, 2026 at 2:50 PM Chi Zhiling <chizhiling@163.com> wrote:
>
> From: Chi Zhiling <chizhiling@kylinos.cn>
>
> Patch 1 fixes sbi->used_clusters accounting when cluster allocation
> fails partway through. This can cause the counter to underflow.
> The issue is caught by generic/476.
>
> Patch 2 takes bitmap_lock before checking and allocating clusters.
>
>
> Chi Zhiling (2):
>   exfat: fix used_clusters accounting during cluster allocation
>   exfat: take bitmap_lock at the start of exfat_alloc_cluster()
Applied them to #dev.
Thanks!