On Fri, Aug 23, 2013 at 11:20:42PM +0300, Pasi K?rkk?inen wrote:
> On Thu, Aug 22, 2013 at 09:12:40AM +0200, Maarten Lankhorst wrote:
> > Op 22-08-13 02:10, Ilia Mirkin schreef:
> > > The code expects non-VRAM mem nodes to have a pages list. If that's not
> > > set, it will do a null deref down the line. Warn on that condition and
> > > return an error.
> > >
> > > See https://bugs.freedesktop.org/show_bug.cgi?id=64774
> > >
> > > Reported-by: Pasi K?rkk?inen <pasik at iki.fi>
> > > Tested-by: Pasi K?rkk?inen <pasik at iki.fi>
> > > Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> > > Cc: <stable at vger.kernel.org> # 3.8+
> > > ---
> > >
> > > I don't exactly understand what's going on, but this is just a
> > > straightforward way to avoid a null deref that you see happens in the
> > > bug. I haven't figured out the root cause of this, but it's getting
> > > well into the "I have no idea how TTM works" space. However this seems
> > > like a bit of defensive programming -- nouveau_vm_map_sg will pass
> > > node->pages as a list down, which will be dereferenced by
> > > nvc0_vm_map_sg. Perhaps the other arguments should make that
> > > dereferencing not happen, but it definitely was happening here, as you
> > > can see in the bug.
> > >
> > > Ben/Maarten, I'll let you judge whether this check is appropriate,
> > > since like I hope I was able to convey above, I'm just not really sure :)
> > Not it really isn't appropriate..
> >
> > You'd have to call call nouveau_vm_map_sg_table instead, the only place
> > that doesn't handle that correctly
> > is where it's not expected to be called.
> >
> > Here, have a completely untested patch to fix things...
> >
>
> Maarten: I've been testing this stuff with Linux 3.10.x, so I had to modify
> the patch a bit to make it apply there..
> I've attached the modified copy that applies to 3.10.9, hopefully I did the
> backport correctly.
>
> With Linux 3.10.9 and the patch applied the kernel doesn't crash anymore, and
> I get this error in dmesg:
>
> [ 76.105643] nouveau W[ DRM] Trying to create a fb in vram with
> valid_domains=00000004
>
> Does that help?
>
Any comments?
Maarten's patch works for me, I get that warning instead of a kernel crash,
but it's also a bigger change that doesn't apply to older kernels as-is.
Ilia's original patch in this thread can be applied to older kernels as-is,
and it also prevents the kernel crash from happening.
Should we get both applied, so Ilia's patch can be CC'd to stable ?
-- Pasi
> ---
> linux-3.10.9-100.fc18.x86_64/drivers/gpu/drm/nouveau/nouveau_display.c.orig
> 2013-07-01 01:13:29.000000000 +0300
> +++ linux-3.10.9-100.fc18.x86_64/drivers/gpu/drm/nouveau/nouveau_display.c
> 2013-08-23 19:56:52.038234281 +0300
> @@ -137,18 +137,31 @@
> {
> struct nouveau_framebuffer *nouveau_fb;
> struct drm_gem_object *gem;
> + struct nouveau_bo *nvbo;
> int ret;
>
> gem = drm_gem_object_lookup(dev, file_priv, mode_cmd->handles[0]);
> if (!gem)
> return ERR_PTR(-ENOENT);
>
> + nvbo = nouveau_gem_object(gem);
> + if (!(nvbo->valid_domains & NOUVEAU_GEM_DOMAIN_VRAM)) {
> + nv_warn(nouveau_drm(dev), "Trying to create a fb in vram with"
> + " valid_domains=%08x\n", nvbo->valid_domains);
> + ret = -EINVAL;
> + drm_gem_object_unreference(gem);
> + return ERR_PTR(ret);
> + }
> +
> nouveau_fb = kzalloc(sizeof(struct nouveau_framebuffer), GFP_KERNEL);
> - if (!nouveau_fb)
> + if (!nouveau_fb) {
> + drm_gem_object_unreference(gem);
> return ERR_PTR(-ENOMEM);
> + }
>
> - ret = nouveau_framebuffer_init(dev, nouveau_fb, mode_cmd,
> nouveau_gem_object(gem));
> + ret = nouveau_framebuffer_init(dev, nouveau_fb, mode_cmd, nvbo);
> if (ret) {
> + kfree(nouveau_fb);
> drm_gem_object_unreference(gem);
> return ERR_PTR(ret);
> }
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
