--- ./fs/binfmt_elf.c.rfs 2006-08-08 15:01:44.000000000 +0400
+++ ./fs/binfmt_elf.c 2006-08-08 15:07:49.000000000 +0400
@@ -1037,10 +1037,8 @@ out_free_interp:
out_free_file:
sys_close(elf_exec_fileno);
out_free_fh:
- if (files) {
- put_files_struct(current->files);
- current->files = files;
- }
+ if (files)
+ reset_files_struct(current, files);
out_free_ph:
kfree(elf_phdata);
goto out;
--- ./fs/binfmt_misc.c.rfs 2006-08-08 15:01:44.000000000 +0400
+++ ./fs/binfmt_misc.c 2006-08-08 15:09:46.000000000 +0400
@@ -215,10 +215,8 @@ _error:
bprm->interp_flags = 0;
bprm->interp_data = 0;
_unshare:
- if (files) {
- put_files_struct(current->files);
- current->files = files;
- }
+ if (files)
+ reset_files_struct(current, files);
goto _ret;
}
--- ./fs/exec.c.rfs 2006-08-08 15:01:44.000000000 +0400
+++ ./fs/exec.c 2006-08-08 15:10:09.000000000 +0400
@@ -903,8 +903,7 @@ int flush_old_exec(struct linux_binprm *
return 0;
mmap_failed:
- put_files_struct(current->files);
- current->files = files;
+ reset_files_struct(current, files);
out:
return retval;
}
--- ./include/linux/file.h.rfs 2006-04-21 11:59:36.000000000 +0400
+++ ./include/linux/file.h 2006-08-08 15:08:19.000000000 +0400
@@ -112,5 +112,6 @@ struct task_struct;
struct files_struct *get_files_struct(struct task_struct *);
void FASTCALL(put_files_struct(struct files_struct *fs));
+void reset_files_struct(struct task_struct *, struct files_struct *);
#endif /* __LINUX_FILE_H */
--- ./kernel/exit.c.rfs 2006-08-08 15:01:44.000000000 +0400
+++ ./kernel/exit.c 2006-08-08 15:13:40.000000000 +0400
@@ -487,6 +487,17 @@ void fastcall put_files_struct(struct fi
EXPORT_SYMBOL(put_files_struct);
+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);
+}
+
static inline void __exit_files(struct task_struct *tsk)
{
struct files_struct * files = tsk->files;
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
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
Eric,
> Sorry but there is something I dont understand. You ignored my point.
Sorry, I missed it thinking that you are talking about another thing...
Pavel described the race in more details and why barrier doesn't help.
Hope, it became more clear now.
> +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.
Kirill
On Tuesday 08 August 2006 17:53, Kirill Korotaev wrote:
> Eric,
>
> > Sorry but there is something I dont understand. You ignored my point.
>
> Sorry, I missed it thinking that you are talking about another thing...
> Pavel described the race in more details and why barrier doesn't help.
> Hope, it became more clear now.
Yes it became very clear :)
Sorry for the confusion.
Thank you
Eric