* [PATCH] drm: inhibit drm drivers register to uninitialized drm core @ 2017-07-08 21:43 Alexandru Moise 2017-07-10 6:52 ` Daniel Vetter 2017-07-11 10:04 ` Daniel Vetter 0 siblings, 2 replies; 9+ messages in thread From: Alexandru Moise @ 2017-07-08 21:43 UTC (permalink / raw) To: linux-kernel; +Cc: dri-devel, airlied, seanpaul, jani.nikula, daniel.vetter 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 <00moses.alexander00@gmail.com> --- drivers/gpu/drm/drm_drv.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 37b8ad3e30d8..2ed2d919beae 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -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; -- 2.13.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: inhibit drm drivers register to uninitialized drm core 2017-07-08 21:43 [PATCH] drm: inhibit drm drivers register to uninitialized drm core Alexandru Moise @ 2017-07-10 6:52 ` Daniel Vetter 2017-07-10 7:14 ` Alexandru Moise 2017-07-11 10:04 ` Daniel Vetter 1 sibling, 1 reply; 9+ messages in thread From: Daniel Vetter @ 2017-07-10 6:52 UTC (permalink / raw) To: Alexandru Moise; +Cc: linux-kernel, daniel.vetter, dri-devel On Sat, Jul 08, 2017 at 11:43:52PM +0200, Alexandru Moise wrote: > 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 <00moses.alexander00@gmail.com> > --- > drivers/gpu/drm/drm_drv.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 37b8ad3e30d8..2ed2d919beae 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -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; 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: inhibit drm drivers register to uninitialized drm core 2017-07-10 6:52 ` Daniel Vetter @ 2017-07-10 7:14 ` Alexandru Moise 2017-07-10 18:00 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Alexandru Moise @ 2017-07-10 7:14 UTC (permalink / raw) To: Daniel Vetter Cc: linux-kernel, daniel.vetter, dri-devel, airlied, seanpaul, jani.nikula On Mon, Jul 10, 2017 at 08:52:46AM +0200, Daniel Vetter wrote: > On Sat, Jul 08, 2017 at 11:43:52PM +0200, Alexandru Moise wrote: > > 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 <00moses.alexander00@gmail.com> > > --- > > drivers/gpu/drm/drm_drv.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > index 37b8ad3e30d8..2ed2d919beae 100644 > > --- a/drivers/gpu/drm/drm_drv.c > > +++ b/drivers/gpu/drm/drm_drv.c > > @@ -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; > > 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? ../Alex > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: inhibit drm drivers register to uninitialized drm core 2017-07-10 7:14 ` Alexandru Moise @ 2017-07-10 18:00 ` Daniel Vetter 2017-07-10 19:56 ` Alexandru Moise 2017-07-11 9:41 ` Greg Kroah-Hartman 0 siblings, 2 replies; 9+ messages in thread From: Daniel Vetter @ 2017-07-10 18:00 UTC (permalink / raw) To: Alexandru Moise, Rusty Russell, Jessica Yu, Greg Kroah-Hartman Cc: Linux Kernel Mailing List, Daniel Vetter, dri-devel, Dave Airlie, Sean Paul, Nikula, Jani On Mon, Jul 10, 2017 at 9:14 AM, Alexandru Moise <00moses.alexander00@gmail.com> wrote: > On Mon, Jul 10, 2017 at 08:52:46AM +0200, Daniel Vetter wrote: >> On Sat, Jul 08, 2017 at 11:43:52PM +0200, Alexandru Moise wrote: >> > 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 <00moses.alexander00@gmail.com> >> > --- >> > drivers/gpu/drm/drm_drv.c | 16 ++++++++++++++++ >> > 1 file changed, 16 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >> > index 37b8ad3e30d8..2ed2d919beae 100644 >> > --- a/drivers/gpu/drm/drm_drv.c >> > +++ b/drivers/gpu/drm/drm_drv.c >> > @@ -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; >> >> 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? 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: inhibit drm drivers register to uninitialized drm core 2017-07-10 18:00 ` Daniel Vetter @ 2017-07-10 19:56 ` Alexandru Moise 2017-07-11 7:53 ` Daniel Vetter 2017-07-11 9:41 ` Greg Kroah-Hartman 1 sibling, 1 reply; 9+ messages in thread From: Alexandru Moise @ 2017-07-10 19:56 UTC (permalink / raw) To: Daniel Vetter, rusty, jeyu, gregkh Cc: linux-kernel, daniel.vetter, dri-devel, airlied, seanpaul, jani.nikula On Mon, Jul 10, 2017 at 08:00:37PM +0200, Daniel Vetter wrote: > On Mon, Jul 10, 2017 at 9:14 AM, Alexandru Moise > <00moses.alexander00@gmail.com> wrote: > > On Mon, Jul 10, 2017 at 08:52:46AM +0200, Daniel Vetter wrote: > >> On Sat, Jul 08, 2017 at 11:43:52PM +0200, Alexandru Moise wrote: > >> > 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 <00moses.alexander00@gmail.com> > >> > --- > >> > drivers/gpu/drm/drm_drv.c | 16 ++++++++++++++++ > >> > 1 file changed, 16 insertions(+) > >> > > >> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > >> > index 37b8ad3e30d8..2ed2d919beae 100644 > >> > --- a/drivers/gpu/drm/drm_drv.c > >> > +++ b/drivers/gpu/drm/drm_drv.c > >> > @@ -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; > >> > >> 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? > > 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. ../Alex > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: inhibit drm drivers register to uninitialized drm core 2017-07-10 19:56 ` Alexandru Moise @ 2017-07-11 7:53 ` Daniel Vetter 0 siblings, 0 replies; 9+ messages in thread From: Daniel Vetter @ 2017-07-11 7:53 UTC (permalink / raw) To: Alexandru Moise Cc: Daniel Vetter, rusty, jeyu, gregkh, linux-kernel, daniel.vetter, dri-devel, airlied, seanpaul, jani.nikula On Mon, Jul 10, 2017 at 09:56:20PM +0200, Alexandru Moise wrote: > On Mon, Jul 10, 2017 at 08:00:37PM +0200, Daniel Vetter wrote: > > On Mon, Jul 10, 2017 at 9:14 AM, Alexandru Moise > > <00moses.alexander00@gmail.com> wrote: > > > On Mon, Jul 10, 2017 at 08:52:46AM +0200, Daniel Vetter wrote: > > >> On Sat, Jul 08, 2017 at 11:43:52PM +0200, Alexandru Moise wrote: > > >> > 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 <00moses.alexander00@gmail.com> > > >> > --- > > >> > drivers/gpu/drm/drm_drv.c | 16 ++++++++++++++++ > > >> > 1 file changed, 16 insertions(+) > > >> > > > >> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > >> > index 37b8ad3e30d8..2ed2d919beae 100644 > > >> > --- a/drivers/gpu/drm/drm_drv.c > > >> > +++ b/drivers/gpu/drm/drm_drv.c > > >> > @@ -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; > > >> > > >> 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? > > > > 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. I'm not worried about drm or drm drivers here, I'm just wondered how all the other subystems handle this same problem. Pretty sure we're not the only ones who register a chardev block in the core subsystem module (and sysfs classes, and tons of other stuff), and then drivers would fall over if that's not there. Having to open code a $foo_init_successful everywhere seems somewhat silly. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: inhibit drm drivers register to uninitialized drm core 2017-07-10 18:00 ` Daniel Vetter 2017-07-10 19:56 ` Alexandru Moise @ 2017-07-11 9:41 ` Greg Kroah-Hartman 2017-07-11 10:02 ` Daniel Vetter 1 sibling, 1 reply; 9+ messages in thread From: Greg Kroah-Hartman @ 2017-07-11 9:41 UTC (permalink / raw) To: Daniel Vetter Cc: Alexandru Moise, Rusty Russell, Jessica Yu, Linux Kernel Mailing List, Daniel Vetter, dri-devel, Dave Airlie, Sean Paul, Nikula, Jani On Mon, Jul 10, 2017 at 08:00:37PM +0200, Daniel Vetter wrote: > On Mon, Jul 10, 2017 at 9:14 AM, Alexandru Moise > <00moses.alexander00@gmail.com> wrote: > > On Mon, Jul 10, 2017 at 08:52:46AM +0200, Daniel Vetter wrote: > >> On Sat, Jul 08, 2017 at 11:43:52PM +0200, Alexandru Moise wrote: > >> > 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(). 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. > 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... thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: inhibit drm drivers register to uninitialized drm core 2017-07-11 9:41 ` Greg Kroah-Hartman @ 2017-07-11 10:02 ` Daniel Vetter 0 siblings, 0 replies; 9+ messages in thread From: Daniel Vetter @ 2017-07-11 10:02 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Alexandru Moise, Rusty Russell, Jessica Yu, Linux Kernel Mailing List, Daniel Vetter, dri-devel, Dave Airlie, Sean Paul, Nikula, Jani On Tue, Jul 11, 2017 at 11:41 AM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Mon, Jul 10, 2017 at 08:00:37PM +0200, Daniel Vetter wrote: >> On Mon, Jul 10, 2017 at 9:14 AM, Alexandru Moise >> <00moses.alexander00@gmail.com> wrote: >> > On Mon, Jul 10, 2017 at 08:52:46AM +0200, Daniel Vetter wrote: >> >> On Sat, Jul 08, 2017 at 11:43:52PM +0200, Alexandru Moise wrote: >> >> > 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(). > > 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. > >> 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... Well there's so much almost-magic helper/support code that very often just asking the right people gives me the answer. Sounds like this is a case where we haven't yet invented that magic, and I don't have a bright idea either. I'll Apply Alex' patch. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: inhibit drm drivers register to uninitialized drm core 2017-07-08 21:43 [PATCH] drm: inhibit drm drivers register to uninitialized drm core Alexandru Moise 2017-07-10 6:52 ` Daniel Vetter @ 2017-07-11 10:04 ` Daniel Vetter 1 sibling, 0 replies; 9+ messages in thread From: Daniel Vetter @ 2017-07-11 10:04 UTC (permalink / raw) To: Alexandru Moise; +Cc: linux-kernel, daniel.vetter, dri-devel On Sat, Jul 08, 2017 at 11:43:52PM +0200, Alexandru Moise wrote: > 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 <00moses.alexander00@gmail.com> Applied to drm-misc-next for 4.14. Since Greg has a pach in 4.13 already for the chardev issue it feels like this isn't justified for pushing through -rc/stable kernels since too much a corner-case of a bug (it exists since decades after all). Thanks, Daniel > --- > drivers/gpu/drm/drm_drv.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 37b8ad3e30d8..2ed2d919beae 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -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; > > -- > 2.13.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-07-11 10:04 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-08 21:43 [PATCH] drm: inhibit drm drivers register to uninitialized drm core Alexandru Moise 2017-07-10 6:52 ` Daniel Vetter 2017-07-10 7:14 ` Alexandru Moise 2017-07-10 18:00 ` Daniel Vetter 2017-07-10 19:56 ` Alexandru Moise 2017-07-11 7:53 ` Daniel Vetter 2017-07-11 9:41 ` Greg Kroah-Hartman 2017-07-11 10:02 ` Daniel Vetter 2017-07-11 10:04 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).