Re: [PATCH v2] videobuf2-core: Verify planes lengths for output buffers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sylwester,

On Monday 26 August 2013 17:03:11 Sylwester Nawrocki wrote:
> On 08/26/2013 04:41 PM, Laurent Pinchart wrote:
> > On Monday 26 August 2013 11:32:01 Mauro Carvalho Chehab wrote:
> >> Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx> escreveu:
> >>> On 08/08/2013 02:35 PM, Laurent Pinchart wrote:
> >>>> On Thursday 08 August 2013 14:14:30 Marek Szyprowski wrote:
> >>>>> On 8/7/2013 12:44 PM, Laurent Pinchart wrote:
> >>>>>> On Monday 12 November 2012 12:35:35 Laurent Pinchart wrote:
> >>>>>>> On Friday 09 November 2012 15:33:22 Pawel Osciak wrote:
> >>>>>>>> On Tue, Oct 16, 2012 at 8:37 AM, Laurent Pinchart wrote:
> >>>>>>>>> For output buffers application provide to the kernel the number of
> >>>>>>>>> bytes they stored in each plane of the buffer. Verify that the
> >>>>>>>>> value is smaller than or equal to the plane length.
> >>>>>>>>> 
> >>>>>>>>> Signed-off-by: Laurent Pinchart
> >>>>>>>>> <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >>>>>>>>> Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >>>>>>>>> ---
> >>>>>>>> 
> >>>>>>>> Acked-by: Pawel Osciak <pawel@xxxxxxxxxx>
> >>>>>>> 
> >>>>>>> You're listed, as well as Marek and Kyungmin, as videobuf2
> >>>>>>> maintainers. When you ack a videobuf2 patch, should we assume that
> >>>>>>> you will take it in your git tree ?
> >>>>>> 
> >>>>>> Ping ? I'd like to get this patch in v3.12, should I send a pull
> >>>>>> request ?
> >>>>> 
> >>>>> Acked-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> >>>>> 
> >>>>> Feel free to include it in your pull-request. I'm sorry for so huge
> >>>>> delay in my response.
> >>>> 
> >>>> No worries. I'll send a pull request to Mauro.
> >>>> 
> >>>>>>>>>  drivers/media/v4l2-core/videobuf2-core.c |   39
> >>>>>>>>>  +++++++++++++++++++
> >>>>>>>>>  1 files changed, 39 insertions(+), 0 deletions(-)
> >>>>>>>>> 
> >>>>>>>>> Changes compared to v1:
> >>>>>>>>> 
> >>>>>>>>> - Sanity check the data_offset value for each plane.
> >>>>>>>>> 
> >>>>>>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> >>>>>>>>> b/drivers/media/v4l2-core/videobuf2-core.c index 432df11..479337d
> >>>>>>>>> 100644
> >>>>>>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
> >>>>>>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> >>>>>>>>> @@ -296,6 +296,41 @@ static int __verify_planes_array(struct
> >>>>>>>>> vb2_buffer
> >>>>>>>>> *vb, const struct v4l2_buffer>
> >>>>>>>>> 
> >>>>>>>>>  }
> >>>>>>>>>  
> >>>>>>>>>  /**
> >>>>>>>>> 
> >>>>>>>>> + * __verify_length() - Verify that the bytesused value for each
> >>>>>>>>> plane
> >>>>>>>>> fits in
> >>>>>>>>> + * the plane length and that the data offset doesn't exceed the
> >>>>>>>>> bytesused value.
> >>>>>>>>> + */
> >>>>>>>>> +static int __verify_length(struct vb2_buffer *vb, const struct
> >>>>>>>>> v4l2_buffer *b)
> >>>>>>>>> +{
> >>>>>>>>> +       unsigned int length;
> >>>>>>>>> +       unsigned int plane;
> >>>>>>>>> +
> >>>>>>>>> +       if (!V4L2_TYPE_IS_OUTPUT(b->type))
> >>>>>>>>> +               return 0;
> >>>>>>>>> +
> >>>>>>>>> +       if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
> >>>>>>>>> +               for (plane = 0; plane < vb->num_planes; ++plane) {
> >>>>>>>>> +                       length = (b->memory ==
> >>>>>>>>> V4L2_MEMORY_USERPTR)
> >>>>>>>>> +                              ? b->m.planes[plane].length
> >>>>>>>>> +                              : vb->v4l2_planes[plane].length;
> >>>>>>>>> +
> >>>>>>>>> +                       if (b->m.planes[plane].bytesused > length)
> >>>>>>>>> +                               return -EINVAL;
> >>>>>>>>> +                       if (b->m.planes[plane].data_offset >=
> >>>>>>>>> +                           b->m.planes[plane].bytesused)
> >>>>>>>>> +                               return -EINVAL;
> >>> 
> >>> This patch causes regressions. After kernel upgrade applications that
> >>> zero the planes array and don't set bytesused will stop working.
> >>> We could say that these are buggy applications, but if it has been
> >>> allowed for several kernel releases failing VIDIOC_QBUF on this check
> >>> now is plainly a regression IMO. I guess Linus wouldn't be happy about
> >>> a change like this.
> >>> 
> >>> With this patch it is no longer possible to queue a buffer with
> >>> bytesused set to 0. I think it shouldn't be disallowed to queue a buffer
> >>> with no data to be used. So the check should likely be instead:
> >>>
> >>>  if (b->m.planes[plane].bytesused > 0 &&
> >>>      b->m.planes[plane].data_offset >=
> >>>      b->m.planes[plane].bytesused)
> >>> 	return -EINVAL;
> > 
> > What about
> > 
> > 	if (b->m.planes[plane].data_offset > 0 &&
> > 	    b->m.planes[plane].data_offset >=
> > 	    b->m.planes[plane].bytesused)
> > 	 	return -EINVAL;
> > 
> > If data_offset is non-zero we don't want to accept a zero bytesused value.
> 
> You're right, that looks better.
> 
> This will at least prevent failure of user space code like this one [1].
> 
> > We could also catch data_offset == 0 && bytesused == 0 with a WARN_ONCE to
> > try and get userspace applications fixed (this should definitely have
> > been caught from the very start).
> 
> But is it really a critical error condition ? IIRC s5p-mfc driver uses
> buffers with bytesused = 0 to signal end of stream (I'm not judging whether
> it is right or not at the moment). I'm no sure if such a configuration
> should be disallowed right at the v4l2-core.

You're absolutely right, bytesused == 0 is indeed valid.

> >>> Sorry for the late review of this.
> >> 
> >> Makes sense. Could you please send such patch?
> 
> Sure, I'm preparing a patch. And...
> 
> [...]
> 
> >>>>>>>>> +       ret = __verify_length(vb, b);
> >>>>>>>>> +       if (ret < 0)
> 
> additionally adding a debug print here, so it is easier to find out
> why QBUF fails.
> 
> >>>>>>>>> +               return ret;
> >>>>>>>>> +
> >>>>>>>>> 
> >>>>>>>>>         switch (q->memory) {
> >>>>>>>>>         case V4L2_MEMORY_MMAP:
> >>>>>>>>>                 ret = __qbuf_mmap(vb, b);
> 
> [1]
> http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/sys/mfc/fimc/fim
> c.c?id=0489f5277649826d1b38213c234fb0fe27206c2c#n543

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux