Re: [syzbot] general protection fault in skb_dequeue (3)

[John Hubbard <jhubbard@nvidia.com>]

On 2/2/23 07:15, David Howells wrote:
> David Hildenbrand <david@redhat.com> wrote:
> 
>> At first, I wondered if that's related to shared anonymous pages getting
>> pinned R/O that would trigger COW-unsharing ... but I don't even see where we
>> are supposed to use FOLL_PIN vs. FOLL_GET here? IOW, we're not even supposed
>> to access user space memory (neither FOLL_GET nor FOLL_PIN) but still end up
>> with a change in behavior.
> 
> I'm not sure that using FOLL_PIN is part of the problem here.

I agree. It's really not.

> 
> sendfile() is creating a transient buffer attached to a pipe, doing a DIO read
> into it (which uses iov_iter_extract_pages() to populate a bio) and then feeds
> the pages from the pipe one at a time using a BVEC-type iterator to a buffered
> write.
> 
> Note that iov_iter_extract_pages() does not pin/get pages when accessing a
> BVEC, KVEC, XARRAY or PIPE iterator.  However, in this case, this should not
> matter as the pipe is holding refs on the pages in the buffer.
> 
> I have found that the issue goes away if I add an extra get_page() into
> iov_iter_extract_pipe_pages() and don't release it (the page is then leaked).
> 
> This makes me think that the problem might be due to the pages getting
> recycled from the pipe before DIO has finished writing to them - but that
> shouldn't be the case as the splice has to be synchronous - we can't mark data
> in the pipe as 'produced' until we've finished reading it.


So I thought about this for a while, and one really big glaring point
that stands out for me is: before this commit, we had this:

iov_iter_get_pages()
   __iov_iter_get_pages_alloc()
     pipe_get_pages()
       get_page()

But now, based on the claim from various folks that "pipe cases don't
require a get_page()", we have boldly--too boldy, I believe-- moved
directly into case that doesn't do a get_page():

iov_iter_extract_pipe_pages()
   ...(nothing)

And your testing backs this up: adding the get_page() back hides the
failure.

So that's as far as I've got, but I am really suspecting that this is
where the root cause is: pipe references are not as locked down as we
think they are. At least in this case.

thanks,
-- 
John Hubbard
NVIDIA