[PATCH v6 09/17] media/videbuf1|2: Mark follow_pfn usage as unsafe

[Hans Verkuil]

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 at chromium.org>
> 
> Acked-by: Hans Verkuil <hverkuil-cisco at xs4all.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 at intel.com>
>> Cc: Jason Gunthorpe <jgg at ziepe.ca>
>> Cc: Kees Cook <keescook at chromium.org>
>> Cc: Dan Williams <dan.j.williams at intel.com>
>> Cc: Andrew Morton <akpm at linux-foundation.org>
>> Cc: John Hubbard <jhubbard at nvidia.com>
>> Cc: Jérôme Glisse <jglisse at redhat.com>
>> Cc: Jan Kara <jack at suse.cz>
>> Cc: Dan Williams <dan.j.williams at intel.com>
>> Cc: linux-mm at kvack.org
>> Cc: linux-arm-kernel at lists.infradead.org
>> Cc: linux-samsung-soc at vger.kernel.org
>> Cc: linux-media at vger.kernel.org
>> Cc: Pawel Osciak <pawel at osciak.com>
>> Cc: Marek Szyprowski <m.szyprowski at samsung.com>
>> Cc: Kyungmin Park <kyungmin.park at samsung.com>
>> Cc: Tomasz Figa <tfiga at chromium.org>
>> Cc: Laurent Dufour <ldufour at linux.ibm.com>
>> Cc: Vlastimil Babka <vbabka at suse.cz>
>> Cc: Daniel Jordan <daniel.m.jordan at oracle.com>
>> Cc: Michel Lespinasse <walken at google.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.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;
>>  
>>
>