Messages in this thread | | | From | Eric Dumazet <> | Subject | Re: [PATCH] unserialized task->files changing (v2) | Date | Tue, 8 Aug 2006 14:51:57 +0200 |
| |
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);
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/
| |