Re: [PATCH v2] mm/filemap: annotate lockless reads of mapping->nrpages

[Jan Kara]

On Mon 12-10-26 19:06:36, Nguyen Ngoc Thang wrote:
> mapping->nrpages is updated under the i_pages lock but is read locklessly
> as a cheap "anything cached?" hint, e.g. in invalidate_bdev() and
> mapping_needs_writeback(). KCSAN reports the race:
> 
>   BUG: KCSAN: data-race in __filemap_add_folio / invalidate_bdev
> 
>   read-write to 0xffff8881004c4178 of 8 bytes by task 3600 on cpu 0:
>    __filemap_add_folio+0x6b7/0x910 mm/filemap.c:919
>    filemap_add_folio+0x1cd/0x380
>    page_cache_ra_unbounded+0x1a4/0x3a0
>    ...
>    blkdev_read_iter+0x21e/0x2d0
>   read to 0xffff8881004c4178 of 8 bytes by task 3601 on cpu 1:
>    invalidate_bdev+0x25/0x70 block/bdev.c:98
>    ext4_fill_super+0x2ccb/0x37b0
> 
> The same counter also races with filemap_write_and_wait_range() through
> mapping_needs_writeback().
> 
> The race is benign: a stale value only makes the caller do or skip a
> harmless invalidate/flush pass. Mark the two lockless readers with
> data_race().
> 
> Reported-by: syzbot+e0624b5bf72b99131402@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=e0624b5bf72b99131402
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@xxxxxxxxx>

...

> diff --git a/mm/filemap.c b/mm/filemap.c
> index 00fd89cf6f55..4c9d98efb7fb 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -631,7 +631,8 @@ EXPORT_SYMBOL(filemap_fdatawait_keep_errors);
>  /* Returns true if writeback might be needed or already in progress. */
>  static bool mapping_needs_writeback(struct address_space *mapping)
>  {
> -	return mapping->nrpages;
> +	/* Lockless hint; a stale value is harmless */
> +	return data_race(mapping->nrpages);
>  }

Rather than annotating inside mapping_needs_writeback() it would be more
future-proof to annotate the two callers of mapping_needs_writeback(). Just
in case some user which require reliable answer appears in the future...

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR