From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-17.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C34D3C63777 for ; Fri, 20 Nov 2020 08:29:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 598A222249 for ; Fri, 20 Nov 2020 08:29:02 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=xs4all.nl header.i=@xs4all.nl header.b="HRWawErq" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726504AbgKTI2l (ORCPT ); Fri, 20 Nov 2020 03:28:41 -0500 Received: from lb3-smtp-cloud7.xs4all.net ([194.109.24.31]:38229 "EHLO lb3-smtp-cloud7.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725858AbgKTI2k (ORCPT ); Fri, 20 Nov 2020 03:28:40 -0500 Received: from cust-b5b5937f ([IPv6:fc0c:c16d:66b8:757f:c639:739b:9d66:799d]) by smtp-cloud7.xs4all.net with ESMTPA id g1mSk0duZlmd2g1mVkPUTx; Fri, 20 Nov 2020 09:28:37 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=xs4all.nl; s=s2; t=1605860917; bh=DcELGYlhxaro30UZ20c6fQBWIf0M1lNcMxmp95cUUJ4=; h=Subject:From:To:Message-ID:Date:MIME-Version:Content-Type:From: Subject; b=HRWawErqqUunyEPr+iUOWOxf9j9aZ4SMzYu6uVe18G1XNJkjgJsIn+181gsSRmz57 NHcU6HQB6G0x/TG/vImNAe9wsTqwf3UlgTFJwC9C5KcbSeTlH2X08z9xkFIIN56QAH R5XaGNbetcBXf9Fo3NIUxFDCexxB4Mlwc3NB/sDFl8gdnKF7z15Vf6s8kknbVbu/Qv 0d/XGi/rAPLUwLnoTI5dABoGKRYjDDdf91G2NtSNHeN+jisPIPo0X+twEfh3nh0EUa 82cJLU/4t1tvuC9+AWKZQZAkKyoGQIs80W/3fFCiXxsqaRA8gvrgc0/dwA9DJPMpaZ P4jGjovIKZquQ== Subject: Re: [PATCH v6 09/17] media/videbuf1|2: Mark follow_pfn usage as unsafe From: Hans Verkuil To: Daniel Vetter , DRI Development , LKML Cc: kvm@vger.kernel.org, linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-media@vger.kernel.org, Tomasz Figa , Daniel Vetter , Jason Gunthorpe , Kees Cook , Dan Williams , Andrew Morton , John Hubbard , =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , Jan Kara , Pawel Osciak , Marek Szyprowski , Kyungmin Park , Laurent Dufour , Vlastimil Babka , Daniel Jordan , Michel Lespinasse , Mauro Carvalho Chehab References: <20201119144146.1045202-1-daniel.vetter@ffwll.ch> <20201119144146.1045202-10-daniel.vetter@ffwll.ch> Message-ID: Date: Fri, 20 Nov 2020 09:28:32 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-CMAE-Envelope: MS4xfKJe3pdseOGoPmUF+moMd4kN4FZ2xAvYBXMYjRkZT3iU9Vg3ajyNtJwJFb0q2cUm3gAK1ctXY2oEOsiYBNG71aHda4Gp4ddYPMg/VldO3Qi5lHBYelxw 6YLDaHrxG8vIPoxkmFj9IM2Uj09t0kl+EcCPKQYO/b+wtsixypImSdlzuX6ZvH4zzQ0ZAxPe47gDW65y8bYPpzZs7VZctFpTz/8nd9BhUAxhOwL61qHANWGA 9ZXGHS2xYSYjMkgRzGXze/DJ8oW2+XVxIv9puTWscNF+2IHMu4NJaXSGm5/2UYkn3nmOc/5cmgQg1VGNrazEYFjDK6ddOnhTUtRJTf0p6u3tOi2wIVFCbu+p VpUReLTQCVy+Sdgf54Hj/gPJvMjeze2onp2dZehOZ3xvn1w7DpE6I1m7zZWRNF039QLBinB53wF77wQX5dh8SvrybcZy6kZAC4N8Oj6InVnsUPasjGDIFh75 EmginXdT45g6btGUmmnaYhHqHYnfmsx6Bfo0LkwWtveJMbMXfZOV4SX3CyTDbniDDMpCy2znjs1DqyLUwMSEZrnLO3WTTYxTkzDOTvLlrj0xbXLSR2E4EGKb A/RQ0F2uP8A7n23s8fAJTnaKzoq1WWAGdhA9QW2k6F8horB1AmiErN5FUcn8SQWiJc3XVB0LxwJoEnx0MjgTaoSAyvqtPtH2oO0XNvh8y3jK74YVhDjiNIzf eZ7ng7g4gfHjtKabePK/N9KRZdIRs7DzXhmTuBh1bfozD/Dt51hVhDfv5UoH3JUUmOUPjZpUUpJJjLsfkQgJY5Htcezce9nFVuvMx9athk8sANbbLHWpr7GA dffJRocQzCrklh1BpIBOkXDV2gzbpx8luDHEj4FFBHA9LFvrv6zBjX+fn7nu9CQpMQxfTFHnOfPe2QvhWAKUmwYCjmYfhQZH5GVHqETod34zXriHmLIO9afa qpRz+cKIb3DlBYolXAffgi2POvhNL6N2X5MiD7cJjHNGBFl0fxDTebdis2R5rxKNtj4CBw== Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On 20/11/2020 09:06, Hans Verkuil wrote: > On 19/11/2020 15:41, Daniel Vetter wrote: >> The media model assumes that buffers are all preallocated, so that >> when a media pipeline is running we never miss a deadline because the >> buffers aren't allocated or available. >> >> This means we cannot fix the v4l follow_pfn usage through >> mmu_notifier, without breaking how this all works. The only real fix >> is to deprecate userptr support for VM_IO | VM_PFNMAP mappings and >> tell everyone to cut over to dma-buf memory sharing for zerocopy. >> >> userptr for normal memory will keep working as-is, this only affects >> the zerocopy userptr usage enabled in 50ac952d2263 ("[media] >> videobuf2-dma-sg: Support io userptr operations on io memory"). >> >> Acked-by: Tomasz Figa > > Acked-by: Hans Verkuil Actually, cancel this Acked-by. So let me see if I understand this right: VM_IO | VM_PFNMAP mappings can move around. There is a mmu_notifier that can be used to be notified when that happens, but that can't be used with media buffers since those buffers must always be available and in the same place. So follow_pfn is replaced by unsafe_follow_pfn to signal that what is attempted is unsafe and unreliable. If CONFIG_STRICT_FOLLOW_PFN is set, then unsafe_follow_pfn will fail, if it is unset, then it writes a warning to the kernel log but just continues while still unsafe. I am very much inclined to just drop VM_IO | VM_PFNMAP support in the media subsystem. For vb2 there is a working alternative in the form of dmabuf, and frankly for vb1 I don't care. If someone really needs this for a vb1 driver, then they can do the work to convert that driver to vb2. I've added Mauro to the CC list and I'll ping a few more people to see what they think, but in my opinion support for USERPTR + VM_IO | VM_PFNMAP should just be killed off. If others would like to keep it, then frame_vector.c needs a comment before the 'while' explaining why the unsafe_follow_pfn is there and that using dmabuf is the proper alternative to use. That will make it easier for developers to figure out why they see a kernel warning and what to do to fix it, rather than having to dig through the git history for the reason. Regards, Hans > > Thanks! > > Hans > >> Signed-off-by: Daniel Vetter >> Cc: Jason Gunthorpe >> Cc: Kees Cook >> Cc: Dan Williams >> Cc: Andrew Morton >> Cc: John Hubbard >> Cc: Jérôme Glisse >> Cc: Jan Kara >> Cc: Dan Williams >> Cc: linux-mm@kvack.org >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-samsung-soc@vger.kernel.org >> Cc: linux-media@vger.kernel.org >> Cc: Pawel Osciak >> Cc: Marek Szyprowski >> Cc: Kyungmin Park >> Cc: Tomasz Figa >> Cc: Laurent Dufour >> Cc: Vlastimil Babka >> Cc: Daniel Jordan >> Cc: Michel Lespinasse >> Signed-off-by: Daniel Vetter >> -- >> v3: >> - Reference the commit that enabled the zerocopy userptr use case to >> make it abundandtly clear that this patch only affects that, and not >> normal memory userptr. The old commit message already explained that >> normal memory userptr is unaffected, but I guess that was not clear >> enough. >> --- >> drivers/media/common/videobuf2/frame_vector.c | 2 +- >> drivers/media/v4l2-core/videobuf-dma-contig.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/common/videobuf2/frame_vector.c b/drivers/media/common/videobuf2/frame_vector.c >> index a0e65481a201..1a82ec13ea00 100644 >> --- a/drivers/media/common/videobuf2/frame_vector.c >> +++ b/drivers/media/common/videobuf2/frame_vector.c >> @@ -70,7 +70,7 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames, >> break; >> >> while (ret < nr_frames && start + PAGE_SIZE <= vma->vm_end) { >> - err = follow_pfn(vma, start, &nums[ret]); >> + err = unsafe_follow_pfn(vma, start, &nums[ret]); >> if (err) { >> if (ret == 0) >> ret = err; >> diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c b/drivers/media/v4l2-core/videobuf-dma-contig.c >> index 52312ce2ba05..821c4a76ab96 100644 >> --- a/drivers/media/v4l2-core/videobuf-dma-contig.c >> +++ b/drivers/media/v4l2-core/videobuf-dma-contig.c >> @@ -183,7 +183,7 @@ static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem, >> user_address = untagged_baddr; >> >> while (pages_done < (mem->size >> PAGE_SHIFT)) { >> - ret = follow_pfn(vma, user_address, &this_pfn); >> + ret = unsafe_follow_pfn(vma, user_address, &this_pfn); >> if (ret) >> break; >> >> >