linux-ext4 - Re: [PATCH 2/3] fs/buffer: avoid races with folio migrations on __find_get_block

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <lj6o73q6nev776uvy7potqrn5gmgtm4o2cev7dloedwasxcsmn@uanvqp3sm35p>
Date: Tue, 1 Apr 2025 12:57:37 +0200
From: Jan Kara <jack@...e.cz>
To: Luis Chamberlain <mcgrof@...nel.org>
Cc: brauner@...nel.org, jack@...e.cz, tytso@....edu, 
	adilger.kernel@...ger.ca, linux-ext4@...r.kernel.org, riel@...riel.com, 
	willy@...radead.org, hannes@...xchg.org, oliver.sang@...el.com, dave@...olabs.net, 
	david@...hat.com, axboe@...nel.dk, hare@...e.de, david@...morbit.com, 
	djwong@...nel.org, ritesh.list@...il.com, linux-fsdevel@...r.kernel.org, 
	linux-block@...r.kernel.org, linux-mm@...ck.org, gost.dev@...sung.com, p.raghav@...sung.com, 
	da.gomez@...sung.com
Subject: Re: [PATCH 2/3] fs/buffer: avoid races with folio migrations on
 __find_get_block_slow()

On Sat 29-03-25 23:47:31, Luis Chamberlain wrote:
> diff --git a/fs/buffer.c b/fs/buffer.c
> index c7abb4a029dc..a4e4455a6ce2 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -208,6 +208,15 @@ __find_get_block_slow(struct block_device *bdev, sector_t block)
>  	head = folio_buffers(folio);
>  	if (!head)
>  		goto out_unlock;
> +
> +	if (folio->mapping->a_ops->migrate_folio &&
> +	    folio->mapping->a_ops->migrate_folio == buffer_migrate_folio_norefs) {

This is always true for bdev mapping we have here, isn't it?

> +		if (folio_test_lru(folio) &&

Do you expect bdev page cache to contain non-LRU folios? I thought every
pagecache folio is on LRU so this seems pointless as well? But I may be
missing something here.

> +		    folio_test_locked(folio) &&
> +		    !folio_test_writeback(folio))
> +			goto out_unlock;

I find this problematic. It fixes the race with migration, alright
(although IMO we should have a comment very well explaining the interplay
of folio lock and mapping->private_lock to make this work - probably in
buffer_migrate_folio_norefs() - and reference it from here), but there are
places which expect that if __find_get_block() doesn't return anything,
this block is not cached in the buffer cache. And your change breaks this
assumption. Look for example at write_boundary_block(), that will fail to
write the block it should write if it races with someone locking the folio
after your changes. Similarly the code tracking state of deleted metadata
blocks in fs/jbd2/revoke.c will fail to properly update buffer's state if
__find_get_block() suddently starts returning NULL although the buffer is
present in cache. 

> +	}
> +
>  	bh = head;
>  	do {
>  		if (!buffer_mapped(bh))

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR