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
