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=-15.8 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,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 0CC7CC63777 for ; Fri, 20 Nov 2020 10:51:51 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 9B3B824196 for ; Fri, 20 Nov 2020 10:51:50 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="MpezDlRH" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727451AbgKTKve (ORCPT ); Fri, 20 Nov 2020 05:51:34 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54658 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727364AbgKTKvd (ORCPT ); Fri, 20 Nov 2020 05:51:33 -0500 Received: from mail-ot1-x344.google.com (mail-ot1-x344.google.com [IPv6:2607:f8b0:4864:20::344]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 228BBC061A04 for ; Fri, 20 Nov 2020 02:51:31 -0800 (PST) Received: by mail-ot1-x344.google.com with SMTP id n12so4793127otk.0 for ; Fri, 20 Nov 2020 02:51:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=9N1VJa6PgaSrJK1UStrZa9k9hVHdgTE/3/Vb+F7qDfg=; b=MpezDlRHhDSLM+N5cZzIEzt77SYyy+A3JHsWxp1YZhY1ZVLefqezDmgWeeVuLW+0Sz hNspdaDLmjlNt+Tms4qqJ/jndEuNmpgedZxgoIIUiCQI7bUtpNW3IvL2VvfxCvxRYz8A tm+ce3BzWttaGumHJxGVfC2fzpl5+ui467JMg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=9N1VJa6PgaSrJK1UStrZa9k9hVHdgTE/3/Vb+F7qDfg=; b=IY8jkxszG7+3/DLZKZr364z8B/YeWVhxPaSfxr596vawRDpQLzKfJPgtMdXPP7Z9HX dUCx8J64D7Da9sMmbRI+W/flYu5KlEfXA7XzdGLP0fdo0wit1Rl5yX0l7a0aJH8ATBvt HnJrLp8UP2Cc1SCD0kAEJSc05Omf8+6XXhpRcBmYzUaFossBhnDy7aCNnk+k9kYjLY9z GSq+R7BDnEgQqYh6Lho6kpuGKbcr7fOmz13sz9oMCOYW6ISBuTbMRmMTt5NKcq3YSd8c vHh2Dy0cWxDka/NUakshsWBHhI+eitcQPeBr93NKhDr0D0vLkrEpztpEPUS+VI3W8Yx9 lrEg== X-Gm-Message-State: AOAM530fsXyvnv0yW0cJcwSYNrxdRk61DghDshqQR5sM3Y6Vr0kiYWz/ SXWSoppvrBYjRImEXmkF4/FoI0mHCEOXUZifxtN5Wg== X-Google-Smtp-Source: ABdhPJwaUr0TH4lPLa5FMJrImRPu3iblpyE9hZFNrMyCqKJhe/bQLEuDQvgi+ftqAXbnts12Il5Vkgn/R3iSW/a/ylM= X-Received: by 2002:a9d:3b4:: with SMTP id f49mr14063332otf.188.1605869490350; Fri, 20 Nov 2020 02:51:30 -0800 (PST) MIME-Version: 1.0 References: <20201119144146.1045202-1-daniel.vetter@ffwll.ch> <20201119144146.1045202-10-daniel.vetter@ffwll.ch> <9035555a-af6b-e2dd-dbad-41ca70235e21@xs4all.nl> In-Reply-To: <9035555a-af6b-e2dd-dbad-41ca70235e21@xs4all.nl> From: Daniel Vetter Date: Fri, 20 Nov 2020 11:51:18 +0100 Message-ID: Subject: Re: [PATCH v6 09/17] media/videbuf1|2: Mark follow_pfn usage as unsafe To: Hans Verkuil Cc: DRI Development , LKML , KVM list , Linux MM , Linux ARM , linux-samsung-soc , "open list:DMA BUFFER SHARING FRAMEWORK" , 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On Fri, Nov 20, 2020 at 11:39 AM Hans Verkuil wrote: > > On 20/11/2020 10:18, Daniel Vetter wrote: > > On Fri, Nov 20, 2020 at 9:28 AM Hans Verkuil wrote= : > >> > >> 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 th= e > >>>> 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 c= an > >> move around. There is a mmu_notifier that can be used to be notified w= hen > >> that happens, but that can't be used with media buffers since those bu= ffers > >> 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 continue= s 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 dmabu= f, and > >> frankly for vb1 I don't care. If someone really needs this for a vb1 d= river, > >> 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 b= efore > >> the 'while' explaining why the unsafe_follow_pfn is there and that usi= ng > >> 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 reas= on. > > > > I'm happy to add a comment, but otherwise if you all want to ditch > > this, can we do this as a follow up on top? There's quite a bit of > > code that can be deleted and I'd like to not hold up this patch set > > here on that - it's already a fairly sprawling pain touching about 7 > > different subsystems (ok only 6-ish now since the s390 patch landed). > > For the comment, is the explanation next to unsafe_follow_pfn not good > > enough? > > No, because that doesn't mention that you should use dma-buf as a replace= ment. > That's really the critical piece of information I'd like to see. That doe= sn't > belong in unsafe_follow_pfn, it needs to be in frame_vector.c since it's > vb2 specific. Ah makes sense, I'll add that. > > > > So ... can I get you to un-cancel your ack? > > Hmm, I really would like to see support for this to be dropped completely= . > > How about this: just replace follow_pfn() by -EINVAL instead of unsafe_fo= llow_pfn(). > > Add a TODO comment that this code now can be cleaned up a lot. Such a cle= an up patch > can be added on top later, and actually that is something that I can do o= nce this > series has landed. > > Regardless, frame_vector.c should mention dma-buf as a replacement in a c= omment > since I don't want users who hit this issue to have to dig through git lo= gs > to find that dma-buf is the right approach. > > BTW, nitpick: the subject line of this patch says 'videbuf' instead of 'v= ideobuf'. Will fix to, and next round will have the additional -EINVAL on top. Iirc Mauro was pretty clear that we can't just delete this, so I kinda don't want to get stuck in this discussion with my patches :-) -Daniel > > Regards, > > Hans > > > > > Thanks, Daniel > > > >> > >> 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=C3=A9r=C3=B4me 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 n= ot > >>>> normal memory userptr. The old commit message already explained th= at > >>>> normal memory userptr is unaffected, but I guess that was not clea= r > >>>> 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 <=3D vma->v= m_end) { > >>>> - err =3D follow_pfn(vma, start, &nums[ret]); > >>>> + err =3D unsafe_follow_pfn(vma, start, &nums[ret= ]); > >>>> if (err) { > >>>> if (ret =3D=3D 0) > >>>> ret =3D 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 v= ideobuf_dma_contig_memory *mem, > >>>> user_address =3D untagged_baddr; > >>>> > >>>> while (pages_done < (mem->size >> PAGE_SHIFT)) { > >>>> - ret =3D follow_pfn(vma, user_address, &this_pfn); > >>>> + ret =3D unsafe_follow_pfn(vma, user_address, &this_pfn)= ; > >>>> if (ret) > >>>> break; > >>>> > >>>> > >>> > >> > > > > > --=20 Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch