Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.161 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.161; helo=mail.nvidia.com; pr=C
Message-ID: <72029a93-1150-1994-916f-b15ef0befd49@nvidia.com>
Date:   Thu, 2 Feb 2023 18:54:54 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [syzbot] general protection fault in skb_dequeue (3)
Content-Language: en-US
To:     David Howells <dhowells@redhat.com>,
        David Hildenbrand <david@redhat.com>
CC:     syzbot <syzbot+a440341a59e3b7142895@syzkaller.appspotmail.com>,
        <davem@davemloft.net>, <edumazet@google.com>, <hch@lst.de>,
        <johannes@sipsolutions.net>, <kuba@kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-wireless@vger.kernel.org>,
        <netdev@vger.kernel.org>, <pabeni@redhat.com>,
        <syzkaller-bugs@googlegroups.com>
References: <e8065d6a-d2f9-60aa-8541-8dfc8e9b608f@redhat.com>
 <000000000000b0b3c005f3a09383@google.com>
 <822863.1675327935@warthog.procyon.org.uk>
 <1265629.1675350909@warthog.procyon.org.uk>
From:   John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <1265629.1675350909@warthog.procyon.org.uk>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Feb 2023 02:55:00.3014
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 4ffb76bb-9b07-495b-716c-08db05920a9e
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.161];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: CO1NAM11FT112.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR12MB5240
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org

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