Re: [PATCH v2 1/1] mm: fix folio_pte_batch() on XEN PV

David Hildenbrand
2025-05-04T06:47:54Z

On 04.05.25 03:28, Andrew Morton wrote:
> On Fri, 2 May 2025 23:50:19 +0200 Petr Vaněk <arkamar@atlas.cz> wrote:
>
>> On XEN PV, folio_pte_batch() can incorrectly batch beyond the end of a
>> folio due to a corner case in pte_advance_pfn(). Specifically, when the
>> PFN following the folio maps to an invalidated MFN,
>>
>> expected_pte = pte_advance_pfn(expected_pte, nr);
>>
>> produces a pte_none(). If the actual next PTE in memory is also
>> pte_none(), the pte_same() succeeds,
>>
>> if (!pte_same(pte, expected_pte))
>> break;
>>
>> the loop is not broken, and batching continues into unrelated memory.
>>
>> ...
>
> Looks OK for now I guess but it looks like we should pay some attention
> to what types we're using.
> >> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -248,11 +248,9 @@ static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
>> pte_t *start_ptep, pte_t pte, int max_nr, fpb_t flags,
>> bool *any_writable, bool *any_young, bool *any_dirty)
>> {
>> - unsigned long folio_end_pfn = folio_pfn(folio) + folio_nr_pages(folio);
>> - const pte_t *end_ptep = start_ptep + max_nr;
>> pte_t expected_pte, *ptep;
>> bool writable, young, dirty;
>> - int nr;
>> + int nr, cur_nr;
>>
>> if (any_writable)
>> *any_writable = false;
>> @@ -265,11 +263,15 @@ static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
>> VM_WARN_ON_FOLIO(!folio_test_large(folio) || max_nr < 1, folio);
>> VM_WARN_ON_FOLIO(page_folio(pfn_to_page(pte_pfn(pte))) != folio, folio);
>>
>> + /* Limit max_nr to the actual remaining PFNs in the folio we could batch. */
>> + max_nr = min_t(unsigned long, max_nr,
>> + folio_pfn(folio) + folio_nr_pages(folio) - pte_pfn(pte));
>> +
>
> Methinks max_nr really wants to be unsigned long.

We only batch within a single PTE table, so an integer was sufficient.

The unsigned value is the result of a discussion with Ryan regarding similar/related
(rmap) functions:

"
Personally I'd go with signed int (since
that's what all the counters in struct folio that we are manipulating are,
underneath the atomic_t) then check that nr_pages > 0 in
__folio_rmap_sanity_checks().
"

https://lore.kernel.org/linux-mm/20231204142146.91437-14-david@redhat.com/T/#ma0bfff0102f0f2391dfa94aa22a8b7219b92c957

As soon as we let "max_nr" be an "unsigned long", also the return value
should be an "unsigned long", and everybody calling that function.

In this case here, we should likely just use whatever type "max_nr" is.

Not sure myself if we should change that here to unsigned long or long. Some
callers also operate with the negative values IIRC (e.g., adjust the RSS by doing -= nr).

> That will permit the
> cleanup of quite a bit of truncation, extension, signedness conversion
> and general type chaos in folio_pte_batch()'s various callers.
> > And...
>
> Why does folio_nr_pages() return a signed quantity? It's a count.

A partial answer is in 1ea5212aed068 ("mm: factor out large folio handling
from folio_nr_pages() into folio_large_nr_pages()"), where I stumbled over the
reason for a signed value myself and at least made the other
functions be consistent with folio_nr_pages():

"
While at it, let's consistently return a "long" value from all these
similar functions. Note that we cannot use "unsigned int" (even though
_folio_nr_pages is of that type), because it would break some callers that
do stuff like "-folio_nr_pages()". Both "int" or "unsigned long" would
work as well.

"

Note that folio_nr_pages() returned a "long" since the very beginning. Probably using
a signed value for consistency because also mapcounts / refcounts are all signed.

>
> And why the heck is folio_pte_batch() inlined? It's larger then my
> first hard disk and it has five callsites!

:)

In case of fork/zap we really want it inlined because

(1) We want to optimize out all of the unnecessary checks we added for other users

(2) Zap/fork code is very sensitive to function call overhead

Probably, as that function sees more widespread use, we might want a
non-inlined variant that can be used in places where performance doesn't
matter all that much (although I am not sure there will be that many).

--
Cheers,

David / dhildenb