[PATCH] ibmphp: Fix module ref count underflow

[PATCH] ibmphp: Fix module ref count underflow

[Neil Horman]

Hey-
	I happened to notice that the ibmphp hotplug driver does something
rather silly in its init routine.  It purposely calls module_put so as to
underflow its module ref count to avoid being removed from the kernel.  This is
bad practice, and wrong, since it provides a window for subsequent module_gets
to reset the refcount to zero, allowing an unload to race in and cause all sorts
of mysterious panics.  If the module is unsafe to load, it should inform the
kernel as such with a call to __unsafe.  The patch below does that.

Regards
Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

 ibmphp_core.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)


diff --git a/drivers/pci/hotplug/ibmphp_core.c b/drivers/pci/hotplug/ibmphp_core.c
index c892daa..3706d4e 100644
--- a/drivers/pci/hotplug/ibmphp_core.c
+++ b/drivers/pci/hotplug/ibmphp_core.c
@@ -1402,9 +1402,11 @@ static int __init ibmphp_init(void)
 		goto error;
 	}
 
-	/* lock ourselves into memory with a module 
-	 * count of -1 so that no one can unload us. */
-	module_put(THIS_MODULE);
+	/*
+	 * Its unsafe to unload this module, so tell
+	 * the kernel to avoid inadvertent unloads
+	 */
+	__unsafe(THIS_MODULE);
 
 exit:
 	return rc;
-- 
/****************************************************
 * Neil Horman <nhorman@tuxdriver.com>
 * Software Engineer, Red Hat
 ****************************************************/

Re: [PATCH] ibmphp: Fix module ref count underflow

[Greg KH]

On Wed, Dec 10, 2008 at 02:43:34PM -0500, Neil Horman wrote:
> Hey-
> 	I happened to notice that the ibmphp hotplug driver does something
> rather silly in its init routine.  It purposely calls module_put so as to
> underflow its module ref count to avoid being removed from the kernel.  This is
> bad practice, and wrong, since it provides a window for subsequent module_gets
> to reset the refcount to zero, allowing an unload to race in and cause all sorts
> of mysterious panics.  If the module is unsafe to load, it should inform the
> kernel as such with a call to __unsafe.  The patch below does that.
> 
> Regards
> Neil
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

Jesse is the PCI maintainer now, you should send this to him to verify
that it goes in.

Feel free to add my:
	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

to this, nice job.

thanks,

greg k-h

Re: [PATCH] ibmphp: Fix module ref count underflow

[Neil Horman]

On Wed, Dec 10, 2008 at 02:28:12PM -0800, Greg KH wrote:
> On Wed, Dec 10, 2008 at 02:43:34PM -0500, Neil Horman wrote:
> > Hey-
> > 	I happened to notice that the ibmphp hotplug driver does something
> > rather silly in its init routine.  It purposely calls module_put so as to
> > underflow its module ref count to avoid being removed from the kernel.  This is
> > bad practice, and wrong, since it provides a window for subsequent module_gets
> > to reset the refcount to zero, allowing an unload to race in and cause all sorts
> > of mysterious panics.  If the module is unsafe to load, it should inform the
> > kernel as such with a call to __unsafe.  The patch below does that.
> > 
> > Regards
> > Neil
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> Jesse is the PCI maintainer now, you should send this to him to verify
> that it goes in.
> 
> Feel free to add my:
> 	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> to this, nice job.
> 
Thanks!  I've forwarded the patch on to Jesse.

Best
Neil

> thanks,
> 
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] ibmphp: Fix module ref count underflow

[Jesse Barnes]

On Wednesday, December 10, 2008 11:43 am Neil Horman wrote:
> Hey-
> 	I happened to notice that the ibmphp hotplug driver does something
> rather silly in its init routine.  It purposely calls module_put so as to
> underflow its module ref count to avoid being removed from the kernel. 
> This is bad practice, and wrong, since it provides a window for subsequent
> module_gets to reset the refcount to zero, allowing an unload to race in
> and cause all sorts of mysterious panics.  If the module is unsafe to load,
> it should inform the kernel as such with a call to __unsafe.  The patch
> below does that.

Thanks Neil, applied this to my for-linus branch since it sounds potentially 
serious (but also low risk since who uses ibmphp anymore? :)

Jesse

Re: [PATCH] ibmphp: Fix module ref count underflow

[Neil Horman]

On Tue, Dec 16, 2008 at 10:42:08AM -0800, Jesse Barnes wrote:
> On Wednesday, December 10, 2008 11:43 am Neil Horman wrote:
> > Hey-
> > 	I happened to notice that the ibmphp hotplug driver does something
> > rather silly in its init routine.  It purposely calls module_put so as to
> > underflow its module ref count to avoid being removed from the kernel. 
> > This is bad practice, and wrong, since it provides a window for subsequent
> > module_gets to reset the refcount to zero, allowing an unload to race in
> > and cause all sorts of mysterious panics.  If the module is unsafe to load,
> > it should inform the kernel as such with a call to __unsafe.  The patch
> > below does that.
> 
> Thanks Neil, applied this to my for-linus branch since it sounds potentially 
> serious (but also low risk since who uses ibmphp anymore? :)
> 
Dang it!  Sorry, Jesse.  Yes, youre absolutely right, it is low risk.  It really
just a bit of sillyness all around.

Unfortunately, I took part in the sillyness.  The problem was reported to me on
RHEL, and I tested there, without checking upstream too closely.  As  aresult,
the patch I gave you is a bit out of date, and won't compile.  I've tested the
new patch here much more closely.  apologies.  I informed akpm who was looking
at it, but neglected to copy you.

This patch corrects the same problem in that it prevents module unloads in a
sane fashion, by not registering an exit routine

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>



 ibmphp_core.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/pci/hotplug/ibmphp_core.c b/drivers/pci/hotplug/ibmphp_core.c
index c892daa..633e743 100644
--- a/drivers/pci/hotplug/ibmphp_core.c
+++ b/drivers/pci/hotplug/ibmphp_core.c
@@ -1402,10 +1402,6 @@ static int __init ibmphp_init(void)
 		goto error;
 	}
 
-	/* lock ourselves into memory with a module 
-	 * count of -1 so that no one can unload us. */
-	module_put(THIS_MODULE);
-
 exit:
 	return rc;
 
@@ -1423,4 +1419,3 @@ static void __exit ibmphp_exit(void)
 }
 
 module_init(ibmphp_init);
-module_exit(ibmphp_exit);
> Jesse
> 

-- 
/****************************************************
 * Neil Horman <nhorman@tuxdriver.com>
 * Software Engineer, Red Hat
 ****************************************************/

Re: [PATCH] ibmphp: Fix module ref count underflow

[Jesse Barnes]

On Tuesday, December 16, 2008 11:56 am Neil Horman wrote:
> On Tue, Dec 16, 2008 at 10:42:08AM -0800, Jesse Barnes wrote:
> > On Wednesday, December 10, 2008 11:43 am Neil Horman wrote:
> > > Hey-
> > > 	I happened to notice that the ibmphp hotplug driver does something
> > > rather silly in its init routine.  It purposely calls module_put so as
> > > to underflow its module ref count to avoid being removed from the
> > > kernel. This is bad practice, and wrong, since it provides a window for
> > > subsequent module_gets to reset the refcount to zero, allowing an
> > > unload to race in and cause all sorts of mysterious panics.  If the
> > > module is unsafe to load, it should inform the kernel as such with a
> > > call to __unsafe.  The patch below does that.
> >
> > Thanks Neil, applied this to my for-linus branch since it sounds
> > potentially serious (but also low risk since who uses ibmphp anymore? :)
>
> Dang it!  Sorry, Jesse.  Yes, youre absolutely right, it is low risk.  It
> really just a bit of sillyness all around.
>
> Unfortunately, I took part in the sillyness.  The problem was reported to
> me on RHEL, and I tested there, without checking upstream too closely.  As 
> aresult, the patch I gave you is a bit out of date, and won't compile. 
> I've tested the new patch here much more closely.  apologies.  I informed
> akpm who was looking at it, but neglected to copy you.
>
> This patch corrects the same problem in that it prevents module unloads in
> a sane fashion, by not registering an exit routine
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

Ah was just doing my testing & building now so I would have caught it in a 
minute. :)  I'll replace the patch I have with this one, thanks.

Jesse

Re: [PATCH] ibmphp: Fix module ref count underflow

[Neil Horman]

On Tue, Dec 16, 2008 at 12:52:50PM -0800, Jesse Barnes wrote:
> On Tuesday, December 16, 2008 11:56 am Neil Horman wrote:
> > On Tue, Dec 16, 2008 at 10:42:08AM -0800, Jesse Barnes wrote:
> > > On Wednesday, December 10, 2008 11:43 am Neil Horman wrote:
> > > > Hey-
> > > > 	I happened to notice that the ibmphp hotplug driver does something
> > > > rather silly in its init routine.  It purposely calls module_put so as
> > > > to underflow its module ref count to avoid being removed from the
> > > > kernel. This is bad practice, and wrong, since it provides a window for
> > > > subsequent module_gets to reset the refcount to zero, allowing an
> > > > unload to race in and cause all sorts of mysterious panics.  If the
> > > > module is unsafe to load, it should inform the kernel as such with a
> > > > call to __unsafe.  The patch below does that.
> > >
> > > Thanks Neil, applied this to my for-linus branch since it sounds
> > > potentially serious (but also low risk since who uses ibmphp anymore? :)
> >
> > Dang it!  Sorry, Jesse.  Yes, youre absolutely right, it is low risk.  It
> > really just a bit of sillyness all around.
> >
> > Unfortunately, I took part in the sillyness.  The problem was reported to
> > me on RHEL, and I tested there, without checking upstream too closely.  As 
> > aresult, the patch I gave you is a bit out of date, and won't compile. 
> > I've tested the new patch here much more closely.  apologies.  I informed
> > akpm who was looking at it, but neglected to copy you.
> >
> > This patch corrects the same problem in that it prevents module unloads in
> > a sane fashion, by not registering an exit routine
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> Ah was just doing my testing & building now so I would have caught it in a 
> minute. :)  I'll replace the patch I have with this one, thanks.
> 
> Jesse
> 
Thank you, that matches what Andrew has in his tree.  Apologies for the noise
Neil