drm: inhibit drm drivers register to uninitialized drm core

If the DRM core fails to init for whatever reason, ensure that no driver ever calls drm_dev_register().

This is best done at drm_dev_init() as it covers drivers that call drm_dev_alloc() as well as drivers that prefer to embed struct drm_device into their own device struct and call drm_dev_init() themselves.

In my case I had so many dynamic device majors used that the major number for DRM (226) was stolen, causing DRM core init to fail after failing to register a chrdev, and ultimately calling debugfs_remove() on drm_debugfs_root in drm_core_exit().

After drm core failed to init, VGEM was still calling drm_dev_register(), ultimately leading to drm_debugfs_init(), with drm_debugfs_root passed as the root for the new debugfs dir at debugfs_create_dir().

This led to a kernel panic once we were either derefencing root->d_inode while it was NULL or calling root->d_inode->i_op->lookup() while it was NULL in debugfs at inode_lock() or lookup_*().

Signed-off-by Alexandru Moise <[email protected]>

---
 drivers/gpu/drm/drm_drv.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/gpu/drm/drm_drv.c +14 -0

index 37b8ad3e30d8..2ed2d919beae 100644
@@ -63,6 +63,15 @@ module_param_named(debug, drm_debug, int, 0600);
 static DEFINE_SPINLOCK(drm_minor_lock);
 static struct idr drm_minors_idr;
 
+/*
+ * If the drm core fails to init for whatever reason,
+ * we should prevent any drivers from registering with it.
+ * It's best to check this at drm_dev_init(), as some drivers
+ * prefer to embed struct drm_device into their own device
+ * structure and call drm_dev_init() themselves.
+ */
+static bool drm_core_init_complete = false;
+
 static struct dentry *drm_debugfs_root;
 
 #define DRM_PRINTK_FMT "[" DRM_NAME ":%s]%s %pV"

@@ -484,6 +493,11 @@ int drm_dev_init(struct drm_device *dev,
 {
 	int ret;
 
+	if (!drm_core_init_complete) {
+		DRM_ERROR("DRM core is not initialized\n");
+		return -ENODEV;
+	}
+
 	kref_init(&dev->ref);
 	dev->dev = parent;
 	dev->driver = driver;

Quoted 62 lines · Alexandru Moise

If the DRM core fails to init for whatever reason, ensure that no driver ever calls drm_dev_register().

This is best done at drm_dev_init() as it covers drivers that call drm_dev_alloc() as well as drivers that prefer to embed struct drm_device into their own device struct and call drm_dev_init() themselves.

In my case I had so many dynamic device majors used that the major number for DRM (226) was stolen, causing DRM core init to fail after failing to register a chrdev, and ultimately calling debugfs_remove() on drm_debugfs_root in drm_core_exit().

After drm core failed to init, VGEM was still calling drm_dev_register(), ultimately leading to drm_debugfs_init(), with drm_debugfs_root passed as the root for the new debugfs dir at debugfs_create_dir().

This led to a kernel panic once we were either derefencing root->d_inode while it was NULL or calling root->d_inode->i_op->lookup() while it was NULL in debugfs at inode_lock() or lookup_*().

Signed-off-by Alexandru Moise <[email protected]>

---
 drivers/gpu/drm/drm_drv.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/gpu/drm/drm_drv.c +14 -0

index 37b8ad3e30d8..2ed2d919beae 100644
@@ -63,6 +63,15 @@ module_param_named(debug, drm_debug, int, 0600);
 static DEFINE_SPINLOCK(drm_minor_lock);
 static struct idr drm_minors_idr;
 
+/*
+ * If the drm core fails to init for whatever reason,
+ * we should prevent any drivers from registering with it.
+ * It's best to check this at drm_dev_init(), as some drivers
+ * prefer to embed struct drm_device into their own device
+ * structure and call drm_dev_init() themselves.
+ */
+static bool drm_core_init_complete = false;
+
 static struct dentry *drm_debugfs_root;
 
 #define DRM_PRINTK_FMT "[" DRM_NAME ":%s]%s %pV"

@@ -484,6 +493,11 @@ int drm_dev_init(struct drm_device *dev,
 {
 	int ret;
 
+	if (!drm_core_init_complete) {
+		DRM_ERROR("DRM core is not initialized\n");
+		return -ENODEV;
+	}
+
 	kref_init(&dev->ref);
 	dev->dev = parent;
 	dev->driver = driver;

@@ -966,6 +980,8 @@ static int __init drm_core_init(void)
 	if (ret < 0)
 		goto error;
 

drivers/gpu/drm/drm_drv.c static int __init drm_core_init(void) · line 986

Quoted 4 lines

drivers/gpu/drm/drm_drv.c +2 -0

+	drm_core_init_complete = true;
+
 	DRM_DEBUG("Initialized\n");
 	return 0;

Isn't the correct fix to pass down the error value, which iirc should make the kmod stuff unload the module again? Or does this not work' -Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 63 lines · Daniel Vetter

Quoted 62 lines · Alexandru Moise

If the DRM core fails to init for whatever reason, ensure that no driver ever calls drm_dev_register().

This is best done at drm_dev_init() as it covers drivers that call drm_dev_alloc() as well as drivers that prefer to embed struct drm_device into their own device struct and call drm_dev_init() themselves.

In my case I had so many dynamic device majors used that the major number for DRM (226) was stolen, causing DRM core init to fail after failing to register a chrdev, and ultimately calling debugfs_remove() on drm_debugfs_root in drm_core_exit().

After drm core failed to init, VGEM was still calling drm_dev_register(), ultimately leading to drm_debugfs_init(), with drm_debugfs_root passed as the root for the new debugfs dir at debugfs_create_dir().

This led to a kernel panic once we were either derefencing root->d_inode while it was NULL or calling root->d_inode->i_op->lookup() while it was NULL in debugfs at inode_lock() or lookup_*().

Signed-off-by Alexandru Moise <[email protected]>

---
 drivers/gpu/drm/drm_drv.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/gpu/drm/drm_drv.c +14 -0

index 37b8ad3e30d8..2ed2d919beae 100644
@@ -63,6 +63,15 @@ module_param_named(debug, drm_debug, int, 0600);
 static DEFINE_SPINLOCK(drm_minor_lock);
 static struct idr drm_minors_idr;
 
+/*
+ * If the drm core fails to init for whatever reason,
+ * we should prevent any drivers from registering with it.
+ * It's best to check this at drm_dev_init(), as some drivers
+ * prefer to embed struct drm_device into their own device
+ * structure and call drm_dev_init() themselves.
+ */
+static bool drm_core_init_complete = false;
+
 static struct dentry *drm_debugfs_root;
 
 #define DRM_PRINTK_FMT "[" DRM_NAME ":%s]%s %pV"

@@ -484,6 +493,11 @@ int drm_dev_init(struct drm_device *dev,
 {
    int ret;
 
+   if (!drm_core_init_complete) {
+           DRM_ERROR("DRM core is not initialized\n");
+           return -ENODEV;
+   }
+
    kref_init(&dev->ref);
    dev->dev = parent;
    dev->driver = driver;

@@ -966,6 +980,8 @@ static int __init drm_core_init(void)
    if (ret < 0)
            goto error;
 

Quoted 8 lines

drivers/gpu/drm/drm_drv.c static int __init drm_core_init(void) · line 986

Quoted 4 lines

drivers/gpu/drm/drm_drv.c +2 -0

+   drm_core_init_complete = true;
+
    DRM_DEBUG("Initialized\n");
    return 0;

Isn't the correct fix to pass down the error value, which iirc should make the kmod stuff unload the module again? Or does this not work' -Daniel

What if everything is built in?

Quoted 73 lines · Alexandru Moise

Quoted 63 lines · Daniel Vetter

Quoted 62 lines · Alexandru Moise

If the DRM core fails to init for whatever reason, ensure that no driver ever calls drm_dev_register().

This is best done at drm_dev_init() as it covers drivers that call drm_dev_alloc() as well as drivers that prefer to embed struct drm_device into their own device struct and call drm_dev_init() themselves.

In my case I had so many dynamic device majors used that the major number for DRM (226) was stolen, causing DRM core init to fail after failing to register a chrdev, and ultimately calling debugfs_remove() on drm_debugfs_root in drm_core_exit().

After drm core failed to init, VGEM was still calling drm_dev_register(), ultimately leading to drm_debugfs_init(), with drm_debugfs_root passed as the root for the new debugfs dir at debugfs_create_dir().

This led to a kernel panic once we were either derefencing root->d_inode while it was NULL or calling root->d_inode->i_op->lookup() while it was NULL in debugfs at inode_lock() or lookup_*().

Signed-off-by Alexandru Moise <[email protected]>

---
 drivers/gpu/drm/drm_drv.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/gpu/drm/drm_drv.c +14 -0

index 37b8ad3e30d8..2ed2d919beae 100644
@@ -63,6 +63,15 @@ module_param_named(debug, drm_debug, int, 0600);
 static DEFINE_SPINLOCK(drm_minor_lock);
 static struct idr drm_minors_idr;
 
+/*
+ * If the drm core fails to init for whatever reason,
+ * we should prevent any drivers from registering with it.
+ * It's best to check this at drm_dev_init(), as some drivers
+ * prefer to embed struct drm_device into their own device
+ * structure and call drm_dev_init() themselves.
+ */
+static bool drm_core_init_complete = false;
+
 static struct dentry *drm_debugfs_root;
 
 #define DRM_PRINTK_FMT "[" DRM_NAME ":%s]%s %pV"

@@ -484,6 +493,11 @@ int drm_dev_init(struct drm_device *dev,
 {
    int ret;
 
+   if (!drm_core_init_complete) {
+           DRM_ERROR("DRM core is not initialized\n");
+           return -ENODEV;
+   }
+
    kref_init(&dev->ref);
    dev->dev = parent;
    dev->driver = driver;

@@ -966,6 +980,8 @@ static int __init drm_core_init(void)
    if (ret < 0)
            goto error;
 

Quoted 8 lines

drivers/gpu/drm/drm_drv.c static int __init drm_core_init(void) · line 986

Quoted 4 lines

drivers/gpu/drm/drm_drv.c +2 -0

+   drm_core_init_complete = true;
+
    DRM_DEBUG("Initialized\n");
    return 0;

Isn't the correct fix to pass down the error value, which iirc should make the kmod stuff unload the module again? Or does this not work' -Daniel

Quoted 1 line

What if everything is built in?

I feared that would be the answer :-/ Still feels funny that everyone will need to hand-roll this, or does everyone simply assume that their subsystem's module_init never fails?

Adding a pile of kmod and driver folks in the hopes of getting a better answer. If there's no better answer pls remind me to merge your patch in 1-2 weeks, I'll likely forget ...
-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Quoted 77 lines · Daniel Vetter

Quoted 73 lines · Alexandru Moise

Quoted 63 lines · Daniel Vetter

Quoted 62 lines · Alexandru Moise

If the DRM core fails to init for whatever reason, ensure that no driver ever calls drm_dev_register().

This is best done at drm_dev_init() as it covers drivers that call drm_dev_alloc() as well as drivers that prefer to embed struct drm_device into their own device struct and call drm_dev_init() themselves.

In my case I had so many dynamic device majors used that the major number for DRM (226) was stolen, causing DRM core init to fail after failing to register a chrdev, and ultimately calling debugfs_remove() on drm_debugfs_root in drm_core_exit().

After drm core failed to init, VGEM was still calling drm_dev_register(), ultimately leading to drm_debugfs_init(), with drm_debugfs_root passed as the root for the new debugfs dir at debugfs_create_dir().

This led to a kernel panic once we were either derefencing root->d_inode while it was NULL or calling root->d_inode->i_op->lookup() while it was NULL in debugfs at inode_lock() or lookup_*().

Signed-off-by Alexandru Moise <[email protected]>

---
 drivers/gpu/drm/drm_drv.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/gpu/drm/drm_drv.c +14 -0

index 37b8ad3e30d8..2ed2d919beae 100644
@@ -63,6 +63,15 @@ module_param_named(debug, drm_debug, int, 0600);
 static DEFINE_SPINLOCK(drm_minor_lock);
 static struct idr drm_minors_idr;
 
+/*
+ * If the drm core fails to init for whatever reason,
+ * we should prevent any drivers from registering with it.
+ * It's best to check this at drm_dev_init(), as some drivers
+ * prefer to embed struct drm_device into their own device
+ * structure and call drm_dev_init() themselves.
+ */
+static bool drm_core_init_complete = false;
+
 static struct dentry *drm_debugfs_root;
 
 #define DRM_PRINTK_FMT "[" DRM_NAME ":%s]%s %pV"

@@ -484,6 +493,11 @@ int drm_dev_init(struct drm_device *dev,
 {
    int ret;
 
+   if (!drm_core_init_complete) {
+           DRM_ERROR("DRM core is not initialized\n");
+           return -ENODEV;
+   }
+
    kref_init(&dev->ref);
    dev->dev = parent;
    dev->driver = driver;

@@ -966,6 +980,8 @@ static int __init drm_core_init(void)
    if (ret < 0)
            goto error;
 

Quoted 8 lines

drivers/gpu/drm/drm_drv.c static int __init drm_core_init(void) · line 986

Quoted 4 lines

drivers/gpu/drm/drm_drv.c +2 -0

+   drm_core_init_complete = true;
+
    DRM_DEBUG("Initialized\n");
    return 0;

Isn't the correct fix to pass down the error value, which iirc should make the kmod stuff unload the module again? Or does this not work' -Daniel

Quoted 1 line

What if everything is built in?

Quoted 8 lines

I feared that would be the answer :-/ Still feels funny that everyone will need to hand-roll this, or does everyone simply assume that their subsystem's module_init never fails?

Adding a pile of kmod and driver folks in the hopes of getting a better answer. If there's no better answer pls remind me to merge your patch in 1-2 weeks, I'll likely forget ...
-Daniel

I took a look at all the DRM drivers and they all seem pretty sane, they all handle the case of the DRM core failure, the fault is clearly on the DRM core side, it makes absolutely no sense trying to register with a subsystem that failed to init.

This is certainly not a subsystem I am intimately acquainted with however, I just stumbled on this crash while working with debugfs.

Most of the DRM core code looks pretty solid, and this is a relatively small corner-case, I believe most people out there won't be eating up that many major numbers. But I believe it's still the Right Thing™ to do.

It might save some headaches in the future as DRM becomes more complex.

Quoted 10 lines · Alexandru Moise

Quoted 9 lines · Daniel Vetter

Quoted 8 lines · Alexandru Moise

If the DRM core fails to init for whatever reason, ensure that no driver ever calls drm_dev_register().

This is best done at drm_dev_init() as it covers drivers that call drm_dev_alloc() as well as drivers that prefer to embed struct drm_device into their own device struct and call drm_dev_init() themselves.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

In my case I had so many dynamic device majors used that the major number for DRM (226) was stolen, causing DRM core init to fail after failing to register a chrdev, and ultimately calling debugfs_remove() on drm_debugfs_root in drm_core_exit().

I feared that would be the answer :-/ Still feels funny that everyone will need to hand-roll this, or does everyone simply assume that their subsystem's module_init never fails?

Quoted 12 lines · Daniel Vetter

Quoted 10 lines · Alexandru Moise

Quoted 9 lines · Daniel Vetter

Quoted 8 lines · Alexandru Moise

If the DRM core fails to init for whatever reason, ensure that no driver ever calls drm_dev_register().

This is best done at drm_dev_init() as it covers drivers that call drm_dev_alloc() as well as drivers that prefer to embed struct drm_device into their own device struct and call drm_dev_init() themselves.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

In my case I had so many dynamic device majors used that the major number for DRM (226) was stolen, causing DRM core init to fail after failing to register a chrdev, and ultimately calling debugfs_remove() on drm_debugfs_root in drm_core_exit().

Note, there are patches in my "to-apply" queue to prevent that from happening, that should show up in 4.14-rc1. So that shouldn't be an issue in the future.

Quoted 3 lines

I feared that would be the answer :-/ Still feels funny that everyone will need to hand-roll this, or does everyone simply assume that their subsystem's module_init never fails?

How would we not "hand-roll" this? Every subsystem works a bit differently. But if you can think of a way to make this generic, that would be great...

If the DRM core fails to init for whatever reason, ensure that no driver ever calls drm_dev_register().

This is best done at drm_dev_init() as it covers drivers that call drm_dev_alloc() as well as drivers that prefer to embed struct drm_device into their own device struct and call drm_dev_init() themselves.

In my case I had so many dynamic device majors used that the major number for DRM (226) was stolen, causing DRM core init to fail after failing to register a chrdev, and ultimately calling debugfs_remove() on drm_debugfs_root in drm_core_exit().

After drm core failed to init, VGEM was still calling drm_dev_register(), ultimately leading to drm_debugfs_init(), with drm_debugfs_root passed as the root for the new debugfs dir at debugfs_create_dir().

This led to a kernel panic once we were either derefencing root->d_inode while it was NULL or calling root->d_inode->i_op->lookup() while it was NULL in debugfs at inode_lock() or lookup_*().

Signed-off-by Alexandru Moise <[email protected]>

---
 drivers/gpu/drm/drm_drv.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/gpu/drm/drm_drv.c +16 -0

index 37b8ad3e30d8..2ed2d919beae 100644
@@ -63,6 +63,15 @@ module_param_named(debug, drm_debug, int, 0600);
 static DEFINE_SPINLOCK(drm_minor_lock);
 static struct idr drm_minors_idr;
 
+/*
+ * If the drm core fails to init for whatever reason,
+ * we should prevent any drivers from registering with it.
+ * It's best to check this at drm_dev_init(), as some drivers
+ * prefer to embed struct drm_device into their own device
+ * structure and call drm_dev_init() themselves.
+ */
+static bool drm_core_init_complete = false;
+
 static struct dentry *drm_debugfs_root;
 
 #define DRM_PRINTK_FMT "[" DRM_NAME ":%s]%s %pV"

@@ -484,6 +493,11 @@ int drm_dev_init(struct drm_device *dev,
 {
 	int ret;
 
+	if (!drm_core_init_complete) {
+		DRM_ERROR("DRM core is not initialized\n");
+		return -ENODEV;
+	}
+
 	kref_init(&dev->ref);
 	dev->dev = parent;
 	dev->driver = driver;

@@ -966,6 +980,8 @@ static int __init drm_core_init(void)
 	if (ret < 0)
 		goto error;
 
+	drm_core_init_complete = true;
+
 	DRM_DEBUG("Initialized\n");
 	return 0;
 

Signature · 6 lines

2.13.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel