Re: Oops on ibmasm

[Max Asbock <masbock@us.ibm.com>]

On Thu, 2006-03-09 at 05:26, Dave Jones wrote:
> On Thu, Mar 09, 2006 at 01:40:23AM -0800, Andrew Morton wrote:
> 
>  > I assume this'll fix it?
>  > 
>  > I suspect there's no point in the locking around that kobject_put() anyway.
>  > Or if there is, it wasn't the right way to fix the race.
>  > 
>  > diff -puN drivers/misc/ibmasm/ibmasm.h~ibmasm-use-after-free-fix drivers/misc/ibmasm/ibmasm.h
>  > --- devel/drivers/misc/ibmasm/ibmasm.h~ibmasm-use-after-free-fix	2006-03-09 01:35:05.000000000 -0800
>  > +++ devel-akpm/drivers/misc/ibmasm/ibmasm.h	2006-03-09 01:35:16.000000000 -0800
>  > @@ -100,11 +100,7 @@ struct command {
>  >  
>  >  static inline void command_put(struct command *cmd)
>  >  {
>  > -	unsigned long flags;
>  > -
>  > -	spin_lock_irqsave(cmd->lock, flags);
>  >          kobject_put(&cmd->kobj);
>  > -	spin_unlock_irqrestore(cmd->lock, flags);
>  >  }
> 
> I don't think this is right.  This is just a kobject-convoluted
> use-after-free afaics.
> 
I put the locks around the kobject_put after reading
Documentation/kref.txt and after realizing that there was a race. In the
kref.txt example there is a lock around kref_put (only a mutex instead
of a spinlock). 
I still think there is a point in putting locks around kobject_put(). Or
is there a better way?
And btw, I am using kobject because this code predates kref. So maybe
one day I should just convert it to kref.
Anyway, I think the locks are necessary, the way they are implemented is
probably ugly and caused me to make the mistake in the first place. But
a while I ago posted the following patch that fixes the situation.
cmd->lock points to a persistent lock that is not freed with cmd. 

max

Original patch:

ibmasm driver:
Fix the command_put() function which uses a pointer for a spinlock that
can be freed before dereferencing it.

Signed-off-by: Max Asbock masbock@us.ibm.com

---

diff -burpN linux-2.6.16-rc1/drivers/misc/ibmasm/ibmasm.h linux-2.6.16-rc1.ibmasm/drivers/misc/ibmasm/ibmasm.h
--- linux-2.6.16-rc1/drivers/misc/ibmasm/ibmasm.h	2006-02-01 11:50:01.000000000 -0800
+++ linux-2.6.16-rc1.ibmasm/drivers/misc/ibmasm/ibmasm.h	2006-02-03 13:57:42.000000000 -0800
@@ -101,10 +101,11 @@ struct command {
 static inline void command_put(struct command *cmd)
 {
 	unsigned long flags;
+	spinlock_t *lock = cmd->lock;
 
-	spin_lock_irqsave(cmd->lock, flags);
+	spin_lock_irqsave(lock, flags);
         kobject_put(&cmd->kobj);
-	spin_unlock_irqrestore(cmd->lock, flags);
+	spin_unlock_irqrestore(lock, flags);
 }
 
 static inline void command_get(struct command *cmd)