| Message ID | 1377130214-17522-1-git-send-email-imirkin@alum.mit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 22, 2013 at 10:10 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote: > 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 :) > > drivers/gpu/drm/nouveau/nouveau_bo.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c > index cdc3282..191145d 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c > @@ -963,6 +963,12 @@ nouveau_vma_getmap(struct nouveau_channel *chan, struct nouveau_bo *nvbo, > struct nouveau_mem *node = mem->mm_node; > int ret; > > + /* If we ever get here for a non-vram mem node that doesn't > + * have pages, we will end up doing a null deref in > + * nouveau_vm_map_sg. */ > + if (WARN_ON(mem->mem_type != TTM_PL_VRAM && !node->pages)) > + return -EINVAL; My guess here is that this is a mapping that requires the use of map_sg_table() (see nouveau_bo_move_ntfy() for the condition). I'm not entirely sure this should even be happening to be honest. I guess TTM is trying to move a shared buffer from GART to VRAM for some reason (userspace probably asked for it?).. And well, this really shouldn't be allowed.. The other device won't be able to touch it then. If you can confirm this is indeed what's happening, we should find out why and fix it (and have the kernel completely reject such attempts). Ben. > + > ret = nouveau_vm_get(nv_client(chan->cli)->vm, mem->num_pages << > PAGE_SHIFT, node->page_shift, > NV_MEM_ACCESS_RW, vma); > -- > 1.8.1.5 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Aug 22, 2013 at 04:41:06PM +1000, Ben Skeggs wrote: > On Thu, Aug 22, 2013 at 10:10 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote: > > 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 :) > > > > drivers/gpu/drm/nouveau/nouveau_bo.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c > > index cdc3282..191145d 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c > > @@ -963,6 +963,12 @@ nouveau_vma_getmap(struct nouveau_channel *chan, struct nouveau_bo *nvbo, > > struct nouveau_mem *node = mem->mm_node; > > int ret; > > > > + /* If we ever get here for a non-vram mem node that doesn't > > + * have pages, we will end up doing a null deref in > > + * nouveau_vm_map_sg. */ > > + if (WARN_ON(mem->mem_type != TTM_PL_VRAM && !node->pages)) > > + return -EINVAL; > My guess here is that this is a mapping that requires the use of > map_sg_table() (see nouveau_bo_move_ntfy() for the condition). > > I'm not entirely sure this should even be happening to be honest. I > guess TTM is trying to move a shared buffer from GART to VRAM for some > reason (userspace probably asked for it?).. And well, this really > shouldn't be allowed.. The other device won't be able to touch it > then. > > If you can confirm this is indeed what's happening, we should find out > why and fix it (and have the kernel completely reject such attempts). > Yes it does happen. I've been experiencing the kernel crash with Linux 3.8.x, 3.9.x and 3.10.x. I'm able to reproduce the crash when having Optimus enabled in BIOS on my Lenovo T430 laptop with Intel IGD + Nvidia GF108 GPU, booting to Xorg desktop, and when I try to enable external DVI monitor connected to nouveau card, the kernel crashes hard.. crash traceback and WARN_ON() tracebacks with this patch applied available in the bugzilla entry: https://bugs.freedesktop.org/show_bug.cgi?id=64774 -- Pasi > Ben. > > > + > > ret = nouveau_vm_get(nv_client(chan->cli)->vm, mem->num_pages << > > PAGE_SHIFT, node->page_shift, > > NV_MEM_ACCESS_RW, vma); > > -- > > 1.8.1.5 > > > > _______________________________________________ > > 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_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c index cdc3282..191145d 100644 --- a/drivers/gpu/drm/nouveau/nouveau_bo.c +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c @@ -963,6 +963,12 @@ nouveau_vma_getmap(struct nouveau_channel *chan, struct nouveau_bo *nvbo, struct nouveau_mem *node = mem->mm_node; int ret; + /* If we ever get here for a non-vram mem node that doesn't + * have pages, we will end up doing a null deref in + * nouveau_vm_map_sg. */ + if (WARN_ON(mem->mem_type != TTM_PL_VRAM && !node->pages)) + return -EINVAL; + ret = nouveau_vm_get(nv_client(chan->cli)->vm, mem->num_pages << PAGE_SHIFT, node->page_shift, NV_MEM_ACCESS_RW, vma);