Re: KCSAN: data-race in fat16_ent_put / fat_search_long

[Marco Elver]

On Wed, 6 Nov 2019 at 09:31, OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx> wrote:
>
> Matthew Wilcox <willy@xxxxxxxxxxxxx> writes:
>
> > On Tue, Nov 05, 2019 at 03:39:23PM +0100, Marco Elver wrote:
> >> On Tue, 05 Nov 2019, syzbot wrote:
> >> > ==================================================================
> >> > BUG: KCSAN: data-race in fat16_ent_put / fat_search_long
> >> >
> >> > write to 0xffff8880a209c96a of 2 bytes by task 11985 on cpu 0:
> >> >  fat16_ent_put+0x5b/0x90 fs/fat/fatent.c:181
> >> >  fat_ent_write+0x6d/0xf0 fs/fat/fatent.c:415
> >> >  fat_chain_add+0x34e/0x400 fs/fat/misc.c:130
> >> >  fat_add_cluster+0x92/0xd0 fs/fat/inode.c:112
> >> >  __fat_get_block fs/fat/inode.c:154 [inline]
> >> >  fat_get_block+0x3ae/0x4e0 fs/fat/inode.c:189
> >> >  __block_write_begin_int+0x2ea/0xf20 fs/buffer.c:1968
> >> >  __block_write_begin fs/buffer.c:2018 [inline]
> >> >  block_write_begin+0x77/0x160 fs/buffer.c:2077
> >> >  cont_write_begin+0x3d6/0x670 fs/buffer.c:2426
> >> >  fat_write_begin+0x72/0xc0 fs/fat/inode.c:235
> >> >  pagecache_write_begin+0x6b/0x90 mm/filemap.c:3148
> >> >  cont_expand_zero fs/buffer.c:2353 [inline]
> >> >  cont_write_begin+0x17a/0x670 fs/buffer.c:2416
> >> >  fat_write_begin+0x72/0xc0 fs/fat/inode.c:235
> >> >  pagecache_write_begin+0x6b/0x90 mm/filemap.c:3148
> >> >  generic_cont_expand_simple+0xb0/0x120 fs/buffer.c:2317
> >> >
> >> > read to 0xffff8880a209c96b of 1 bytes by task 11990 on cpu 1:
> >> >  fat_search_long+0x20a/0xc60 fs/fat/dir.c:484
> >> >  vfat_find+0xc1/0xd0 fs/fat/namei_vfat.c:698
> >> >  vfat_lookup+0x75/0x350 fs/fat/namei_vfat.c:712
> >> >  lookup_open fs/namei.c:3203 [inline]
> >> >  do_last fs/namei.c:3314 [inline]
> >> >  path_openat+0x15b6/0x36e0 fs/namei.c:3525
> >> >  do_filp_open+0x11e/0x1b0 fs/namei.c:3555
> >> >  do_sys_open+0x3b3/0x4f0 fs/open.c:1097
> >> >  __do_sys_open fs/open.c:1115 [inline]
> >> >  __se_sys_open fs/open.c:1110 [inline]
> >> >  __x64_sys_open+0x55/0x70 fs/open.c:1110
> >> >  do_syscall_64+0xcc/0x370 arch/x86/entry/common.c:290
> >> >  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >> >
> >> > Reported by Kernel Concurrency Sanitizer on:
> >> > CPU: 1 PID: 11990 Comm: syz-executor.2 Not tainted 5.4.0-rc3+ #0
> >> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> >> > Google 01/01/2011
> >> > ==================================================================
> >>
> >> I was trying to understand what is happening here, but fail to see how
> >> this can happen. So it'd be good if somebody who knows this code can
> >> explain. We are quite positive this is not a false positive, given the
> >> addresses accessed match.
> >
> > Both of these accesses are into a buffer head; ie the data being accessed
> > is stored in the page cache.  Is it possible the page was reused for
> > different data between these two accesses?
>
> No and yes. Reader side is directory buffer, writer side is FAT buffer.
> So FAT buffer never be reused as directory buffer.  But the page cache
> itself can be freed and reused as different index. So if KCSAN can't
> detect the page cache recycle, it would be possible.
>
> Is there anyway to know "why KCSAN thought this as data race"?

KCSAN set up a watchpoint on the plain read, simply stalling that
thread for a few microsec. While stalling, a concurrent plain write
occurred which matches the watchpoint the reader set up. Whenever
KCSAN detects a data race, the 2 operations *must* actually be
happening in parallel at the time.

I will try to reproduce this somehow.

> >> The two bits of code in question here are:
> >>
> >> static void fat16_ent_put(struct fat_entry *fatent, int new)
> >> {
> >>      if (new == FAT_ENT_EOF)
> >>              new = EOF_FAT16;
> >>
> >>      *fatent->u.ent16_p = cpu_to_le16(new);   <<== data race here
> >>      mark_buffer_dirty_inode(fatent->bhs[0], fatent->fat_inode);
> >> }
>
> This is updating FAT entry (index for data cluster placement) on FAT buffer.
>
> >> int fat_search_long(struct inode *inode, const unsigned char *name,
> >>                  int name_len, struct fat_slot_info *sinfo)
> >> {
> >>      struct super_block *sb = inode->i_sb;
> >>      struct msdos_sb_info *sbi = MSDOS_SB(sb);
> >>      struct buffer_head *bh = NULL;
> >>      struct msdos_dir_entry *de;
> >>      unsigned char nr_slots;
> >>      wchar_t *unicode = NULL;
> >>      unsigned char bufname[FAT_MAX_SHORT_SIZE];
> >>      loff_t cpos = 0;
> >>      int err, len;
> >>
> >>      err = -ENOENT;
> >>      while (1) {
> >>              if (fat_get_entry(inode, &cpos, &bh, &de) == -1)
> >>                      goto end_of_dir;
> >> parse_record:
> >>              nr_slots = 0;
> >>              if (de->name[0] == DELETED_FLAG)
> >>                      continue;
> >>              if (de->attr != ATTR_EXT && (de->attr & ATTR_VOLUME))  <<== data race here
>
> Checking attribute on directory buffer.
>
> >>                      continue;
> >>              if (de->attr != ATTR_EXT && IS_FREE(de->name))
> >>                      continue;
> >>              <snip>
> >> }
> >>
> >> Thanks,
> >> -- Marco
>
> --
> OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>