linux-kernel - Re: [QUESTION] xas_reload() in iter_xarray_populate

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] [day] [month] [year] [list]

Message-ID: <aDS-8RigFM-ii2mz@casper.infradead.org>
Date: Mon, 26 May 2025 20:20:17 +0100
From: Matthew Wilcox <willy@...radead.org>
To: David Hildenbrand <david@...hat.com>
Cc: Dev Jain <dev.jain@....com>, ziy@...dia.com, dhowells@...hat.com,
	hughd@...gle.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	ryan.roberts@....com, aneesh.kumar@...nel.org
Subject: Re: [QUESTION] xas_reload() in iter_xarray_populate_pages()

On Mon, May 26, 2025 at 09:40:06AM +0200, David Hildenbrand wrote:
> On 26.05.25 08:35, Dev Jain wrote:
> > Hello all,
> > 
> > After doing an xas_load() and xas_retry(), we take neither a reference nor a lock
> > on the folio, and we do an xas_reload(). Is this just to reduce the time window
> > for a race?
> > 
> > If the above is true, then, there is a negligible window between xas_load() and
> > xas_reload(), because only xas_retry() exists between them, so why to even reload()?
> 
> The usual sequence for the pagecache is (see filemap_get_entry())
> 
> 1) xas_load(): Load the entry
> 
> 2) xas_retry(): Test if we have to retry immediately
> 
> 3) folio || xa_is_value(folio): check if the entry stores a folio
> 
> 4) folio_try_get(): try getting a folio reference, might get freed
>    concurrently, so a folio_get() is not safe
> 
> 5) folio != xas_reload(&xas): recheck whether the entry was changed
>    concurrently
> 
> iter_xarray_get_pages()->iter_xarray_populate_pages() works on whatever
> xarray was provided to iov_iter_xarray().
> 
> erofs/netfs/orangefs seem to pass the pagecache ... so I would also assume
> that we have to use the same sequence as above.
> 
> Willy and me had a look ad that code in b57f4f4f186d ("iov_iter: convert
> iter_xarray_populate_pages() to use folios").
> 
> But looking at it now, I think that code is incorrect. At least the
> folio_get() and reload-before-folio-get is weird.

Well, I just converted it; I didn't think hard about what it was doing
was right.  I think I may have mentioned to dhowells that I thought the
xas_reload() was unnecessary as the folios are required to be referenced
by the caller.  So if you can look them up, they're guaranteed to not
be replaced [1].

[1] the xas_retry() cannot be skipped as it guards against a rearrangement
of the tree which can happen even with the folio pinned.