On Sun, Mar 08, 2020 at 03:55:24AM +0000, Alex Belits wrote:
> From: Yuri Norov <ynorov@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> [abelits@xxxxxxxxxxx: updated to prevent race with isolation breaking]
> Signed-off-by: Alex Belits <abelits@xxxxxxxxxxx>
> ---
> 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é
