linux-kernel - Re: [PATCH v6 09/17] media/videbuf1|2: Mark follow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e1f7d30b-2012-0249-66c7-cf9d7d6246ad@xs4all.nl>
Date:   Fri, 20 Nov 2020 09:28:32 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Daniel Vetter <daniel.vetter@...ll.ch>,
        DRI Development <dri-devel@...ts.freedesktop.org>,
        LKML <linux-kernel@...r.kernel.org>
Cc:     kvm@...r.kernel.org, linux-mm@...ck.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org, linux-media@...r.kernel.org,
        Tomasz Figa <tfiga@...omium.org>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Kees Cook <keescook@...omium.org>,
        Dan Williams <dan.j.williams@...el.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        John Hubbard <jhubbard@...dia.com>,
        Jérôme Glisse <jglisse@...hat.com>,
        Jan Kara <jack@...e.cz>, Pawel Osciak <pawel@...iak.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Kyungmin Park <kyungmin.park@...sung.com>,
        Laurent Dufour <ldufour@...ux.ibm.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Daniel Jordan <daniel.m.jordan@...cle.com>,
        Michel Lespinasse <walken@...gle.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>
Subject: Re: [PATCH v6 09/17] media/videbuf1|2: Mark follow_pfn usage as
 unsafe

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 <tfiga@...omium.org>
> 
> Acked-by: Hans Verkuil <hverkuil-cisco@...all.nl>

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 <daniel.vetter@...el.com>
>> Cc: Jason Gunthorpe <jgg@...pe.ca>
>> Cc: Kees Cook <keescook@...omium.org>
>> Cc: Dan Williams <dan.j.williams@...el.com>
>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>> Cc: John Hubbard <jhubbard@...dia.com>
>> Cc: Jérôme Glisse <jglisse@...hat.com>
>> Cc: Jan Kara <jack@...e.cz>
>> Cc: Dan Williams <dan.j.williams@...el.com>
>> Cc: linux-mm@...ck.org
>> Cc: linux-arm-kernel@...ts.infradead.org
>> Cc: linux-samsung-soc@...r.kernel.org
>> Cc: linux-media@...r.kernel.org
>> Cc: Pawel Osciak <pawel@...iak.com>
>> Cc: Marek Szyprowski <m.szyprowski@...sung.com>
>> Cc: Kyungmin Park <kyungmin.park@...sung.com>
>> Cc: Tomasz Figa <tfiga@...omium.org>
>> Cc: Laurent Dufour <ldufour@...ux.ibm.com>
>> Cc: Vlastimil Babka <vbabka@...e.cz>
>> Cc: Daniel Jordan <daniel.m.jordan@...cle.com>
>> Cc: Michel Lespinasse <walken@...gle.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter@...ll.ch>
>> --
>> 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;
>>  
>>
>