Skip to contentSkip to conversation

unserialized task->files changing (v2)

PATCH In review Kirill Korotaev 3 participants 5 messages Aug 8, 2006

Subsystems (2)
VFS exec and binfmt
lore
Share & tools
lore

Kirill Korotaev

author
#0 lore raw

PATCH 1/1 unserialized task->files changing (v2)

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 <[email protected]>
Signed-off-by Kirill Korotaev <[email protected]>
fs/binfmt_elf.c renamed from fs/binfmt_elf.c.rfs +2 -4
@@ -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 renamed from fs/binfmt_misc.c.rfs +2 -4
@@ -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 renamed from fs/exec.c.rfs +1 -2
@@ -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 renamed from include/linux/file.h.rfs +1 -0
@@ -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 renamed from kernel/exit.c.rfs +11 -0
@@ -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;
#1 lore raw
Quoted 11 lines · Kirill Korotaev

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

#2 lore raw
Quoted 33 lines · Eric Dumazet
Quoted 11 lines · Kirill Korotaev

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)

kernel/exit.c renamed from kernel/exit.c.rfs +9 -0
+{
+       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);"

Quoted 6 lines

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.

Quoted 10 lines

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

author
#3 lore raw
In reply to Eric Dumazet

Eric,

Quoted 1 line

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.

Quoted 22 lines
kernel/exit.c renamed from kernel/exit.c.rfs +10 -0
+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);

Quoted 5 lines

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

#4 lore raw
Quoted 7 lines · Kirill Korotaev

Eric,

Quoted 1 line

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