Messages in this thread Patch in this message | | | Subject | Re: Oops on ibmasm | From | Max Asbock <> | Date | Thu, 09 Mar 2006 09:37:20 -0800 |
| |
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/
| |