Re: [syzbot] [mm?] kernel BUG in sanity_check_pinned_pages

[David Hildenbrand]

On 23.06.25 18:48, Pavel Begunkov wrote:
> On 6/23/25 16:11, David Hildenbrand wrote:
> > On 23.06.25 16:58, Jens Axboe wrote:
> > > On 6/23/25 6:22 AM, David Hildenbrand wrote:
> > > > On 23.06.25 12:10, David Hildenbrand wrote:
> > > > > On 23.06.25 11:53, Alexander Potapenko wrote:
> > > > > > On Mon, Jun 23, 2025 at 11:29?AM 'David Hildenbrand' via
> > > > > > syzkaller-bugs <syzkaller-bugs@xxxxxxxxxxxxxxxx> wrote:
> > > > > > >
> ...>>> When only pinning a single tail page (iovec.iov_len = pagesize), it works as expected.
> > > > So, if we pinned two tail pages but end up calling io_release_ubuf()->unpin_user_page()
> > > > on the head page, meaning that "imu->bvec[i].bv_page" points at the wrong folio page
> > > > (IOW, one we never pinned).
> > > >
> > > > So it's related to the io_coalesce_buffer() machinery.
> > > >
> > > > And in fact, in there, we have this weird logic:
> > > >
> > > > /* Store head pages only*/
> > > > new_array = kvmalloc_array(nr_folios, sizeof(struct page *), GFP_KERNEL);
> > > > ...
> > > >
> > > >
> > > > Essentially discarding the subpage information when coalescing tail pages.
> > > >
> > > >
> > > > I am afraid the whole io_check_coalesce_buffer + io_coalesce_buffer() logic might be
> > > > flawed (we can -- in theory -- coalesc different folio page ranges in
> > > > a GUP result?).
> > > >
> > > > @Jens, not sure if this only triggers a warning when unpinning or if we actually mess up
> > > > imu->bvec[i].bv_page, to end up pointing at (reading/writing) pages we didn't even pin in the first
> > > > place.
> > > >
> > > > Can you look into that, as you are more familiar with the logic?
> > >
> > > Leaving this all quoted and adding Pavel, who wrote that code. I'm
> > > currently away, so can't look into this right now.
>
> Chenliang Li did, but not like it matters
>
> > I did some more digging, but ended up being all confused about io_check_coalesce_buffer() and io_imu_folio_data().
> >
> > Assuming we pass a bunch of consecutive tail pages that all belong to the same folio, then the loop in io_check_coalesce_buffer() will always
> > run into the
> >
> > if (page_folio(page_array[i]) == folio &&
> >       page_array[i] == page_array[i-1] + 1) {
> >       count++;
> >       continue;
> > }
> >
> > case, making the function return "true" ... in io_coalesce_buffer(), we then store the head page ... which seems very wrong.
> >
> > In general, storing head pages when they are not the first page to be coalesced seems wrong.
>
> Yes, it stores the head page even if the range passed to
> pin_user_pages() doesn't cover the head page.
> > It should be converted to unpin_user_folio(), which doesn't seem
> to do sanity_check_pinned_pages(). Do you think that'll be enough
> (conceptually)? Nobody is actually touching the head page in those
> cases apart from the final unpin, and storing the head page is
> more convenient than keeping folios. I'll take a look if it can
> be fully converted to folios w/o extra overhead.

Assuming we had from GUP

nr_pages = 2
pages[0] = folio_page(folio, 1)
pages[1] = folio_page(folio, 2)

After io_coalesce_buffer() we have

nr_pages = 1
pages[0] = folio_page(folio, 0)


Using unpin_user_folio() in all places where we could see something like 
that would be the right thing to do. The sanity checks are not in 
unpin_user_folio() for exactly that reason: we don't know which folio 
pages we pinned.

But now I wonder where you make sure that "Nobody is actually touching 
the head page"?

How do you get back the "which folio range" information after 
io_coalesce_buffer() ?


If you rely on alignment in virtual address space for you, combined with 
imu->folio_shift, that might not work reliably ...

--
Cheers,

David / dhildenb