| Message ID | 5215B9E8.5080108@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@iki.fi> > > Tested-by: Pasi Kärkkäinen <pasik@iki.fi> > > Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu> > > Cc: <stable@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... > Thanks! I'll give it a try later today.. -- Pasi > diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c > --- a/drivers/gpu/drm/nouveau/nouveau_display.c > +++ b/drivers/gpu/drm/nouveau/nouveau_display.c > @@ -138,17 +143,26 @@ nouveau_user_framebuffer_create(struct drm_device *dev, > { > struct nouveau_framebuffer *nouveau_fb; > struct drm_gem_object *gem; > + struct nouveau_bo *nvbo; > int ret = -ENOMEM; > > 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; > + goto err_unref; > + } > + > nouveau_fb = kzalloc(sizeof(struct nouveau_framebuffer), GFP_KERNEL); > if (!nouveau_fb) > goto err_unref; > > - 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) > goto err; > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Aug 22, 2013 at 5:12 PM, Maarten Lankhorst <maarten.lankhorst@canonical.com> 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@iki.fi> >> Tested-by: Pasi Kärkkäinen <pasik@iki.fi> >> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu> >> Cc: <stable@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... > > diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c > --- a/drivers/gpu/drm/nouveau/nouveau_display.c > +++ b/drivers/gpu/drm/nouveau/nouveau_display.c > @@ -138,17 +143,26 @@ nouveau_user_framebuffer_create(struct drm_device *dev, > { > struct nouveau_framebuffer *nouveau_fb; > struct drm_gem_object *gem; > + struct nouveau_bo *nvbo; > int ret = -ENOMEM; > > 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; > + goto err_unref; > + } > + Definitely the right idea, we can't handle this case right now. However, we may someday want/need to be able to scan out of system memory, so this is the wrong place. I suspect the correct thing to do (which'll also handle the "defensive" part) is to bail in nouveau_bo_move() on attempts to move a DMA-BUF backed object into VRAM. Sound OK? Ben. > nouveau_fb = kzalloc(sizeof(struct nouveau_framebuffer), GFP_KERNEL); > if (!nouveau_fb) > goto err_unref; > > - 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) > goto err; > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -138,17 +143,26 @@ nouveau_user_framebuffer_create(struct drm_device *dev, { struct nouveau_framebuffer *nouveau_fb; struct drm_gem_object *gem; + struct nouveau_bo *nvbo; int ret = -ENOMEM; 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; + goto err_unref; + } + nouveau_fb = kzalloc(sizeof(struct nouveau_framebuffer), GFP_KERNEL); if (!nouveau_fb) goto err_unref; - 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) goto err;