From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: Use kcalloc more Date: Sat, 21 Sep 2013 00:37:42 +0200 Message-ID: <20130920223741.GT32145@phenom.ffwll.local> References: <20130919104655.GE6566@nuc-i3427.alporthouse.com> <1379592399-20360-1-git-send-email-daniel.vetter@ffwll.ch> <20130919123022.GH6566@nuc-i3427.alporthouse.com> <20130919124153.GI6566@nuc-i3427.alporthouse.com> <20130919125110.GX32145@phenom.ffwll.local> <20130919125809.GJ6566@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f47.google.com (mail-ee0-f47.google.com [74.125.83.47]) by gabe.freedesktop.org (Postfix) with ESMTP id 4B2E1E5CCB for ; Fri, 20 Sep 2013 15:37:28 -0700 (PDT) Received: by mail-ee0-f47.google.com with SMTP id d49so544165eek.34 for ; Fri, 20 Sep 2013 15:37:27 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20130919125809.GJ6566@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson , Daniel Vetter , Daniel Vetter , Intel Graphics Development , Jani Nikula List-Id: intel-gfx@lists.freedesktop.org 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 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch