[PATCH] unserialized task->files changing (v2)

[PATCH] unserialized task->files changing (v2)

[Kirill Korotaev]

[-- Attachment #1: Type: text/plain, Size: 475 bytes --]

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.

Signed-Off-By: Pavel Emelianov <xemul@openvz.org>
Signed-Off-By: Kirill Korotaev <dev@openvz.org>


[-- Attachment #2: diff-ms-files-race-fix-200600808 --]
[-- Type: text/plain, Size: 2028 bytes --]

--- ./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;

Re: [PATCH] unserialized task->files changing (v2)

[Eric Dumazet]

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

Re: [PATCH] unserialized task->files changing (v2)

[Pavel V. Emelianov]

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

Re: [PATCH] unserialized task->files changing (v2)

[Kirill Korotaev]

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

Re: [PATCH] unserialized task->files changing (v2)

[Eric Dumazet]

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