lkml.org 
[lkml]   [2006]   [Mar]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: Oops on ibmasm
From
Date
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)
-
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/

\
 
 \ /
  Last update: 2006-03-09 18:39    [W:0.090 / U:0.284 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site