From: Kees Cook <keescook@chromium.org>
To: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: Russell King <linux@armlinux.org.uk>,
Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
linux-rt-users@vger.kernel.org,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
LKML <linux-kernel@vger.kernel.org>,
Laura Abbott <labbott@redhat.com>
Subject: Re: [v4.9-rt PATCH] ARM: mm: remove tasklist locking from update_sections_early()
Date: Wed, 19 Apr 2017 13:01:34 -0700 [thread overview]
Message-ID: <CAGXu5jJmTN8eseFm50uu7cCJWkAkk8iegsvFuhv0gKPZmkMY+Q@mail.gmail.com> (raw)
In-Reply-To: <05456050-7dc2-d58e-5250-bbb369d5d927@ti.com>
On Wed, Apr 19, 2017 at 12:58 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
>
>
> On 04/18/2017 07:15 PM, Kees Cook wrote:
>> On Tue, Apr 18, 2017 at 1:48 PM, Grygorii Strashko
>> <grygorii.strashko@ti.com> wrote:
>>> 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
>>> to simplify code analize in the future.
>>
>> Hm, yes, good point. It's only every called while other CPUs are stopped.
>>
>>>
>>> Cc: Kees Cook <keescook@chromium.org>
>>> Cc: Laura Abbott <labbott@redhat.com>
>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>>> ---
>>> arch/arm/mm/init.c | 8 +++-----
>>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
>>> index 370581a..a77953a 100644
>>> --- a/arch/arm/mm/init.c
>>> +++ b/arch/arm/mm/init.c
>
> /**
> * update_sections_early intended to be called only through stop_machine
> * framework and be executed by only one CPU while all other CPUs will spin and
> * wait, so no locking is required in this function.
> */
>>> @@ -693,30 +693,28 @@ static void update_sections_early(struct section_perm perms[], int n)
>>
>> Maybe this should be renamed update_sections_stopped()? Or at least
>> comments added to help see why it's safe.
>
> would it be ok if I add above comment before update_sections_early?
A comment is fine. It'll just help people looking at this in the future. :)
Thanks!
-Kees
--
Kees Cook
Pixel Security
prev parent reply other threads:[~2017-04-19 20:01 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-18 20:48 [v4.9-rt PATCH] ARM: mm: remove tasklist locking from update_sections_early() Grygorii Strashko
2017-04-19 0:15 ` Kees Cook
2017-04-19 19:58 ` Grygorii Strashko
2017-04-19 20:01 ` Kees Cook [this message]
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=CAGXu5jJmTN8eseFm50uu7cCJWkAkk8iegsvFuhv0gKPZmkMY+Q@mail.gmail.com \
--to=keescook@chromium.org \
--cc=bigeasy@linutronix.de \
--cc=grygorii.strashko@ti.com \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).