Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965961AbbDWNOd (ORCPT ); Thu, 23 Apr 2015 09:14:33 -0400 Received: from mail-oi0-f41.google.com ([209.85.218.41]:32838 "EHLO mail-oi0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932206AbbDWNOa (ORCPT ); Thu, 23 Apr 2015 09:14:30 -0400 MIME-Version: 1.0 In-Reply-To: <1429721111-22845-1-git-send-email-mika.kuoppala@intel.com> References: <1429721111-22845-1-git-send-email-mika.kuoppala@intel.com> Date: Thu, 23 Apr 2015 09:14:30 -0400 X-Google-Sender-Auth: 4TdCoG2tzJ-Bs2J2ZeoaBfBD10U Message-ID: Subject: Re: [PATCH] drm/i915: Add checks to i915_bind_vma From: Josh Boyer To: Mika Kuoppala Cc: Linus Torvalds , Daniel Vetter , Jani Nikula , Dave Airlie , ben@bwidawsk.net, Intel Graphics Development , "Linux-Kernel@Vger. Kernel. Org" , Chris Wilson , Michel Thierry Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2830 Lines: 72 On Wed, Apr 22, 2015 at 12:45 PM, Mika Kuoppala wrote: > The current aliasing ppgtt implementation allocates > the page table structures on driver initialization > for the entire vm address space. Earlier the page tables > were allocated as array of struct pages, but introduction > of dynamic allocation of page structures changed the page > tables to be inside a page directory. > > We have a detailed bug report where traversing of tables and > deferencing page_table[pte]->page oopses. This indicates that > our pre allocation of page tables has failed or that we get > corrupt vma which does not fit inside the vm area and throws > pte > 511. > > Add more checks to catch the latter. Warn and bail out if > we get vma which is out of bounds for binding. > > v2: Check vma node early (Chris) > > Cc: Linus Torvalds > Cc: Chris Wilson > Cc: Michel Thierry > Signed-off-by: Mika Kuoppala > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 0239fbf..2ffa8f6 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -2746,6 +2746,13 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma) > int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, > u32 flags) > { > + > + if (WARN_ON(!drm_mm_node_allocated(&vma->node))) > + return -EINVAL; > + > + if (WARN_ON(vma->node.start > vma->vm->total - vma->node.size)) > + return -EINVAL; > + Do you really need a full backtrace in these cases? From an end user perspective, they're going to get a popup saying they have a kernel problem or an automated tool will file a bug for them and they will have no idea what any of this means. I understand the need for the check, but could it be done in a way that doesn't splat an oops on a user's system? The i915 driver has tons of these kinds of WARN_ONs already and they don't seem to be helping anything overall... josh > if (i915_is_ggtt(vma->vm)) { > int ret = i915_get_ggtt_vma_pages(vma); > > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/