sched: fix confusing PFA_NO_NEW_PRIVS constant

PATCH Superseded Zefan Li 3 participants 6 messages Sep 22, 2014

Subsystems (3)

Scheduler cgroup Control group - cpuset

lore

Share & tools

lore

Zefan Li

author

2014-09-22 03:50 UTC #0 lore raw

PATCH 1/3 sched: fix confusing PFA_NO_NEW_PRIVS constant

From: Tetsuo Handa <penguin-kernel-JPay3/[email protected]>

Commit 1d4457f99928 ("sched: move no_new_privs into new atomic flags") defined PFA_NO_NEW_PRIVS as hexadecimal value, but it is confusing because it is used as bit number. Redefine it as decimal bit number.

Signed-off-by Tetsuo Handa <penguin-kernel-JPay3/[email protected]>

Acked-by Kees Cook <[email protected]>

Signed-off-by Zefan Li <[email protected]>

---
 include/linux/sched.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/linux/sched.h +1 -1

index 5c2c885..4557765 100644
@@ -1957,7 +1957,7 @@ static inline void memalloc_noio_restore(unsigned int flags)
 }
 
 /* Per-process atomic flags. */
-#define PFA_NO_NEW_PRIVS 0x00000001	/* May not gain new privileges. */
+#define PFA_NO_NEW_PRIVS 0	/* May not gain new privileges. */
 
 static inline bool task_no_new_privs(struct task_struct *p)
 {

Signature · 1 line

1.8.0.2

Zefan Li

author

2014-09-22 03:51 UTC #1 lore raw

PATCH 2/3 sched: add a macro to define bitops for task atomic flags

This will simplify code when we add new flags.

Signed-off-by Zefan Li <[email protected]>

---
 include/linux/sched.h | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

include/linux/sched.h +11 -9

index 4557765..04a2ae2 100644
@@ -1959,15 +1959,17 @@ static inline void memalloc_noio_restore(unsigned int flags)
 /* Per-process atomic flags. */
 #define PFA_NO_NEW_PRIVS 0	/* May not gain new privileges. */
 
-static inline bool task_no_new_privs(struct task_struct *p)
-{
-	return test_bit(PFA_NO_NEW_PRIVS, &p->atomic_flags);
-}
-
-static inline void task_set_no_new_privs(struct task_struct *p)
-{
-	set_bit(PFA_NO_NEW_PRIVS, &p->atomic_flags);
-}
+#define TASK_PFA_BITOPS(name, func)				\
+static inline bool task_##func(struct task_struct *p)		\
+{ return test_bit(PFA_##name, &p->atomic_flags); }		\
+								\
+static inline void task_set_##func(struct task_struct *p)	\
+{ set_bit(PFA_##name, &p->atomic_flags); }			\
+								\
+static inline void task_clear_##func(struct task_struct *p)	\
+{ clear_bit(PFA_##name, &p->atomic_flags); }
+
+TASK_PFA_BITOPS(NO_NEW_PRIVS, no_new_privs)
 
 /*
  * task->jobctl flags

Signature · 1 line

1.8.0.2

Peter Zijlstra

maintainer

2014-09-22 11:45 UTC #2 lore raw

Re: [PATCH 2/3] sched: add a macro to define bitops for task atomic flags

Quoted 25 lines · Zefan Li

This will simplify code when we add new flags.

Signed-off-by Zefan Li <[email protected]>

---
 include/linux/sched.h | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

include/linux/sched.h +1 -9

index 4557765..04a2ae2 100644
@@ -1959,15 +1959,17 @@ static inline void memalloc_noio_restore(unsigned int flags)
 /* Per-process atomic flags. */
 #define PFA_NO_NEW_PRIVS 0	/* May not gain new privileges. */
 
-static inline bool task_no_new_privs(struct task_struct *p)
-{
-	return test_bit(PFA_NO_NEW_PRIVS, &p->atomic_flags);
-}
-
-static inline void task_set_no_new_privs(struct task_struct *p)
-{
-	set_bit(PFA_NO_NEW_PRIVS, &p->atomic_flags);
-}
+#define TASK_PFA_BITOPS(name, func)				\

include/linux/sched.h static inline void memalloc_noio_restore(unsigned int flags) · line 1972

Quoted 10 lines

include/linux/sched.h +10 -0

+static inline bool task_##func(struct task_struct *p)		\
+{ return test_bit(PFA_##name, &p->atomic_flags); }		\
+								\
+static inline void task_set_##func(struct task_struct *p)	\
+{ set_bit(PFA_##name, &p->atomic_flags); }			\
+								\
+static inline void task_clear_##func(struct task_struct *p)	\
+{ clear_bit(PFA_##name, &p->atomic_flags); }
+
+TASK_PFA_BITOPS(NO_NEW_PRIVS, no_new_privs)

This could really do with a corresponding script/tags.sh update.

	--regex-c++='TASK_PFA_BITOPS\([^,]*,([^)]*)\)/task_\1/'
	--regex-c++='TASK_PFA_BITOPS\([^,]*,([^)]*)\)/task_set_\1/'
	--regex-c++='TASK_PFA_BITOPS\([^,]*,([^)]*)\)/task_clear_\1/'

For exhuberant and matching bits for emacs.

Zefan Li

author

2014-09-22 03:52 UTC #3 lore raw

PATCH 3/3 cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should be atomic flags

In reply to Zefan Li

When we change cpuset.memory_spread_{page,slab}, cpuset will flip PF_SPREAD_{PAGE,SLAB} bit of tsk->flags for each task in that cpuset. This should be done using atomic bitops, but currently we don't, which is broken.

Tetsuo reported a hard-to-reproduce kernel crash on RHEL6, which happend when one thread tried to clear PF_USED_MATH while at the same time another thread tried to flip PF_SPREAD_PAGE/PF_SPREAD_SLAB. They both operate on the same task.

Here's the full report:
https://lkml.org/lkml/2014/9/19/230

To fix this, we make PF_SPREAD_PAGE and PF_SPARED_SLAB atomic flags.

Fixes 950592f7b991 ("cpusets: update tasks' page/slab spread flags in time")

Cc <[email protected]> # 2.6.31+

Reported-by Tetsuo Handa <[email protected]>

Signed-off-by Zefan Li <[email protected]>

---
 include/linux/cpuset.h | 4 ++--
 include/linux/sched.h  | 6 ++++--
 kernel/cpuset.c        | 9 +++++----
 3 files changed, 11 insertions(+), 8 deletions(-)

include/linux/cpuset.h +2 -2

index 0d4e067..2f073db 100644
@@ -94,12 +94,12 @@ extern int cpuset_slab_spread_node(void);
 
 static inline int cpuset_do_page_mem_spread(void)
 {
-	return current->flags & PF_SPREAD_PAGE;
+	return task_spread_page(current);
 }
 
 static inline int cpuset_do_slab_mem_spread(void)
 {
-	return current->flags & PF_SPREAD_SLAB;
+	return task_spread_slab(current);
 }
 
 extern int current_cpuset_is_being_rebound(void);

include/linux/sched.h +4 -2

index 04a2ae2..ee634d1 100644
@@ -1903,8 +1903,6 @@ extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t *ut,
 #define PF_KTHREAD	0x00200000	/* I am a kernel thread */
 #define PF_RANDOMIZE	0x00400000	/* randomize virtual address space */
 #define PF_SWAPWRITE	0x00800000	/* Allowed to write to swap */
-#define PF_SPREAD_PAGE	0x01000000	/* Spread page cache over cpuset */
-#define PF_SPREAD_SLAB	0x02000000	/* Spread some slab caches over cpuset */
 #define PF_NO_SETAFFINITY 0x04000000	/* Userland is not allowed to meddle with cpus_allowed */
 #define PF_MCE_EARLY    0x08000000      /* Early kill for mce process policy */
 #define PF_MUTEX_TESTER	0x20000000	/* Thread belongs to the rt mutex tester */

@@ -1958,6 +1956,8 @@ static inline void memalloc_noio_restore(unsigned int flags)
 
 /* Per-process atomic flags. */
 #define PFA_NO_NEW_PRIVS 0	/* May not gain new privileges. */
+#define PFA_SPREAD_PAGE  1      /* Spread page cache over cpuset */
+#define PFA_SPREAD_SLAB  2      /* Spread some slab caches over cpuset */
 
 #define TASK_PFA_BITOPS(name, func)				\
 static inline bool task_##func(struct task_struct *p)		\

@@ -1970,6 +1970,8 @@ static inline void task_clear_##func(struct task_struct *p)	\
 { clear_bit(PFA_##name, &p->atomic_flags); }
 
 TASK_PFA_BITOPS(NO_NEW_PRIVS, no_new_privs)
+TASK_PFA_BITOPS(SPREAD_PAGE, spread_page)
+TASK_PFA_BITOPS(SPREAD_SLAB, spread_slab)
 
 /*
  * task->jobctl flags

kernel/cpuset.c +5 -4

index a37f4ed..1f107c7 100644
@@ -365,13 +365,14 @@ static void cpuset_update_task_spread_flag(struct cpuset *cs,
 					struct task_struct *tsk)
 {
 	if (is_spread_page(cs))
-		tsk->flags |= PF_SPREAD_PAGE;
+		task_set_spread_page(tsk);
 	else
-		tsk->flags &= ~PF_SPREAD_PAGE;
+		task_clear_spread_page(tsk);
+
 	if (is_spread_slab(cs))
-		tsk->flags |= PF_SPREAD_SLAB;
+		task_set_spread_slab(tsk);
 	else
-		tsk->flags &= ~PF_SPREAD_SLAB;
+		task_clear_spread_slab(tsk);
 }
 
 /*

Signature · 1 line

1.8.0.2

Tejun Heo

maintainer

2014-09-22 04:02 UTC #4 lore raw

In reply to Zefan Li

Quoted 5 lines · Zefan Li

From: Tetsuo Handa <penguin-kernel-JPay3/[email protected]>
Commit 1d4457f99928 ("sched: move no_new_privs into new atomic flags") defined PFA_NO_NEW_PRIVS as hexadecimal value, but it is confusing because it is used as bit number. Redefine it as decimal bit number.

It's also changing the bit position, which is fine but should be mentioned in the description.

Thanks.

Signature · 1 line

tejun

Zefan Li

author

2014-09-22 04:44 UTC #5 lore raw

Quoted 10 lines · Tejun Heo

Quoted 5 lines · Zefan Li
From: Tetsuo Handa <[email protected]>
Commit 1d4457f99928 ("sched: move no_new_privs into new atomic flags") defined PFA_NO_NEW_PRIVS as hexadecimal value, but it is confusing because it is used as bit number. Redefine it as decimal bit number.
It's also changing the bit position, which is fine but should be mentioned in the description.

I've resent the patch with changelog updated.