From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH] drm/i915/ppgtt: Prevent NULL deref in reset ioctl Date: Fri, 20 Dec 2013 10:21:27 -0800 Message-ID: <20131220182127.GA1204@bwidawsk.net> References: <1387495360-15114-1-git-send-email-benjamin.widawsky@intel.com> <20131220065556.GA31581@bwidawsk.net> <20131220141044.GA23400@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail.bwidawsk.net (bwidawsk.net [166.78.191.112]) by gabe.freedesktop.org (Postfix) with ESMTP id 9F6AFFA4F4 for ; Fri, 20 Dec 2013 10:21:33 -0800 (PST) Content-Disposition: inline In-Reply-To: <20131220141044.GA23400@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Daniel Vetter Cc: Intel GFX , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On Fri, Dec 20, 2013 at 03:11:09PM +0100, Daniel Vetter wrote: > On Thu, Dec 19, 2013 at 10:55:56PM -0800, Ben Widawsky wrote: > > On Fri, Dec 20, 2013 at 07:05:10AM +0100, Daniel Vetter wrote: > > > On Fri, Dec 20, 2013 at 12:22 AM, Ben Widawsky > > > wrote: > > > > ctx = i915_gem_context_get(file->driver_priv, args->ctx_id); > > > > - if (IS_ERR(ctx)) { > > > > + if (IS_ERR_OR_NULL(ctx)) { > > > > > > We now have half the callers check for IS_ERR and the others not. > > > Afaics i915_gem_context_get can only return NULL or a real context > > > though. Also from a quite read the expected error for a lookup failure > > > is ENOENT (and there seems to be a testcase for this). > > > -Daniel > > > > > > To your first point: > > I think checking null is always the right thing currently, but for > > future proofing, IS_ERR_OR_NULL is really the right thing. After his > > patch, I believe only i915_gem_context_destroy_ioctl is still incorrect. > > Using IS_ERR_OR_NULL on a return value which can never contain an > encoded errno value is imo confusing, more so when it's inconsitently > applied. > > > To the second: > > This is only based on my memory, so feel free to change whatever you > > need. When I retuned -ENXIO, the test failed. > > > > It should be: > > return ctx ? PTR_ERR(ctx) : -ENOENT; > > On a cursory read that's been the semantics before your patch. And there > seems to be a testcase in gem_reset_stat for this, have you run all > subtests? As I mentioned on IRC, but I think you missed it - a later subtest hits a really bad BUG (pre-ppgtt). It was all pass up to that subtest. > > > I had > > return ctx ? PTR_ERR(ctx) : -ENXIO; > > which made the subtest fail. However, as we've noted, this itself was > > not correct. Try the return above. > > Thanks, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Ben Widawsky, Intel Open Source Technology Center