Re: KCSAN: data-race in fat16_ent_put / fat_search_long

[Matthew Wilcox <willy@infradead.org>]

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?

> 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);
> }
> 
> 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
> 			continue;
> 		if (de->attr != ATTR_EXT && IS_FREE(de->name))
> 			continue;
> 		<snip>
> }
> 
> Thanks,
> -- Marco