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=-3.9 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 51F70C4363D for ; Wed, 7 Oct 2020 14:23:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D5098212CC for ; Wed, 7 Oct 2020 14:23:08 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="HxN9CeEl" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728567AbgJGOXH (ORCPT ); Wed, 7 Oct 2020 10:23:07 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58938 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728053AbgJGOXH (ORCPT ); Wed, 7 Oct 2020 10:23:07 -0400 Received: from mail-oi1-x244.google.com (mail-oi1-x244.google.com [IPv6:2607:f8b0:4864:20::244]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 40705C0613D3 for ; Wed, 7 Oct 2020 07:23:07 -0700 (PDT) Received: by mail-oi1-x244.google.com with SMTP id 26so2606383ois.5 for ; Wed, 07 Oct 2020 07:23:07 -0700 (PDT) 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; bh=C6FRvFu0sftzaO7u7hAn4jdJ4FF4CGkT7QzNcdFtRx0=; b=HxN9CeEl6dg2fnJo70BaHxKXccc7EEzG66whVAVFPq4AH9BV/iG5k/B+RBQ74hh3Qh vMBh897K/acdcVcspzwrD+Be0V1z7QbLcaLqhtoxu656rcFUTrqE+PYX+Jopp+M50JUE 1UpmjO7U9bHLA27shpixoevJKUgn1Y0vP2DSM= 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; bh=C6FRvFu0sftzaO7u7hAn4jdJ4FF4CGkT7QzNcdFtRx0=; b=r2uiCQPQyqJAE7gVpcS5Tx2EQWpqMI/LNvmDfudag12bmuheogVraRguDmjs8QzijA COgnZybrHnrKK4Tyq+hVRt0Jg6Q2HKl8yOJF37G5/6Ms7KIgfs/60jKIs7E0lLcdN89j CNmy5ugl80Gbh1rO1Ztm84vTLEEL+5Cttsj4ORgLSFbmQVqorWiz755D6ogqHzpDJmdl FVp+dunaKwlW1ZPJkxjSnLOh+33cAPqMTEZnf1v2RAnIt646qA+SIEAZ0pdesuAmhrfm UhDdl3QnPh7kWE+tfvAmURigv1QV5RUcbkov0JCV9wsHfuMRYuybW/Yo3yNLCDb0LFZc BA1w== X-Gm-Message-State: AOAM532L0DkOu/1FCww5vGpqdXewd6bv0LvIttXxXBQEXZca52ImrPUy 0uEorcxfr6A9S2N4FqeZ/dw4rwWD4isIdSoRqsWDuA== X-Google-Smtp-Source: ABdhPJykVRNPxZpjwtBJp2l3oQqrHTgzTF48u7oukqSME6wbdcmRxZ9GAc1QKmyX+l7NgyvInm/j9HuaUKL8z913hkQ= X-Received: by 2002:aca:6083:: with SMTP id u125mr2144090oib.14.1602080586311; Wed, 07 Oct 2020 07:23:06 -0700 (PDT) MIME-Version: 1.0 References: <20201002233118.GM9916@ziepe.ca> <725819e9-4f07-3f04-08f8-b6180406b339@samsung.com> <20201007124409.GN5177@ziepe.ca> <20201007130610.GP5177@ziepe.ca> In-Reply-To: From: Daniel Vetter Date: Wed, 7 Oct 2020 16:22:54 +0200 Message-ID: Subject: Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM To: Tomasz Figa Cc: Jason Gunthorpe , Marek Szyprowski , DRI Development , LKML , Daniel Vetter , Andrew Morton , John Hubbard , =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , Jan Kara , Dan Williams , Linux MM , Linux ARM , Pawel Osciak , Kyungmin Park , Inki Dae , Joonyoung Shim , Seung-Woo Kim , linux-samsung-soc , "open list:DMA BUFFER SHARING FRAMEWORK" , Oded Gabbay Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Oct 7, 2020 at 4:12 PM Tomasz Figa wrote: > > On Wed, Oct 7, 2020 at 4:09 PM Daniel Vetter wrote: > > > > On Wed, Oct 7, 2020 at 3:34 PM Tomasz Figa wrote: > > > > > > On Wed, Oct 7, 2020 at 3:06 PM Jason Gunthorpe wrote: > > > > > > > > On Wed, Oct 07, 2020 at 02:58:33PM +0200, Daniel Vetter wrote: > > > > > On Wed, Oct 7, 2020 at 2:48 PM Tomasz Figa wrote: > > > > > > > > > > > > On Wed, Oct 7, 2020 at 2:44 PM Jason Gunthorpe wrote: > > > > > > > > > > > > > > On Wed, Oct 07, 2020 at 02:33:56PM +0200, Marek Szyprowski wrote: > > > > > > > > Well, it was in vb2_get_vma() function, but now I see that it has been > > > > > > > > lost in fb639eb39154 and 6690c8c78c74 some time ago... > > > > > > > > > > > > > > There is no guarentee that holding a get on the file says anthing > > > > > > > about the VMA. This needed to check that the file was some special > > > > > > > kind of file that promised the VMA layout and file lifetime are > > > > > > > connected. > > > > > > > > > > > > > > Also, cloning a VMA outside the mm world is just really bad. That > > > > > > > would screw up many assumptions the drivers make. > > > > > > > > > > > > > > If it is all obsolete I say we hide it behind a default n config > > > > > > > symbol and taint the kernel if anything uses it. > > > > > > > > > > > > > > Add a big comment above the follow_pfn to warn others away from this > > > > > > > code. > > > > > > > > > > > > Sadly it's just verbally declared as deprecated and not formally noted > > > > > > anyway. There are a lot of userspace applications relying on user > > > > > > pointer support. > > > > > > > > > > userptr can stay, it's the userptr abuse for zerocpy buffer sharing > > > > > which doesn't work anymore. At least without major surgery (you'd need > > > > > an mmu notifier to zap mappings and recreate them, and that pretty > > > > > much breaks the v4l model of preallocating all buffers to make sure we > > > > > never underflow the buffer queue). And static mappings are not coming > > > > > back I think, we'll go ever more into the direction of dynamic > > > > > mappings and moving stuff around as needed. > > > > > > > > Right, and to be clear, the last time I saw a security flaw of this > > > > magnitude from a subsystem badly mis-designing itself, Linus's > > > > knee-jerk reaction was to propose to remove the whole subsystem. > > > > > > > > Please don't take status-quo as acceptable, V4L community has to work > > > > to resolve this, uABI breakage or not. The follow_pfn related code > > > > must be compiled out of normal distro kernel builds. > > > > > > I think the userptr zero-copy hack should be able to go away indeed, > > > given that we now have CMA that allows having carveouts backed by > > > struct pages and having the memory represented as DMA-buf normally. > > > > Not sure whether there's a confusion here: dma-buf supports memory not > > backed by struct page. > > > > That's new to me. The whole API relies on sg_tables a lot, which in > turn rely on struct page pointers to describe the physical memory. You're not allowed to look at struct page pointers from the importer side, those might not be there. Which isn't the prettiest thing, but it works. And even if there's a struct page, you're still not allowed to look at it, since it's fully managed by the exporter under whatever rules that might need. So no touching it, ever. This is also not news, supporting this was in the design brief from the kickoff session 10+ years ago at some linaro connect thing (in Budapest iirc). And we have implementations doing that for almost as long merged in upstream. > > > How about the regular userptr use case, though? > > > > > > The existing code resolves the user pointer into pages by following > > > the get_vaddr_frames() -> frame_vector_to_pages() -> > > > sg_alloc_table_from_pages() / vm_map_ram() approach. > > > get_vaddr_frames() seems to use pin_user_pages() behind the scenes if > > > the vma is not an IO or a PFNMAP, falling back to follow_pfn() > > > otherwise. > > > > Yeah pin_user_pages is fine, it's just the VM_IO | VM_PFNMAP vma that > > don't work. > > Ack. > > > > > > > Is your intention to drop get_vaddr_frames() or we could still keep > > > using it and if vec->is_pfns is true: > > > a) if CONFIG_VIDEO_LEGACY_PFN_USERPTR is set, taint the kernel > > > b) otherwise just undo and fail? > > > > I'm typing that patch series (plus a pile more) right now. > > Cool, thanks! > > We also need to bring back the vma_open() that somehow disappeared > around 4.2, as Marek found. The vm_open isn't enough to stop the problems (it doesn't and cannot protect against unmap_mapping_range), I don't think keeping an incomplete solution around has much benefit. People who need this can disable CONFIG_STRICT_FOLLOW_PFN to keep things working, everyone else probably doesn't want these mm internals leaking into the media subsystem. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch 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=-4.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 B8ECDC4363D for ; Wed, 7 Oct 2020 14:24:31 +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 33C1E2076C for ; Wed, 7 Oct 2020 14:24:31 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="U28zLcU4"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="HxN9CeEl" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 33C1E2076C 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:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=LH8sq8opmvQatm7mlP0N72qDgmF9fzs1yf1vVER/+Yg=; b=U28zLcU4L+H3CJmgelxF6T8Dj b7P2lPFmFfpZ+MVKsOlwsQT3G5wRoxtc5lE2TPq/4tshTWZhFsRdaiv4NV4nLLKxJ5KsTnve8Gad7 kkFcjRDA1+57px2hjDvb3vdHPzpSFdtXJQBOVtuXNibQWtVqEezuV2OHEl3LsyRzlFMl61RbJiqwb CUZ9sHwpvDA5x/ikrKncLA2UCOEBkj1TPTmFTxJkmTBc90K03vVFMOiZ/aPBGk3RfUJjzVncS79yY H3qcyAc9is5VqmDxPisBN2nWpiB6g5rsnF+1soK7DQiHGWHd3jsHPtPsgVZLl0L/EzPnBi50bSpG+ j/9q2VnDA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kQALX-0003XN-8l; Wed, 07 Oct 2020 14:23:11 +0000 Received: from mail-oi1-x241.google.com ([2607:f8b0:4864:20::241]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kQALU-0003W5-8O for linux-arm-kernel@lists.infradead.org; Wed, 07 Oct 2020 14:23:09 +0000 Received: by mail-oi1-x241.google.com with SMTP id c13so2594408oiy.6 for ; Wed, 07 Oct 2020 07:23:07 -0700 (PDT) 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; bh=C6FRvFu0sftzaO7u7hAn4jdJ4FF4CGkT7QzNcdFtRx0=; b=HxN9CeEl6dg2fnJo70BaHxKXccc7EEzG66whVAVFPq4AH9BV/iG5k/B+RBQ74hh3Qh vMBh897K/acdcVcspzwrD+Be0V1z7QbLcaLqhtoxu656rcFUTrqE+PYX+Jopp+M50JUE 1UpmjO7U9bHLA27shpixoevJKUgn1Y0vP2DSM= 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; bh=C6FRvFu0sftzaO7u7hAn4jdJ4FF4CGkT7QzNcdFtRx0=; b=UxjLA8TQxyDzvySPNYL0uOMd9vZWjECUqTM0JStrC56Ym+dTbUepurZhxAl0j54Zj/ lQgvwRweNE3AkFx/0ep+IS7ryQj+SvtN2c7fS6QP3ryahC4ipvYHs42Zsb4zd45jn+nT /3ZionM8fIhS0lXXsOpjyABOf30BEYkVWdEaOHWizTXnokho+vx4KkWJvV2sJRATdwLN LlS77Qr0LTtNeELJf+PfBo+dY64ZojykInCWlPBxlGVqFC20FXcAP+Wr0J8SNCTHp2Ue eF+CTYuXwc7jp/oyrIddVAMZ3+hLRVFsBcDIV2szM8xElRkzEFpdv0H/M3dJy4yR0rI1 9/5g== X-Gm-Message-State: AOAM533+B3Wmq7nEt9asf0qw9H/qN54m51jtG2+w4T5ARaFF/SkOx/k+ KdB2MXKgxw5ukN1SM3K05rHSoPKcZuHfixbmJ9brEQ== X-Google-Smtp-Source: ABdhPJykVRNPxZpjwtBJp2l3oQqrHTgzTF48u7oukqSME6wbdcmRxZ9GAc1QKmyX+l7NgyvInm/j9HuaUKL8z913hkQ= X-Received: by 2002:aca:6083:: with SMTP id u125mr2144090oib.14.1602080586311; Wed, 07 Oct 2020 07:23:06 -0700 (PDT) MIME-Version: 1.0 References: <20201002233118.GM9916@ziepe.ca> <725819e9-4f07-3f04-08f8-b6180406b339@samsung.com> <20201007124409.GN5177@ziepe.ca> <20201007130610.GP5177@ziepe.ca> In-Reply-To: From: Daniel Vetter Date: Wed, 7 Oct 2020 16:22:54 +0200 Message-ID: Subject: Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM To: Tomasz Figa X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201007_102308_320752_054475F1 X-CRM114-Status: GOOD ( 42.65 ) 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: Oded Gabbay , Inki Dae , linux-samsung-soc , Jan Kara , Joonyoung Shim , Pawel Osciak , Linux MM , John Hubbard , Seung-Woo Kim , LKML , DRI Development , Kyungmin Park , Jason Gunthorpe , =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , Daniel Vetter , Andrew Morton , "open list:DMA BUFFER SHARING FRAMEWORK" , Dan Williams , Linux ARM , Marek Szyprowski Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Oct 7, 2020 at 4:12 PM Tomasz Figa wrote: > > On Wed, Oct 7, 2020 at 4:09 PM Daniel Vetter wrote: > > > > On Wed, Oct 7, 2020 at 3:34 PM Tomasz Figa wrote: > > > > > > On Wed, Oct 7, 2020 at 3:06 PM Jason Gunthorpe wrote: > > > > > > > > On Wed, Oct 07, 2020 at 02:58:33PM +0200, Daniel Vetter wrote: > > > > > On Wed, Oct 7, 2020 at 2:48 PM Tomasz Figa wrote: > > > > > > > > > > > > On Wed, Oct 7, 2020 at 2:44 PM Jason Gunthorpe wrote: > > > > > > > > > > > > > > On Wed, Oct 07, 2020 at 02:33:56PM +0200, Marek Szyprowski wrote: > > > > > > > > Well, it was in vb2_get_vma() function, but now I see that it has been > > > > > > > > lost in fb639eb39154 and 6690c8c78c74 some time ago... > > > > > > > > > > > > > > There is no guarentee that holding a get on the file says anthing > > > > > > > about the VMA. This needed to check that the file was some special > > > > > > > kind of file that promised the VMA layout and file lifetime are > > > > > > > connected. > > > > > > > > > > > > > > Also, cloning a VMA outside the mm world is just really bad. That > > > > > > > would screw up many assumptions the drivers make. > > > > > > > > > > > > > > If it is all obsolete I say we hide it behind a default n config > > > > > > > symbol and taint the kernel if anything uses it. > > > > > > > > > > > > > > Add a big comment above the follow_pfn to warn others away from this > > > > > > > code. > > > > > > > > > > > > Sadly it's just verbally declared as deprecated and not formally noted > > > > > > anyway. There are a lot of userspace applications relying on user > > > > > > pointer support. > > > > > > > > > > userptr can stay, it's the userptr abuse for zerocpy buffer sharing > > > > > which doesn't work anymore. At least without major surgery (you'd need > > > > > an mmu notifier to zap mappings and recreate them, and that pretty > > > > > much breaks the v4l model of preallocating all buffers to make sure we > > > > > never underflow the buffer queue). And static mappings are not coming > > > > > back I think, we'll go ever more into the direction of dynamic > > > > > mappings and moving stuff around as needed. > > > > > > > > Right, and to be clear, the last time I saw a security flaw of this > > > > magnitude from a subsystem badly mis-designing itself, Linus's > > > > knee-jerk reaction was to propose to remove the whole subsystem. > > > > > > > > Please don't take status-quo as acceptable, V4L community has to work > > > > to resolve this, uABI breakage or not. The follow_pfn related code > > > > must be compiled out of normal distro kernel builds. > > > > > > I think the userptr zero-copy hack should be able to go away indeed, > > > given that we now have CMA that allows having carveouts backed by > > > struct pages and having the memory represented as DMA-buf normally. > > > > Not sure whether there's a confusion here: dma-buf supports memory not > > backed by struct page. > > > > That's new to me. The whole API relies on sg_tables a lot, which in > turn rely on struct page pointers to describe the physical memory. You're not allowed to look at struct page pointers from the importer side, those might not be there. Which isn't the prettiest thing, but it works. And even if there's a struct page, you're still not allowed to look at it, since it's fully managed by the exporter under whatever rules that might need. So no touching it, ever. This is also not news, supporting this was in the design brief from the kickoff session 10+ years ago at some linaro connect thing (in Budapest iirc). And we have implementations doing that for almost as long merged in upstream. > > > How about the regular userptr use case, though? > > > > > > The existing code resolves the user pointer into pages by following > > > the get_vaddr_frames() -> frame_vector_to_pages() -> > > > sg_alloc_table_from_pages() / vm_map_ram() approach. > > > get_vaddr_frames() seems to use pin_user_pages() behind the scenes if > > > the vma is not an IO or a PFNMAP, falling back to follow_pfn() > > > otherwise. > > > > Yeah pin_user_pages is fine, it's just the VM_IO | VM_PFNMAP vma that > > don't work. > > Ack. > > > > > > > Is your intention to drop get_vaddr_frames() or we could still keep > > > using it and if vec->is_pfns is true: > > > a) if CONFIG_VIDEO_LEGACY_PFN_USERPTR is set, taint the kernel > > > b) otherwise just undo and fail? > > > > I'm typing that patch series (plus a pile more) right now. > > Cool, thanks! > > We also need to bring back the vma_open() that somehow disappeared > around 4.2, as Marek found. The vm_open isn't enough to stop the problems (it doesn't and cannot protect against unmap_mapping_range), I don't think keeping an incomplete solution around has much benefit. People who need this can disable CONFIG_STRICT_FOLLOW_PFN to keep things working, everyone else probably doesn't want these mm internals leaking into the media subsystem. -Daniel -- 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 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=-3.5 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no 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 24745C41604 for ; Wed, 7 Oct 2020 14:23:10 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (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 635CF212CC for ; Wed, 7 Oct 2020 14:23:09 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="HxN9CeEl" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 635CF212CC Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 3253F6E0AF; Wed, 7 Oct 2020 14:23:08 +0000 (UTC) Received: from mail-oi1-x243.google.com (mail-oi1-x243.google.com [IPv6:2607:f8b0:4864:20::243]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1AA3F6E0AF for ; Wed, 7 Oct 2020 14:23:07 +0000 (UTC) Received: by mail-oi1-x243.google.com with SMTP id x62so2565841oix.11 for ; Wed, 07 Oct 2020 07:23:07 -0700 (PDT) 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; bh=C6FRvFu0sftzaO7u7hAn4jdJ4FF4CGkT7QzNcdFtRx0=; b=HxN9CeEl6dg2fnJo70BaHxKXccc7EEzG66whVAVFPq4AH9BV/iG5k/B+RBQ74hh3Qh vMBh897K/acdcVcspzwrD+Be0V1z7QbLcaLqhtoxu656rcFUTrqE+PYX+Jopp+M50JUE 1UpmjO7U9bHLA27shpixoevJKUgn1Y0vP2DSM= 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; bh=C6FRvFu0sftzaO7u7hAn4jdJ4FF4CGkT7QzNcdFtRx0=; b=mx78NNF4jphTpIvWYZV5D30/nwgAJ3leMLmUR9/Gbsw0ruEm1tUQNKMm9vqsWI1QYx 8vMFWocFyDlCYuIqH82JFqidwJ6Bk0ufhv/Wx7oEV9xrNLKFYDDcAbF+/UxyTZxvN4zO +O2wSsPto4i9B3YxPu64+9p5xgkwu4RVY41eo5R+VdssRZ4qmFx72t6oAiCw7PhqVS1O CL4kHV3n1n5qQydPOC6p6AQsnIKpsPaozfoIp2AjVmSPYtowI4dVqrP8raR15OlOa7Y8 8iEPz5izdEHV5yOzfMtmnLWm9SZbHt6YpFOw9jl3GmLQ043nRCif4ISbtHGMj35Ahlxo vnuw== X-Gm-Message-State: AOAM532lWujIyhnnqIHs9BabUYEIlpCKug3+yTuzrTuDdgEY/4p9suu3 fPSkCIYWH8We5BgU4PDUh5tuGcov60cvIyx3OQvCWwEUGppjqg== X-Google-Smtp-Source: ABdhPJykVRNPxZpjwtBJp2l3oQqrHTgzTF48u7oukqSME6wbdcmRxZ9GAc1QKmyX+l7NgyvInm/j9HuaUKL8z913hkQ= X-Received: by 2002:aca:6083:: with SMTP id u125mr2144090oib.14.1602080586311; Wed, 07 Oct 2020 07:23:06 -0700 (PDT) MIME-Version: 1.0 References: <20201002233118.GM9916@ziepe.ca> <725819e9-4f07-3f04-08f8-b6180406b339@samsung.com> <20201007124409.GN5177@ziepe.ca> <20201007130610.GP5177@ziepe.ca> In-Reply-To: From: Daniel Vetter Date: Wed, 7 Oct 2020 16:22:54 +0200 Message-ID: Subject: Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM To: Tomasz Figa X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-samsung-soc , Jan Kara , Joonyoung Shim , Pawel Osciak , Linux MM , John Hubbard , Seung-Woo Kim , LKML , DRI Development , Kyungmin Park , Jason Gunthorpe , =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , Daniel Vetter , Andrew Morton , "open list:DMA BUFFER SHARING FRAMEWORK" , Dan Williams , Linux ARM , Marek Szyprowski Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Wed, Oct 7, 2020 at 4:12 PM Tomasz Figa wrote: > > On Wed, Oct 7, 2020 at 4:09 PM Daniel Vetter wrote: > > > > On Wed, Oct 7, 2020 at 3:34 PM Tomasz Figa wrote: > > > > > > On Wed, Oct 7, 2020 at 3:06 PM Jason Gunthorpe wrote: > > > > > > > > On Wed, Oct 07, 2020 at 02:58:33PM +0200, Daniel Vetter wrote: > > > > > On Wed, Oct 7, 2020 at 2:48 PM Tomasz Figa wrote: > > > > > > > > > > > > On Wed, Oct 7, 2020 at 2:44 PM Jason Gunthorpe wrote: > > > > > > > > > > > > > > On Wed, Oct 07, 2020 at 02:33:56PM +0200, Marek Szyprowski wrote: > > > > > > > > Well, it was in vb2_get_vma() function, but now I see that it has been > > > > > > > > lost in fb639eb39154 and 6690c8c78c74 some time ago... > > > > > > > > > > > > > > There is no guarentee that holding a get on the file says anthing > > > > > > > about the VMA. This needed to check that the file was some special > > > > > > > kind of file that promised the VMA layout and file lifetime are > > > > > > > connected. > > > > > > > > > > > > > > Also, cloning a VMA outside the mm world is just really bad. That > > > > > > > would screw up many assumptions the drivers make. > > > > > > > > > > > > > > If it is all obsolete I say we hide it behind a default n config > > > > > > > symbol and taint the kernel if anything uses it. > > > > > > > > > > > > > > Add a big comment above the follow_pfn to warn others away from this > > > > > > > code. > > > > > > > > > > > > Sadly it's just verbally declared as deprecated and not formally noted > > > > > > anyway. There are a lot of userspace applications relying on user > > > > > > pointer support. > > > > > > > > > > userptr can stay, it's the userptr abuse for zerocpy buffer sharing > > > > > which doesn't work anymore. At least without major surgery (you'd need > > > > > an mmu notifier to zap mappings and recreate them, and that pretty > > > > > much breaks the v4l model of preallocating all buffers to make sure we > > > > > never underflow the buffer queue). And static mappings are not coming > > > > > back I think, we'll go ever more into the direction of dynamic > > > > > mappings and moving stuff around as needed. > > > > > > > > Right, and to be clear, the last time I saw a security flaw of this > > > > magnitude from a subsystem badly mis-designing itself, Linus's > > > > knee-jerk reaction was to propose to remove the whole subsystem. > > > > > > > > Please don't take status-quo as acceptable, V4L community has to work > > > > to resolve this, uABI breakage or not. The follow_pfn related code > > > > must be compiled out of normal distro kernel builds. > > > > > > I think the userptr zero-copy hack should be able to go away indeed, > > > given that we now have CMA that allows having carveouts backed by > > > struct pages and having the memory represented as DMA-buf normally. > > > > Not sure whether there's a confusion here: dma-buf supports memory not > > backed by struct page. > > > > That's new to me. The whole API relies on sg_tables a lot, which in > turn rely on struct page pointers to describe the physical memory. You're not allowed to look at struct page pointers from the importer side, those might not be there. Which isn't the prettiest thing, but it works. And even if there's a struct page, you're still not allowed to look at it, since it's fully managed by the exporter under whatever rules that might need. So no touching it, ever. This is also not news, supporting this was in the design brief from the kickoff session 10+ years ago at some linaro connect thing (in Budapest iirc). And we have implementations doing that for almost as long merged in upstream. > > > How about the regular userptr use case, though? > > > > > > The existing code resolves the user pointer into pages by following > > > the get_vaddr_frames() -> frame_vector_to_pages() -> > > > sg_alloc_table_from_pages() / vm_map_ram() approach. > > > get_vaddr_frames() seems to use pin_user_pages() behind the scenes if > > > the vma is not an IO or a PFNMAP, falling back to follow_pfn() > > > otherwise. > > > > Yeah pin_user_pages is fine, it's just the VM_IO | VM_PFNMAP vma that > > don't work. > > Ack. > > > > > > > Is your intention to drop get_vaddr_frames() or we could still keep > > > using it and if vec->is_pfns is true: > > > a) if CONFIG_VIDEO_LEGACY_PFN_USERPTR is set, taint the kernel > > > b) otherwise just undo and fail? > > > > I'm typing that patch series (plus a pile more) right now. > > Cool, thanks! > > We also need to bring back the vma_open() that somehow disappeared > around 4.2, as Marek found. The vm_open isn't enough to stop the problems (it doesn't and cannot protect against unmap_mapping_range), I don't think keeping an incomplete solution around has much benefit. People who need this can disable CONFIG_STRICT_FOLLOW_PFN to keep things working, everyone else probably doesn't want these mm internals leaking into the media subsystem. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel