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@xxxxxx> > > > Tested-by: Pasi Kärkkäinen <pasik@xxxxxx> > > > Signed-off-by: Ilia Mirkin <imirkin@xxxxxxxxxxxx> > > > Cc: <stable@xxxxxxxxxxxxxxx> # 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
