Re: [Intel-gfx] [PATCH v2 2/3] drm/i915: Document our internal limit on object size
On Tue, Oct 18, 2016 at 10:27:58AM +0100, Tvrtko Ursulin wrote: > > On 17/10/2016 09:00, Chris Wilson wrote: > >In many places, we try to count pages using a 32 bit integer. That > >implies if we are asked to create an object larger than 43bits, we will > >subtly crash much later. Catch this on the boundary, and add a warning > >to remind ourselves later on our exabyte systems. > > > >Signed-off-by: Chris Wilson> >--- > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > drivers/gpu/drm/i915/i915_gem.c | 17 +++-- > > 2 files changed, 16 insertions(+), 3 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_drv.h > >b/drivers/gpu/drm/i915/i915_drv.h > >index 092c5a0a44f0..a2b5fc72fdd9 100644 > >--- a/drivers/gpu/drm/i915/i915_drv.h > >+++ b/drivers/gpu/drm/i915/i915_drv.h > >@@ -3105,7 +3105,7 @@ void i915_gem_object_free(struct drm_i915_gem_object > >*obj); > > void i915_gem_object_init(struct drm_i915_gem_object *obj, > > const struct drm_i915_gem_object_ops *ops); > > struct drm_i915_gem_object *i915_gem_object_create(struct drm_device *dev, > >- size_t size); > >+ u64 size); > > struct drm_i915_gem_object *i915_gem_object_create_from_data( > > struct drm_device *dev, const void *data, size_t size); > > void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file > > *file); > >diff --git a/drivers/gpu/drm/i915/i915_gem.c > >b/drivers/gpu/drm/i915/i915_gem.c > >index 838dc159a2d1..181bda2db587 100644 > >--- a/drivers/gpu/drm/i915/i915_gem.c > >+++ b/drivers/gpu/drm/i915/i915_gem.c > >@@ -4131,14 +4131,27 @@ static const struct drm_i915_gem_object_ops > >i915_gem_object_ops = { > > .put_pages = i915_gem_object_put_pages_gtt, > > }; > >-struct drm_i915_gem_object *i915_gem_object_create(struct drm_device *dev, > >- size_t size) > >+#define overflows_type(x, T) \ > >+(sizeof(x) > sizeof(T) && (x) >> (sizeof(T) * BITS_PER_BYTE)) > >+ > > Looks like it wouldn't detect storing unsigned int in a signed int > but I guess we don't care that much as long as this is local use > only. Just slightly relevant because of the int page_count situation > we mention below. Hmm. Yeah, definitely worth improving. Quick googling shows that you are the first to notice! :-p I was thinking of trying gcc's __builtin_add_overflowp(x, 0, T) or something like that. But I also wonder if we can use signed T *var vs unsigned T *var in any way to generalise the number of positive bits in a type. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 2/3] drm/i915: Document our internal limit on object size
On 17/10/2016 09:00, Chris Wilson wrote: In many places, we try to count pages using a 32 bit integer. That implies if we are asked to create an object larger than 43bits, we will subtly crash much later. Catch this on the boundary, and add a warning to remind ourselves later on our exabyte systems. Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/i915_gem.c | 17 +++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 092c5a0a44f0..a2b5fc72fdd9 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3105,7 +3105,7 @@ void i915_gem_object_free(struct drm_i915_gem_object *obj); void i915_gem_object_init(struct drm_i915_gem_object *obj, const struct drm_i915_gem_object_ops *ops); struct drm_i915_gem_object *i915_gem_object_create(struct drm_device *dev, - size_t size); + u64 size); struct drm_i915_gem_object *i915_gem_object_create_from_data( struct drm_device *dev, const void *data, size_t size); void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *file); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 838dc159a2d1..181bda2db587 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4131,14 +4131,27 @@ static const struct drm_i915_gem_object_ops i915_gem_object_ops = { .put_pages = i915_gem_object_put_pages_gtt, }; -struct drm_i915_gem_object *i915_gem_object_create(struct drm_device *dev, - size_t size) +#define overflows_type(x, T) \ + (sizeof(x) > sizeof(T) && (x) >> (sizeof(T) * BITS_PER_BYTE)) + Looks like it wouldn't detect storing unsigned int in a signed int but I guess we don't care that much as long as this is local use only. Just slightly relevant because of the int page_count situation we mention below. +struct drm_i915_gem_object * +i915_gem_object_create(struct drm_device *dev, u64 size) { struct drm_i915_gem_object *obj; struct address_space *mapping; gfp_t mask; int ret; + /* There is a prevalence of the assumption that we fit the object's +* page count inside a 32bit variable. Let's document this and catch _Signed_ 32-bit integer as you have explained to justify the INT_MAX below. +* if we ever need to fix it. +*/ + if (WARN_ON(size >> PAGE_SHIFT > INT_MAX)) + return ERR_PTR(-E2BIG); + + if (overflows_type(size, obj->base.size)) + return ERR_PTR(-E2BIG); + obj = i915_gem_object_alloc(dev); if (obj == NULL) return ERR_PTR(-ENOMEM); With the comment clarification, Reviewed-by: Tvrtko Ursulin Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 2/3] drm/i915: Document our internal limit on object size
In many places, we try to count pages using a 32 bit integer. That implies if we are asked to create an object larger than 43bits, we will subtly crash much later. Catch this on the boundary, and add a warning to remind ourselves later on our exabyte systems. Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/i915_gem.c | 17 +++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 092c5a0a44f0..a2b5fc72fdd9 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3105,7 +3105,7 @@ void i915_gem_object_free(struct drm_i915_gem_object *obj); void i915_gem_object_init(struct drm_i915_gem_object *obj, const struct drm_i915_gem_object_ops *ops); struct drm_i915_gem_object *i915_gem_object_create(struct drm_device *dev, - size_t size); + u64 size); struct drm_i915_gem_object *i915_gem_object_create_from_data( struct drm_device *dev, const void *data, size_t size); void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *file); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 838dc159a2d1..181bda2db587 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4131,14 +4131,27 @@ static const struct drm_i915_gem_object_ops i915_gem_object_ops = { .put_pages = i915_gem_object_put_pages_gtt, }; -struct drm_i915_gem_object *i915_gem_object_create(struct drm_device *dev, - size_t size) +#define overflows_type(x, T) \ + (sizeof(x) > sizeof(T) && (x) >> (sizeof(T) * BITS_PER_BYTE)) + +struct drm_i915_gem_object * +i915_gem_object_create(struct drm_device *dev, u64 size) { struct drm_i915_gem_object *obj; struct address_space *mapping; gfp_t mask; int ret; + /* There is a prevalence of the assumption that we fit the object's +* page count inside a 32bit variable. Let's document this and catch +* if we ever need to fix it. +*/ + if (WARN_ON(size >> PAGE_SHIFT > INT_MAX)) + return ERR_PTR(-E2BIG); + + if (overflows_type(size, obj->base.size)) + return ERR_PTR(-E2BIG); + obj = i915_gem_object_alloc(dev); if (obj == NULL) return ERR_PTR(-ENOMEM); -- 2.9.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx