On Fri 12 October 2012 09:44:05 Tomasz Stanislawski wrote:
> Hi Laurent,
> Thank you for the review.
> Please refer to the comments below.
>
> On 10/11/2012 11:36 PM, Laurent Pinchart wrote:
> > Hi Tomasz,
> >
> > Thanks for the patch.
> >
> > On Wednesday 10 October 2012 16:46:41 Tomasz Stanislawski wrote:
> >> From: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> >>
> >> The DMA transfer must be aligned to a specific value. If userptr is not
> >> aligned to DMA requirements then unexpected corruptions of the memory may
> >> occur before or after a buffer. To prevent such situations, all unligned
> >> userptr buffers are rejected at VIDIOC_QBUF.
> >>
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> >> Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >> ---
> >> drivers/media/v4l2-core/videobuf2-dma-contig.c | 12 ++++++++++++
> >> 1 file changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> b/drivers/media/v4l2-core/videobuf2-dma-contig.c index 2d661fd..571a919
> >> 100644
> >> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> @@ -493,6 +493,18 @@ static void *vb2_dc_get_userptr(void *alloc_ctx,
> >> unsigned long vaddr, struct vm_area_struct *vma;
> >> struct sg_table *sgt;
> >> unsigned long contig_size;
> >> + unsigned long dma_align = dma_get_cache_alignment();
> >> +
> >> + /* Only cache aligned DMA transfers are reliable */
> >> + if (!IS_ALIGNED(vaddr | size, dma_align)) {
> >> + pr_debug("user data must be aligned to %lu bytes\n", dma_align);
> >> + return ERR_PTR(-EINVAL);
> >> + }
> >
> > Looks good to me.
> >
> >> + if (!size) {
> >> + pr_debug("size is zero\n");
> >> + return ERR_PTR(-EINVAL);
> >> + }
> >
> > Can this happen ? The vb2 core already has
> >
> > /* Check if the provided plane buffer is large enough */
> > if (planes[plane].length < q->plane_sizes[plane]) {
> > ret = -EINVAL;
> > goto err;
> > }
> >
> > Unless queue_setup sets plane_sizes to 0 we can't reach vb2_dc_get_userptr.
> >
>
> Yes.. unfortunately, some drivers set plane_size to 0 at queue_setup.
> Especially, if REQBUFS is called before any S_FMT.
> Maybe it is just a driver bug.
That's a driver bug. Planes with size 0 make no sense whatsoever. vb2 should
WARN_ON on that and return an error.
My guess is that these drivers do not set up a default format as they should.
Regards,
Hans
> However, VB2 makes no sanity check if plane_sizes[] is zero.
> I was not able to find in Documentation nor code comments
> any explicit statement that plane_size cannot be zero.
>
> Therefore I have to reject reject a 0-bytes-long user pointer
> at vb2_dc_get_userptr before creating an empty scatterlist
> and passing it to the DMA layer.
>
> Regards,
> Tomasz Stanislawski
>
> >> buf = kzalloc(sizeof *buf, GFP_KERNEL);
> >> if (!buf)
> >
>
--
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
