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.1 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,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=ham 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 76805C56202 for ; Fri, 20 Nov 2020 10:39:10 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 9878A22403 for ; Fri, 20 Nov 2020 10:39:09 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=xs4all.nl header.i=@xs4all.nl header.b="kcuAXQ1B" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9878A22403 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=xs4all.nl Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id AC19D6B0036; Fri, 20 Nov 2020 05:39:08 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id A70D66B005C; Fri, 20 Nov 2020 05:39:08 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 960706B005D; Fri, 20 Nov 2020 05:39:08 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0208.hostedemail.com [216.40.44.208]) by kanga.kvack.org (Postfix) with ESMTP id 5E65C6B0036 for ; Fri, 20 Nov 2020 05:39:08 -0500 (EST) Received: from smtpin04.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id D0BFD8249980 for ; Fri, 20 Nov 2020 10:39:07 +0000 (UTC) X-FDA: 77504449134.04.paint52_110f6c82734a Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin04.hostedemail.com (Postfix) with ESMTP id B0D96800FCC7 for ; Fri, 20 Nov 2020 10:39:07 +0000 (UTC) X-HE-Tag: paint52_110f6c82734a X-Filterd-Recvd-Size: 11062 Received: from lb2-smtp-cloud9.xs4all.net (lb2-smtp-cloud9.xs4all.net [194.109.24.26]) by imf04.hostedemail.com (Postfix) with ESMTP for ; Fri, 20 Nov 2020 10:39:06 +0000 (UTC) Received: from cust-b5b5937f ([IPv6:fc0c:c16d:66b8:757f:c639:739b:9d66:799d]) by smtp-cloud9.xs4all.net with ESMTPA id g3odkqci3WTbog3ogkflSv; Fri, 20 Nov 2020 11:39:04 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=xs4all.nl; s=s2; t=1605868744; bh=UJbsoc+m2DQXtat1J+FSEFEQVZTUCPoXOGzxzQw644s=; h=Subject:To:From:Message-ID:Date:MIME-Version:Content-Type:From: Subject; b=kcuAXQ1B/GTzWwQml12T3GbJXUhrFJRX5LaU3qV0GdLQGzXoDChd8SkjAllwPt3zH lub6Gp+k/Kvre6ug0Fa5Abl8/Hrxr0ywrpyek9moKX8pNHcYfPWWCVh4VjaK2tgHsa GkgnglWwjplRJEYBPSj2a8s+e4KZ/ySUEeABdvYY9EcNL3hnaU2R9DFQbBX4AvDwYl v55zKKQOeWUtg0JTeDGQ/aETRbVDYpz0piVOHQi0i4kFvqiTbOOJRECUUwNxNUeFKx yUK2HHoEGu/IzTAWVM6QKCSprtR9qfLYwNS6rKhxbvJQHHGoStiTaI9a0DXd7Pp9sP p9N9Sjv3vSUGg== Subject: Re: [PATCH v6 09/17] media/videbuf1|2: Mark follow_pfn usage as unsafe To: Daniel Vetter 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 References: <20201119144146.1045202-1-daniel.vetter@ffwll.ch> <20201119144146.1045202-10-daniel.vetter@ffwll.ch> From: Hans Verkuil Message-ID: <9035555a-af6b-e2dd-dbad-41ca70235e21@xs4all.nl> Date: Fri, 20 Nov 2020 11:38:55 +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 X-CMAE-Envelope: MS4xfDss0FwL2fqdI2i/Ld6jk5YpMRTothqQOe/9Yd2P1xeqdCmohX7XXb96LZlza31qpcmNMYJwPa81XchZVa1S7q0JJklTc6tNeVGRAAYdr+AbM+yqKuxh HYR4y9BVgxtQf4jgcpuWAISFzS9JO9asb6c6vsbpOUMsoZVmKvX3blng9eCgwzmJEFWVy6RFBjEP+YuywmY0Cbsra6piUdkAnXxA28LQBb4c4pANgMwK3/20 bqwtn2oqGhmwrstC4HjCP7+ky7n9e+nny11fIZoZXpYud8ngwlCvv3JO5DEhppBBMyU4wzh6wey7nRYMfQcZKL3tBnPLxG084wGKlaIDxqlPIqzON2yPqUtM BdjovZp1X9w67sDw2YnPtJI+t5QjtH7wDE8HymLGzpo/iSWefSev4TB6DyT8z5q4VwW3nHLvv+AKe+XDfQUaFLrmIHckyHvzBresJqeVQNqozNuU1qToV+l/ ErEPsay/ueUS5Hj5b6jRplY77yT/PNOmchZCs7zBkOUY6YONev0nZYJIoJ6WsUMULo52xVhX1Tta1mgMr4SugcE1W1C9YPZPWPqPWe/3L6bhxjXGPcTMvgGG EfWAwqzm38P0NR/GybtBl/gWm/ZJOCla09xasqa1HdLeHPFcGIUr2RcK8vk1M1FL+s1q9wqIJ1k+h51TQcL8ss+oalkYZ+Es/zXbGn8CGFesXujAIbbI9SeX RbG5KoDDqiF0i/z9VR5/dyz1ChlFanbUWs7vGG6zLRPUB94JMiDLcyq0sgkrQslnPzRt5atIb4ACU1K5prEyGZXzYGJ67wH+5Ote3N/ZKwr9ZzDCJOlZyz1Y 7NCIKU+LJw+dc7vff5v2/nfaydMU8yk1Sjiw2vMM60fyIjtgLWiysLdmBcHztcTuIRFp2fTCyaD4dNzasFyk6gZzUhJytpwVEiTZ4cIcT8wwnB8yzHSlynCb PBnOPcalqnNL1YwgL4G+XJik3k/M6qqSrY43N1O21jk+k7JmerrFuMJh03HUnX+RhONUkg== Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: 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. >=20 > 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. >=20 > 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'. Regards, Hans >=20 > Thanks, Daniel >=20 >> >> 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 >=20