Nouveau - Jul 2018 - [PATCH 0/2] drm/nouveau: Fix panic on nouveau unload.

If have a couple patches I found while looking at a panic
I was seeing while unloading the nouveau module.

Unloading the nouveau module on my optimus notebook machine causes
the system to panic. This started occuring when moving from 4.4
to 4.14.

These patches make it such that the system does not panic
when unloading the module.

4.14 also requires commit 34112bf4935d ("drm/nouveau/fbcon: fix NULL
pointer access in nouveau_fbcon_destroy") which was already included in
the 4.18 tree.

These patches make it so I can unload the module without a panic but
there is a warning when unloading the module:
 sysfs group 'power' not found for kobject 'nv_backlight'
 WARNING: CPU: 2 PID: 1434 at fs/sysfs/group.c:235 sysfs_remove_group+0x76/0x80
 RIP: 0010:sysfs_remove_group+0x76/0x80
 Call Trace:
  device_del+0x56/0x350
  ? down_write+0xe/0x40
  device_unregister+0x16/0x60
  nouveau_backlight_exit+0x4a/0x60 [nouveau]
  nouveau_display_destroy+0x29/0x80 [nouveau]
  nouveau_drm_unload+0x61/0xd0 [nouveau]
  drm_dev_unregister+0x3f/0xe0 [drm]
  drm_put_dev+0x27/0x50 [drm]
  nouveau_drm_device_remove+0x47/0x70 [nouveau]
  pci_device_remove+0x3b/0xb0
  device_release_driver_internal+0x180/0x250
  driver_detach+0x32/0x5f
  bus_remove_driver+0x74/0xc6
  pci_unregister_driver+0x22/0xa0
  nouveau_drm_exit+0x15/0x16b [nouveau]

I don't beleive them to be related. Perhaps there is another issue here?

Jeffery Miller (2):
  drm/nouveau/fbcon: Fix NULL pointer access in nouveau_fbcon_destroy.
  drm/nouveau/bl: Allocate backlight connector nodes.

 drivers/gpu/drm/nouveau/nouveau_backlight.c | 70 ++++++++++++++-------
 drivers/gpu/drm/nouveau/nouveau_fbcon.c     |  5 +-
 2 files changed, 51 insertions(+), 24 deletions(-)

-- 
2.17.1

It is possible for this to get called with a null helper.fb. This can
cause a bad pointer access when unloading the nouveau module on an
Optimus system.

Signed-off-by: Jeffery Miller <jmiller at neverware.com>
---
 drivers/gpu/drm/nouveau/nouveau_fbcon.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
index 85c1f10bc2b6..99b2e5bb2bce 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
@@ -420,7 +420,10 @@ nouveau_fbcon_create(struct drm_fb_helper *helper,
 static int
 nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *fbcon)
 {
-	struct nouveau_framebuffer *nouveau_fb =
nouveau_framebuffer(fbcon->helper.fb);
+	struct nouveau_framebuffer *nouveau_fb = NULL;
+
+	if (fbcon->helper.fb)
+		nouveau_fb = nouveau_framebuffer(fbcon->helper.fb);
 
 	drm_fb_helper_unregister_fbi(&fbcon->helper);
 	drm_fb_helper_fini(&fbcon->helper);
-- 
2.17.1

Avoid adding nodes to the bl_connectors list from
the stack.

Nodes on the stack were being added to the bl_connectors list.
nouveau_backlight_exit would panic while traversing the bl_connectors list.

This panic was observed on an optimus system when unloading the nouveau
module.

Signed-off-by: Jeffery Miller <jmiller at neverware.com>
---
 drivers/gpu/drm/nouveau/nouveau_backlight.c | 70 ++++++++++++++-------
 1 file changed, 47 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c
b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index debbbf0fd4bd..fce2441cce0d 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -46,19 +46,40 @@ struct backlight_connector {
 	int id;
 };
 
-static bool
+static void
 nouveau_get_backlight_name(char backlight_name[BL_NAME_SIZE], struct
backlight_connector
 		*connector)
 {
-	const int nb = ida_simple_get(&bl_ida, 0, 0, GFP_KERNEL);
-	if (nb < 0 || nb >= 100)
-		return false;
-	if (nb > 0)
-		snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight%d", nb);
+	if (connector->id > 0)
+		snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight%d",
+				connector->id);
 	else
 		snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight");
-	connector->id = nb;
-	return true;
+}
+
+static struct backlight_connector *
+nouveau_alloc_bl_connector(void)
+{
+	const int nb = ida_simple_get(&bl_ida, 0, 100, GFP_KERNEL);
+	struct backlight_connector *bl_connector;
+
+	if (nb < 0)
+		return ERR_PTR(nb);
+	bl_connector = kzalloc(sizeof(*bl_connector), GFP_KERNEL);
+	if (!bl_connector) {
+		ida_simple_remove(&bl_ida, nb);
+		return ERR_PTR(-ENOMEM);
+	}
+	bl_connector->id = nb;
+	return bl_connector;
+}
+
+static void
+nouveau_free_bl_connector(struct backlight_connector *bl_connector)
+{
+	if (bl_connector->id >= 0)
+		ida_simple_remove(&bl_ida, bl_connector->id);
+	kfree(bl_connector);
 }
 
 static int
@@ -99,7 +120,7 @@ nv40_backlight_init(struct drm_connector *connector)
 	struct nvif_object *device = &drm->client.device.object;
 	struct backlight_properties props;
 	struct backlight_device *bd;
-	struct backlight_connector bl_connector;
+	struct backlight_connector *bl_connector;
 	char backlight_name[BL_NAME_SIZE];
 
 	if (!(nvif_rd32(device, NV40_PMC_BACKLIGHT) & NV40_PMC_BACKLIGHT_MASK))
@@ -108,19 +129,21 @@ nv40_backlight_init(struct drm_connector *connector)
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_RAW;
 	props.max_brightness = 31;
-	if (!nouveau_get_backlight_name(backlight_name, &bl_connector)) {
+	bl_connector = nouveau_alloc_bl_connector();
+	if (IS_ERR(bl_connector)) {
 		NV_ERROR(drm, "Failed to retrieve a unique name for the backlight
interface\n");
-		return 0;
+		return PTR_ERR(bl_connector);
 	}
+
+	nouveau_get_backlight_name(backlight_name, bl_connector);
 	bd = backlight_device_register(backlight_name , connector->kdev, drm,
 				       &nv40_bl_ops, &props);
 
 	if (IS_ERR(bd)) {
-		if (bl_connector.id > 0)
-			ida_simple_remove(&bl_ida, bl_connector.id);
+		nouveau_free_bl_connector(bl_connector);
 		return PTR_ERR(bd);
 	}
-	list_add(&bl_connector.head, &drm->bl_connectors);
+	list_add(&bl_connector->head, &drm->bl_connectors);
 	drm->backlight = bd;
 	bd->props.brightness = nv40_get_intensity(bd);
 	backlight_update_status(bd);
@@ -218,7 +241,7 @@ nv50_backlight_init(struct drm_connector *connector)
 	struct backlight_properties props;
 	struct backlight_device *bd;
 	const struct backlight_ops *ops;
-	struct backlight_connector bl_connector;
+	struct backlight_connector *bl_connector;
 	char backlight_name[BL_NAME_SIZE];
 
 	nv_encoder = find_encoder(connector, DCB_OUTPUT_LVDS);
@@ -241,20 +264,21 @@ nv50_backlight_init(struct drm_connector *connector)
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_RAW;
 	props.max_brightness = 100;
-	if (!nouveau_get_backlight_name(backlight_name, &bl_connector)) {
+	bl_connector = nouveau_alloc_bl_connector();
+	if (IS_ERR(bl_connector)) {
 		NV_ERROR(drm, "Failed to retrieve a unique name for the backlight
interface\n");
-		return 0;
+		return PTR_ERR(bl_connector);
 	}
+	nouveau_get_backlight_name(backlight_name, bl_connector);
 	bd = backlight_device_register(backlight_name , connector->kdev,
 				       nv_encoder, ops, &props);
 
 	if (IS_ERR(bd)) {
-		if (bl_connector.id > 0)
-			ida_simple_remove(&bl_ida, bl_connector.id);
+		nouveau_free_bl_connector(bl_connector);
 		return PTR_ERR(bd);
 	}
 
-	list_add(&bl_connector.head, &drm->bl_connectors);
+	list_add(&bl_connector->head, &drm->bl_connectors);
 	drm->backlight = bd;
 	bd->props.brightness = bd->ops->get_brightness(bd);
 	backlight_update_status(bd);
@@ -302,10 +326,10 @@ nouveau_backlight_exit(struct drm_device *dev)
 {
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct backlight_connector *connector;
+	struct backlight_connector *safe;
 
-	list_for_each_entry(connector, &drm->bl_connectors, head) {
-		if (connector->id >= 0)
-			ida_simple_remove(&bl_ida, connector->id);
+	list_for_each_entry_safe(connector, safe, &drm->bl_connectors, head) {
+		nouveau_free_bl_connector(connector);
 	}
 
 	if (drm->backlight) {
-- 
2.17.1

does this also happen with the newest kernel? I was kind of under the
impression we already fixed such issues.

On Thu, Jul 5, 2018 at 9:09 PM, Jeffery Miller <jmiller at neverware.com>
wrote:
> If have a couple patches I found while looking at a panic
> I was seeing while unloading the nouveau module.
>
> Unloading the nouveau module on my optimus notebook machine causes
> the system to panic. This started occuring when moving from 4.4
> to 4.14.
>
> These patches make it such that the system does not panic
> when unloading the module.
>
> 4.14 also requires commit 34112bf4935d ("drm/nouveau/fbcon: fix NULL
> pointer access in nouveau_fbcon_destroy") which was already included
in
> the 4.18 tree.
>
> These patches make it so I can unload the module without a panic but
> there is a warning when unloading the module:
>  sysfs group 'power' not found for kobject 'nv_backlight'
>  WARNING: CPU: 2 PID: 1434 at fs/sysfs/group.c:235
sysfs_remove_group+0x76/0x80
>  RIP: 0010:sysfs_remove_group+0x76/0x80
>  Call Trace:
>   device_del+0x56/0x350
>   ? down_write+0xe/0x40
>   device_unregister+0x16/0x60
>   nouveau_backlight_exit+0x4a/0x60 [nouveau]
>   nouveau_display_destroy+0x29/0x80 [nouveau]
>   nouveau_drm_unload+0x61/0xd0 [nouveau]
>   drm_dev_unregister+0x3f/0xe0 [drm]
>   drm_put_dev+0x27/0x50 [drm]
>   nouveau_drm_device_remove+0x47/0x70 [nouveau]
>   pci_device_remove+0x3b/0xb0
>   device_release_driver_internal+0x180/0x250
>   driver_detach+0x32/0x5f
>   bus_remove_driver+0x74/0xc6
>   pci_unregister_driver+0x22/0xa0
>   nouveau_drm_exit+0x15/0x16b [nouveau]
>
> I don't beleive them to be related. Perhaps there is another issue
here?
>
> Jeffery Miller (2):
>   drm/nouveau/fbcon: Fix NULL pointer access in nouveau_fbcon_destroy.
>   drm/nouveau/bl: Allocate backlight connector nodes.
>
>  drivers/gpu/drm/nouveau/nouveau_backlight.c | 70 ++++++++++++++-------
>  drivers/gpu/drm/nouveau/nouveau_fbcon.c     |  5 +-
>  2 files changed, 51 insertions(+), 24 deletions(-)
>
> --
> 2.17.1
>
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

nevermind, I just hit it today. Will test your patches

On Tue, Jul 17, 2018 at 12:46 PM, Karol Herbst <kherbst at redhat.com>
wrote:
> does this also happen with the newest kernel? I was kind of under the
> impression we already fixed such issues.
>
> On Thu, Jul 5, 2018 at 9:09 PM, Jeffery Miller <jmiller at
neverware.com> wrote:
>> If have a couple patches I found while looking at a panic
>> I was seeing while unloading the nouveau module.
>>
>> Unloading the nouveau module on my optimus notebook machine causes
>> the system to panic. This started occuring when moving from 4.4
>> to 4.14.
>>
>> These patches make it such that the system does not panic
>> when unloading the module.
>>
>> 4.14 also requires commit 34112bf4935d ("drm/nouveau/fbcon: fix
NULL
>> pointer access in nouveau_fbcon_destroy") which was already
included in
>> the 4.18 tree.
>>
>> These patches make it so I can unload the module without a panic but
>> there is a warning when unloading the module:
>>  sysfs group 'power' not found for kobject
'nv_backlight'
>>  WARNING: CPU: 2 PID: 1434 at fs/sysfs/group.c:235
sysfs_remove_group+0x76/0x80
>>  RIP: 0010:sysfs_remove_group+0x76/0x80
>>  Call Trace:
>>   device_del+0x56/0x350
>>   ? down_write+0xe/0x40
>>   device_unregister+0x16/0x60
>>   nouveau_backlight_exit+0x4a/0x60 [nouveau]
>>   nouveau_display_destroy+0x29/0x80 [nouveau]
>>   nouveau_drm_unload+0x61/0xd0 [nouveau]
>>   drm_dev_unregister+0x3f/0xe0 [drm]
>>   drm_put_dev+0x27/0x50 [drm]
>>   nouveau_drm_device_remove+0x47/0x70 [nouveau]
>>   pci_device_remove+0x3b/0xb0
>>   device_release_driver_internal+0x180/0x250
>>   driver_detach+0x32/0x5f
>>   bus_remove_driver+0x74/0xc6
>>   pci_unregister_driver+0x22/0xa0
>>   nouveau_drm_exit+0x15/0x16b [nouveau]
>>
>> I don't beleive them to be related. Perhaps there is another issue
here?
>>
>> Jeffery Miller (2):
>>   drm/nouveau/fbcon: Fix NULL pointer access in nouveau_fbcon_destroy.
>>   drm/nouveau/bl: Allocate backlight connector nodes.
>>
>>  drivers/gpu/drm/nouveau/nouveau_backlight.c | 70 ++++++++++++++-------
>>  drivers/gpu/drm/nouveau/nouveau_fbcon.c     |  5 +-
>>  2 files changed, 51 insertions(+), 24 deletions(-)
>>
>> --
>> 2.17.1
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau