Re: [PATCH v1 2/3] io_uring/rsrc: don't rely on user vaddr alignment

[Pavel Begunkov <asml.silence@gmail.com>]

On 6/24/25 13:26, David Hildenbrand wrote:
> On 24.06.25 14:20, Pavel Begunkov wrote:
>> On 6/24/25 12:53, David Hildenbrand wrote:
>>> On 24.06.25 12:35, Pavel Begunkov wrote:
>>>> There is no guaranteed alignment for user pointers, however the
>>>> calculation of an offset of the first page into a folio after
>>>> coalescing uses some weird bit mask logic, get rid of it.
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Reported-by: David Hildenbrand <david@redhat.com>
>>>> Fixes: a8edbb424b139 ("io_uring/rsrc: enable multi-hugepage buffer coalescing")
>>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>>> ---
>>>>    io_uring/rsrc.c | 8 +++++++-
>>>>    io_uring/rsrc.h | 1 +
>>>>    2 files changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
>>>> index e83a294c718b..5132f8df600f 100644
>>>> --- a/io_uring/rsrc.c
>>>> +++ b/io_uring/rsrc.c
>>>> @@ -734,6 +734,8 @@ bool io_check_coalesce_buffer(struct page **page_array, int nr_pages,
>>>>        data->nr_pages_mid = folio_nr_pages(folio);
>>>>        data->folio_shift = folio_shift(folio);
>>>> +    data->first_page_offset = page_array[0] - compound_head(page_array[0]);
>>>> +    data->first_page_offset <<= PAGE_SHIFT;
>>>
>>> Would that also cover when we have something like
>>>
>>> nr_pages = 4
>>> pages[0] = folio_page(folio, 1);
>>> pages[1] = folio_page(folio, 2);
>>> pages[2] = folio_page(folio2, 1);
>>> pages[3] = folio_page(folio2, 2);
>>>
>>> Note that we can create all kinds of crazy partially-mapped THP layouts using VMAs.
>>
>> It'll see that pages[2] is not the first page of folio2
>> and return that it can't be coalesced
>>
>> if (/* ... */ || folio_page_idx(folio, page_array[i]) != 0)
>>     return false;
> 
> Ah okay, that makes sense.
> 
> It might be clearer at some point to coalesce folio ranges (e.g., folio,idx,len) instead, representing them in a different temporary structure.

It could, but it needs a fast way to apply an arbitrary offset
and iov_iter_advance() is not exactly fast, especially so for a
long bvec array. That's why it keeps all segments of the same
length (apart from first/last).

-- 
Pavel Begunkov