All of lore.kernel.org
 help / color / mirror / Atom feed
From: Grygorii Strashko <grygorii.strashko@ti.com>
To: Russell King <linux@armlinux.org.uk>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	<linux-rt-users@vger.kernel.org>
Cc: <linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>,
	Grygorii Strashko <grygorii.strashko@ti.com>,
	Kees Cook <keescook@chromium.org>,
	Laura Abbott <labbott@redhat.com>
Subject: [v4.9-rt PATCH v2] ARM: mm: remove tasklist locking from update_sections_early()
Date: Wed, 19 Apr 2017 15:10:47 -0500	[thread overview]
Message-ID: <20170419201047.31578-1-grygorii.strashko@ti.com> (raw)

The below backtrace can be observed on -rt kernel with CONFIG_DEBUG_RODATA
option enabled:

 BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:993
 in_atomic(): 1, irqs_disabled(): 128, pid: 14, name: migration/0
 1 lock held by migration/0/14:
  #0:  (tasklist_lock){+.+...}, at: [<c01183e8>] update_sections_early+0x24/0xdc
 irq event stamp: 38
 hardirqs last  enabled at (37): [<c08f6f7c>] _raw_spin_unlock_irq+0x24/0x68
 hardirqs last disabled at (38): [<c01fdfe8>] multi_cpu_stop+0xd8/0x138
 softirqs last  enabled at (0): [<c01303ec>] copy_process.part.5+0x238/0x1b64
 softirqs last disabled at (0): [<  (null)>]   (null)
 Preemption disabled at: [<c01fe244>] cpu_stopper_thread+0x80/0x10c
 CPU: 0 PID: 14 Comm: migration/0 Not tainted 4.9.21-rt16-02220-g49e319c #15
 Hardware name: Generic DRA74X (Flattened Device Tree)
 [<c0112014>] (unwind_backtrace) from [<c010d370>] (show_stack+0x10/0x14)
 [<c010d370>] (show_stack) from [<c049beb8>] (dump_stack+0xa8/0xd4)
 [<c049beb8>] (dump_stack) from [<c01631a0>] (___might_sleep+0x1bc/0x2ac)
 [<c01631a0>] (___might_sleep) from [<c08f7244>] (__rt_spin_lock+0x1c/0x30)
 [<c08f7244>] (__rt_spin_lock) from [<c08f77a4>] (rt_read_lock+0x54/0x68)
 [<c08f77a4>] (rt_read_lock) from [<c01183e8>] (update_sections_early+0x24/0xdc)
 [<c01183e8>] (update_sections_early) from [<c01184b0>] (__fix_kernmem_perms+0x10/0x1c)
 [<c01184b0>] (__fix_kernmem_perms) from [<c01fe010>] (multi_cpu_stop+0x100/0x138)
 [<c01fe010>] (multi_cpu_stop) from [<c01fe24c>] (cpu_stopper_thread+0x88/0x10c)
 [<c01fe24c>] (cpu_stopper_thread) from [<c015edc4>] (smpboot_thread_fn+0x174/0x31c)
 [<c015edc4>] (smpboot_thread_fn) from [<c015a988>] (kthread+0xf0/0x108)
 [<c015a988>] (kthread) from [<c0108818>] (ret_from_fork+0x14/0x3c)
 Freeing unused kernel memory: 1024K (c0d00000 - c0e00000)

The stop_machine() is called with cpus = NULL from fix_kernmem_perms() and
mark_rodata_ro() which means only one CPU will execute
update_sections_early() while all other CPUs will spin and wait. Hence,
it's safe to remove tasklist locking from update_sections_early(). As part
of this change also mark functions which are local to this module as
static.

Cc: Kees Cook <keescook@chromium.org>
Cc: Laura Abbott <labbott@redhat.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
As I've checked it also can be applied to LKML as is.

Changes in v2:
 - added comment to update_sections_early()

v1: https://patchwork.kernel.org/patch/9686289/

 arch/arm/mm/init.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 370581a..838f6b35 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -689,34 +689,37 @@ void set_section_perms(struct section_perm *perms, int n, bool set,
 
 }
 
+/**
+ * update_sections_early intended to be called only through stop_machine
+ * framework and executed by only one CPU while all other CPUs will spin and
+ * wait, so no locking is required in this function.
+ */
 static void update_sections_early(struct section_perm perms[], int n)
 {
 	struct task_struct *t, *s;
 
-	read_lock(&tasklist_lock);
 	for_each_process(t) {
 		if (t->flags & PF_KTHREAD)
 			continue;
 		for_each_thread(t, s)
 			set_section_perms(perms, n, true, s->mm);
 	}
-	read_unlock(&tasklist_lock);
 	set_section_perms(perms, n, true, current->active_mm);
 	set_section_perms(perms, n, true, &init_mm);
 }
 
-int __fix_kernmem_perms(void *unused)
+static int __fix_kernmem_perms(void *unused)
 {
 	update_sections_early(nx_perms, ARRAY_SIZE(nx_perms));
 	return 0;
 }
 
-void fix_kernmem_perms(void)
+static void fix_kernmem_perms(void)
 {
 	stop_machine(__fix_kernmem_perms, NULL, NULL);
 }
 
-int __mark_rodata_ro(void *unused)
+static int __mark_rodata_ro(void *unused)
 {
 	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
 	return 0;
-- 
2.10.1

WARNING: multiple messages have this Message-ID (diff)
From: Grygorii Strashko <grygorii.strashko@ti.com>
To: Russell King <linux@armlinux.org.uk>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	<linux-rt-users@vger.kernel.org>
Cc: Laura Abbott <labbott@redhat.com>,
	Grygorii Strashko <grygorii.strashko@ti.com>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Kees Cook <keescook@chromium.org>
Subject: [v4.9-rt PATCH v2] ARM: mm: remove tasklist locking from update_sections_early()
Date: Wed, 19 Apr 2017 15:10:47 -0500	[thread overview]
Message-ID: <20170419201047.31578-1-grygorii.strashko@ti.com> (raw)

The below backtrace can be observed on -rt kernel with CONFIG_DEBUG_RODATA
option enabled:

 BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:993
 in_atomic(): 1, irqs_disabled(): 128, pid: 14, name: migration/0
 1 lock held by migration/0/14:
  #0:  (tasklist_lock){+.+...}, at: [<c01183e8>] update_sections_early+0x24/0xdc
 irq event stamp: 38
 hardirqs last  enabled at (37): [<c08f6f7c>] _raw_spin_unlock_irq+0x24/0x68
 hardirqs last disabled at (38): [<c01fdfe8>] multi_cpu_stop+0xd8/0x138
 softirqs last  enabled at (0): [<c01303ec>] copy_process.part.5+0x238/0x1b64
 softirqs last disabled at (0): [<  (null)>]   (null)
 Preemption disabled at: [<c01fe244>] cpu_stopper_thread+0x80/0x10c
 CPU: 0 PID: 14 Comm: migration/0 Not tainted 4.9.21-rt16-02220-g49e319c #15
 Hardware name: Generic DRA74X (Flattened Device Tree)
 [<c0112014>] (unwind_backtrace) from [<c010d370>] (show_stack+0x10/0x14)
 [<c010d370>] (show_stack) from [<c049beb8>] (dump_stack+0xa8/0xd4)
 [<c049beb8>] (dump_stack) from [<c01631a0>] (___might_sleep+0x1bc/0x2ac)
 [<c01631a0>] (___might_sleep) from [<c08f7244>] (__rt_spin_lock+0x1c/0x30)
 [<c08f7244>] (__rt_spin_lock) from [<c08f77a4>] (rt_read_lock+0x54/0x68)
 [<c08f77a4>] (rt_read_lock) from [<c01183e8>] (update_sections_early+0x24/0xdc)
 [<c01183e8>] (update_sections_early) from [<c01184b0>] (__fix_kernmem_perms+0x10/0x1c)
 [<c01184b0>] (__fix_kernmem_perms) from [<c01fe010>] (multi_cpu_stop+0x100/0x138)
 [<c01fe010>] (multi_cpu_stop) from [<c01fe24c>] (cpu_stopper_thread+0x88/0x10c)
 [<c01fe24c>] (cpu_stopper_thread) from [<c015edc4>] (smpboot_thread_fn+0x174/0x31c)
 [<c015edc4>] (smpboot_thread_fn) from [<c015a988>] (kthread+0xf0/0x108)
 [<c015a988>] (kthread) from [<c0108818>] (ret_from_fork+0x14/0x3c)
 Freeing unused kernel memory: 1024K (c0d00000 - c0e00000)

The stop_machine() is called with cpus = NULL from fix_kernmem_perms() and
mark_rodata_ro() which means only one CPU will execute
update_sections_early() while all other CPUs will spin and wait. Hence,
it's safe to remove tasklist locking from update_sections_early(). As part
of this change also mark functions which are local to this module as
static.

Cc: Kees Cook <keescook@chromium.org>
Cc: Laura Abbott <labbott@redhat.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
As I've checked it also can be applied to LKML as is.

Changes in v2:
 - added comment to update_sections_early()

v1: https://patchwork.kernel.org/patch/9686289/

 arch/arm/mm/init.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 370581a..838f6b35 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -689,34 +689,37 @@ void set_section_perms(struct section_perm *perms, int n, bool set,
 
 }
 
+/**
+ * update_sections_early intended to be called only through stop_machine
+ * framework and executed by only one CPU while all other CPUs will spin and
+ * wait, so no locking is required in this function.
+ */
 static void update_sections_early(struct section_perm perms[], int n)
 {
 	struct task_struct *t, *s;
 
-	read_lock(&tasklist_lock);
 	for_each_process(t) {
 		if (t->flags & PF_KTHREAD)
 			continue;
 		for_each_thread(t, s)
 			set_section_perms(perms, n, true, s->mm);
 	}
-	read_unlock(&tasklist_lock);
 	set_section_perms(perms, n, true, current->active_mm);
 	set_section_perms(perms, n, true, &init_mm);
 }
 
-int __fix_kernmem_perms(void *unused)
+static int __fix_kernmem_perms(void *unused)
 {
 	update_sections_early(nx_perms, ARRAY_SIZE(nx_perms));
 	return 0;
 }
 
-void fix_kernmem_perms(void)
+static void fix_kernmem_perms(void)
 {
 	stop_machine(__fix_kernmem_perms, NULL, NULL);
 }
 
-int __mark_rodata_ro(void *unused)
+static int __mark_rodata_ro(void *unused)
 {
 	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
 	return 0;
-- 
2.10.1

WARNING: multiple messages have this Message-ID (diff)
From: grygorii.strashko@ti.com (Grygorii Strashko)
To: linux-arm-kernel@lists.infradead.org
Subject: [v4.9-rt PATCH v2] ARM: mm: remove tasklist locking from update_sections_early()
Date: Wed, 19 Apr 2017 15:10:47 -0500	[thread overview]
Message-ID: <20170419201047.31578-1-grygorii.strashko@ti.com> (raw)

The below backtrace can be observed on -rt kernel with CONFIG_DEBUG_RODATA
option enabled:

 BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:993
 in_atomic(): 1, irqs_disabled(): 128, pid: 14, name: migration/0
 1 lock held by migration/0/14:
  #0:  (tasklist_lock){+.+...}, at: [<c01183e8>] update_sections_early+0x24/0xdc
 irq event stamp: 38
 hardirqs last  enabled at (37): [<c08f6f7c>] _raw_spin_unlock_irq+0x24/0x68
 hardirqs last disabled at (38): [<c01fdfe8>] multi_cpu_stop+0xd8/0x138
 softirqs last  enabled at (0): [<c01303ec>] copy_process.part.5+0x238/0x1b64
 softirqs last disabled at (0): [<  (null)>]   (null)
 Preemption disabled at: [<c01fe244>] cpu_stopper_thread+0x80/0x10c
 CPU: 0 PID: 14 Comm: migration/0 Not tainted 4.9.21-rt16-02220-g49e319c #15
 Hardware name: Generic DRA74X (Flattened Device Tree)
 [<c0112014>] (unwind_backtrace) from [<c010d370>] (show_stack+0x10/0x14)
 [<c010d370>] (show_stack) from [<c049beb8>] (dump_stack+0xa8/0xd4)
 [<c049beb8>] (dump_stack) from [<c01631a0>] (___might_sleep+0x1bc/0x2ac)
 [<c01631a0>] (___might_sleep) from [<c08f7244>] (__rt_spin_lock+0x1c/0x30)
 [<c08f7244>] (__rt_spin_lock) from [<c08f77a4>] (rt_read_lock+0x54/0x68)
 [<c08f77a4>] (rt_read_lock) from [<c01183e8>] (update_sections_early+0x24/0xdc)
 [<c01183e8>] (update_sections_early) from [<c01184b0>] (__fix_kernmem_perms+0x10/0x1c)
 [<c01184b0>] (__fix_kernmem_perms) from [<c01fe010>] (multi_cpu_stop+0x100/0x138)
 [<c01fe010>] (multi_cpu_stop) from [<c01fe24c>] (cpu_stopper_thread+0x88/0x10c)
 [<c01fe24c>] (cpu_stopper_thread) from [<c015edc4>] (smpboot_thread_fn+0x174/0x31c)
 [<c015edc4>] (smpboot_thread_fn) from [<c015a988>] (kthread+0xf0/0x108)
 [<c015a988>] (kthread) from [<c0108818>] (ret_from_fork+0x14/0x3c)
 Freeing unused kernel memory: 1024K (c0d00000 - c0e00000)

The stop_machine() is called with cpus = NULL from fix_kernmem_perms() and
mark_rodata_ro() which means only one CPU will execute
update_sections_early() while all other CPUs will spin and wait. Hence,
it's safe to remove tasklist locking from update_sections_early(). As part
of this change also mark functions which are local to this module as
static.

Cc: Kees Cook <keescook@chromium.org>
Cc: Laura Abbott <labbott@redhat.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
As I've checked it also can be applied to LKML as is.

Changes in v2:
 - added comment to update_sections_early()

v1: https://patchwork.kernel.org/patch/9686289/

 arch/arm/mm/init.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 370581a..838f6b35 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -689,34 +689,37 @@ void set_section_perms(struct section_perm *perms, int n, bool set,
 
 }
 
+/**
+ * update_sections_early intended to be called only through stop_machine
+ * framework and executed by only one CPU while all other CPUs will spin and
+ * wait, so no locking is required in this function.
+ */
 static void update_sections_early(struct section_perm perms[], int n)
 {
 	struct task_struct *t, *s;
 
-	read_lock(&tasklist_lock);
 	for_each_process(t) {
 		if (t->flags & PF_KTHREAD)
 			continue;
 		for_each_thread(t, s)
 			set_section_perms(perms, n, true, s->mm);
 	}
-	read_unlock(&tasklist_lock);
 	set_section_perms(perms, n, true, current->active_mm);
 	set_section_perms(perms, n, true, &init_mm);
 }
 
-int __fix_kernmem_perms(void *unused)
+static int __fix_kernmem_perms(void *unused)
 {
 	update_sections_early(nx_perms, ARRAY_SIZE(nx_perms));
 	return 0;
 }
 
-void fix_kernmem_perms(void)
+static void fix_kernmem_perms(void)
 {
 	stop_machine(__fix_kernmem_perms, NULL, NULL);
 }
 
-int __mark_rodata_ro(void *unused)
+static int __mark_rodata_ro(void *unused)
 {
 	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
 	return 0;
-- 
2.10.1

             reply	other threads:[~2017-04-19 20:13 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-19 20:10 Grygorii Strashko [this message]
2017-04-19 20:10 ` [v4.9-rt PATCH v2] ARM: mm: remove tasklist locking from update_sections_early() Grygorii Strashko
2017-04-19 20:10 ` Grygorii Strashko
2017-04-20  0:36 ` Laura Abbott
2017-04-20  0:36   ` Laura Abbott
2017-04-21 23:11   ` Kees Cook
2017-04-21 23:11     ` Kees Cook
2017-04-21 23:11     ` Kees Cook
2017-04-25 20:22     ` Grygorii Strashko
2017-04-25 20:22       ` Grygorii Strashko
2017-04-25 20:22       ` Grygorii Strashko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170419201047.31578-1-grygorii.strashko@ti.com \
    --to=grygorii.strashko@ti.com \
    --cc=bigeasy@linutronix.de \
    --cc=keescook@chromium.org \
    --cc=labbott@redhat.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rt-users@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.