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

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Edward Adam Davis <eadavis@qq.com> writes:

> The writer and reader access FAT32 entry without any lock, so the data
> obtained by the reader is incomplete.
>
> Add spin lock to solve the race condition that occurs when accessing
> FAT32 entry.
>
> FAT16 entry has the same issue and is handled together.

What is the real issue? Counting free entries doesn't care whether EOF
(0xffffff) or allocate (0x000068), so it should be same result on both
case.

We may want to use READ_ONCE/WRITE_ONCE though, I can't see the reason
to add spin_lock.

Thanks.

> Reported-by: syzbot+d3c29ed63db6ddf8406e@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=d3c29ed63db6ddf8406e
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
>  fs/fat/fatent.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c
> index a7061c2ad8e4..0e64875e932c 100644
> --- a/fs/fat/fatent.c
> +++ b/fs/fat/fatent.c
> @@ -19,6 +19,8 @@ struct fatent_operations {
>  };
>  
>  static DEFINE_SPINLOCK(fat12_entry_lock);
> +static DEFINE_SPINLOCK(fat16_entry_lock);
> +static DEFINE_SPINLOCK(fat32_entry_lock);
>  
>  static void fat12_ent_blocknr(struct super_block *sb, int entry,
>  			      int *offset, sector_t *blocknr)
> @@ -137,8 +139,13 @@ static int fat12_ent_get(struct fat_entry *fatent)
>  
>  static int fat16_ent_get(struct fat_entry *fatent)
>  {
> -	int next = le16_to_cpu(*fatent->u.ent16_p);
> +	int next;
> +
> +	spin_lock(&fat16_entry_lock);
> +	next = le16_to_cpu(*fatent->u.ent16_p);
>  	WARN_ON((unsigned long)fatent->u.ent16_p & (2 - 1));
> +	spin_unlock(&fat16_entry_lock);
> +
>  	if (next >= BAD_FAT16)
>  		next = FAT_ENT_EOF;
>  	return next;
> @@ -146,8 +153,13 @@ static int fat16_ent_get(struct fat_entry *fatent)
>  
>  static int fat32_ent_get(struct fat_entry *fatent)
>  {
> -	int next = le32_to_cpu(*fatent->u.ent32_p) & 0x0fffffff;
> +	int next;
> +
> +	spin_lock(&fat32_entry_lock);
> +	next = le32_to_cpu(*fatent->u.ent32_p) & 0x0fffffff;
>  	WARN_ON((unsigned long)fatent->u.ent32_p & (4 - 1));
> +	spin_unlock(&fat32_entry_lock);
> +
>  	if (next >= BAD_FAT32)
>  		next = FAT_ENT_EOF;
>  	return next;
> @@ -180,15 +192,21 @@ static void fat16_ent_put(struct fat_entry *fatent, int new)
>  	if (new == FAT_ENT_EOF)
>  		new = EOF_FAT16;
>  
> +	spin_lock(&fat16_entry_lock);
>  	*fatent->u.ent16_p = cpu_to_le16(new);
> +	spin_unlock(&fat16_entry_lock);
> +
>  	mark_buffer_dirty_inode(fatent->bhs[0], fatent->fat_inode);
>  }
>  
>  static void fat32_ent_put(struct fat_entry *fatent, int new)
>  {
>  	WARN_ON(new & 0xf0000000);
> +	spin_lock(&fat32_entry_lock);
>  	new |= le32_to_cpu(*fatent->u.ent32_p) & ~0x0fffffff;
>  	*fatent->u.ent32_p = cpu_to_le32(new);
> +	spin_unlock(&fat32_entry_lock);
> +
>  	mark_buffer_dirty_inode(fatent->bhs[0], fatent->fat_inode);
>  }

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>