Re: [PATCH] mm/filemap: mark lockless accesses to mapping->nrpages

[Jan Kara]

On Sun 11-10-26 18:29:40, 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. Annotate the readers with READ_ONCE() and
> the writers in filemap with WRITE_ONCE().
> 
> Reported-by: syzbot+e0624b5bf72b99131402@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=e0624b5bf72b99131402
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@xxxxxxxxx>

I think in this case the proper fix is to annotate the problematic read
with data_race() to silence the checker. As you mentioned there's no real
bug to fix so no reason to convolute the code with READ_ONCE() and
WRITE_ONCE().

								Honza

> ---
>  block/bdev.c | 2 +-
>  mm/filemap.c | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/block/bdev.c b/block/bdev.c
> index cd8323083740..d97071160e86 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -95,7 +95,7 @@ void invalidate_bdev(struct block_device *bdev)
>  {
>  	struct address_space *mapping = bdev->bd_mapping;
>  
> -	if (mapping->nrpages) {
> +	if (READ_ONCE(mapping->nrpages)) {
>  		invalidate_bh_lrus();
>  		lru_add_drain_all();	/* make sure all lru add caches are flushed */
>  		invalidate_mapping_pages(mapping, 0, -1);
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 00fd89cf6f55..007fe10b2e1d 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -144,7 +144,7 @@ static void page_cache_delete(struct address_space *mapping,
>  
>  	folio->mapping = NULL;
>  	/* Leave folio->index set: truncation lookup relies upon it */
> -	mapping->nrpages -= nr;
> +	WRITE_ONCE(mapping->nrpages, mapping->nrpages - nr);
>  }
>  
>  static void filemap_unaccount_folio(struct address_space *mapping,
> @@ -314,7 +314,7 @@ static void page_cache_delete_batch(struct address_space *mapping,
>  		xas_store(&xas, NULL);
>  		total_pages += folio_nr_pages(folio);
>  	}
> -	mapping->nrpages -= total_pages;
> +	WRITE_ONCE(mapping->nrpages, mapping->nrpages - total_pages);
>  }
>  
>  void delete_from_page_cache_batch(struct address_space *mapping,
> @@ -631,7 +631,7 @@ 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;
> +	return READ_ONCE(mapping->nrpages);
>  }
>  
>  bool filemap_range_has_writeback(struct address_space *mapping,
> @@ -916,7 +916,7 @@ noinline int __filemap_add_folio(struct address_space *mapping,
>  		if (xas_error(&xas))
>  			goto unlock;
>  
> -		mapping->nrpages += nr;
> +		WRITE_ONCE(mapping->nrpages, mapping->nrpages + nr);
>  
>  		/* hugetlb pages do not participate in page cache accounting */
>  		if (!huge) {
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR