diff mbox series

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

Message ID 20201119144146.1045202-10-daniel.vetter@ffwll.ch
State Superseded
Headers show
Series follow_pfn and other iomap races | expand

Commit Message

Daniel Vetter Nov. 19, 2020, 2:41 p.m. UTC
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@chromium.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Kees Cook <keescook@chromium.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Jérôme Glisse <jglisse@redhat.com>
Cc: Jan Kara <jack@suse.cz>
Cc: Dan Williams <dan.j.williams@intel.com>
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 <pawel@osciak.com>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Cc: Laurent Dufour <ldufour@linux.ibm.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Michel Lespinasse <walken@google.com>
Signed-off-by: Daniel Vetter <daniel.vetter@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(-)

Comments

Hans Verkuil Nov. 20, 2020, 8:06 a.m. UTC | #1
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@chromium.org>


Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>


Thanks!

	Hans

> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

> Cc: Jason Gunthorpe <jgg@ziepe.ca>

> Cc: Kees Cook <keescook@chromium.org>

> Cc: Dan Williams <dan.j.williams@intel.com>

> Cc: Andrew Morton <akpm@linux-foundation.org>

> Cc: John Hubbard <jhubbard@nvidia.com>

> Cc: Jérôme Glisse <jglisse@redhat.com>

> Cc: Jan Kara <jack@suse.cz>

> Cc: Dan Williams <dan.j.williams@intel.com>

> 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 <pawel@osciak.com>

> Cc: Marek Szyprowski <m.szyprowski@samsung.com>

> Cc: Kyungmin Park <kyungmin.park@samsung.com>

> Cc: Tomasz Figa <tfiga@chromium.org>

> Cc: Laurent Dufour <ldufour@linux.ibm.com>

> Cc: Vlastimil Babka <vbabka@suse.cz>

> Cc: Daniel Jordan <daniel.m.jordan@oracle.com>

> Cc: Michel Lespinasse <walken@google.com>

> Signed-off-by: Daniel Vetter <daniel.vetter@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;

>  

>
Hans Verkuil Nov. 20, 2020, 8:28 a.m. UTC | #2
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@chromium.org>
> 
> Acked-by: Hans Verkuil <hverkuil-cisco@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@intel.com>
>> Cc: Jason Gunthorpe <jgg@ziepe.ca>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: John Hubbard <jhubbard@nvidia.com>
>> Cc: Jérôme Glisse <jglisse@redhat.com>
>> Cc: Jan Kara <jack@suse.cz>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> 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 <pawel@osciak.com>
>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>> Cc: Tomasz Figa <tfiga@chromium.org>
>> Cc: Laurent Dufour <ldufour@linux.ibm.com>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
>> Cc: Michel Lespinasse <walken@google.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter@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;
>>  
>>
>
Tomasz Figa Nov. 20, 2020, 8:32 a.m. UTC | #3
On Fri, Nov 20, 2020 at 5:28 PM Hans Verkuil <hverkuil@xs4all.nl> 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 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@chromium.org>
> >
> > Acked-by: Hans Verkuil <hverkuil-cisco@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.

I'm all for dropping that code.

Best regards,
Tomasz

>
> Regards,
>
>         Hans
>
> >
> > Thanks!
> >
> >       Hans
> >
> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> >> Cc: Kees Cook <keescook@chromium.org>
> >> Cc: Dan Williams <dan.j.williams@intel.com>
> >> Cc: Andrew Morton <akpm@linux-foundation.org>
> >> Cc: John Hubbard <jhubbard@nvidia.com>
> >> Cc: Jérôme Glisse <jglisse@redhat.com>
> >> Cc: Jan Kara <jack@suse.cz>
> >> Cc: Dan Williams <dan.j.williams@intel.com>
> >> 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 <pawel@osciak.com>
> >> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> >> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> >> Cc: Tomasz Figa <tfiga@chromium.org>
> >> Cc: Laurent Dufour <ldufour@linux.ibm.com>
> >> Cc: Vlastimil Babka <vbabka@suse.cz>
> >> Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
> >> Cc: Michel Lespinasse <walken@google.com>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@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;
> >>
> >>
> >
>
Daniel Vetter Nov. 20, 2020, 9:18 a.m. UTC | #4
On Fri, Nov 20, 2020 at 9:28 AM Hans Verkuil <hverkuil@xs4all.nl> 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 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@chromium.org>
> >
> > Acked-by: Hans Verkuil <hverkuil-cisco@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.

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?

So ... can I get you to un-cancel your ack?

Thanks, Daniel

>
> Regards,
>
>         Hans
>
> >
> > Thanks!
> >
> >       Hans
> >
> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> >> Cc: Kees Cook <keescook@chromium.org>
> >> Cc: Dan Williams <dan.j.williams@intel.com>
> >> Cc: Andrew Morton <akpm@linux-foundation.org>
> >> Cc: John Hubbard <jhubbard@nvidia.com>
> >> Cc: Jérôme Glisse <jglisse@redhat.com>
> >> Cc: Jan Kara <jack@suse.cz>
> >> Cc: Dan Williams <dan.j.williams@intel.com>
> >> 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 <pawel@osciak.com>
> >> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> >> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> >> Cc: Tomasz Figa <tfiga@chromium.org>
> >> Cc: Laurent Dufour <ldufour@linux.ibm.com>
> >> Cc: Vlastimil Babka <vbabka@suse.cz>
> >> Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
> >> Cc: Michel Lespinasse <walken@google.com>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@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;
> >>
> >>
> >
>
Hans Verkuil Nov. 20, 2020, 10:38 a.m. UTC | #5
On 20/11/2020 10:18, Daniel Vetter wrote:
> On Fri, Nov 20, 2020 at 9:28 AM Hans Verkuil <hverkuil@xs4all.nl> 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 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@chromium.org>
>>>
>>> Acked-by: Hans Verkuil <hverkuil-cisco@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.
> 
> 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 replacement.
That's really the critical piece of information I'd like to see. That doesn't
belong in unsafe_follow_pfn, it needs to be in frame_vector.c since it's
vb2 specific.

> 
> 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_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 in a comment
since I don't want users who hit this issue to have to dig through git logs
to find that dma-buf is the right approach.

BTW, nitpick: the subject line of this patch says 'videbuf' instead of 'videobuf'.

Regards,

	Hans

> 
> Thanks, Daniel
> 
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> Thanks!
>>>
>>>       Hans
>>>
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>> Cc: Jason Gunthorpe <jgg@ziepe.ca>
>>>> Cc: Kees Cook <keescook@chromium.org>
>>>> Cc: Dan Williams <dan.j.williams@intel.com>
>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>> Cc: John Hubbard <jhubbard@nvidia.com>
>>>> Cc: Jérôme Glisse <jglisse@redhat.com>
>>>> Cc: Jan Kara <jack@suse.cz>
>>>> Cc: Dan Williams <dan.j.williams@intel.com>
>>>> 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 <pawel@osciak.com>
>>>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>>>> Cc: Tomasz Figa <tfiga@chromium.org>
>>>> Cc: Laurent Dufour <ldufour@linux.ibm.com>
>>>> Cc: Vlastimil Babka <vbabka@suse.cz>
>>>> Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
>>>> Cc: Michel Lespinasse <walken@google.com>
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@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;
>>>>
>>>>
>>>
>>
> 
>
Daniel Vetter Nov. 20, 2020, 10:51 a.m. UTC | #6
On Fri, Nov 20, 2020 at 11:39 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 20/11/2020 10:18, Daniel Vetter wrote:
> > On Fri, Nov 20, 2020 at 9:28 AM Hans Verkuil <hverkuil@xs4all.nl> 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 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@chromium.org>
> >>>
> >>> Acked-by: Hans Verkuil <hverkuil-cisco@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.
> >
> > 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 replacement.
> That's really the critical piece of information I'd like to see. That 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 completely.
>
> How about this: just replace follow_pfn() by -EINVAL instead of unsafe_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 in a comment
> since I don't want users who hit this issue to have to dig through git 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 :-)
-Daniel

>
> Regards,
>
>         Hans
>
> >
> > Thanks, Daniel
> >
> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >>>
> >>> Thanks!
> >>>
> >>>       Hans
> >>>
> >>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >>>> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> >>>> Cc: Kees Cook <keescook@chromium.org>
> >>>> Cc: Dan Williams <dan.j.williams@intel.com>
> >>>> Cc: Andrew Morton <akpm@linux-foundation.org>
> >>>> Cc: John Hubbard <jhubbard@nvidia.com>
> >>>> Cc: Jérôme Glisse <jglisse@redhat.com>
> >>>> Cc: Jan Kara <jack@suse.cz>
> >>>> Cc: Dan Williams <dan.j.williams@intel.com>
> >>>> 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 <pawel@osciak.com>
> >>>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> >>>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> >>>> Cc: Tomasz Figa <tfiga@chromium.org>
> >>>> Cc: Laurent Dufour <ldufour@linux.ibm.com>
> >>>> Cc: Vlastimil Babka <vbabka@suse.cz>
> >>>> Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
> >>>> Cc: Michel Lespinasse <walken@google.com>
> >>>> Signed-off-by: Daniel Vetter <daniel.vetter@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;
> >>>>
> >>>>
> >>>
> >>
> >
> >
>
Hans Verkuil Nov. 20, 2020, 12:08 p.m. UTC | #7
On 20/11/2020 11:51, Daniel Vetter wrote:
> On Fri, Nov 20, 2020 at 11:39 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 20/11/2020 10:18, Daniel Vetter wrote:
>>> On Fri, Nov 20, 2020 at 9:28 AM Hans Verkuil <hverkuil@xs4all.nl> 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 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@chromium.org>
>>>>>
>>>>> Acked-by: Hans Verkuil <hverkuil-cisco@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.
>>>
>>> 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 replacement.
>> That's really the critical piece of information I'd like to see. That 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 completely.
>>
>> How about this: just replace follow_pfn() by -EINVAL instead of unsafe_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 in a comment
>> since I don't want users who hit this issue to have to dig through git 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 patch or
not.

I don't see why we would want to continue supporting a broken model that 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?

Regards,

	Hans

> -Daniel
> 
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> Thanks, Daniel
>>>
>>>>
>>>> Regards,
>>>>
>>>>         Hans
>>>>
>>>>>
>>>>> Thanks!
>>>>>
>>>>>       Hans
>>>>>
>>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>>>> Cc: Jason Gunthorpe <jgg@ziepe.ca>
>>>>>> Cc: Kees Cook <keescook@chromium.org>
>>>>>> Cc: Dan Williams <dan.j.williams@intel.com>
>>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>>> Cc: John Hubbard <jhubbard@nvidia.com>
>>>>>> Cc: Jérôme Glisse <jglisse@redhat.com>
>>>>>> Cc: Jan Kara <jack@suse.cz>
>>>>>> Cc: Dan Williams <dan.j.williams@intel.com>
>>>>>> 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 <pawel@osciak.com>
>>>>>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
>>>>>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>>>>>> Cc: Tomasz Figa <tfiga@chromium.org>
>>>>>> Cc: Laurent Dufour <ldufour@linux.ibm.com>
>>>>>> Cc: Vlastimil Babka <vbabka@suse.cz>
>>>>>> Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
>>>>>> Cc: Michel Lespinasse <walken@google.com>
>>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@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;
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
> 
>
Tomasz Figa Nov. 20, 2020, 12:23 p.m. UTC | #8
On Fri, Nov 20, 2020 at 9:08 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 20/11/2020 11:51, Daniel Vetter wrote:
> > On Fri, Nov 20, 2020 at 11:39 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 20/11/2020 10:18, Daniel Vetter wrote:
> >>> On Fri, Nov 20, 2020 at 9:28 AM Hans Verkuil <hverkuil@xs4all.nl> 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 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@chromium.org>
> >>>>>
> >>>>> Acked-by: Hans Verkuil <hverkuil-cisco@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.
> >>>
> >>> 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 replacement.
> >> That's really the critical piece of information I'd like to see. That 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 completely.
> >>
> >> How about this: just replace follow_pfn() by -EINVAL instead of unsafe_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 in a comment
> >> since I don't want users who hit this issue to have to dig through git 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 patch or
> not.
>
> I don't see why we would want to continue supporting a broken model that 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.

> Regards,
>
>         Hans
>
> > -Daniel
> >
> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >>>
> >>> Thanks, Daniel
> >>>
> >>>>
> >>>> Regards,
> >>>>
> >>>>         Hans
> >>>>
> >>>>>
> >>>>> Thanks!
> >>>>>
> >>>>>       Hans
> >>>>>
> >>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >>>>>> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> >>>>>> Cc: Kees Cook <keescook@chromium.org>
> >>>>>> Cc: Dan Williams <dan.j.williams@intel.com>
> >>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
> >>>>>> Cc: John Hubbard <jhubbard@nvidia.com>
> >>>>>> Cc: Jérôme Glisse <jglisse@redhat.com>
> >>>>>> Cc: Jan Kara <jack@suse.cz>
> >>>>>> Cc: Dan Williams <dan.j.williams@intel.com>
> >>>>>> 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 <pawel@osciak.com>
> >>>>>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> >>>>>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> >>>>>> Cc: Tomasz Figa <tfiga@chromium.org>
> >>>>>> Cc: Laurent Dufour <ldufour@linux.ibm.com>
> >>>>>> Cc: Vlastimil Babka <vbabka@suse.cz>
> >>>>>> Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
> >>>>>> Cc: Michel Lespinasse <walken@google.com>
> >>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@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;
> >>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>>
> >>
> >
> >
>
Daniel Vetter Nov. 24, 2020, 2:16 p.m. UTC | #9
On Fri, Nov 20, 2020 at 09:23:12PM +0900, Tomasz Figa wrote:
> On Fri, Nov 20, 2020 at 9:08 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >
> > On 20/11/2020 11:51, Daniel Vetter wrote:
> > > On Fri, Nov 20, 2020 at 11:39 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > >>
> > >> On 20/11/2020 10:18, Daniel Vetter wrote:
> > >>> On Fri, Nov 20, 2020 at 9:28 AM Hans Verkuil <hverkuil@xs4all.nl> 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 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@chromium.org>
> > >>>>>
> > >>>>> Acked-by: Hans Verkuil <hverkuil-cisco@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.
> > >>>
> > >>> 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 replacement.
> > >> That's really the critical piece of information I'd like to see. That 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 completely.
> > >>
> > >> How about this: just replace follow_pfn() by -EINVAL instead of unsafe_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 in a comment
> > >> since I don't want users who hit this issue to have to dig through git 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 patch or
> > not.
> >
> > I don't see why we would want to continue supporting a broken model that 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 <daniel.vetter@intel.com>
> > >>>>>> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > >>>>>> Cc: Kees Cook <keescook@chromium.org>
> > >>>>>> Cc: Dan Williams <dan.j.williams@intel.com>
> > >>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
> > >>>>>> Cc: John Hubbard <jhubbard@nvidia.com>
> > >>>>>> Cc: Jérôme Glisse <jglisse@redhat.com>
> > >>>>>> Cc: Jan Kara <jack@suse.cz>
> > >>>>>> Cc: Dan Williams <dan.j.williams@intel.com>
> > >>>>>> 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 <pawel@osciak.com>
> > >>>>>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> > >>>>>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> > >>>>>> Cc: Tomasz Figa <tfiga@chromium.org>
> > >>>>>> Cc: Laurent Dufour <ldufour@linux.ibm.com>
> > >>>>>> Cc: Vlastimil Babka <vbabka@suse.cz>
> > >>>>>> Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
> > >>>>>> Cc: Michel Lespinasse <walken@google.com>
> > >>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@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;
> > >>>>>>
> > >>>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>>
> > >>
> > >
> > >
> >
diff mbox series

Patch

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;