[PATCH 1/7] drm/udl: fix error-path when damage-req fails

[PATCH 1/7] drm/udl: fix error-path when damage-req fails

[David Herrmann]

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,
-- 
1.8.5.3

[PATCH 2/7] drm/udl: fix Bpp calculation in dumb_create()

[David Herrmann]

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);
-- 
1.8.5.3

Re: [PATCH 2/7] drm/udl: fix Bpp calculation in dumb_create()

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/7] drm/udl: fix Bpp calculation in dumb_create()

[David Herrmann]

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

[PATCH v2 2/6] drm/udl: fix Bpp calculation in dumb_create()

[David Herrmann]

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

Re: [PATCH v2 2/6] drm/udl: fix Bpp calculation in dumb_create()

[David Herrmann]

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
>

[PATCH 3/7] drm/udl: import prime-fds with proper page-alignment

[David Herrmann]

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);
-- 
1.8.5.3

Re: [PATCH 3/7] drm/udl: import prime-fds with proper page-alignment

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/7] drm/udl: import prime-fds with proper page-alignment

[David Herrmann]

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

[PATCH 4/7] drm/gem: fix indentation

[David Herrmann]

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

Re: [PATCH 4/7] drm/gem: fix indentation

[David Herrmann]

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
>

[PATCH 5/7] drm/gem: free vma-node during object-cleanup

[David Herrmann]

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

Re: [PATCH 5/7] drm/gem: free vma-node during object-cleanup

[David Herrmann]

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
>

[PATCH 6/7] drm/crtc: add sanity checks to create_dumb()

[David Herrmann]

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);
 }
 
-- 
1.8.5.3

Re: [PATCH 6/7] drm/crtc: add sanity checks to create_dumb()

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/7] drm/crtc: add sanity checks to create_dumb()

[David Herrmann]

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

Re: [PATCH 6/7] drm/crtc: add sanity checks to create_dumb()

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 6/7] drm/crtc: add sanity checks to create_dumb()

[David Herrmann]

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

Re: [PATCH 6/7] drm/crtc: add sanity checks to create_dumb()

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2 5/6] drm/crtc: add sanity checks to create_dumb()

[David Herrmann]

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);
 }
 
-- 
1.8.5.3

Re: [PATCH v2 5/6] drm/crtc: add sanity checks to create_dumb()

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2 5/6] drm/crtc: add sanity checks to create_dumb()

[David Herrmann]

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

Re: [PATCH v2 5/6] drm/crtc: add sanity checks to create_dumb()

[David Herrmann]

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

[PATCH 7/7] drm/gem: dont init "ret" in drm_gem_mmap()

[David Herrmann]

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;
-- 
1.8.5.3

Re: [PATCH 7/7] drm/gem: dont init "ret" in drm_gem_mmap()

[Daniel Vetter]

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

Re: [PATCH 7/7] drm/gem: dont init "ret" in drm_gem_mmap()

[David Herrmann]

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

Re: [PATCH 1/7] drm/udl: fix error-path when damage-req fails

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v2 1/6] drm/udl: fix error-path when damage-req fails

[David Herrmann]

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

Re: [PATCH v2 1/6] drm/udl: fix error-path when damage-req fails

[David Herrmann]

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
>