| Message ID | 5226DA41.5060203@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 04, 2013 at 08:59:13AM +0200, Maarten Lankhorst wrote: > Op 04-09-13 05:41, Ben Skeggs schreef: > > 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? > > > If it has a WARN_ON or something that would be ok, I didn't find any other places that attempt to move buffers to VRAM though, so it's probably harmless. > So hmm.. I guess another patch is needed for the original issue in this thread. Is someone going to submit that? > When looking into this bug I noticed that nouveau_bo_vma_add needs to have a check for nvbo->page_shift == vma->vm->vmm->spg_shift, > and only if the check is true it should map the page in TTM_PL_TT. Patch below. > Should probably also be cc'd to stable. > Thanks! Is this patch ready to be merged? -- Pasi > ~Maarten > > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c > index 89b992e..355a1b7 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c > @@ -1560,7 +1560,8 @@ nouveau_bo_vma_add(struct nouveau_bo *nvbo, struct nouveau_vm *vm, > > if (nvbo->bo.mem.mem_type == TTM_PL_VRAM) > nouveau_vm_map(vma, nvbo->bo.mem.mm_node); > - else if (nvbo->bo.mem.mem_type == TTM_PL_TT) { > + else if (nvbo->bo.mem.mem_type == TTM_PL_TT && > + nvbo->page_shift == vma->vm->vmm->spg_shift) { > if (node->sg) > nouveau_vm_map_sg_table(vma, 0, size, node); > else >
Hello, On Wed, Sep 04, 2013 at 08:59:13AM +0200, Maarten Lankhorst wrote: > Op 04-09-13 05:41, Ben Skeggs schreef: > > 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? > > > If it has a WARN_ON or something that would be ok, I didn't find any other places that attempt to move buffers to VRAM though, so it's probably harmless. > Ben/Maarten: Are you guys planning to take a look at this and submit another patch, or.. ? I tested the two earlier patches from this thread, and they both fixed the problem (hard kernel crash). I'm hoping this bug could be finally solved in the kernel.. Thanks, -- Pasi
On Wed, Sep 04, 2013 at 08:59:13AM +0200, Maarten Lankhorst wrote: > > When looking into this bug I noticed that nouveau_bo_vma_add needs to have a check for nvbo->page_shift == vma->vm->vmm->spg_shift, > and only if the check is true it should map the page in TTM_PL_TT. Patch below. > Should probably also be cc'd to stable. > How about this patch? Is it ready to go in? Thanks, -- Pasi > ~Maarten > > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c > index 89b992e..355a1b7 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c > @@ -1560,7 +1560,8 @@ nouveau_bo_vma_add(struct nouveau_bo *nvbo, struct nouveau_vm *vm, > > if (nvbo->bo.mem.mem_type == TTM_PL_VRAM) > nouveau_vm_map(vma, nvbo->bo.mem.mm_node); > - else if (nvbo->bo.mem.mem_type == TTM_PL_TT) { > + else if (nvbo->bo.mem.mem_type == TTM_PL_TT && > + nvbo->page_shift == vma->vm->vmm->spg_shift) { > if (node->sg) > nouveau_vm_map_sg_table(vma, 0, size, node); > else >
On Thu, Sep 26, 2013 at 12:41 AM, Pasi Kärkkäinen <pasik@iki.fi> wrote: > Hello, > > On Wed, Sep 04, 2013 at 08:59:13AM +0200, Maarten Lankhorst wrote: >> Op 04-09-13 05:41, Ben Skeggs schreef: >> > 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? >> > >> If it has a WARN_ON or something that would be ok, I didn't find any other places that attempt to move buffers to VRAM though, so it's probably harmless. >> > > Ben/Maarten: Are you guys planning to take a look at this and submit another patch, or.. ? > > I tested the two earlier patches from this thread, and they both fixed the problem (hard kernel crash). > I'm hoping this bug could be finally solved in the kernel.. I shall be looking at it properly once I'm back from XDC next week. Thanks, Ben. > > Thanks, > > -- Pasi >
On Thu, Sep 26, 2013 at 02:48:49AM +1000, Ben Skeggs wrote: > On Thu, Sep 26, 2013 at 12:41 AM, Pasi Kärkkäinen <pasik@iki.fi> wrote: > > Hello, > > > > On Wed, Sep 04, 2013 at 08:59:13AM +0200, Maarten Lankhorst wrote: > >> Op 04-09-13 05:41, Ben Skeggs schreef: > >> > 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? > >> > > >> If it has a WARN_ON or something that would be ok, I didn't find any other places that attempt to move buffers to VRAM though, so it's probably harmless. > >> > > > > Ben/Maarten: Are you guys planning to take a look at this and submit another patch, or.. ? > > > > I tested the two earlier patches from this thread, and they both fixed the problem (hard kernel crash). > > I'm hoping this bug could be finally solved in the kernel.. > I shall be looking at it properly once I'm back from XDC next week. > Great, thanks! -- Pasi > Thanks, > Ben. >
On Thu, Sep 26, 2013 at 02:48:49AM +1000, Ben Skeggs wrote: > > > > Ben/Maarten: Are you guys planning to take a look at this and submit another patch, or.. ? > > > > I tested the two earlier patches from this thread, and they both fixed the problem (hard kernel crash). > > I'm hoping this bug could be finally solved in the kernel.. > > I shall be looking at it properly once I'm back from XDC next week. > Any thoughts about the patch? -- Pasi > Thanks, > Ben. > > > > > Thanks, > > > > -- Pasi > >
On Wed, Sep 25, 2013 at 05:42:46PM +0300, Pasi Kärkkäinen wrote: > On Wed, Sep 04, 2013 at 08:59:13AM +0200, Maarten Lankhorst wrote: > > > > When looking into this bug I noticed that nouveau_bo_vma_add needs to have a check for nvbo->page_shift == vma->vm->vmm->spg_shift, > > and only if the check is true it should map the page in TTM_PL_TT. Patch below. > > Should probably also be cc'd to stable. > > > > How about this patch? Is it ready to go in? > Ping on this patch aswell.. -- Pasi > Thanks, > > -- Pasi > > > > ~Maarten > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c > > index 89b992e..355a1b7 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c > > @@ -1560,7 +1560,8 @@ nouveau_bo_vma_add(struct nouveau_bo *nvbo, struct nouveau_vm *vm, > > > > if (nvbo->bo.mem.mem_type == TTM_PL_VRAM) > > nouveau_vm_map(vma, nvbo->bo.mem.mm_node); > > - else if (nvbo->bo.mem.mem_type == TTM_PL_TT) { > > + else if (nvbo->bo.mem.mem_type == TTM_PL_TT && > > + nvbo->page_shift == vma->vm->vmm->spg_shift) { > > if (node->sg) > > nouveau_vm_map_sg_table(vma, 0, size, node); > > else > >
On Fri, Oct 18, 2013 at 05:44:45PM +0300, Pasi Kärkkäinen wrote: > On Thu, Sep 26, 2013 at 02:48:49AM +1000, Ben Skeggs wrote: > > > > > > Ben/Maarten: Are you guys planning to take a look at this and submit another patch, or.. ? > > > > > > I tested the two earlier patches from this thread, and they both fixed the problem (hard kernel crash). > > > I'm hoping this bug could be finally solved in the kernel.. > > > > I shall be looking at it properly once I'm back from XDC next week. > > > > Any thoughts about the patch? > > Ben: ping again? This nouveau bug is still causing hard kernel crashes out there.. recent bug report against Fedora 20: https://bugzilla.redhat.com/show_bug.cgi?id=1047169 Thanks, -- Pasi > > > Thanks, > > Ben. > >
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c index 89b992e..355a1b7 100644 --- a/drivers/gpu/drm/nouveau/nouveau_bo.c +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c @@ -1560,7 +1560,8 @@ nouveau_bo_vma_add(struct nouveau_bo *nvbo, struct nouveau_vm *vm, if (nvbo->bo.mem.mem_type == TTM_PL_VRAM) nouveau_vm_map(vma, nvbo->bo.mem.mm_node); - else if (nvbo->bo.mem.mem_type == TTM_PL_TT) { + else if (nvbo->bo.mem.mem_type == TTM_PL_TT && + nvbo->page_shift == vma->vm->vmm->spg_shift) { if (node->sg) nouveau_vm_map_sg_table(vma, 0, size, node); else