fix ->shm_file leak
Subsystems (2)
cover fix ->shm_file leak
Andrew,
These 2 patches are completely orthogonal, and either patch can fix the problem reported by Andrey. However, I think they both make sense.
The 2nd patch was already acked by Eric/Andrey. However it is not as trivial as it looks.
The 1st one looks more straightforward, and perhaps it is 3.10 material.
Oleg.
PATCH 1/2 fput: task_work_add() can fail if the caller has passed exit_task_work()
fput() assumes that it can't be called after exit_task_work() but this is not true, for example free_ipc_ns()->shm_destroy() can do this. In this case fput() silently leaks the file.
Change it to fallback to delayed_fput_work if task_work_add() fails. The patch looks complicated but it is not, it changes the code from
if (PF_KTHREAD) {
schedule_work(...);
return;
}
task_work_add(...)to
if (!PF_KTHREAD) {
if (!task_work_add(...))
return;
/* fallback */
}
schedule_work(...);As for shm_destroy() in particular, we could make another fix but I think this change makes sense anyway. There could be another similar user, it is not safe to assume that task_work_add() can't fail.
--- fs/file_table.c | 19 ++++++++++--------- 1 files changed, 10 insertions(+), 9 deletions(-)
fs/file_table.c +10 -9
@@ -306,17 +306,18 @@ void fput(struct file *file) {if (atomic_long_dec_and_test (&file->f_count)) {struct task_struct *task = current; +unsigned long flags; +file_sb_list_del (file); -if (unlikely (in_interrupt () || task->flags &PF_KTHREAD )) { -unsigned long flags; -spin_lock_irqsave (&delayed_fput_lock, flags); -list_add (&file->f_u.fu_list, &delayed_fput_list); -schedule_work (&delayed_fput_work); -spin_unlock_irqrestore (&delayed_fput_lock, flags); -return ; +if (likely (!in_interrupt () && !(task->flags &PF_KTHREAD ))) { +init_task_work (&file->f_u.fu_rcuhead, ____fput); +if (!task_work_add (task, &file->f_u.fu_rcuhead, true)) +return ; } -init_task_work (&file->f_u.fu_rcuhead, ____fput); -task_work_add (task, &file->f_u.fu_rcuhead, true); +spin_lock_irqsave (&delayed_fput_lock, flags); +list_add (&file->f_u.fu_list, &delayed_fput_list); +schedule_work (&delayed_fput_work); +spin_unlock_irqrestore (&delayed_fput_lock, flags); } }
Signature · 1 line
1.5.5.1
Andrew Morton
maintainerRe: [PATCH 1/2] fput: task_work_add() can fail if the caller has passed exit_task_work()
Quoted 37 lines · On Fri, 14 Jun 2013 21:09:47 +0200 Oleg Nesterov <[email protected]> wrote:
fput()assumes that it can't be called afterexit_task_work()but this is not true, for example free_ipc_ns()->shm_destroy() can do this. In this casefput()silently leaks the file.Change it to fallback to
delayed_fput_workiftask_work_add()fails. The patch looks complicated but it is not, it changes the code fromif (PF_KTHREAD) { schedule_work(...); return; } task_work_add(...)to
if (!PF_KTHREAD) { if (!task_work_add(...)) return; /* fallback */ } schedule_work(...);As for
shm_destroy()in particular, we could make another fix but I think this change makes sense anyway. There could be another similar user, it is not safe to assume thattask_work_add()can't fail....
fs/file_table.c +2
-1@@ -306,17 +306,18 @@ void fput(struct file *file) {if (atomic_long_dec_and_test (&file->f_count)) {struct task_struct *task = current; +unsigned long flags; +file_sb_list_del (file); -if (unlikely (in_interrupt () || task->flags &PF_KTHREAD )) {
fs/file_table.c void fput(struct file *file) · line 315
Quoted 10 lines
fs/file_table.c +4
-6-unsigned long flags; -spin_lock_irqsave (&delayed_fput_lock, flags); -list_add (&file->f_u.fu_list, &delayed_fput_list); -schedule_work (&delayed_fput_work); -spin_unlock_irqrestore (&delayed_fput_lock, flags); -return ; +if (likely (!in_interrupt () && !(task->flags &PF_KTHREAD ))) { +init_task_work (&file->f_u.fu_rcuhead, ____fput); +if (!task_work_add (task, &file->f_u.fu_rcuhead, true)) +return ;
A comment here would be useful, explaining the circumstances under which we fall through to the delayed fput. This is particularly needed because kernel/task_work.c is such undocumented crap.
This?
fs/file_table.c renamed from fs/file_table.c~fput-task_work_add-can-fail-if-the-caller-has-passed-exit_task_work-fix +6 -0
@@ -313,6 +313,12 @@ void fput(struct file *file)init_task_work (&file->f_u.fu_rcuhead, ____fput);if (!task_work_add (task, &file->f_u.fu_rcuhead, true))return ; +/* +* After this task has run exit_task_work(), +* task_work_add() will fail. free_ipc_ns()-> +* shm_destroy() can do this. Fall through to delayed +* fput to avoid leaking *file. +*/ }spin_lock_irqsave (&delayed_fput_lock, flags);list_add (&file->f_u.fu_list, &delayed_fput_list);
fs/file_table.c void fput(struct file *file) · line 320
Quoted 7 lines
fs/file_table.c +4
-2} -init_task_work (&file->f_u.fu_rcuhead, ____fput); -task_work_add (task, &file->f_u.fu_rcuhead, true); +spin_lock_irqsave (&delayed_fput_lock, flags); +list_add (&file->f_u.fu_list, &delayed_fput_list); +schedule_work (&delayed_fput_work); +spin_unlock_irqrestore (&delayed_fput_lock, flags);
OT: I don't think that schedule_work() needs to be inside the locked region. Scalability improvements beckon!
Quoted 2 lines
fs/file_table.c +0
-0} }
cover (Was: fput: task_work_add() can fail if the caller has passed exit_task_work())
Quoted 10 lines · Andrew Morton
Review
Quoted 4 lines · On Fri, 14 Jun 2013 21:09:47 +0200 Oleg Nesterov <[email protected]> wrote:
(quoted diff) +4
-0+ if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) { + init_task_work(&file->f_u.fu_rcuhead, ____fput); + if (!task_work_add(task, &file->f_u.fu_rcuhead, true)) + return;A comment here would be useful, explaining the circumstances under which we fall through to the delayed fput.
Thanks!
Quoted 2 lines
This is particularly needed
becausekernel/task_work.cis such undocumented crap.
It seems that you are trying to force me to make the doc patch ;) OK, I'll try. task_work.c needs a couple of cosmetic cleanups anyway.
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ spin_lock_irqsave(&delayed_fput_lock, flags); + list_add(&file->f_u.fu_list, &delayed_fput_list); + schedule_work(&delayed_fput_work); + spin_unlock_irqrestore(&delayed_fput_lock, flags);OT: I don't think that
schedule_work()needs to be inside the locked region. Scalability improvements beckon!
Yeees, I thought about this too.
Performance-wise this can't really help, this case is unlikely. But I think this change makes this code a bit simpler, so please see 1/3.
2/3 fixes the (theoretical) bug in llist_add() and imho cleanups the code.
3/3 comes as a separate change because I do not want to argue if someone dislike the non-inline llist_add(). But once again, we can make llist_add_batch() inline, and I believe it is never good to duplicate the code even if it is simple.
Oleg.
PATCH 1/3 fput: turn "list_head delayed_fput_list" into llist_head
fput() and delayed_fput() can use llist and avoid the locking.
This is unlikely path, it is not that this change can improve the performance, but this way the code looks simpler.
--- fs/file_table.c | 25 ++++++++++--------------- include/linux/fs.h | 2 ++ 2 files changed, 12 insertions(+), 15 deletions(-)
fs/file_table.c +10 -15
@@ -265,18 +265,15 @@ static void __fput(struct file *file)mntput (mnt); } -static DEFINE_SPINLOCK (delayed_fput_lock); -static LIST_HEAD (delayed_fput_list); +static LLIST_HEAD (delayed_fput_list);static void delayed_fput (struct work_struct *unused) { -LIST_HEAD (head); -spin_lock_irq (&delayed_fput_lock); -list_splice_init (&delayed_fput_list, &head); -spin_unlock_irq (&delayed_fput_lock); -while (!list_empty (&head)) { - struct file *f = list_first_entry(&head, struct file, f_u.fu_list); -list_del_init (&f->f_u.fu_list); -__fput (f); +struct llist_node *node =llist_del_all (&delayed_fput_list); +struct llist_node *next; + +for (; node; node = next) { + next =llist_next (node); + __fput(llist_entry(node, struct file, f_u.fu_llist)); } }
@@ -306,7 +303,6 @@ void fput(struct file *file) {if (atomic_long_dec_and_test (&file->f_count)) {struct task_struct *task = current; -unsigned long flags;file_sb_list_del (file);if (likely (!in_interrupt () && !(task->flags &PF_KTHREAD ))) {
@@ -320,10 +316,9 @@ void fput(struct file *file)* fput to avoid leaking *file. */ } -spin_lock_irqsave (&delayed_fput_lock, flags); -list_add (&file->f_u.fu_list, &delayed_fput_list); -schedule_work (&delayed_fput_work); -spin_unlock_irqrestore (&delayed_fput_lock, flags); + +if (llist_add (&file->f_u.fu_llist, &delayed_fput_list)) +schedule_work (&delayed_fput_work); } }
include/linux/fs.h +2 -0
@@ -10,6 +10,7 @@#include <linux/stat.h> #include <linux/cache.h> #include <linux/list.h> +#include <linux/llist.h> #include <linux/radix-tree.h> #include <linux/rbtree.h> #include <linux/init.h>
@@ -767,6 +768,7 @@ struct file {*/ union {struct list_head fu_list; +struct llist_node fu_llist;struct rcu_head fu_rcuhead; } f_u;struct path f_path;
Signature · 1 line
1.5.5.1
PATCH 2/3 llist: fix/simplify llist_add() and llist_add_batch()
In reply to Oleg Nesterov1. This is mostly theoretical, but llist_add*() need ACCESS_ONCE().
Otherwise it is not guaranteed that the first cmpxchg() uses the same value for old_entry and new_last->next.
2. These helpers cache the result of cmpxchg() and read the initial value of head->first before the main loop. I do not think this makes sense. In the likely case cmpxchg() succeeds, otherwise it doesn't hurt to reload head->first.
I think it would be better to simplify the code and simply read ->first before cmpxchg().
--- include/linux/llist.h | 19 +++++++------------ lib/llist.c | 15 +++++---------- 2 files changed, 12 insertions(+), 22 deletions(-)
include/linux/llist.h +7 -12
@@ -151,18 +151,13 @@ static inline struct llist_node *llist_next(struct llist_node *node)*/ static inline bool llist_add (struct llist_node *new,struct llist_head *head) { -struct llist_node *entry, *old_entry; - - entry = head->first; -for (;;) { - old_entry = entry; - new->next = entry; - entry =cmpxchg (&head->first, old_entry, new); -if (entry == old_entry) -break ; - } - -return old_entry ==NULL ; +struct llist_node *first; + +do { + new->next = first =ACCESS_ONCE (head->first); + }while (cmpxchg (&head->first, first, new) != first); + +return !first; }/**
lib/llist.c +5 -10
@@ -39,18 +39,13 @@bool llist_add_batch (struct llist_node *new_first,struct llist_node *new_last,struct llist_head *head) { -struct llist_node *entry, *old_entry; +struct llist_node *first; - entry = head->first; -for (;;) { - old_entry = entry; - new_last->next = entry; - entry =cmpxchg (&head->first, old_entry, new_first); -if (entry == old_entry) -break ; - } +do { + new_last->next = first =ACCESS_ONCE (head->first); + }while (cmpxchg (&head->first, first, new_first) != first); -return old_entry ==NULL ; +return !first; }EXPORT_SYMBOL_GPL (llist_add_batch);
Signature · 1 line
1.5.5.1
PATCH 3/3 llist: llist_add() can use llist_add_batch()
In reply to Oleg Nesterovllist_add(new, head) can simply use llist_add_batch(new, new, head), no need to duplicate the code.
This obviously uninlines llist_add() and to me this is a win. But we can make llist_add_batch() inline if this is desirable, in this case gcc can notice that new_first == new_last if the caller is llist_add().
--- include/linux/llist.h | 14 ++++---------- 1 files changed, 4 insertions(+), 10 deletions(-)
include/linux/llist.h +4 -10
@@ -142,6 +142,9 @@ static inline struct llist_node *llist_next(struct llist_node *node)return node->next; } +extern bool llist_add_batch (struct llist_node *new_first, +struct llist_node *new_last, +struct llist_head *head);/** * llist_add - add a new entry * @new: new entry to be added
@@ -151,13 +154,7 @@ static inline struct llist_node *llist_next(struct llist_node *node)*/ static inline bool llist_add (struct llist_node *new,struct llist_head *head) { -struct llist_node *first; - -do { - new->next = first =ACCESS_ONCE (head->first); - }while (cmpxchg (&head->first, first, new) != first); - -return !first; +return llist_add_batch (new, new, head); }/**
@@ -173,9 +170,6 @@ static inline struct llist_node *llist_del_all(struct llist_head *head) return xchg(&head->first, NULL); } -extern bool llist_add_batch (struct llist_node *new_first, -struct llist_node *new_last, -struct llist_head *head);extern struct llist_node *llist_del_first (struct llist_head *head);#endif /* LLIST_H */
Signature · 1 line
1.5.5.1
Re: [PATCH 0/3] (Was: fput: task_work_add() can fail if the caller has passed exit_task_work())
In reply to Oleg Nesterovsorry, forgot to mention...
Quoted 8 lines · Oleg Nesterov
Quoted 3 lines
OT: I don't think that
schedule_work()needs to be inside the locked region. Scalability improvements beckon!Yeees, I thought about this too.
Performance-wise this can't really help, this case is unlikely. But I think this change makes this code a bit simpler, so please see 1/3.
This is on top of fput-task_work_add-can-fail-if-the-caller-has-passed-exit_task_work-fix.patch
it textually depends on the comment block in fput() added by that patch.
Oleg.
PATCH 2/2 move exit_task_namespaces() outside of exit_notify()
In reply to Oleg Nesterovexit_notify() does exit_task_namespaces() after forget_original_parent(). This was needed to ensure that ->nsproxy can't be cleared prematurely, an exiting child we are going to reparent can do do_notify_parent() and use the parent's (ours) pid_ns.
However, after 32084504 "pidns: use task_active_pid_ns in do_notify_parent" ->nsproxy != NULL is no longer needed, we rely on task_active_pid_ns().
Move exit_task_namespaces() from exit_notify() to do_exit(), after exit_fs() and before exit_task_work().
This solves the problem reported by Andrey, free_ipc_ns()->shm_destroy() does fput() which needs task_work_add().
Note: this particular problem can be fixed if we change fput(), and that change makes sense anyway. But there is another reason to move the callsite. The original reason for exit_task_namespaces() from the middle of exit_notify() was subtle and it has already gone away, now this looks confusing. And this allows us do simplify exit_notify(), we can avoid unlock/lock(tasklist) and we can use ->exit_state instead of PF_EXITING in forget_original_parent().
--- kernel/exit.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/exit.c +1 -1
@@ -644,7 +644,6 @@ static void exit_notify(struct task_struct *tsk, int group_dead)* jobs, send them a SIGHUP and then a SIGCONT. (POSIX 3.2.2.2) */ forget_original_parent (tsk); -exit_task_namespaces (tsk);write_lock_irq (&tasklist_lock);if (group_dead)
@@ -790,6 +789,7 @@ void do_exit(long code)exit_shm (tsk);exit_files (tsk);exit_fs (tsk); +exit_task_namespaces (tsk);exit_task_work (tsk);check_stack_usage ();exit_thread ();
Signature · 1 line
1.5.5.1