Re: [PATCHv10 22/26] v4l: vb2-dma-contig: fail if user ptr buffer is not correctly aligned
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.szyprow...@samsung.com
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.szyprow...@samsung.com
Acked-by: Hans Verkuil hans.verk...@cisco.com
---
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.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv10 22/26] v4l: vb2-dma-contig: fail if user ptr buffer is not correctly aligned
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.szyprow...@samsung.com
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.szyprow...@samsung.com
Acked-by: Hans Verkuil hans.verk...@cisco.com
---
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv10 22/26] v4l: vb2-dma-contig: fail if user ptr buffer is not correctly aligned
Hi Tomasz,
On Friday 12 October 2012 09:44:05 Tomasz Stanislawski wrote:
On 10/11/2012 11:36 PM, Laurent Pinchart wrote:
On Wednesday 10 October 2012 16:46:41 Tomasz Stanislawski wrote:
From: Marek Szyprowski m.szyprow...@samsung.com
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.szyprow...@samsung.com
Acked-by: Hans Verkuil hans.verk...@cisco.com
---
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.
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.
Wouldn't it then be better to add the sanity checks in the core ?
buf = kzalloc(sizeof *buf, GFP_KERNEL);
if (!buf)
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv10 22/26] v4l: vb2-dma-contig: fail if user ptr buffer is not correctly aligned
Hi Tomasz,
Thanks for the patch.
On Wednesday 10 October 2012 16:46:41 Tomasz Stanislawski wrote:
From: Marek Szyprowski m.szyprow...@samsung.com
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.szyprow...@samsung.com
Acked-by: Hans Verkuil hans.verk...@cisco.com
---
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.
buf = kzalloc(sizeof *buf, GFP_KERNEL);
if (!buf)
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
