| Message ID | 20201119144146.1045202-10-daniel.vetter@ffwll.ch |
|---|---|
| State | Superseded |
| Headers | show |
| Series | follow_pfn and other iomap races | expand |
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; > >
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; >> >> >
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; > >> > >> > > >
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; > >> > >> > > >
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; >>>> >>>> >>> >> > >
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; > >>>> > >>>> > >>> > >> > > > > >
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; >>>>>> >>>>>> >>>>> >>>> >>> >>> >> > >
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; > >>>>>> > >>>>>> > >>>>> > >>>> > >>> > >>> > >> > > > > >
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 --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;
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(-)