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=-13.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,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 CFE4EC2D0E4 for ; Tue, 24 Nov 2020 14:17:53 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 358DE2063A for ; Tue, 24 Nov 2020 14:17:53 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="AoDHHXov"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="Jqu3MmOx" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 358DE2063A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=lOn9+GR3pg5HUyfROqlGu0CDFtehLbMvv6JM6T9MXTs=; b=AoDHHXovap9DkWV7C5Qbpoy9d tKyQyXhHqYtSTVkrvVtVKaywuCNSa8YhwuwPt6JfR+3iBzCo+pV2MgsRUfP0uKMUkQUU8vlXPXuyM Hj79M8+XyM+oS00vbcj3o9lyX/eWkDjfuGZlVBwS6tWsa5kpRYP0HcQavLQE0duU0clKtpnGZMEAA y56FgOJqBJ+yhE7Wk0ffDkrI+bGY/iqUSJRaNu6u3E0VN8cMV+giy46LnAjVc2EZyW3sf9Eh4ZsSm ndgQtnIqibLBaSAN52r4GlplQ23bIUcUwUuHccz17KgRrFB5b/E3ccyah3ETB33ReIjUq4YHXgJeo +1njUAsdw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1khZ85-0005Im-4z; Tue, 24 Nov 2020 14:17:13 +0000 Received: from mail-wr1-x442.google.com ([2a00:1450:4864:20::442]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1khZ7r-0005EV-5A for linux-arm-kernel@lists.infradead.org; Tue, 24 Nov 2020 14:17:00 +0000 Received: by mail-wr1-x442.google.com with SMTP id u12so22548078wrt.0 for ; Tue, 24 Nov 2020 06:16:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=lxaE8Not9jvLvGdPE1uCtN/cVkn9pPEtSLdbQyT9QXw=; b=Jqu3MmOxKtv9AEVJ44kKeuVO3gO1NOKfm+ACsbPcrxj2eHlEcRRMnE0TnNyC+zJYnI MKrPTvaoz4din/kg4k7SyOw5E6Rymxd/rfxXl/XrvCFIkFNXPpHJXYjcLwa+ea5x8oiU SW2i+f/oPr9w2wLEKdEgW7VtTgihlydGLaYjg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :content-transfer-encoding:in-reply-to; bh=lxaE8Not9jvLvGdPE1uCtN/cVkn9pPEtSLdbQyT9QXw=; b=BR/1T56OKXNoSTjTjRazyQFDI5jhng4J8cWcMGNR6JUtltBrRW90pK4fHOlgZJUlt8 M/NhiWOmFlUYpZnd5vPT0Um/uaPWj0ZQ2YPCgoVu6pI5zK1Sv2i8V69LHp2F1AhSsrHa /bbMmAk8Ho1dkP8xj/qvA1a+CaLdM14S4uNSUAuHckDuLR8WP93XLKsLUp6/rGDqYF1W /WTiu9XJvvHuEp2B7SDX6w84DXR4iM2Az5lTYtQDH3oYWslTqvq8nwjZyGJy/rd56W7a blZJTBybJYkWqHeC0iUe1BYtuunocqFiG9zPez5LImRugrr6acwuZHnxqkxJLvja7Epo RRlQ== X-Gm-Message-State: AOAM532Xmi8eWxPtXI+H59gOujGU3/lx0bgOesTq7UjaHjIEJrzfndOp tCwHOMTQ6Dz+dVZaSv/jB5diBA== X-Google-Smtp-Source: ABdhPJyvnAcNxBa7hPAnh3R0lC3dqHK5F5aliN5y0/6utGHWrgRKu8r7ObJFr2IEqd22MvzLgTn5ig== X-Received: by 2002:adf:9cc6:: with SMTP id h6mr5554394wre.341.1606227416430; Tue, 24 Nov 2020 06:16:56 -0800 (PST) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id h2sm6243879wme.45.2020.11.24.06.16.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 24 Nov 2020 06:16:55 -0800 (PST) Date: Tue, 24 Nov 2020 15:16:52 +0100 From: Daniel Vetter To: Tomasz Figa Subject: Re: [PATCH v6 09/17] media/videbuf1|2: Mark follow_pfn usage as unsafe Message-ID: <20201124141652.GL401619@phenom.ffwll.local> Mail-Followup-To: Tomasz Figa , Hans Verkuil , DRI Development , LKML , KVM list , Linux MM , Linux ARM , linux-samsung-soc , "open list:DMA BUFFER SHARING FRAMEWORK" , Daniel Vetter , Jason Gunthorpe , Kees Cook , Dan Williams , Andrew Morton , John Hubbard , =?iso-8859-1?B?Suly9G1l?= Glisse , 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> <9035555a-af6b-e2dd-dbad-41ca70235e21@xs4all.nl> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Operating-System: Linux phenom 5.7.0-1-amd64 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201124_091659_361677_8972B068 X-CRM114-Status: GOOD ( 61.57 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Jan Kara , KVM list , Daniel Vetter , DRI Development , Hans Verkuil , Linux MM , Daniel Vetter , Michel Lespinasse , Marek Szyprowski , linux-samsung-soc , Daniel Jordan , Jason Gunthorpe , Linux ARM , "open list:DMA BUFFER SHARING FRAMEWORK" , Kees Cook , Pawel Osciak , John Hubbard , Mauro Carvalho Chehab , =?iso-8859-1?B?Suly9G1l?= Glisse , Dan Williams , Laurent Dufour , Vlastimil Babka , LKML , Kyungmin Park , Andrew Morton Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Nov 20, 2020 at 09:23:12PM +0900, Tomasz Figa wrote: > On Fri, Nov 20, 2020 at 9:08 PM Hans Verkuil wrote: > > > > On 20/11/2020 11:51, Daniel Vetter wrote: > > > On Fri, Nov 20, 2020 at 11:39 AM Hans Verkuil wr= ote: > > >> > > >> On 20/11/2020 10:18, Daniel Vetter wrote: > > >>> On Fri, Nov 20, 2020 at 9:28 AM Hans Verkuil w= rote: > > >>>> > > >>>> 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 th= at > > >>>>>> when a media pipeline is running we never miss a deadline becaus= e 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 a= nd > > >>>>>> tell everyone to cut over to dma-buf memory sharing for zerocopy. > > >>>>>> > > >>>>>> userptr for normal memory will keep working as-is, this only aff= ects > > >>>>>> 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 mappin= gs can > > >>>> move around. There is a mmu_notifier that can be used to be notifi= ed when > > >>>> that happens, but that can't be used with media buffers since thos= e 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 fa= il, if it > > >>>> is unset, then it writes a warning to the kernel log but just cont= inues 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 d= mabuf, and > > >>>> frankly for vb1 I don't care. If someone really needs this for a v= b1 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_PFN= MAP > > >>>> should just be killed off. > > >>>> > > >>>> If others would like to keep it, then frame_vector.c needs a comme= nt 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. > > >>> > > >>> 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 g= ood > > >>> enough? > > >> > > >> No, because that doesn't mention that you should use dma-buf as a re= placement. > > >> That's really the critical piece of information I'd like to see. Tha= t doesn'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 compl= etely. > > >> > > >> How about this: just replace follow_pfn() by -EINVAL instead of unsa= fe_follow_pfn(). > > >> > > >> Add a TODO comment that this code now can be cleaned up a lot. Such = a clean up patch > > >> can be added on top later, and actually that is something that I can= do once this > > >> series has landed. > > >> > > >> Regardless, frame_vector.c should mention dma-buf as a replacement i= n a comment > > >> since I don't want users who hit this issue to have to dig through g= it logs > > >> to find that dma-buf is the right approach. > > >> > > >> BTW, nitpick: the subject line of this patch says 'videbuf' instead = of 'videobuf'. > > > > > > 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 :-) > > > > Ah, I found that discussion for the v2 of this series. > > > > Yes, add that on top and we can discuss whether to Ack that -EINVAL pat= ch or > > not. > > > > I don't see why we would want to continue supporting a broken model tha= t is > > also a security risk, as I understand it. > > > > Tomasz, can you look at the discussion for this old RFC patch of mine: > > > > https://patchwork.linuxtv.org/project/linux-media/patch/20200221084531.= 576156-9-hverkuil-cisco@xs4all.nl/ > > > > Specifically, if we just drop support for follow_pfn(), would that cause > > problems for Chromium since that is apparently still using USERPTR for = encoders? > > > = > Nope, we use regular page-backed user pointers and not IO/PFNMAP ones. > = > By the way, for any inter-device sharing we're using DMABUF. USERPTR > is left only in case of the data coming from the CPU, e.g. network. Yeah Mauro wasn't too enthusiastic even about this patch here, so I think I'll just leave it as-is. I fixed the typo in the commit message subject. -Daniel > = > > Regards, > > > > Hans > > > > > -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=E9r=F4me 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 cas= e to > > >>>>>> make it abundandtly clear that this patch only affects that, a= nd not > > >>>>>> normal memory userptr. The old commit message already explaine= d 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/dri= vers/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, unsi= gned int nr_frames, > > >>>>>> break; > > >>>>>> > > >>>>>> while (ret < nr_frames && start + PAGE_SIZE <=3D vm= a->vm_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/dri= vers/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(stru= ct videobuf_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; > > >>>>>> > > >>>>>> > > >>>>> > > >>>> > > >>> > > >>> > > >> > > > > > > > > -- = Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel