Re: [EXT] Re: [PATCH v5 9/9] task_isolation: kick_all_cpus_sync: don't kick isolated cpus
On Tue, Nov 24, 2020 at 12:21:06AM +0100, Frederic Weisbecker wrote:
> On Mon, Nov 23, 2020 at 10:39:34PM +, Alex Belits wrote:
> >
> > On Mon, 2020-11-23 at 23:29 +0100, Frederic Weisbecker wrote:
> > > External Email
> > >
> > > ---
> > > ---
> > > On Mon, Nov 23, 2020 at 05:58:42PM +, Alex Belits wrote:
> > > > From: Yuri Norov
> > > >
> > > > Make sure that kick_all_cpus_sync() does not call CPUs that are
> > > > running
> > > > isolated tasks.
> > > >
> > > > Signed-off-by: Yuri Norov
> > > > [abel...@marvell.com: use safe task_isolation_cpumask()
> > > > implementation]
> > > > Signed-off-by: Alex Belits
> > > > ---
> > > > kernel/smp.c | 14 +-
> > > > 1 file changed, 13 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/kernel/smp.c b/kernel/smp.c
> > > > index 4d17501433be..b2faecf58ed0 100644
> > > > --- a/kernel/smp.c
> > > > +++ b/kernel/smp.c
> > > > @@ -932,9 +932,21 @@ static void do_nothing(void *unused)
> > > > */
> > > > void kick_all_cpus_sync(void)
> > > > {
> > > > + struct cpumask mask;
> > > > +
> > > > /* Make sure the change is visible before we kick the cpus */
> > > > smp_mb();
> > > > - smp_call_function(do_nothing, NULL, 1);
> > > > +
> > > > + preempt_disable();
> > > > +#ifdef CONFIG_TASK_ISOLATION
> > > > + cpumask_clear(&mask);
> > > > + task_isolation_cpumask(&mask);
> > > > + cpumask_complement(&mask, &mask);
> > > > +#else
> > > > + cpumask_setall(&mask);
> > > > +#endif
> > > > + smp_call_function_many(&mask, do_nothing, NULL, 1);
> > > > + preempt_enable();
> > >
> > > Same comment about IPIs here.
> >
> > This is different from timers. The original design was based on the
> > idea that every CPU should be able to enter kernel at any time and run
> > kernel code with no additional preparation. Then the only solution is
> > to always do full broadcast and require all CPUs to process it.
> >
> > What I am trying to introduce is the idea of CPU that is not likely to
> > run kernel code any soon, and can afford to go through an additional
> > synchronization procedure on the next entry into kernel. The
> > synchronization is not skipped, it simply happens later, early in
> > kernel entry code.
Perhaps a bitmask of pending flushes makes more sense?
static_key_enable IPIs is one of the users, but for its case it would
be necessary to differentiate between in-kernel mode and out of kernel
mode atomically (since i-cache flush must be performed if isolated CPU
is in kernel mode).
> Ah I see, this is ordered that way:
>
> ll_isol_flags = ISOLATED
>
> CPU 0CPU 1
> -- -
> // kernel entry
> data_to_sync = 1ll_isol_flags = ISOLATED_BROKEN
> smp_mb()smp_mb()
> if ll_isol_flags(CPU 1) == ISOLATED READ data_to_sync
> smp_call(CPU 1)
Since isolated mode with syscalls is a desired feature, having a
separate atomic with in_kernel_mode = 0/1 (that is set/cleared
on kernel entry / kernel exit, while on TIF_TASK_ISOLATION), would be
necessary (and a similar race-free logic as above).
> You should document that, ie: explain why what you're doing is safe.
>
> Also Beware though that the data to sync in question doesn't need to be
> visible
> in the entry code before task_isolation_kernel_enter(). You need to audit all
> the callers of kick_all_cpus_sync().
Cscope tag: flush_icache_range
# line filename / context / line
1 96 arch/arc/kernel/jump_label.c <>
flush_icache_range(entry->code, entry->code + JUMP_LABEL_NOP_SIZE);
This case would be OK for delayed processing before kernel entry, as long as
no code before task_isolation_kernel_enter can be modified (which i am
not sure about).
But:
36 28 arch/ia64/include/asm/cacheflush.h <>
flush_icache_range(_addr, _addr + (len)); \
Is less certain.
Alex do you recall if arch_jump_label_transform was the only offender or
there were others as well? (suppose handling only the ones which matter
in production at the moment, and later fixing individual ones makes most
sense).
Re: [EXT] Re: [PATCH v5 9/9] task_isolation: kick_all_cpus_sync: don't kick isolated cpus
On Tue, 2020-11-24 at 00:21 +0100, Frederic Weisbecker wrote:
> On Mon, Nov 23, 2020 at 10:39:34PM +, Alex Belits wrote:
> >
> > This is different from timers. The original design was based on the
> > idea that every CPU should be able to enter kernel at any time and
> > run
> > kernel code with no additional preparation. Then the only solution
> > is
> > to always do full broadcast and require all CPUs to process it.
> >
> > What I am trying to introduce is the idea of CPU that is not likely
> > to
> > run kernel code any soon, and can afford to go through an
> > additional
> > synchronization procedure on the next entry into kernel. The
> > synchronization is not skipped, it simply happens later, early in
> > kernel entry code.
>
> Ah I see, this is ordered that way:
>
> ll_isol_flags = ISOLATED
>
> CPU 0CPU 1
> -- -
> // kernel entry
> data_to_sync = 1ll_isol_flags = ISOLATED_BROKEN
> smp_mb()smp_mb()
> if ll_isol_flags(CPU 1) == ISOLATED READ data_to_sync
> smp_call(CPU 1)
>
The check for ll_isol_flags(CPU 1) is reversed, and it's a bit more
complex. In terms of scenarios, on entry from isolation the following
can happen:
1. Kernel entry happens simultaneously with operation that requires
synchronization, kernel entry processing happens before the check for
isolation on the sender side:
ll_isol_flags(CPU 1) = ISOLATED
CPU 0CPU 1
-- -
// kernel entry
if (ll_isol_flags == ISOLATED) {
ll_isol_flags =
ISOLATED_BROKEN
data_to_sync = 1 smp_mb()
// data_to_sync undetermined
smp_mb()}
// ll_isol_flags(CPU 1) updated
if ll_isol_flags(CPU 1) != ISOLATED
// interrupts enabled
smp_call(CPU 1) // kernel entry again
if (ll_isol_flags == ISOLATED)
// nothing happens
// explicit or implied
barriers
// data_to_sync updated
// kernel exit
// CPU 0 assumes, CPU 1 will seeREAD data_to_sync
// data_to_sync = 1 when in kernel
2. Kernel entry happens simultaneously with operation that requires
synchronization, kernel entry processing happens after the check for
isolation on the sender side:
ll_isol_flags(CPU 1) = ISOLATED
CPU 0CPU 1
-- -
data_to_sync = 1// kernel entry
smp_mb()// data_to_sync undetermined
// should not access data_to_sync
here
if (ll_isol_flags == ISOLATED) {
ll_isol_flags =
ISOLATED_BROKEN
// ll_isol_flags(CPU 1) undetermined smp_mb()
// data_to_sync updated
if ll_isol_flags(CPU 1) != ISOLATED }
// possibly nothing happens
// CPU 0 assumes, CPU 1 will seeREAD data_to_sync
// data_to_sync = 1 when in kernel
3. Kernel entry processing completed before the check for isolation on the
sender
side:
ll_isol_flags(CPU 1) = ISOLATED
CPU 0CPU 1
-- -
// kernel entry
if (ll_isol_flags == ISOLATED) {
ll_isol_flags =
ISOLATED_BROKEN
smp_mb()
}
// interrupts are enabled at some
data_to_sync = 1// point here, data_to_sync value
smp_mb()// is undetermined, CPU 0 makes no
// ll_isol_flags(CPU 1) updated // assumptions about it
if ll_isol_flags(CPU 1) != ISOLATED //
smp_call(CPU 1) // kernel entry again
if (ll_isol_flags == ISOLATED)
// nothing happens
Re: [EXT] Re: [PATCH v5 9/9] task_isolation: kick_all_cpus_sync: don't kick isolated cpus
On Mon, Nov 23, 2020 at 10:39:34PM +, Alex Belits wrote:
>
> On Mon, 2020-11-23 at 23:29 +0100, Frederic Weisbecker wrote:
> > External Email
> >
> > ---
> > ---
> > On Mon, Nov 23, 2020 at 05:58:42PM +, Alex Belits wrote:
> > > From: Yuri Norov
> > >
> > > Make sure that kick_all_cpus_sync() does not call CPUs that are
> > > running
> > > isolated tasks.
> > >
> > > Signed-off-by: Yuri Norov
> > > [abel...@marvell.com: use safe task_isolation_cpumask()
> > > implementation]
> > > Signed-off-by: Alex Belits
> > > ---
> > > kernel/smp.c | 14 +-
> > > 1 file changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/smp.c b/kernel/smp.c
> > > index 4d17501433be..b2faecf58ed0 100644
> > > --- a/kernel/smp.c
> > > +++ b/kernel/smp.c
> > > @@ -932,9 +932,21 @@ static void do_nothing(void *unused)
> > > */
> > > void kick_all_cpus_sync(void)
> > > {
> > > + struct cpumask mask;
> > > +
> > > /* Make sure the change is visible before we kick the cpus */
> > > smp_mb();
> > > - smp_call_function(do_nothing, NULL, 1);
> > > +
> > > + preempt_disable();
> > > +#ifdef CONFIG_TASK_ISOLATION
> > > + cpumask_clear(&mask);
> > > + task_isolation_cpumask(&mask);
> > > + cpumask_complement(&mask, &mask);
> > > +#else
> > > + cpumask_setall(&mask);
> > > +#endif
> > > + smp_call_function_many(&mask, do_nothing, NULL, 1);
> > > + preempt_enable();
> >
> > Same comment about IPIs here.
>
> This is different from timers. The original design was based on the
> idea that every CPU should be able to enter kernel at any time and run
> kernel code with no additional preparation. Then the only solution is
> to always do full broadcast and require all CPUs to process it.
>
> What I am trying to introduce is the idea of CPU that is not likely to
> run kernel code any soon, and can afford to go through an additional
> synchronization procedure on the next entry into kernel. The
> synchronization is not skipped, it simply happens later, early in
> kernel entry code.
Ah I see, this is ordered that way:
ll_isol_flags = ISOLATED
CPU 0CPU 1
-- -
// kernel entry
data_to_sync = 1ll_isol_flags = ISOLATED_BROKEN
smp_mb()smp_mb()
if ll_isol_flags(CPU 1) == ISOLATED READ data_to_sync
smp_call(CPU 1)
You should document that, ie: explain why what you're doing is safe.
Also Beware though that the data to sync in question doesn't need to be visible
in the entry code before task_isolation_kernel_enter(). You need to audit all
the callers of kick_all_cpus_sync().
Re: [EXT] Re: [PATCH v5 9/9] task_isolation: kick_all_cpus_sync: don't kick isolated cpus
On Mon, 2020-11-23 at 23:29 +0100, Frederic Weisbecker wrote:
> External Email
>
> ---
> ---
> On Mon, Nov 23, 2020 at 05:58:42PM +, Alex Belits wrote:
> > From: Yuri Norov
> >
> > Make sure that kick_all_cpus_sync() does not call CPUs that are
> > running
> > isolated tasks.
> >
> > Signed-off-by: Yuri Norov
> > [abel...@marvell.com: use safe task_isolation_cpumask()
> > implementation]
> > Signed-off-by: Alex Belits
> > ---
> > kernel/smp.c | 14 +-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/smp.c b/kernel/smp.c
> > index 4d17501433be..b2faecf58ed0 100644
> > --- a/kernel/smp.c
> > +++ b/kernel/smp.c
> > @@ -932,9 +932,21 @@ static void do_nothing(void *unused)
> > */
> > void kick_all_cpus_sync(void)
> > {
> > + struct cpumask mask;
> > +
> > /* Make sure the change is visible before we kick the cpus */
> > smp_mb();
> > - smp_call_function(do_nothing, NULL, 1);
> > +
> > + preempt_disable();
> > +#ifdef CONFIG_TASK_ISOLATION
> > + cpumask_clear(&mask);
> > + task_isolation_cpumask(&mask);
> > + cpumask_complement(&mask, &mask);
> > +#else
> > + cpumask_setall(&mask);
> > +#endif
> > + smp_call_function_many(&mask, do_nothing, NULL, 1);
> > + preempt_enable();
>
> Same comment about IPIs here.
This is different from timers. The original design was based on the
idea that every CPU should be able to enter kernel at any time and run
kernel code with no additional preparation. Then the only solution is
to always do full broadcast and require all CPUs to process it.
What I am trying to introduce is the idea of CPU that is not likely to
run kernel code any soon, and can afford to go through an additional
synchronization procedure on the next entry into kernel. The
synchronization is not skipped, it simply happens later, early in
kernel entry code.
--
Alex
