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

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,