[tip: sched/urgent] sched: Fix RANDSTRUCT build fail

[tip: sched/urgent] sched: Fix RANDSTRUCT build fail

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the sched/urgent branch of tip:

Commit-ID:     bfb9fbe0f7e70ec5c8e51ee55b6968d4dff14456
Gitweb:        https://git.kernel.org/tip/bfb9fbe0f7e70ec5c8e51ee55b6968d4dff14456
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Wed, 10 Jun 2020 12:14:09 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 10 Jun 2020 12:30:19 +02:00

sched: Fix RANDSTRUCT build fail

As a temporary build fix, the proper cleanup needs more work.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Reported-by: Eric Biggers <ebiggers@kernel.org>
Suggested-by: Eric Biggers <ebiggers@kernel.org>
Suggested-by: Kees Cook <keescook@chromium.org>
Fixes: a148866489fb ("sched: Replace rq::wake_list")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/sched.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 57a5ce9..59caeb9 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -653,8 +653,10 @@ struct task_struct {
 	unsigned int			ptrace;
 
 #ifdef CONFIG_SMP
-	struct llist_node		wake_entry;
-	unsigned int			wake_entry_type;
+	struct {
+		struct llist_node		wake_entry;
+		unsigned int			wake_entry_type;
+	};
 	int				on_cpu;
 #ifdef CONFIG_THREAD_INFO_IN_TASK
 	/* Current CPU: */

Re: [tip: sched/urgent] sched: Fix RANDSTRUCT build fail

[Kees Cook]

On Wed, Jun 10, 2020 at 10:34:16AM -0000, tip-bot2 for Peter Zijlstra wrote:
> The following commit has been merged into the sched/urgent branch of tip:
> 
> Commit-ID:     bfb9fbe0f7e70ec5c8e51ee55b6968d4dff14456
> Gitweb:        https://git.kernel.org/tip/bfb9fbe0f7e70ec5c8e51ee55b6968d4dff14456
> Author:        Peter Zijlstra <peterz@infradead.org>
> AuthorDate:    Wed, 10 Jun 2020 12:14:09 +02:00
> Committer:     Peter Zijlstra <peterz@infradead.org>
> CommitterDate: Wed, 10 Jun 2020 12:30:19 +02:00
> 
> sched: Fix RANDSTRUCT build fail
> 
> As a temporary build fix, the proper cleanup needs more work.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Reported-by: Eric Biggers <ebiggers@kernel.org>
> Suggested-by: Eric Biggers <ebiggers@kernel.org>
> Suggested-by: Kees Cook <keescook@chromium.org>
> Fixes: a148866489fb ("sched: Replace rq::wake_list")
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Hi, can this please get sent to Linus before -rc2? With a148866489fb in
-rc1, all the CI with the GCC plugins installed have been failing their
all*config builds. This entered -next 9 days ago (and fixed the -next
builds), but Linus's tree is still failing:

https://kernelci.org/build/mainline/branch/master/kernel/v5.8-rc1-226-g4333a9b0b67b/
...
https://kernelci.org/build/mainline/branch/master/kernel/v5.8-rc1/

Thanks!

-Kees

> ---
>  include/linux/sched.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 57a5ce9..59caeb9 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -653,8 +653,10 @@ struct task_struct {
>  	unsigned int			ptrace;
>  
>  #ifdef CONFIG_SMP
> -	struct llist_node		wake_entry;
> -	unsigned int			wake_entry_type;
> +	struct {
> +		struct llist_node		wake_entry;
> +		unsigned int			wake_entry_type;
> +	};
>  	int				on_cpu;
>  #ifdef CONFIG_THREAD_INFO_IN_TASK
>  	/* Current CPU: */

-- 
Kees Cook

Re: [tip: sched/urgent] sched: Fix RANDSTRUCT build fail

[Linus Torvalds]

On Fri, Jun 19, 2020 at 8:14 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Jun 10, 2020 at 10:34:16AM -0000, tip-bot2 for Peter Zijlstra wrote:
> > The following commit has been merged into the sched/urgent branch of tip:
> >
> > Commit-ID:     bfb9fbe0f7e70ec5c8e51ee55b6968d4dff14456
> > Gitweb:        https://git.kernel.org/tip/bfb9fbe0f7e70ec5c8e51ee55b6968d4dff14456
> > Author:        Peter Zijlstra <peterz@infradead.org>
> > AuthorDate:    Wed, 10 Jun 2020 12:14:09 +02:00
> > Committer:     Peter Zijlstra <peterz@infradead.org>
> > CommitterDate: Wed, 10 Jun 2020 12:30:19 +02:00
> >
> > sched: Fix RANDSTRUCT build fail
> >
> > As a temporary build fix, the proper cleanup needs more work.
> >
> > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > Reported-by: Eric Biggers <ebiggers@kernel.org>
> > Suggested-by: Eric Biggers <ebiggers@kernel.org>
> > Suggested-by: Kees Cook <keescook@chromium.org>
> > Fixes: a148866489fb ("sched: Replace rq::wake_list")
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>
> Hi, can this please get sent to Linus before -rc2? With a148866489fb in
> -rc1, all the CI with the GCC plugins installed have been failing their
> all*config builds. This entered -next 9 days ago (and fixed the -next
> builds), but Linus's tree is still failing:

Ugh.

I actually think the problem goes deeper than that.

The code expects the list entries to be of type 'call_single_data_t'

Then they damn well should be that type.

Note how "call_single_data_t" also implies certain alignment rules
that the hack in 'struct task_struct' does *not* have, and while that
doesn't matter on x86, it could matter on other architectures.

So no, I don't think Peter's patch is correct. It may make the build
pass, but that "check the offsets between two fields" is not
sufficient.

Now, if we could create a new

   struct __call_single_list_entry {
        struct llist_node llist;
        unsigned int flags;
   } call_single_list_entry_t;

and use that as part of thecall_single_data_t and only use that for
tyhe traversal of the list, then that would avoid the alignment issue
and the waste of space in struct task_struct.

Hmm?

Peter/Ingo?

                 Linus

Re: [tip: sched/urgent] sched: Fix RANDSTRUCT build fail

[Peter Zijlstra]

On Sat, Jun 20, 2020 at 09:32:26AM -0700, Linus Torvalds wrote:
> Peter/Ingo?

I have the below (and a whole fair number of even more patches), but
I've been struggling with a possibly related NULL pointer deref, which
is why I've been holding off on landing this.

It looks like we finally have a handle on the NULL deref, and it isn't
related to the IPI stuff (as it was initially bisected to), so I'll go
polish that a bit and then push stuff (including the below) into
sched/urgent.

---

Subject: smp, irq_work: Continue smp_call_function*() and irq_work*() integration
From: Peter Zijlstra <peterz@infradead.org>
Date: Mon Jun 15 11:27:58 CEST 2020

Instead of relying on BUG_ON() to ensure the various data structures
line up, use a bunch of horrible unions.

Much of the union magic is to ensure irq_work and smp_call_function do
not (yet) see the members of their respective data structures change
name.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
---
 include/linux/irq_work.h  |   26 +++++-------------
 include/linux/sched.h     |    5 ---
 include/linux/smp.h       |   23 +++++-----------
 include/linux/smp_types.h |   66 ++++++++++++++++++++++++++++++++++++++++++++++
 kernel/sched/core.c       |    6 ++--
 kernel/smp.c              |   18 ------------
 6 files changed, 86 insertions(+), 58 deletions(-)

--- a/include/linux/irq_work.h
+++ b/include/linux/irq_work.h
@@ -2,7 +2,7 @@
 #ifndef _LINUX_IRQ_WORK_H
 #define _LINUX_IRQ_WORK_H
 
-#include <linux/llist.h>
+#include <linux/smp_types.h>
 
 /*
  * An entry can be in one of four states:
@@ -13,24 +13,14 @@
  * busy      NULL, 2 -> {free, claimed} : callback in progress, can be claimed
  */
 
-/* flags share CSD_FLAG_ space */
-
-#define IRQ_WORK_PENDING	BIT(0)
-#define IRQ_WORK_BUSY		BIT(1)
-
-/* Doesn't want IPI, wait for tick: */
-#define IRQ_WORK_LAZY		BIT(2)
-/* Run hard IRQ context, even on RT */
-#define IRQ_WORK_HARD_IRQ	BIT(3)
-
-#define IRQ_WORK_CLAIMED	(IRQ_WORK_PENDING | IRQ_WORK_BUSY)
-
-/*
- * structure shares layout with single_call_data_t.
- */
 struct irq_work {
-	struct llist_node llnode;
-	atomic_t flags;
+	union {
+		struct __call_single_node node;
+		struct {
+			struct llist_node llnode;
+			atomic_t flags;
+		};
+	};
 	void (*func)(struct irq_work *);
 };
 
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -653,11 +653,8 @@ struct task_struct {
 	unsigned int			ptrace;
 
 #ifdef CONFIG_SMP
-	struct {
-		struct llist_node		wake_entry;
-		unsigned int			wake_entry_type;
-	};
 	int				on_cpu;
+	struct __call_single_node	wake_entry;
 #ifdef CONFIG_THREAD_INFO_IN_TASK
 	/* Current CPU: */
 	unsigned int			cpu;
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -12,29 +12,22 @@
 #include <linux/list.h>
 #include <linux/cpumask.h>
 #include <linux/init.h>
-#include <linux/llist.h>
+#include <linux/smp_types.h>
 
 typedef void (*smp_call_func_t)(void *info);
 typedef bool (*smp_cond_func_t)(int cpu, void *info);
 
-enum {
-	CSD_FLAG_LOCK		= 0x01,
-
-	/* IRQ_WORK_flags */
-
-	CSD_TYPE_ASYNC		= 0x00,
-	CSD_TYPE_SYNC		= 0x10,
-	CSD_TYPE_IRQ_WORK	= 0x20,
-	CSD_TYPE_TTWU		= 0x30,
-	CSD_FLAG_TYPE_MASK	= 0xF0,
-};
-
 /*
  * structure shares (partial) layout with struct irq_work
  */
 struct __call_single_data {
-	struct llist_node llist;
-	unsigned int flags;
+	union {
+		struct __call_single_node node;
+		struct {
+			struct llist_node llist;
+			unsigned int flags;
+		};
+	};
 	smp_call_func_t func;
 	void *info;
 };
--- /dev/null
+++ b/include/linux/smp_types.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_SMP_TYPES_H
+#define __LINUX_SMP_TYPES_H
+
+#include <linux/llist.h>
+
+enum {
+	CSD_FLAG_LOCK		= 0x01,
+
+	IRQ_WORK_PENDING	= 0x01,
+	IRQ_WORK_BUSY		= 0x02,
+	IRQ_WORK_LAZY		= 0x04, /* No IPI, wait for tick */
+	IRQ_WORK_HARD_IRQ	= 0x08, /* IRQ context on PREEMPT_RT */
+
+	IRQ_WORK_CLAIMED	= (IRQ_WORK_PENDING | IRQ_WORK_BUSY),
+
+	CSD_TYPE_ASYNC		= 0x00,
+	CSD_TYPE_SYNC		= 0x10,
+	CSD_TYPE_IRQ_WORK	= 0x20,
+	CSD_TYPE_TTWU		= 0x30,
+
+	CSD_FLAG_TYPE_MASK	= 0xF0,
+};
+
+/*
+ * struct __call_single_node is the primary type on
+ * smp.c:call_single_queue.
+ *
+ * flush_smp_call_function_queue() only reads the type from
+ * __call_single_node::u_flags as a regular load, the above
+ * (anonymous) enum defines all the bits of this word.
+ *
+ * Other bits are not modified until the type is known.
+ *
+ * CSD_TYPE_SYNC/ASYNC:
+ *	struct {
+ *		struct llist_node node;
+ *		unsigned int flags;
+ *		smp_call_func_t func;
+ *		void *info;
+ *	};
+ *
+ * CSD_TYPE_IRQ_WORK:
+ *	struct {
+ *		struct llist_node node;
+ *		atomic_t flags;
+ *		void (*func)(struct irq_work *);
+ *	};
+ *
+ * CSD_TYPE_TTWU:
+ *	struct {
+ *		struct llist_node node;
+ *		unsigned int flags;
+ *	};
+ *
+ */
+
+struct __call_single_node {
+	struct llist_node	llist;
+	union {
+		unsigned int	u_flags;
+		atomic_t	a_flags;
+	};
+};
+
+#endif /* __LINUX_SMP_TYPES_H */
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2293,7 +2293,7 @@ void sched_ttwu_pending(void *arg)
 	rq_lock_irqsave(rq, &rf);
 	update_rq_clock(rq);
 
-	llist_for_each_entry_safe(p, t, llist, wake_entry)
+	llist_for_each_entry_safe(p, t, llist, wake_entry.llist)
 		ttwu_do_activate(rq, p, p->sched_remote_wakeup ? WF_MIGRATED : 0, &rf);
 
 	rq_unlock_irqrestore(rq, &rf);
@@ -2322,7 +2322,7 @@ static void __ttwu_queue_wakelist(struct
 	p->sched_remote_wakeup = !!(wake_flags & WF_MIGRATED);
 
 	WRITE_ONCE(rq->ttwu_pending, 1);
-	__smp_call_single_queue(cpu, &p->wake_entry);
+	__smp_call_single_queue(cpu, &p->wake_entry.llist);
 }
 
 void wake_up_if_idle(int cpu)
@@ -2774,7 +2774,7 @@ static void __sched_fork(unsigned long c
 #endif
 	init_numa_balancing(clone_flags, p);
 #ifdef CONFIG_SMP
-	p->wake_entry_type = CSD_TYPE_TTWU;
+	p->wake_entry.u_flags = CSD_TYPE_TTWU;
 #endif
 }
 
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -671,24 +671,6 @@ void __init smp_init(void)
 {
 	int num_nodes, num_cpus;
 
-	/*
-	 * Ensure struct irq_work layout matches so that
-	 * flush_smp_call_function_queue() can do horrible things.
-	 */
-	BUILD_BUG_ON(offsetof(struct irq_work, llnode) !=
-		     offsetof(struct __call_single_data, llist));
-	BUILD_BUG_ON(offsetof(struct irq_work, func) !=
-		     offsetof(struct __call_single_data, func));
-	BUILD_BUG_ON(offsetof(struct irq_work, flags) !=
-		     offsetof(struct __call_single_data, flags));
-
-	/*
-	 * Assert the CSD_TYPE_TTWU layout is similar enough
-	 * for task_struct to be on the @call_single_queue.
-	 */
-	BUILD_BUG_ON(offsetof(struct task_struct, wake_entry_type) - offsetof(struct task_struct, wake_entry) !=
-		     offsetof(struct __call_single_data, flags) - offsetof(struct __call_single_data, llist));
-
 	idle_threads_init();
 	cpuhp_threads_init();
 

Re: [tip: sched/urgent] sched: Fix RANDSTRUCT build fail

[Peter Zijlstra]

On Mon, Jun 22, 2020 at 10:10:27AM +0200, Peter Zijlstra wrote:

> Instead of relying on BUG_ON() to ensure the various data structures
> line up, use a bunch of horrible unions.

>  struct irq_work {
> +	union {
> +		struct __call_single_node node;
> +		struct {
> +			struct llist_node llnode;
> +			atomic_t flags;
> +		};
> +	};
>  	void (*func)(struct irq_work *);
>  };

>  struct __call_single_data {
> +	union {
> +		struct __call_single_node node;
> +		struct {
> +			struct llist_node llist;
> +			unsigned int flags;
> +		};
> +	};
>  	smp_call_func_t func;
>  	void *info;
>  };

FWIW, I have 2 further patches, one for each of these structures to get
rid of the horrible union. They are somewhat larger and I was planning
to hold on to them for next round, but if you want them now, I can
certainly do that.

Re: [tip: sched/urgent] sched: Fix RANDSTRUCT build fail

[Peter Zijlstra]

On Mon, Jun 22, 2020 at 10:10:27AM +0200, Peter Zijlstra wrote:
> On Sat, Jun 20, 2020 at 09:32:26AM -0700, Linus Torvalds wrote:
> > Peter/Ingo?
> 
> I have the below (and a whole fair number of even more patches), but
> I've been struggling with a possibly related NULL pointer deref, which
> is why I've been holding off on landing this.

FYI the NULL pointer thing:

  https://lkml.kernel.org/r/20200620184622.GA19696@paulmck-ThinkPad-P72

Re: [tip: sched/urgent] sched: Fix RANDSTRUCT build fail

[Guenter Roeck]

On 6/20/20 9:32 AM, Linus Torvalds wrote:
> On Fri, Jun 19, 2020 at 8:14 PM Kees Cook <keescook@chromium.org> wrote:
>>
>> On Wed, Jun 10, 2020 at 10:34:16AM -0000, tip-bot2 for Peter Zijlstra wrote:
>>> The following commit has been merged into the sched/urgent branch of tip:
>>>
>>> Commit-ID:     bfb9fbe0f7e70ec5c8e51ee55b6968d4dff14456
>>> Gitweb:        https://git.kernel.org/tip/bfb9fbe0f7e70ec5c8e51ee55b6968d4dff14456
>>> Author:        Peter Zijlstra <peterz@infradead.org>
>>> AuthorDate:    Wed, 10 Jun 2020 12:14:09 +02:00
>>> Committer:     Peter Zijlstra <peterz@infradead.org>
>>> CommitterDate: Wed, 10 Jun 2020 12:30:19 +02:00
>>>
>>> sched: Fix RANDSTRUCT build fail
>>>
>>> As a temporary build fix, the proper cleanup needs more work.
>>>
>>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>>> Reported-by: Eric Biggers <ebiggers@kernel.org>
>>> Suggested-by: Eric Biggers <ebiggers@kernel.org>
>>> Suggested-by: Kees Cook <keescook@chromium.org>
>>> Fixes: a148866489fb ("sched: Replace rq::wake_list")
>>> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>>
>> Hi, can this please get sent to Linus before -rc2? With a148866489fb in
>> -rc1, all the CI with the GCC plugins installed have been failing their
>> all*config builds. This entered -next 9 days ago (and fixed the -next
>> builds), but Linus's tree is still failing:
> 
> Ugh.
> 
> I actually think the problem goes deeper than that.
> 
> The code expects the list entries to be of type 'call_single_data_t'
> 
> Then they damn well should be that type.
> 
> Note how "call_single_data_t" also implies certain alignment rules
> that the hack in 'struct task_struct' does *not* have, and while that
> doesn't matter on x86, it could matter on other architectures.
> 

Yes, that came up before. At least, with this patch, I don't see any compile
or boot test failures. This patch wasn't supposed to be a clean solution
(Peter is working on that) but a quick fix that can be applied easily
and at least improves the situation.

As it is, the mainline kernel currently relies on having RANDSTRUCT
disabled, and still has all the other problems you mentioned here,
so it is definitely much worse.

On the other side, a148866489fb has been in the tree for more
than three weeks now. Maybe we can wait more time for a more
comprehensive solution which hopefully doesn't introduce other
problems.

Guenter

> So no, I don't think Peter's patch is correct. It may make the build
> pass, but that "check the offsets between two fields" is not
> sufficient.
> 
> Now, if we could create a new
> 
>    struct __call_single_list_entry {
>         struct llist_node llist;
>         unsigned int flags;
>    } call_single_list_entry_t;
> 
> and use that as part of thecall_single_data_t and only use that for
> tyhe traversal of the list, then that would avoid the alignment issue
> and the waste of space in struct task_struct.
> 
> Hmm?
> 
> Peter/Ingo?
> 
>                  Linus
>