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 [email protected]
---
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
