We need to call dma_buf_end_cpu_access() in case a damage-request. Unlikely, but might happen during device unplug.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/udl/udl_fb.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index dbadd49..50f564d 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -421,9 +421,10 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, clips[i].x2 - clips[i].x1, clips[i].y2 - clips[i].y1); if (ret) - goto unlock; + goto end_access; }
+end_access: if (ufb->obj->base.import_attach) { dma_buf_end_cpu_access(ufb->obj->base.import_attach->dmabuf, 0, ufb->obj->base.size,
Probably a typo.. we obviously need "(bpp + 7) / 8" instead of "(bpp + 1) / 8". Unlikely to be hit in any sane code, but lets be safe.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/udl/udl_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index 8d67b94..df963a1 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -60,7 +60,7 @@ int udl_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args) { - args->pitch = args->width * ((args->bpp + 1) / 8); + args->pitch = args->width * ((args->bpp + 7) / 8); args->size = args->pitch * args->height; return udl_gem_create(file, dev, args->size, &args->handle);
On Mon, Jan 20, 2014 at 08:26:24PM +0100, David Herrmann wrote:
Probably a typo.. we obviously need "(bpp + 7) / 8" instead of "(bpp + 1) / 8". Unlikely to be hit in any sane code, but lets be safe.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/udl/udl_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index 8d67b94..df963a1 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -60,7 +60,7 @@ int udl_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args) {
- args->pitch = args->width * ((args->bpp + 1) / 8);
- args->pitch = args->width * ((args->bpp + 7) / 8);
DIV_ROUND_UP?
args->size = args->pitch * args->height; return udl_gem_create(file, dev, args->size, &args->handle); -- 1.8.5.3
Hi
On Tue, Jan 21, 2014 at 10:38 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Jan 20, 2014 at 08:26:24PM +0100, David Herrmann wrote:
Probably a typo.. we obviously need "(bpp + 7) / 8" instead of "(bpp + 1) / 8". Unlikely to be hit in any sane code, but lets be safe.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/udl/udl_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index 8d67b94..df963a1 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -60,7 +60,7 @@ int udl_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args) {
args->pitch = args->width * ((args->bpp + 1) / 8);
args->pitch = args->width * ((args->bpp + 7) / 8);DIV_ROUND_UP?
Hm, udl doesn't use this anywhere, so I'd like to keep consistency. I guess I will replace all those occurrences by DIV_ROUND_UP() and meld it into this fix (except if someone wants two separate commits?).
Thanks David
args->size = args->pitch * args->height; return udl_gem_create(file, dev, args->size, &args->handle);-- 1.8.5.3
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Probably a typo.. we obviously need "(bpp + 7) / 8" instead of "(bpp + 1) / 8". Unlikely to be hit in any sane code, but lets be safe. Use DIV_ROUND_UP() to avoid the problem entirely and make the core more readable.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/udl/udl_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index 8d67b94..be4fcd0 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -60,7 +60,7 @@ int udl_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args) { - args->pitch = args->width * ((args->bpp + 1) / 8); + args->pitch = args->width * DIV_ROUND_UP(args->bpp, 8); args->size = args->pitch * args->height; return udl_gem_create(file, dev, args->size, &args->handle);
ping..
On Thu, Jan 23, 2014 at 1:50 PM, David Herrmann dh.herrmann@gmail.com wrote:
Probably a typo.. we obviously need "(bpp + 7) / 8" instead of "(bpp + 1) / 8". Unlikely to be hit in any sane code, but lets be safe. Use DIV_ROUND_UP() to avoid the problem entirely and make the core more readable.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/udl/udl_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index 8d67b94..be4fcd0 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -60,7 +60,7 @@ int udl_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args) {
args->pitch = args->width * ((args->bpp + 1) / 8);
args->pitch = args->width * DIV_ROUND_UP(args->bpp, 8); args->size = args->pitch * args->height; return udl_gem_create(file, dev, args->size, &args->handle);-- 1.8.5.3
Instead of rounding down to the next lower page-boundary, round up. dma-buf guarantees that we can map buffers in multiples of a page, so if an exporter does not page-align, do it ourselves.
This avoids issues if the exported buffer contains an unaligned size and we crop it. In this case, the buffer is too small for the UDL CRTC. So we round up to page-size now and avoid black borders. Worst case is we end up reading out some random kernel memory, but we can never fault as the whole page has the same access-rights. And in this case it's an issue of the buggy exporting driver, not the importing one.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/udl/udl_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index df963a1..1069e57 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -227,7 +227,7 @@ static int udl_prime_create(struct drm_device *dev, struct udl_gem_object *obj; int npages;
- npages = size / PAGE_SIZE; + npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
*obj_p = NULL; obj = udl_gem_alloc_object(dev, npages * PAGE_SIZE);
On Mon, Jan 20, 2014 at 08:26:25PM +0100, David Herrmann wrote:
Instead of rounding down to the next lower page-boundary, round up. dma-buf guarantees that we can map buffers in multiples of a page, so if an exporter does not page-align, do it ourselves.
This avoids issues if the exported buffer contains an unaligned size and we crop it. In this case, the buffer is too small for the UDL CRTC. So we round up to page-size now and avoid black borders. Worst case is we end up reading out some random kernel memory, but we can never fault as the whole page has the same access-rights. And in this case it's an issue of the buggy exporting driver, not the importing one.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/udl/udl_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index df963a1..1069e57 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -227,7 +227,7 @@ static int udl_prime_create(struct drm_device *dev, struct udl_gem_object *obj; int npages;
- npages = size / PAGE_SIZE;
- npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
Imo the proper patch would be to reject exporting anything which isn't page-aligned as a dma-buf in the driver core (together with a quick review of the docs to make sure this requirement is clear). This is just a bug in the exporter. -Daniel
*obj_p = NULL; obj = udl_gem_alloc_object(dev, npages * PAGE_SIZE); -- 1.8.5.3
Hi
On Tue, Jan 21, 2014 at 10:41 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Jan 20, 2014 at 08:26:25PM +0100, David Herrmann wrote:
Instead of rounding down to the next lower page-boundary, round up. dma-buf guarantees that we can map buffers in multiples of a page, so if an exporter does not page-align, do it ourselves.
This avoids issues if the exported buffer contains an unaligned size and we crop it. In this case, the buffer is too small for the UDL CRTC. So we round up to page-size now and avoid black borders. Worst case is we end up reading out some random kernel memory, but we can never fault as the whole page has the same access-rights. And in this case it's an issue of the buggy exporting driver, not the importing one.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/udl/udl_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index df963a1..1069e57 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -227,7 +227,7 @@ static int udl_prime_create(struct drm_device *dev, struct udl_gem_object *obj; int npages;
npages = size / PAGE_SIZE;
npages = PAGE_ALIGN(size) >> PAGE_SHIFT;Imo the proper patch would be to reject exporting anything which isn't page-aligned as a dma-buf in the driver core (together with a quick review of the docs to make sure this requirement is clear). This is just a bug in the exporter.
Ok, I've dropped this one for now. I think the best way to deal with it is to make dma_buf disallow any non-aligned sizes during initialization. So we should just assume it's always page-aligned.
Thanks David
Remove double-whitespace and wrong indentation.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_gem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index bed5c3b..c1eaf35 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -691,7 +691,7 @@ drm_gem_object_release(struct drm_gem_object *obj) WARN_ON(obj->dma_buf);
if (obj->filp) - fput(obj->filp); + fput(obj->filp); } EXPORT_SYMBOL(drm_gem_object_release);
@@ -781,7 +781,7 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; vma->vm_ops = dev->driver->gem_vm_ops; vma->vm_private_data = obj; - vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags)); + vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
/* Take a ref for this mapping of the object, so that the fault * handler can dereference the mmap offset's pointer to the object.
ping
On Mon, Jan 20, 2014 at 8:26 PM, David Herrmann dh.herrmann@gmail.com wrote:
Remove double-whitespace and wrong indentation.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/drm_gem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index bed5c3b..c1eaf35 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -691,7 +691,7 @@ drm_gem_object_release(struct drm_gem_object *obj) WARN_ON(obj->dma_buf);
if (obj->filp)
fput(obj->filp);
fput(obj->filp);} EXPORT_SYMBOL(drm_gem_object_release);
@@ -781,7 +781,7 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; vma->vm_ops = dev->driver->gem_vm_ops; vma->vm_private_data = obj;
vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags)); /* Take a ref for this mapping of the object, so that the fault * handler can dereference the mmap offset's pointer to the object.-- 1.8.5.3
All drivers currently need to clean up the vma-node manually. There is no fancy logic involved so lets just clean it up unconditionally. The vma-manager correctly catches multiple calls so we are fine.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_gem.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index c1eaf35..7bf374e 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -692,6 +692,8 @@ drm_gem_object_release(struct drm_gem_object *obj)
if (obj->filp) fput(obj->filp); + + drm_gem_free_mmap_offset(obj); } EXPORT_SYMBOL(drm_gem_object_release);
ping
On Mon, Jan 20, 2014 at 8:26 PM, David Herrmann dh.herrmann@gmail.com wrote:
All drivers currently need to clean up the vma-node manually. There is no fancy logic involved so lets just clean it up unconditionally. The vma-manager correctly catches multiple calls so we are fine.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/drm_gem.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index c1eaf35..7bf374e 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -692,6 +692,8 @@ drm_gem_object_release(struct drm_gem_object *obj)
if (obj->filp) fput(obj->filp);
drm_gem_free_mmap_offset(obj);} EXPORT_SYMBOL(drm_gem_object_release);
-- 1.8.5.3
Lets make sure some basic expressions are always true: bpp != NULL width != NULL height != NULL stride = bpp * width < 2^32 size = stride * height < 2^32 PAGE_ALIGN(size) < 2^32
At least the udl driver doesn't check for multiplication-overflows, so lets just make sure it will never happen. These checks allow drivers to do any 32bit math without having to test for mult-overflows themselves.
The two divisions might hurt performance a bit, but dumb_create() is only used for scanout-buffers, so that should be fine. We could use 64bit math to avoid the divisions, but that may be slow on 32bit machines.. Or maybe there should just be a "safe_mult32()" helper, which currently doesn't exist (I think?).
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_crtc.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 266a01d..ff647fa 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3738,9 +3738,24 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_create_dumb *args = data; + u32 Bpp, stride, size;
if (!dev->driver->dumb_create) return -ENOSYS; + if (!args->width || !args->height || !args->bpp) + return -EINVAL; + + /* overflow checks for 32bit size calculations */ + Bpp = (args->bpp + 7) / 8; + if (Bpp > 0xffffffffU / args->width) + return -EINVAL; + stride = Bpp * args->width; + if (args->height > 0xffffffffU / stride) + return -EINVAL; + size = args->height * stride; + if (PAGE_ALIGN(size) < size) + return -EINVAL; + return dev->driver->dumb_create(file_priv, dev, args); }
On Mon, Jan 20, 2014 at 08:26:28PM +0100, David Herrmann wrote:
Lets make sure some basic expressions are always true: bpp != NULL width != NULL height != NULL stride = bpp * width < 2^32 size = stride * height < 2^32 PAGE_ALIGN(size) < 2^32
At least the udl driver doesn't check for multiplication-overflows, so lets just make sure it will never happen. These checks allow drivers to do any 32bit math without having to test for mult-overflows themselves.
The two divisions might hurt performance a bit, but dumb_create() is only used for scanout-buffers, so that should be fine. We could use 64bit math to avoid the divisions, but that may be slow on 32bit machines.. Or maybe there should just be a "safe_mult32()" helper, which currently doesn't exist (I think?).
Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/drm_crtc.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 266a01d..ff647fa 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3738,9 +3738,24 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_create_dumb *args = data;
- u32 Bpp, stride, size;
s/Bpp/bpp/
if (!dev->driver->dumb_create) return -ENOSYS;
- if (!args->width || !args->height || !args->bpp)
return -EINVAL;- /* overflow checks for 32bit size calculations */
- Bpp = (args->bpp + 7) / 8;
Again DIV_ROUND_UP
- if (Bpp > 0xffffffffU / args->width)
return -EINVAL;- stride = Bpp * args->width;
- if (args->height > 0xffffffffU / stride)
return -EINVAL;- size = args->height * stride;
- if (PAGE_ALIGN(size) < size)
Maybe check for 0 here and add a small comment that this checks wrap-around. -Daniel
return -EINVAL;- return dev->driver->dumb_create(file_priv, dev, args);
}
-- 1.8.5.3
Hi
On Tue, Jan 21, 2014 at 10:49 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Jan 20, 2014 at 08:26:28PM +0100, David Herrmann wrote:
Lets make sure some basic expressions are always true: bpp != NULL width != NULL height != NULL stride = bpp * width < 2^32 size = stride * height < 2^32 PAGE_ALIGN(size) < 2^32
At least the udl driver doesn't check for multiplication-overflows, so lets just make sure it will never happen. These checks allow drivers to do any 32bit math without having to test for mult-overflows themselves.
The two divisions might hurt performance a bit, but dumb_create() is only used for scanout-buffers, so that should be fine. We could use 64bit math to avoid the divisions, but that may be slow on 32bit machines.. Or maybe there should just be a "safe_mult32()" helper, which currently doesn't exist (I think?).
Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/drm_crtc.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 266a01d..ff647fa 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3738,9 +3738,24 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_create_dumb *args = data;
u32 Bpp, stride, size;s/Bpp/bpp/
It's actually "Bytes per pixel", not "Bits per pixel". We generally use "bpp" as "bits per pixel" so I'd like to avoid the confusion. But yeah, upper-case names are unusual so I can also use bytes_pp or sth similar?
if (!dev->driver->dumb_create) return -ENOSYS;
if (!args->width || !args->height || !args->bpp)return -EINVAL;/* overflow checks for 32bit size calculations */Bpp = (args->bpp + 7) / 8;Again DIV_ROUND_UP
yepp, fixed.
if (Bpp > 0xffffffffU / args->width)return -EINVAL;stride = Bpp * args->width;if (args->height > 0xffffffffU / stride)return -EINVAL;size = args->height * stride;if (PAGE_ALIGN(size) < size)Maybe check for 0 here and add a small comment that this checks wrap-around.
Hm, the comment above those if()s already says "overflow checks", and it should be obvious that all three are overflow checks, so I don't think we need another comment (especially because I didn't add any empty lines between them).
I will change it to "if (!PAGE_ALIGN(size))". The "x + off < x" is an addition-overflow-check so I think it doesn't apply here.
Thanks David
return -EINVAL;return dev->driver->dumb_create(file_priv, dev, args);}
-- 1.8.5.3
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, Jan 21, 2014 at 12:17:35PM +0100, David Herrmann wrote:
Hi
On Tue, Jan 21, 2014 at 10:49 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Jan 20, 2014 at 08:26:28PM +0100, David Herrmann wrote:
Lets make sure some basic expressions are always true: bpp != NULL width != NULL height != NULL stride = bpp * width < 2^32 size = stride * height < 2^32 PAGE_ALIGN(size) < 2^32
At least the udl driver doesn't check for multiplication-overflows, so lets just make sure it will never happen. These checks allow drivers to do any 32bit math without having to test for mult-overflows themselves.
The two divisions might hurt performance a bit, but dumb_create() is only used for scanout-buffers, so that should be fine. We could use 64bit math to avoid the divisions, but that may be slow on 32bit machines.. Or maybe there should just be a "safe_mult32()" helper, which currently doesn't exist (I think?).
Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/drm_crtc.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 266a01d..ff647fa 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3738,9 +3738,24 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_create_dumb *args = data;
u32 Bpp, stride, size;s/Bpp/bpp/
It's actually "Bytes per pixel", not "Bits per pixel". We generally use "bpp" as "bits per pixel" so I'd like to avoid the confusion. But yeah, upper-case names are unusual so I can also use bytes_pp or sth similar?
cpp is fairly commonly used for bytes per pixel, if you want to stick to something short but still recognizable.
if (!dev->driver->dumb_create) return -ENOSYS;
if (!args->width || !args->height || !args->bpp)return -EINVAL;/* overflow checks for 32bit size calculations */Bpp = (args->bpp + 7) / 8;Again DIV_ROUND_UP
yepp, fixed.
if (Bpp > 0xffffffffU / args->width)return -EINVAL;stride = Bpp * args->width;if (args->height > 0xffffffffU / stride)return -EINVAL;size = args->height * stride;if (PAGE_ALIGN(size) < size)Maybe check for 0 here and add a small comment that this checks wrap-around.
Hm, the comment above those if()s already says "overflow checks", and it should be obvious that all three are overflow checks, so I don't think we need another comment (especially because I didn't add any empty lines between them).
I will change it to "if (!PAGE_ALIGN(size))". The "x + off < x" is an addition-overflow-check so I think it doesn't apply here.
Thanks David
return -EINVAL;return dev->driver->dumb_create(file_priv, dev, args);}
-- 1.8.5.3
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi
On Tue, Jan 21, 2014 at 12:42 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Tue, Jan 21, 2014 at 12:17:35PM +0100, David Herrmann wrote:
Hi
On Tue, Jan 21, 2014 at 10:49 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Jan 20, 2014 at 08:26:28PM +0100, David Herrmann wrote:
Lets make sure some basic expressions are always true: bpp != NULL width != NULL height != NULL stride = bpp * width < 2^32 size = stride * height < 2^32 PAGE_ALIGN(size) < 2^32
At least the udl driver doesn't check for multiplication-overflows, so lets just make sure it will never happen. These checks allow drivers to do any 32bit math without having to test for mult-overflows themselves.
The two divisions might hurt performance a bit, but dumb_create() is only used for scanout-buffers, so that should be fine. We could use 64bit math to avoid the divisions, but that may be slow on 32bit machines.. Or maybe there should just be a "safe_mult32()" helper, which currently doesn't exist (I think?).
Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/drm_crtc.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 266a01d..ff647fa 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3738,9 +3738,24 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_create_dumb *args = data;
u32 Bpp, stride, size;s/Bpp/bpp/
It's actually "Bytes per pixel", not "Bits per pixel". We generally use "bpp" as "bits per pixel" so I'd like to avoid the confusion. But yeah, upper-case names are unusual so I can also use bytes_pp or sth similar?
cpp is fairly commonly used for bytes per pixel, if you want to stick to something short but still recognizable.
Because "c" comes after "b"? Or is there any other logic here? But sounds good, will send a v2 shortly.
Thanks David
On Tue, Jan 21, 2014 at 12:52:36PM +0100, David Herrmann wrote:
Hi
On Tue, Jan 21, 2014 at 12:42 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Tue, Jan 21, 2014 at 12:17:35PM +0100, David Herrmann wrote:
Hi
On Tue, Jan 21, 2014 at 10:49 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Jan 20, 2014 at 08:26:28PM +0100, David Herrmann wrote:
Lets make sure some basic expressions are always true: bpp != NULL width != NULL height != NULL stride = bpp * width < 2^32 size = stride * height < 2^32 PAGE_ALIGN(size) < 2^32
At least the udl driver doesn't check for multiplication-overflows, so lets just make sure it will never happen. These checks allow drivers to do any 32bit math without having to test for mult-overflows themselves.
The two divisions might hurt performance a bit, but dumb_create() is only used for scanout-buffers, so that should be fine. We could use 64bit math to avoid the divisions, but that may be slow on 32bit machines.. Or maybe there should just be a "safe_mult32()" helper, which currently doesn't exist (I think?).
Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/drm_crtc.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 266a01d..ff647fa 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3738,9 +3738,24 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_create_dumb *args = data;
u32 Bpp, stride, size;s/Bpp/bpp/
It's actually "Bytes per pixel", not "Bits per pixel". We generally use "bpp" as "bits per pixel" so I'd like to avoid the confusion. But yeah, upper-case names are unusual so I can also use bytes_pp or sth similar?
cpp is fairly commonly used for bytes per pixel, if you want to stick to something short but still recognizable.
Because "c" comes after "b"? Or is there any other logic here? But sounds good, will send a v2 shortly.
chars (octets) per pixel. -Chris
Lets make sure some basic expressions are always true: bpp != NULL width != NULL height != NULL stride = bpp * width < 2^32 size = stride * height < 2^32 PAGE_ALIGN(size) < 2^32
At least the udl driver doesn't check for multiplication-overflows, so lets just make sure it will never happen. These checks allow drivers to do any 32bit math without having to test for mult-overflows themselves.
The two divisions might hurt performance a bit, but dumb_create() is only used for scanout-buffers, so that should be fine. We could use 64bit math to avoid the divisions, but that may be slow on 32bit machines.. Or maybe there should just be a "safe_mult32()" helper, which currently doesn't exist (I think?).
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_crtc.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 266a01d..2b50702 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3738,9 +3738,26 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_create_dumb *args = data; + u32 cpp, stride, size;
if (!dev->driver->dumb_create) return -ENOSYS; + if (!args->width || !args->height || !args->bpp) + return -EINVAL; + + /* overflow checks for 32bit size calculations */ + cpp = DIV_ROUND_UP(args->bpp, 8); + if (cpp > 0xffffffffU / args->width) + return -EINVAL; + stride = cpp * args->width; + if (args->height > 0xffffffffU / stride) + return -EINVAL; + + /* test for wrap-around */ + size = args->height * stride; + if (PAGE_ALIGN(size) == 0) + return -EINVAL; + return dev->driver->dumb_create(file_priv, dev, args); }
On Thu, Jan 23, 2014 at 01:53:15PM +0100, David Herrmann wrote:
Lets make sure some basic expressions are always true: bpp != NULL width != NULL height != NULL stride = bpp * width < 2^32 size = stride * height < 2^32 PAGE_ALIGN(size) < 2^32
At least the udl driver doesn't check for multiplication-overflows, so lets just make sure it will never happen. These checks allow drivers to do any 32bit math without having to test for mult-overflows themselves.
The two divisions might hurt performance a bit, but dumb_create() is only used for scanout-buffers, so that should be fine. We could use 64bit math to avoid the divisions, but that may be slow on 32bit machines.. Or maybe there should just be a "safe_mult32()" helper, which currently doesn't exist (I think?).
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/drm_crtc.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 266a01d..2b50702 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3738,9 +3738,26 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_create_dumb *args = data;
u32 cpp, stride, size;
if (!dev->driver->dumb_create) return -ENOSYS;
if (!args->width || !args->height || !args->bpp)
return -EINVAL;/* overflow checks for 32bit size calculations */
cpp = DIV_ROUND_UP(args->bpp, 8);
if (cpp > 0xffffffffU / args->width)
return -EINVAL;
IIRC I used -ERANGE for such things in some drm_framebuffer checks. Not sure if that's the best error value or not, but using the same one in both places would be nice.
- stride = cpp * args->width;
- if (args->height > 0xffffffffU / stride)
return -EINVAL;- /* test for wrap-around */
- size = args->height * stride;
- if (PAGE_ALIGN(size) == 0)
return -EINVAL;- return dev->driver->dumb_create(file_priv, dev, args);
}
-- 1.8.5.3
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi
On Thu, Jan 23, 2014 at 2:55 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Thu, Jan 23, 2014 at 01:53:15PM +0100, David Herrmann wrote:
Lets make sure some basic expressions are always true: bpp != NULL width != NULL height != NULL stride = bpp * width < 2^32 size = stride * height < 2^32 PAGE_ALIGN(size) < 2^32
At least the udl driver doesn't check for multiplication-overflows, so lets just make sure it will never happen. These checks allow drivers to do any 32bit math without having to test for mult-overflows themselves.
The two divisions might hurt performance a bit, but dumb_create() is only used for scanout-buffers, so that should be fine. We could use 64bit math to avoid the divisions, but that may be slow on 32bit machines.. Or maybe there should just be a "safe_mult32()" helper, which currently doesn't exist (I think?).
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/drm_crtc.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 266a01d..2b50702 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3738,9 +3738,26 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_create_dumb *args = data;
u32 cpp, stride, size; if (!dev->driver->dumb_create) return -ENOSYS;if (!args->width || !args->height || !args->bpp)return -EINVAL;/* overflow checks for 32bit size calculations */cpp = DIV_ROUND_UP(args->bpp, 8);if (cpp > 0xffffffffU / args->width)return -EINVAL;IIRC I used -ERANGE for such things in some drm_framebuffer checks. Not sure if that's the best error value or not, but using the same one in both places would be nice.
I'm actually a fan of EINVAL here, as this is really more about "do the arguments make sense?" than "does the range exceed the driver's capabilities?" But what do I know.. So more comments on that are welcome. And btw., we already mix EINVAL and ERANGE so this would just contribute to the already existing mess (see the stride verifications which usually return EINVAL, but the overflow checks often return ERANGE).
Thanks David
stride = cpp * args->width;if (args->height > 0xffffffffU / stride)return -EINVAL;/* test for wrap-around */size = args->height * stride;if (PAGE_ALIGN(size) == 0)return -EINVAL;return dev->driver->dumb_create(file_priv, dev, args);}
-- 1.8.5.3
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Ville Syrjälä Intel OTC
ping
On Thu, Jan 23, 2014 at 3:10 PM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
On Thu, Jan 23, 2014 at 2:55 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Thu, Jan 23, 2014 at 01:53:15PM +0100, David Herrmann wrote:
Lets make sure some basic expressions are always true: bpp != NULL width != NULL height != NULL stride = bpp * width < 2^32 size = stride * height < 2^32 PAGE_ALIGN(size) < 2^32
At least the udl driver doesn't check for multiplication-overflows, so lets just make sure it will never happen. These checks allow drivers to do any 32bit math without having to test for mult-overflows themselves.
The two divisions might hurt performance a bit, but dumb_create() is only used for scanout-buffers, so that should be fine. We could use 64bit math to avoid the divisions, but that may be slow on 32bit machines.. Or maybe there should just be a "safe_mult32()" helper, which currently doesn't exist (I think?).
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/drm_crtc.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 266a01d..2b50702 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3738,9 +3738,26 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_create_dumb *args = data;
u32 cpp, stride, size; if (!dev->driver->dumb_create) return -ENOSYS;if (!args->width || !args->height || !args->bpp)return -EINVAL;/* overflow checks for 32bit size calculations */cpp = DIV_ROUND_UP(args->bpp, 8);if (cpp > 0xffffffffU / args->width)return -EINVAL;IIRC I used -ERANGE for such things in some drm_framebuffer checks. Not sure if that's the best error value or not, but using the same one in both places would be nice.
I'm actually a fan of EINVAL here, as this is really more about "do the arguments make sense?" than "does the range exceed the driver's capabilities?" But what do I know.. So more comments on that are welcome. And btw., we already mix EINVAL and ERANGE so this would just contribute to the already existing mess (see the stride verifications which usually return EINVAL, but the overflow checks often return ERANGE).
Thanks David
stride = cpp * args->width;if (args->height > 0xffffffffU / stride)return -EINVAL;/* test for wrap-around */size = args->height * stride;if (PAGE_ALIGN(size) == 0)return -EINVAL;return dev->driver->dumb_create(file_priv, dev, args);}
-- 1.8.5.3
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Ville Syrjälä Intel OTC
There is no need to initialize this variable, so drop it. Otherwise, the compiler won't warn if we use it unintialized.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 7bf374e..700e8df 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -819,7 +819,7 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; struct drm_gem_object *obj; struct drm_vma_offset_node *node; - int ret = 0; + int ret;
if (drm_device_is_unplugged(dev)) return -ENODEV;
On Mon, Jan 20, 2014 at 08:26:29PM +0100, David Herrmann wrote:
There is no need to initialize this variable, so drop it. Otherwise, the compiler won't warn if we use it unintialized.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
I've replied with a few small comments on some patches, with those addressed all but patch 3 are
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
A follow-up to 4 to remove callsites from drivers would be neat though. -Daniel
drivers/gpu/drm/drm_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 7bf374e..700e8df 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -819,7 +819,7 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; struct drm_gem_object *obj; struct drm_vma_offset_node *node;
- int ret = 0;
int ret;
if (drm_device_is_unplugged(dev)) return -ENODEV;
-- 1.8.5.3
ping
On Tue, Jan 21, 2014 at 10:51 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Jan 20, 2014 at 08:26:29PM +0100, David Herrmann wrote:
There is no need to initialize this variable, so drop it. Otherwise, the compiler won't warn if we use it unintialized.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
I've replied with a few small comments on some patches, with those addressed all but patch 3 are
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
A follow-up to 4 to remove callsites from drivers would be neat though. -Daniel
drivers/gpu/drm/drm_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 7bf374e..700e8df 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -819,7 +819,7 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; struct drm_gem_object *obj; struct drm_vma_offset_node *node;
int ret = 0;
int ret; if (drm_device_is_unplugged(dev)) return -ENODEV;-- 1.8.5.3
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Jan 20, 2014 at 08:26:23PM +0100, David Herrmann wrote:
We need to call dma_buf_end_cpu_access() in case a damage-request. Unlikely, but might happen during device unplug.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/udl/udl_fb.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index dbadd49..50f564d 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -421,9 +421,10 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, clips[i].x2 - clips[i].x1, clips[i].y2 - clips[i].y1); if (ret)
goto unlock;
goto end_access;
break instead of a goto?
}
+end_access: if (ufb->obj->base.import_attach) { dma_buf_end_cpu_access(ufb->obj->base.import_attach->dmabuf, 0, ufb->obj->base.size, -- 1.8.5.3
We need to call dma_buf_end_cpu_access() in case a damage-request. Unlikely, but might happen during device unplug.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/udl/udl_fb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index dbadd49..3771763 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -421,7 +421,7 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, clips[i].x2 - clips[i].x1, clips[i].y2 - clips[i].y1); if (ret) - goto unlock; + break; }
if (ufb->obj->base.import_attach) {
ping..
On Thu, Jan 23, 2014 at 1:48 PM, David Herrmann dh.herrmann@gmail.com wrote:
We need to call dma_buf_end_cpu_access() in case a damage-request. Unlikely, but might happen during device unplug.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/udl/udl_fb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index dbadd49..3771763 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -421,7 +421,7 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb, clips[i].x2 - clips[i].x1, clips[i].y2 - clips[i].y1); if (ret)
goto unlock;
break; } if (ufb->obj->base.import_attach) {-- 1.8.5.3
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
David Herrmann -
Ville Syrjälä