staging: android/lowmemorykiller: Don't grab tasklist_lock

Review static int lowmem_shrink(struct shrinker *s, struct shrink_control *sc) · line 136

Quoted 7 lines · Anton Vorontsov

(quoted diff) +1 -1

@@ -132,7 +133,7 @@ static int lowmem_shrink(struct shrinker *s, struct shrink_control *sc)
 	}
 	selected_oom_adj = min_adj;
 
-	read_lock(&tasklist_lock);
+	rcu_read_lock();

This has the same problem, force_sig() becomes unsafe.

Why do you need force_? Do you really want to kill /sbin/init (or sub-namespace init) ?

We could change force_sig_info() to use lock_task_sighand(), but I'd like to avoid this. Imho, this interface should be cleanuped, and it should be used for synchronous signals only.

With or without this patch, sig == NULL is not possible but !mm is not right, there could be other other threads with mm != NULL.

From now on using force_sig() is not safe, as it can race with an

drivers/staging/android/lowmemorykiller.c +1 -1

@@ -132,7 +133,7 @@ static int lowmem_shrink(struct shrinker *s, struct shrink_control *sc)
 	}
 	selected_oom_adj = min_adj;
 
-	read_lock(&tasklist_lock);
+	rcu_read_lock();
 	for_each_process(p) {
 		struct mm_struct *mm;
 		struct signal_struct *sig;

@@ -180,12 +181,12 @@ static int lowmem_shrink(struct shrinker *s, struct shrink_control *sc)
 		lowmem_deathpending = selected;

Quoted 2 lines · Oleg Nesterov

This has the same problem, force_sig() becomes unsafe.

Ouch, I think I finally got it. So, lock_task_sighand() is trying to gracefully grab the lock, checking if the sighand is not NULL (which means, per __exit_signal(), that the task is halfway into the grave).

Would the following fix work for the sysrq?

- - - -
From: Anton Vorontsov <[email protected]>
Subject: [PATCH] sysrq: Fix unsafe operations on tasks

sysrq should grab the tasklist lock, otherwise calling force_sig() is not safe, as it might race with exiting task, which ->sighand might be set to NULL already.

drivers/tty/sysrq.c +0 -0

 > With or without this patch, sig == NULL is not possible but !mm is not right,

Quoted 1 line

there could be other other threads with mm != NULL.

I'm not sure I completely follow. In the current LMK code, we check for !mm because otherwise the potential victim is useless for us (i.e. killing it will not free much memory anyway).

This is needed so that callers would not get 'context imbalance' warnings from the sparse tool.

As a side effect, this patch fixes the following sparse warnings:

CHECK mm/oom_kill.c
mm/oom_kill.c:201:28: warning: context imbalance in 'oom_badness' - unexpected unlock
include/linux/rcupdate.h:249:30: warning: context imbalance in 'dump_tasks' - unexpected unlock
mm/oom_kill.c:453:9: warning: context imbalance in 'oom_kill_task' - unexpected unlock
CHECK mm/memcontrol.c
...
mm/memcontrol.c:1130:17: warning: context imbalance in 'task_in_mem_cgroup' - unexpected unlock

Signed-off-by Anton Vorontsov <[email protected]>

---
 include/linux/oom.h |   11 ++++++++++-
 mm/oom_kill.c       |    2 +-
 2 files changed, 11 insertions(+), 2 deletions(-)

include/linux/oom.h +1 -0

index 552fba9..e64bfd2 100644
@@ -21,6 +21,7 @@
 
 #ifdef __KERNEL__
 
+#include <linux/compiler.h>
 #include <linux/sched.h>
 #include <linux/types.h>
 #include <linux/nodemask.h>

@@ -65,7 +66,15 @@ static inline void oom_killer_enable(void)
 	oom_killer_disabled = false;
 }
 

Quoted 6 lines

include/linux/oom.h +5 -1

-extern struct task_struct *find_lock_task_mm(struct task_struct *p);
+extern struct task_struct *__find_lock_task_mm(struct task_struct *p);
+
+#define find_lock_task_mm(p)						\
+({									\
+	struct task_struct *__ret;					\

include/linux/oom.h static inline void oom_killer_enable(void) · line 77

Quoted 4 lines

include/linux/oom.h +4 -0

+									\
+	__cond_lock(&(p)->alloc_lock, __ret = __find_lock_task_mm(p));	\
+	__ret;								\
+})

Please use the proper "do...while" style thing here for multi-line, complex #defines like the rest of the kernel does so that you don't end up debugging horrible problems later.

On Mon, Feb 06, 2012 at 08:35:42AM -0800, Greg KH wrote:
[...]

Quoted 6 lines

Quoted 6 lines

include/linux/oom.h +5 -1

-extern struct task_struct *find_lock_task_mm(struct task_struct *p);
+extern struct task_struct *__find_lock_task_mm(struct task_struct *p);
+
+#define find_lock_task_mm(p)						\
+({									\
+	struct task_struct *__ret;					\

Quoted 8 lines

include/linux/oom.h static inline void oom_killer_enable(void) · line 77

Quoted 4 lines

include/linux/oom.h +4 -0

+									\
+	__cond_lock(&(p)->alloc_lock, __ret = __find_lock_task_mm(p));	\
+	__ret;								\
+})

Please use the proper "do...while" style thing here for multi-line, complex #defines like the rest of the kernel does so that you don't end up debugging horrible problems later.

Unfortunately this isn't possible in this case. Unlike '({})' GCC extension, do-while statement does not evaluate to a value, i.e. 'x = do { 123; } while (0);' is illegal.

Quoted 17 lines · Greg KH

[...]

Quoted 14 lines

include/linux/oom.h static inline void oom_killer_enable(void) · line 77

Quoted 10 lines

include/linux/oom.h +9 -1

-extern struct task_struct *find_lock_task_mm(struct task_struct *p);
+extern struct task_struct *__find_lock_task_mm(struct task_struct *p);
+
+#define find_lock_task_mm(p)                                             \
+({                                                                       \
+ struct task_struct *__ret;                                      \
+                                                                 \
+ __cond_lock(&(p)->alloc_lock, __ret = __find_lock_task_mm(p));  \
+ __ret;                                                          \
+})

Please use the proper "do...while" style thing here for multi-line, complex #defines like the rest of the kernel does so that you don't end up debugging horrible problems later.

Quoted 3 lines

Unfortunately this isn't possible in this case. Unlike '({})' GCC extension, do-while statement does not evaluate to a value, i.e. 'x = do { 123; } while (0);' is illegal.

Ah, you are right, my bad, sorry about that.

greg k-h

Quoted 7 lines

Quoted 3 lines

Unfortunately this isn't possible in this case. Unlike '({})' GCC extension, do-while statement does not evaluate to a value, i.e. 'x = do { 123; } while (0);' is illegal.

Ah, you are right, my bad, sorry about that.

greg k-h

Some __cond_lock() caller are inline functions. Is this bad?

Quoted 1 line

Some __cond_lock() caller are inline functions. Is this bad?

No, that's great, actually. :-) Not obvious, but seems like sparse understands __cond_lock in inline functions, so I'd better use it.

Thanks,

Quoted 2 lines · KOSAKI Motohiro

Some __cond_lock() caller are inline functions. Is this bad?

No, that's great, actually. :-) Not obvious, but seems like sparse understands __cond_lock in inline functions, so I'd better use it.

Quoted 5 lines · Anton Vorontsov

Quoted 2 lines · KOSAKI Motohiro

Some __cond_lock() caller are inline functions. Is this bad?

Quoted 3 lines

No, that's great, actually. :-) Not obvious, but seems like sparse understands __cond_lock in inline functions, so I'd better use it.

Hmm, great...

may be you can update lock_task_sighand() too ? (in a separate patch of course).

Sparse flood makes it hard to catch newly-introduced warnings. So let's fix the the sparse warnings in the oom killer:

 CHECK   mm/oom_kill.c
 mm/oom_kill.c:139:20: warning: context imbalance in '__find_lock_task_mm' - wrong count at exit mm/oom_kill.c:771:9: warning: context imbalance in 'out_of_memory' - different lock contexts for basic block

The first one is fixed by assuring sparse that we know that we exit with the lock held.

The second one is caused by the fact that sparse isn't smart enough to handle noreturn attribute.

Signed-off-by Anton Vorontsov <[email protected]>

---
 mm/oom_kill.c |    9 ++++++++-

 1 files changed, 8 insertions(+), 1 deletions(-)

mm/oom_kill.c +0 -0

index 0ebb383..49569b6 100644

mm/oom_kill.c +2 -1

                        return t;
+               }
                task_unlock(t);
        } while_each_thread(p, t);
 

@@ -766,6 +772,7 @@ retry:
                dump_header(NULL, gfp_mask, order, NULL, mpol_mask);
                read_unlock(&tasklist_lock);
                panic("Out of memory and no killable processes...\n");
+               return;
        }
 
        if (oom_kill_process(p, gfp_mask, order, points, totalpages, NULL,
--

1.7.7.6

2012/2/6 Anton Vorontsov <[email protected]>:

Quoted 26 lines

Sparse flood makes it hard to catch newly-introduced warnings. So let's fix the the sparse warnings in the oom killer:

 CHECK   mm/oom_kill.c
 mm/oom_kill.c:139:20: warning: context imbalance in '__find_lock_task_mm' - wrong count at exit mm/oom_kill.c:771:9: warning: context imbalance in 'out_of_memory' - different lock contexts for basic block

The first one is fixed by assuring sparse that we know that we exit with the lock held.

The second one is caused by the fact that sparse isn't smart enough to handle noreturn attribute.

Signed-off-by Anton Vorontsov <[email protected]>

---
 mm/oom_kill.c |    9 ++++++++-

 1 files changed, 8 insertions(+), 1 deletions(-)

mm/oom_kill.c +0 -0

index 0ebb383..49569b6 100644
@@ -142,8 +142,14 @@ struct task_struct *__find_lock_task_mm(struct task_struct *p)
 

mm/oom_kill.c struct task_struct *__find_lock_task_mm(struct task_struct *p) · line 150

Quoted 9 lines

mm/oom_kill.c +6 -1

        do {
                task_lock(t);
-               if (likely(t->mm))
+               if (likely(t->mm)) {
+                       /*
+                        * Shut up sparse: we do know that we exit w/ the
+                        * task locked.
+                        */
+                       __release(&t->alloc_loc);

task struct only have allock_lock, not alloc_loc.

Moreover we don't release
the lock in this code path. Seems odd.

Quoted 1 line

task struct only have allock_lock, not alloc_loc.

Funnily, but sparse does not care. :-) __release(foo) will work as well. Seems like sparse counts locking balance globally.

This is now fixed in the patch down below, thanks for catching.

Quoted 2 lines

Moreover we don't release
the lock in this code path. Seems odd.

Indeed. That's exactly what sparse seeing is as well. We exit without releasing the lock, which is bad (in sparse' eyes). So we lie to sparse, telling it that we do release, so it shut ups.

Quoted 5 lines

Quoted 1 line

task struct only have allock_lock, not alloc_loc.

Funnily, but sparse does not care. :-) __release(foo) will work as well. Seems like sparse counts locking balance globally.

Quoted 8 lines

This is now fixed in the patch down below, thanks for catching.

Quoted 2 lines

Moreover we don't release
the lock in this code path. Seems odd.

Indeed. That's exactly what sparse seeing is as well. We exit without releasing the lock, which is bad (in sparse' eyes). So we lie to sparse, telling it that we do release, so it shut ups.

Hmmm....

To be honest, I really dislike any lie annotation. Why? It is very fragile and easily
become broken from unrelated but near line changes. Please consider to enhance sparse at first.

Quoted 14 lines · KOSAKI Motohiro

Quoted 5 lines

Quoted 1 line

task struct only have allock_lock, not alloc_loc.

Funnily, but sparse does not care. :-) __release(foo) will work as well. Seems like sparse counts locking balance globally.

Quoted 8 lines

This is now fixed in the patch down below, thanks for catching.

Quoted 2 lines

Moreover we don't release
the lock in this code path. Seems odd.

Indeed. That's exactly what sparse seeing is as well. We exit without releasing the lock, which is bad (in sparse' eyes). So we lie to sparse, telling it that we do release, so it shut ups.

Quoted 6 lines

Hmmm....

To be honest, I really dislike any lie annotation. Why? It is very fragile and easily
become broken from unrelated but near line changes. Please consider to enhance sparse at first.

I somewhat doubt that it is possible to "enhance it". We keep the lock held conditionaly, so we need too place the hint in the code itself. I believe the best we can do would be something like this:

include/linux/compiler.h +2 -0

index 4a24354..61d91f2 100644
@@ -14,6 +14,7 @@
  # define __releases(x)	__attribute__((context(x,1,0)))
  # define __acquire(x)	__context__(x,1)
  # define __release(x)	__context__(x,-1)
+# define __ret_with_lock(x)	__context__(x,-1)
  # define __cond_lock(x,c)	((c) ? ({ __acquire(x); 1; }) : 0)
  # define __percpu	__attribute__((noderef, address_space(3)))
  #ifdef CONFIG_SPARSE_RCU_POINTER

@@ -37,6 +38,7 @@ extern void __chk_io_ptr(const volatile void __iomem *);
  # define __releases(x)
  # define __acquire(x) (void)0
  # define __release(x) (void)0
+# define __ret_with_lock(x)
  # define __cond_lock(x,c) (c)
  # define __percpu
  # define __rcu

And then use it instead of __release().

From now on using force_sig() is not safe, as it can race with an

Grabbing tasklist_lock has its disadvantages, i.e. it blocks process creation and destruction. If there are lots of processes, blocking doesn't sound as a great idea.

For LMK, it is sufficient to surround tasks list traverse with rcu_read_{,un}lock().

Quoted 1 line

From now on using force_sig() is not safe, as it can race with an

already exiting task, so we use send_sig() now. As a downside, it won't kill PID namespace init processes, but that's not what we want anyway.

Suggested-by Oleg Nesterov <[email protected]>

Signed-off-by Anton Vorontsov <[email protected]>

Quoted 12 lines · Anton Vorontsov

Grabbing tasklist_lock has its disadvantages, i.e. it blocks process creation and destruction. If there are lots of processes, blocking doesn't sound as a great idea.

For LMK, it is sufficient to surround tasks list traverse with rcu_read_{,un}lock().

Quoted 1 line

From now on using force_sig() is not safe, as it can race with an

already exiting task, so we use send_sig() now. As a downside, it won't kill PID namespace init processes, but that's not what we want anyway.

Quoted 2 lines

Suggested-by Oleg Nesterov <[email protected]>

Signed-off-by Anton Vorontsov <[email protected]>

Are these last 4 patches independant of the first 2 and can be taken through the staging tree now?

Quoted 13 lines · Greg KH

Quoted 12 lines · Anton Vorontsov

Grabbing tasklist_lock has its disadvantages, i.e. it blocks process creation and destruction. If there are lots of processes, blocking doesn't sound as a great idea.

For LMK, it is sufficient to surround tasks list traverse with rcu_read_{,un}lock().

Quoted 1 line

From now on using force_sig() is not safe, as it can race with an

already exiting task, so we use send_sig() now. As a downside, it won't kill PID namespace init processes, but that's not what we want anyway.

Quoted 5 lines

Quoted 2 lines

Suggested-by Oleg Nesterov <[email protected]>

Signed-off-by Anton Vorontsov <[email protected]>

Are these last 4 patches independant of the first 2 and can be taken through the staging tree now?

Yep, without the first two there is just a bit of sparse warnings. Not a big deal.

Grabbing tasklist_lock has its disadvantages, i.e. it blocks process creation and destruction. If there are lots of processes, blocking doesn't sound as a great idea.

For LMK, it is sufficient to surround tasks list traverse with rcu_read_{,un}lock().

From now on using force_sig() is not safe, as it can race with an already exiting task, so we use send_sig() now. As a downside, it won't kill PID namespace init processes, but that's not what we want anyway.

 static int lowmem_shrink(struct shrinker *s, struct shrink_control *sc) {
-       struct task_struct *p;
+       struct task_struct *tsk;
       struct task_struct *selected = NULL;
       int rem = 0;
       int tasksize;

drivers/staging/android/lowmemorykiller.c +7 -5

@@ -134,15 +134,17 @@ static int lowmem_shrink(struct shrinker *s, struct shrink_control *sc)
        selected_oom_adj = min_adj;
 
        rcu_read_lock();
-       for_each_process(p) {
-               struct mm_struct *mm;
+       for_each_process(tsk) {
+               struct task_struct *p;
                struct signal_struct *sig;
                int oom_adj;
 
-               task_lock(p);
-               mm = p->mm;
+               p = find_lock_task_mm(tsk);
+               if (!p)
+                       continue;
+
                sig = p->signal;
-               if (!mm || !sig) {
+               if (!sig) {
                        task_unlock(p);
                        continue;
                }

LMK should not directly check for task->mm. The reason is that the process' threads may exit or detach its mm via use_mm(), but other threads may still have a valid mm. To catch this we use find_lock_task_mm(), which walks up all threads and returns an appropriate task (with lock held).

 drivers/staging/android/lowmemorykiller.c |    8 +-------

 1 files changed, 1 insertions(+), 7 deletions(-)

drivers/staging/android/lowmemorykiller.c +0 -1

index 0755e2f..6e800d3 100644
@@ -136,19 +136,13 @@ static int lowmem_shrink(struct shrinker *s, struct shrink_control *sc)
        rcu_read_lock();
        for_each_process(tsk) {
                struct task_struct *p;
-               struct signal_struct *sig;
                int oom_adj;
 
                p = find_lock_task_mm(tsk);
                if (!p)
                        continue;
 

task->signal == NULL is not possible, so no need for these checks.

LMK should not try killing kernel threads.

Suggested-by Oleg Nesterov <[email protected]>

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/staging/android/lowmemorykiller.c |    3 +++

 1 files changed, 3 insertions(+), 0 deletions(-)

drivers/staging/android/lowmemorykiller.c +0 -0

index 6e800d3..ae38c39 100644

LMK should not try killing kernel threads.