Messages in this thread | | | Date | Tue, 08 Aug 2006 17:18:09 +0400 | From | "Pavel V. Emelianov" <> | Subject | Re: [PATCH] unserialized task->files changing (v2) |
| |
Eric Dumazet wrote: > On Tuesday 08 August 2006 13:31, Kirill Korotaev wrote: >> Fixed race on put_files_struct on exec with proc. >> Restoring files on current on error path may lead >> to proc having a pointer to already kfree-d files_struct. >> >> ->files changing at exit.c and khtread.c are safe as >> exit_files() makes all things under lock. >> >> v2 patch changes: >> - introduced reset_files_struct() as Christoph Hellwig suggested >> >> Found during OpenVZ stress testing. > > Sorry but there is something I dont understand. You ignored my point. > > +void reset_files_struct(struct task_struct *tsk, struct files_struct > *files) > +{ > + struct files_struct *old; > + > + old = tsk->files; > + task_lock(tsk); > + tsk->files = files; > + task_unlock(tsk); > + put_files_struct(old); > +} > > Its seems very strange to protect tsk->files = files with a > task_lock()/task_unlock(). What is it supposed to guard against ??? > > If this patch corrects the 'bug', then a simpler fix would be to use a > memory > barrier between "tsk->files = files" and "put_files_struct(old);" > > No need to perform 2 atomics ops on the task lock. > > old = tsk->files; > tsk->files = files; > smp_mb(); > put_files_struct(old);
No. The race being discussed is:
proc code: resetting code: ============================================================================= task_lock(tsk); files = tsk->files; old = tsk->files; tsk->files = files; put_files_struct(old); /* dec to 0 */ `- kmem_cache_free(files); get_files_struct(file); /* already free */ task_unlock(tsk);
So having smp_mb() before put_files_struct() does not fix the problem.
> > That would be enough to guard against proc code (because this code > only needs > to read tsk->files of course) > > The same remark can be said for __exit_files() from kernel/exit.c > > If this task_lock()/task_unlock() patch is really needed, then a > comment in > the source would be very fair. > > Eric
- 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/
| |