| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200406042709.e4isjrvrrwsusjc4@x1>
Date: Mon, 6 Apr 2020 00:27:09 -0400
From: Kevyn-Alexandre Paré <kapare@...teo.net>
To: Alex Belits <abelits@...vell.com>
Cc: "frederic@...nel.org" <frederic@...nel.org>,
"rostedt@...dmis.org" <rostedt@...dmis.org>,
"mingo@...nel.org" <mingo@...nel.org>,
"peterz@...radead.org" <peterz@...radead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Prasun Kapoor <pkapoor@...vell.com>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
"catalin.marinas@....com" <catalin.marinas@....com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
"will@...nel.org" <will@...nel.org>
Subject: Re: [PATCH v2 10/12] task_isolation: ringbuffer: don't interrupt
CPUs running isolated tasks on buffer resize
On Sun, Mar 08, 2020 at 03:55:24AM +0000, Alex Belits wrote:
> From: Yuri Norov <ynorov@...vell.com>
>
> CPUs running isolated tasks are in userspace, so they don't have to
> perform ring buffer updates immediately. If ring_buffer_resize()
> schedules the update on those CPUs, isolation is broken. To prevent
> that, updates for CPUs running isolated tasks are performed locally,
> like for offline CPUs.
>
> A race condition between this update and isolation breaking is avoided
> at the cost of disabling per_cpu buffer writing for the time of update
> when it coincides with isolation breaking.
>
> Signed-off-by: Yuri Norov <ynorov@...vell.com>
> [abelits@...vell.com: updated to prevent race with isolation breaking]
> Signed-off-by: Alex Belits <abelits@...vell.com>
> ---
> kernel/trace/ring_buffer.c | 62 ++++++++++++++++++++++++++++++++++----
> 1 file changed, 56 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 61f0e92ace99..593effe40183 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -21,6 +21,7 @@
> #include <linux/delay.h>
> #include <linux/slab.h>
> #include <linux/init.h>
> +#include <linux/isolation.h>
> #include <linux/hash.h>
> #include <linux/list.h>
> #include <linux/cpu.h>
> @@ -1701,6 +1702,37 @@ static void update_pages_handler(struct work_struct *work)
> complete(&cpu_buffer->update_done);
> }
>
> +static bool update_if_isolated(struct ring_buffer_per_cpu *cpu_buffer,
> + int cpu)
> +{
> + bool rv = false;
> +
> + if (task_isolation_on_cpu(cpu)) {
> + /*
> + * CPU is running isolated task. Since it may lose
> + * isolation and re-enter kernel simultaneously with
> + * this update, disable recording until it's done.
> + */
> + atomic_inc(&cpu_buffer->record_disabled);
> + /* Make sure, update is done, and isolation state is current */
> + smp_mb();
> + if (task_isolation_on_cpu(cpu)) {
> + /*
> + * If CPU is still running isolated task, we
> + * can be sure that breaking isolation will
> + * happen while recording is disabled, and CPU
> + * will not touch this buffer until the update
> + * is done.
> + */
> + rb_update_pages(cpu_buffer);
> + cpu_buffer->nr_pages_to_update = 0;
> + rv = true;
> + }
> + atomic_dec(&cpu_buffer->record_disabled);
> + }
> + return rv;
> +}
> +
> /**
> * ring_buffer_resize - resize the ring buffer
> * @buffer: the buffer to resize.
> @@ -1784,13 +1816,22 @@ int ring_buffer_resize(struct trace_buffer *buffer, unsigned long size,
> if (!cpu_buffer->nr_pages_to_update)
> continue;
>
> - /* Can't run something on an offline CPU. */
> + /*
> + * Can't run something on an offline CPU.
> + *
> + * CPUs running isolated tasks don't have to
> + * update ring buffers until they exit
> + * isolation because they are in
> + * userspace. Use the procedure that prevents
> + * race condition with isolation breaking.
> + */
> if (!cpu_online(cpu)) {
> rb_update_pages(cpu_buffer);
> cpu_buffer->nr_pages_to_update = 0;
> } else {
> - schedule_work_on(cpu,
> - &cpu_buffer->update_pages_work);
> + if (!update_if_isolated(cpu_buffer, cpu))
> + schedule_work_on(cpu,
> + &cpu_buffer->update_pages_work);
> }
> }
>
> @@ -1829,13 +1870,22 @@ int ring_buffer_resize(struct trace_buffer *buffer, unsigned long size,
>
> get_online_cpus();
>
> - /* Can't run something on an offline CPU. */
> + /*
> + * Can't run something on an offline CPU.
> + *
> + * CPUs running isolated tasks don't have to update
> + * ring buffers until they exit isolation because they
> + * are in userspace. Use the procedure that prevents
> + * race condition with isolation breaking.
> + */
> if (!cpu_online(cpu_id))
> rb_update_pages(cpu_buffer);
> else {
> - schedule_work_on(cpu_id,
> + if (!update_if_isolated(cpu_buffer, cpu_id))
> + schedule_work_on(cpu_id,
> &cpu_buffer->update_pages_work);
> - wait_for_completion(&cpu_buffer->update_done);
> + wait_for_completion(&cpu_buffer->update_done);
> + }
> }
>
> cpu_buffer->nr_pages_to_update = 0;
gcc output:
kernel/trace/ring_buffer.c: In function 'ring_buffer_resize':
kernel/trace/ring_buffer.c:1884:4: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
if (!update_if_isolated(cpu_buffer, cpu_id))
^~
kernel/trace/ring_buffer.c:1887:5: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
wait_for_completion(&cpu_buffer->update_done);
^~~~~~~~~~~~~~~~~~~
kernel/trace/ring_buffer.c:1858:4: error: label 'out' used but not defined
goto out;
^~~~
kernel/trace/ring_buffer.c:1868:4: error: label 'out_err' used but not defined
goto out_err;
^~~~
My fix:
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 593effe40183..8b458400ac31 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -1881,9 +1881,8 @@ int ring_buffer_resize(struct trace_buffer *buffer, unsigned long size,
if (!cpu_online(cpu_id))
rb_update_pages(cpu_buffer);
else {
- if (!update_if_isolated(cpu_buffer, cpu_id))
- schedule_work_on(cpu_id,
- &cpu_buffer->update_pages_work);
+ if (!update_if_isolated(cpu_buffer, cpu_id)) {
+ schedule_work_on(cpu_id, &cpu_buffer->update_pages_work);
wait_for_completion(&cpu_buffer->update_done);
}
}
thx,
-- Kevyn-Alexandre Paré
Powered by blists - more mailing lists