Re: [PATCH V2] fat: Prevent the race of read/write the FAT32 entry

[Al Viro <viro@zeniv.linux.org.uk>]

On Tue, Jul 29, 2025 at 10:35:58AM +0100, Al Viro wrote:
> On Tue, Jul 29, 2025 at 02:17:10PM +0800, Edward Adam Davis wrote:
> > syzbot reports data-race in fat32_ent_get/fat32_ent_put. 
> > 
> > 	CPU0(Task A)			CPU1(Task B)
> > 	====				====
> > 	vfs_write
> > 	new_sync_write
> > 	generic_file_write_iter
> > 	fat_write_begin
> > 	block_write_begin		vfs_statfs
> > 	fat_get_block			statfs_by_dentry
> > 	fat_add_cluster			fat_statfs

Sorry, no - you've missed an intermediate fat_chain_add() in the call chain
here.  And that makes your race a non-issue.

> > 	fat_ent_write			fat_count_free_clusters
> > 	fat32_ent_put			fat32_ent_get

fat_count_free_clusters() doesn't care about exact value of entry;
the only thing that matters is whether it's equal to FAT_ENT_FREE.

Actualy changes of that predicate (i.e. allocating and freeing of
clusters) still happens under fat_lock() - nothing has changed in
that area.  *That* is not happening simultaneously with reads in
fat_count_free_clusters().  It's attaching the new element to the
end of chain that is outside of fat_lock().

And that operation does not affect that predicate at all; it changes
the value of the entry for last cluster of file (FAT_ENT_EOF) to the
number of cluster being added to the file.  Neither is equal to
FAT_ENT_FREE, so there's no problem - value you get ->ent_get()
is affected by that store, the value of
	ops->ent_get(&fatent) == FAT_ENT_FREE
isn't.  Probably worth a comment in fat_chain_add() re "this store
is safe outside of fat_lock() because of <list of reasons>".
Changes to this chain (both extending and truncating it) are
excluded by <lock> and as far as allocator is concerned, we are
not changing the state of any cluster.

Basically, FAT encodes both the free block map (entry[block] == FAT_ENT_FREE
<=> block is not in use) and linked lists of blocks for individual files -
they store the number of file's first block within directory entry and
use FAT for forwards pointers in the list.  FAT_ENT_EOF is used for "no
more blocks, it's the end of file".  Allocator and fat_count_free_clusters
care only about the free block map part of that thing; access to file
contents - the linked list for that file.