| Message ID | 1379592399-20360-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 19, 2013 at 02:06:39PM +0200, Daniel Vetter wrote: > No buffer overflows here, but better safe than sorry. > > v2: > - Fixup the sizeof conversion, I've missed the pointer deref (Jani). > - Drop the redundant GFP_ZERO, kcalloc alreads memsets (Jani). > - Use kmalloc_array for the execbuf fastpath to avoid the memset > (Chris). I've opted to leave all other conversions as-is since they > aren't in a fastpath and dealing with cleared memory instead of > random garbage is just generally nicer. I still don't agree with this change to kmalloc_array. The code is written explicitly such that an invalid buffer_count is reported as EINVAL and not ENOMEM. -Chris
On Thu, Sep 19, 2013 at 2:30 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Thu, Sep 19, 2013 at 02:06:39PM +0200, Daniel Vetter wrote: >> No buffer overflows here, but better safe than sorry. >> >> v2: >> - Fixup the sizeof conversion, I've missed the pointer deref (Jani). >> - Drop the redundant GFP_ZERO, kcalloc alreads memsets (Jani). >> - Use kmalloc_array for the execbuf fastpath to avoid the memset >> (Chris). I've opted to leave all other conversions as-is since they >> aren't in a fastpath and dealing with cleared memory instead of >> random garbage is just generally nicer. > > I still don't agree with this change to kmalloc_array. The code is > written explicitly such that an invalid buffer_count is reported as > EINVAL and not ENOMEM. It's just paranoia - imo consistently using kcalloc/kmalloc array where possible is just safer. Note also that the subtest I've added explicitly checks for EINVAL, so if we ever botch this it should get caught. -Daniel
On Thu, Sep 19, 2013 at 02:35:42PM +0200, Daniel Vetter wrote: > On Thu, Sep 19, 2013 at 2:30 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Thu, Sep 19, 2013 at 02:06:39PM +0200, Daniel Vetter wrote: > >> No buffer overflows here, but better safe than sorry. > >> > >> v2: > >> - Fixup the sizeof conversion, I've missed the pointer deref (Jani). > >> - Drop the redundant GFP_ZERO, kcalloc alreads memsets (Jani). > >> - Use kmalloc_array for the execbuf fastpath to avoid the memset > >> (Chris). I've opted to leave all other conversions as-is since they > >> aren't in a fastpath and dealing with cleared memory instead of > >> random garbage is just generally nicer. > > > > I still don't agree with this change to kmalloc_array. The code is > > written explicitly such that an invalid buffer_count is reported as > > EINVAL and not ENOMEM. > > It's just paranoia - imo consistently using kcalloc/kmalloc array > where possible is just safer. Note also that the subtest I've added > explicitly checks for EINVAL, so if we ever botch this it should get > caught. Paranoia for what? Checking the same thing twice in case the compiler changes it mind? -Chris
On Thu, Sep 19, 2013 at 01:41:53PM +0100, Chris Wilson wrote: > On Thu, Sep 19, 2013 at 02:35:42PM +0200, Daniel Vetter wrote: > > On Thu, Sep 19, 2013 at 2:30 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > On Thu, Sep 19, 2013 at 02:06:39PM +0200, Daniel Vetter wrote: > > >> No buffer overflows here, but better safe than sorry. > > >> > > >> v2: > > >> - Fixup the sizeof conversion, I've missed the pointer deref (Jani). > > >> - Drop the redundant GFP_ZERO, kcalloc alreads memsets (Jani). > > >> - Use kmalloc_array for the execbuf fastpath to avoid the memset > > >> (Chris). I've opted to leave all other conversions as-is since they > > >> aren't in a fastpath and dealing with cleared memory instead of > > >> random garbage is just generally nicer. > > > > > > I still don't agree with this change to kmalloc_array. The code is > > > written explicitly such that an invalid buffer_count is reported as > > > EINVAL and not ENOMEM. > > > > It's just paranoia - imo consistently using kcalloc/kmalloc array > > where possible is just safer. Note also that the subtest I've added > > explicitly checks for EINVAL, so if we ever botch this it should get > > caught. > > Paranoia for what? Checking the same thing twice in case the compiler > changes it mind? The compiler actually removes the 2nd check since it's the same ;-) I just like the consisten pattern and cozy feeling that we'll have less to worry for potential overflows. I can ditch it if you deem it too offensive. -Daniel
On Thu, Sep 19, 2013 at 02:51:10PM +0200, Daniel Vetter wrote: > On Thu, Sep 19, 2013 at 01:41:53PM +0100, Chris Wilson wrote: > > On Thu, Sep 19, 2013 at 02:35:42PM +0200, Daniel Vetter wrote: > > > On Thu, Sep 19, 2013 at 2:30 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > On Thu, Sep 19, 2013 at 02:06:39PM +0200, Daniel Vetter wrote: > > > >> No buffer overflows here, but better safe than sorry. > > > >> > > > >> v2: > > > >> - Fixup the sizeof conversion, I've missed the pointer deref (Jani). > > > >> - Drop the redundant GFP_ZERO, kcalloc alreads memsets (Jani). > > > >> - Use kmalloc_array for the execbuf fastpath to avoid the memset > > > >> (Chris). I've opted to leave all other conversions as-is since they > > > >> aren't in a fastpath and dealing with cleared memory instead of > > > >> random garbage is just generally nicer. > > > > > > > > I still don't agree with this change to kmalloc_array. The code is > > > > written explicitly such that an invalid buffer_count is reported as > > > > EINVAL and not ENOMEM. > > > > > > It's just paranoia - imo consistently using kcalloc/kmalloc array > > > where possible is just safer. Note also that the subtest I've added > > > explicitly checks for EINVAL, so if we ever botch this it should get > > > caught. > > > > Paranoia for what? Checking the same thing twice in case the compiler > > changes it mind? > > The compiler actually removes the 2nd check since it's the same ;-) I just > like the consisten pattern and cozy feeling that we'll have less to worry > for potential overflows. I can ditch it if you deem it too offensive. Having been along this road before, I preferred the explicit checking that also gets the right return value. The goal here is perform all sanity checks as early as possible - but I'm not going to fight to move the cliprects test as cliprects are broken by design. -Chris
On Thu, 19 Sep 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > No buffer overflows here, but better safe than sorry. > > v2: > - Fixup the sizeof conversion, I've missed the pointer deref (Jani). > - Drop the redundant GFP_ZERO, kcalloc alreads memsets (Jani). > - Use kmalloc_array for the execbuf fastpath to avoid the memset > (Chris). I've opted to leave all other conversions as-is since they > aren't in a fastpath and dealing with cleared memory instead of > random garbage is just generally nicer. Whether the change to kmalloc_array makes sense or not I leave for you and Chris to figure out; otherwise it looks like it does what it says on the box. Reviewed-by: Jani Nikula <jani.nikula@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 +++++--- > drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++-- > drivers/gpu/drm/i915/i915_gem_tiling.c | 6 +++--- > drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++-- > drivers/gpu/drm/i915/intel_display.c | 2 +- > 5 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index ee93357..ccfb8e6 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1047,7 +1047,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > return -EINVAL; > } > > - cliprects = kmalloc(args->num_cliprects * sizeof(*cliprects), > + cliprects = kcalloc(args->num_cliprects, > + sizeof(*cliprects), > GFP_KERNEL); > if (cliprects == NULL) { > ret = -ENOMEM; > @@ -1302,8 +1303,9 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data, > return -EINVAL; > } > > - exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count, > - GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY); > + exec2_list = kmalloc_array(args->buffer_count, sizeof(*exec2_list), > + GFP_TEMPORARY | > + __GFP_NOWARN | __GFP_NORETRY); > if (exec2_list == NULL) > exec2_list = drm_malloc_ab(sizeof(*exec2_list), > args->buffer_count); > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 212f6d8..e999496 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -336,7 +336,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) > ppgtt->base.insert_entries = gen6_ppgtt_insert_entries; > ppgtt->base.cleanup = gen6_ppgtt_cleanup; > ppgtt->base.scratch = dev_priv->gtt.base.scratch; > - ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries, > + ppgtt->pt_pages = kcalloc(ppgtt->num_pd_entries, sizeof(struct page *), > GFP_KERNEL); > if (!ppgtt->pt_pages) > return -ENOMEM; > @@ -347,7 +347,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) > goto err_pt_alloc; > } > > - ppgtt->pt_dma_addr = kzalloc(sizeof(dma_addr_t) *ppgtt->num_pd_entries, > + ppgtt->pt_dma_addr = kcalloc(ppgtt->num_pd_entries, sizeof(dma_addr_t), > GFP_KERNEL); > if (!ppgtt->pt_dma_addr) > goto err_pt_alloc; > diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c > index 032e9ef..ac9ebe9 100644 > --- a/drivers/gpu/drm/i915/i915_gem_tiling.c > +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c > @@ -393,7 +393,7 @@ i915_gem_set_tiling(struct drm_device *dev, void *data, > /* Try to preallocate memory required to save swizzling on put-pages */ > if (i915_gem_object_needs_bit17_swizzle(obj)) { > if (obj->bit_17 == NULL) { > - obj->bit_17 = kmalloc(BITS_TO_LONGS(obj->base.size >> PAGE_SHIFT) * > + obj->bit_17 = kcalloc(BITS_TO_LONGS(obj->base.size >> PAGE_SHIFT), > sizeof(long), GFP_KERNEL); > } > } else { > @@ -504,8 +504,8 @@ i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj) > int i; > > if (obj->bit_17 == NULL) { > - obj->bit_17 = kmalloc(BITS_TO_LONGS(page_count) * > - sizeof(long), GFP_KERNEL); > + obj->bit_17 = kcalloc(BITS_TO_LONGS(page_count), > + sizeof(long), GFP_KERNEL); > if (obj->bit_17 == NULL) { > DRM_ERROR("Failed to allocate memory for bit 17 " > "record\n"); > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > index c38d575..fde7c4d 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -791,7 +791,7 @@ static void i915_gem_record_rings(struct drm_device *dev, > > error->ring[i].num_requests = count; > error->ring[i].requests = > - kmalloc(count*sizeof(struct drm_i915_error_request), > + kcalloc(count, sizeof(*error->ring[i].requests), > GFP_ATOMIC); > if (error->ring[i].requests == NULL) { > error->ring[i].num_requests = 0; > @@ -833,7 +833,7 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv, > error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx]; > > if (i) { > - active_bo = kmalloc(sizeof(*active_bo)*i, GFP_ATOMIC); > + active_bo = kcalloc(i, sizeof(*active_bo), GFP_ATOMIC); > if (active_bo) > pinned_bo = active_bo + error->active_bo_count[ndx]; > } > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index fe8db37..6b8a107 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9031,7 +9031,7 @@ static int __intel_set_mode(struct drm_crtc *crtc, > unsigned disable_pipes, prepare_pipes, modeset_pipes; > int ret = 0; > > - saved_mode = kmalloc(2 * sizeof(*saved_mode), GFP_KERNEL); > + saved_mode = kcalloc(2, sizeof(*saved_mode), GFP_KERNEL); > if (!saved_mode) > return -ENOMEM; > saved_hwmode = saved_mode + 1; > -- > 1.8.4.rc3 >
On Thu, Sep 19, 2013 at 01:58:09PM +0100, Chris Wilson wrote: > On Thu, Sep 19, 2013 at 02:51:10PM +0200, Daniel Vetter wrote: > > On Thu, Sep 19, 2013 at 01:41:53PM +0100, Chris Wilson wrote: > > > On Thu, Sep 19, 2013 at 02:35:42PM +0200, Daniel Vetter wrote: > > > > On Thu, Sep 19, 2013 at 2:30 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > > On Thu, Sep 19, 2013 at 02:06:39PM +0200, Daniel Vetter wrote: > > > > >> No buffer overflows here, but better safe than sorry. > > > > >> > > > > >> v2: > > > > >> - Fixup the sizeof conversion, I've missed the pointer deref (Jani). > > > > >> - Drop the redundant GFP_ZERO, kcalloc alreads memsets (Jani). > > > > >> - Use kmalloc_array for the execbuf fastpath to avoid the memset > > > > >> (Chris). I've opted to leave all other conversions as-is since they > > > > >> aren't in a fastpath and dealing with cleared memory instead of > > > > >> random garbage is just generally nicer. > > > > > > > > > > I still don't agree with this change to kmalloc_array. The code is > > > > > written explicitly such that an invalid buffer_count is reported as > > > > > EINVAL and not ENOMEM. > > > > > > > > It's just paranoia - imo consistently using kcalloc/kmalloc array > > > > where possible is just safer. Note also that the subtest I've added > > > > explicitly checks for EINVAL, so if we ever botch this it should get > > > > caught. > > > > > > Paranoia for what? Checking the same thing twice in case the compiler > > > changes it mind? > > > > The compiler actually removes the 2nd check since it's the same ;-) I just > > like the consisten pattern and cozy feeling that we'll have less to worry > > for potential overflows. I can ditch it if you deem it too offensive. > > Having been along this road before, I preferred the explicit checking > that also gets the right return value. The goal here is perform all > sanity checks as early as possible - but I'm not going to fight to move > the cliprects test as cliprects are broken by design. I've dropped the contentious hunk and merged all the reviewed patches. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index ee93357..ccfb8e6 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1047,7 +1047,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, return -EINVAL; } - cliprects = kmalloc(args->num_cliprects * sizeof(*cliprects), + cliprects = kcalloc(args->num_cliprects, + sizeof(*cliprects), GFP_KERNEL); if (cliprects == NULL) { ret = -ENOMEM; @@ -1302,8 +1303,9 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data, return -EINVAL; } - exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count, - GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY); + exec2_list = kmalloc_array(args->buffer_count, sizeof(*exec2_list), + GFP_TEMPORARY | + __GFP_NOWARN | __GFP_NORETRY); if (exec2_list == NULL) exec2_list = drm_malloc_ab(sizeof(*exec2_list), args->buffer_count); diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 212f6d8..e999496 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -336,7 +336,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) ppgtt->base.insert_entries = gen6_ppgtt_insert_entries; ppgtt->base.cleanup = gen6_ppgtt_cleanup; ppgtt->base.scratch = dev_priv->gtt.base.scratch; - ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries, + ppgtt->pt_pages = kcalloc(ppgtt->num_pd_entries, sizeof(struct page *), GFP_KERNEL); if (!ppgtt->pt_pages) return -ENOMEM; @@ -347,7 +347,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) goto err_pt_alloc; } - ppgtt->pt_dma_addr = kzalloc(sizeof(dma_addr_t) *ppgtt->num_pd_entries, + ppgtt->pt_dma_addr = kcalloc(ppgtt->num_pd_entries, sizeof(dma_addr_t), GFP_KERNEL); if (!ppgtt->pt_dma_addr) goto err_pt_alloc; diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c index 032e9ef..ac9ebe9 100644 --- a/drivers/gpu/drm/i915/i915_gem_tiling.c +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c @@ -393,7 +393,7 @@ i915_gem_set_tiling(struct drm_device *dev, void *data, /* Try to preallocate memory required to save swizzling on put-pages */ if (i915_gem_object_needs_bit17_swizzle(obj)) { if (obj->bit_17 == NULL) { - obj->bit_17 = kmalloc(BITS_TO_LONGS(obj->base.size >> PAGE_SHIFT) * + obj->bit_17 = kcalloc(BITS_TO_LONGS(obj->base.size >> PAGE_SHIFT), sizeof(long), GFP_KERNEL); } } else { @@ -504,8 +504,8 @@ i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj) int i; if (obj->bit_17 == NULL) { - obj->bit_17 = kmalloc(BITS_TO_LONGS(page_count) * - sizeof(long), GFP_KERNEL); + obj->bit_17 = kcalloc(BITS_TO_LONGS(page_count), + sizeof(long), GFP_KERNEL); if (obj->bit_17 == NULL) { DRM_ERROR("Failed to allocate memory for bit 17 " "record\n"); diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index c38d575..fde7c4d 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -791,7 +791,7 @@ static void i915_gem_record_rings(struct drm_device *dev, error->ring[i].num_requests = count; error->ring[i].requests = - kmalloc(count*sizeof(struct drm_i915_error_request), + kcalloc(count, sizeof(*error->ring[i].requests), GFP_ATOMIC); if (error->ring[i].requests == NULL) { error->ring[i].num_requests = 0; @@ -833,7 +833,7 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv, error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx]; if (i) { - active_bo = kmalloc(sizeof(*active_bo)*i, GFP_ATOMIC); + active_bo = kcalloc(i, sizeof(*active_bo), GFP_ATOMIC); if (active_bo) pinned_bo = active_bo + error->active_bo_count[ndx]; } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index fe8db37..6b8a107 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9031,7 +9031,7 @@ static int __intel_set_mode(struct drm_crtc *crtc, unsigned disable_pipes, prepare_pipes, modeset_pipes; int ret = 0; - saved_mode = kmalloc(2 * sizeof(*saved_mode), GFP_KERNEL); + saved_mode = kcalloc(2, sizeof(*saved_mode), GFP_KERNEL); if (!saved_mode) return -ENOMEM; saved_hwmode = saved_mode + 1;
No buffer overflows here, but better safe than sorry. v2: - Fixup the sizeof conversion, I've missed the pointer deref (Jani). - Drop the redundant GFP_ZERO, kcalloc alreads memsets (Jani). - Use kmalloc_array for the execbuf fastpath to avoid the memset (Chris). I've opted to leave all other conversions as-is since they aren't in a fastpath and dealing with cleared memory instead of random garbage is just generally nicer. Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 +++++--- drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++-- drivers/gpu/drm/i915/i915_gem_tiling.c | 6 +++--- drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++-- drivers/gpu/drm/i915/intel_display.c | 2 +- 5 files changed, 13 insertions(+), 11 deletions(-)