Re: [PATCH 2/3] ARM: Move thread_info into task_struct (v7 only)

[Ard Biesheuvel]

On Mon, 6 Sept 2021 at 08:14, Keith Packard <keithp@xxxxxxxxxx> wrote:
>
> Ard Biesheuvel <ardb@xxxxxxxxxx> writes:
>
> > c13 is not a register, it is a value in one of the dimensions of the
> > ARM sysreg space, and probably covers other system registers that
> > pre-v7 cores do implement.
>
> > Better to use its architectural name (TPIDRPRW) and clarify that
> > pre-v6k/v7 cores simply don't implement it.
>
> Thanks, I'll reword the commit message
>
> > Could we split this up?
>
> I could split up the assembler macro changes which add a temporary
> register to the calls getting the current thread_info/task_struct value?
> If that would help get this reviewed, I'd be happy to do
> that. Otherwise, it's pretty much all-or-nothing as enabling
> THREAD_INFO_IN_TASK requires a bunch of synchronized changes.
>

Sure, so it is precisely for that reason that it is better to isolate
changes that can be isolated.

...
> >> +DECLARE_PER_CPU(struct task_struct *, current_task);
> >> +
> >> +static __always_inline struct task_struct *get_current(void)
> >> +{
> >> +       return raw_cpu_read(current_task);
> >
> > This needs to run with preemption disabled, or we might get migrated
> > to another CPU halfway through, and produce the wrong result (i.e.,
> > the current task of the CPU we migrated from). However, it appears
> > that manipulating the preempt count will create a circular dependency
> > here.
>
> Yeah, I really don't understand the restrictions of this API well. Any
> code fetching the current task pointer better not be subject to
> preemption or that value will be stale at some point after it was
> computed. Do you know if it could ever be run in a context allowing
> preemption?

All the time. 'current' essentially never changes value from the POV
of code running in task context, so there is usually no reason to care
about preemption/migration when referring to it. Using per-CPU
variables is what creates the problem here.

> >
> > Instead of using a per-CPU variable for current, I think it might be
> > better to borrow another system register (TPIDRURO or TPIDRURW) to
> > carry 'current' when THREAD_INFO_IN_TASK is in effect, similar to how
> > arm64 uses SP_EL0 - those registers could be preserved/restored in the
> > entry/exit from/to user space paths rather than in the context switch
> > path, and then be used any way we like while running in the kernel.
>
> Making sure these values don't leak through to user space somehow? I'm
> not excited by that prospect at all.

Moving the code that pokes the right user space value into these
registers from the context switch patch to the user space exit path
should be rather straight-forward, so I am not too concerned about
security issues here (famous last words)

> But, perhaps someone can help me
> understand the conditions under which the current task value can be
> computed where preemption was enabled and have that not be a problem for
> the enclosing code?
>

In principle, it should be sufficient to run the per-CPU variable load
sequence with preemption disabled. For instance, your asm version

       movw    \dst, #:lower16:\sym
       movt    \dst, #:upper16:\sym
       this_cpu_offset \tmp
       ldr     \dst, [\dst, \tmp]

must not be preempted and migrated right before the ldr instruction,
because that would end up accessing another CPU's value for 'current'.

However, the preempt_count itself is stored in thread_info as well, so
I'm not sure there is a way we can safely disable preemption for this
sequence to begin with, unless we run the above with interrupts
disabled.

Given that we are already relying on the MP extensions for this
anyway, I personally think that using another thread ID register to
carry 'current' is a reasonable approach as well, since it would also
allow us to get per-task stack protector support into the compiler.
But I would like to hear from some other folks on cc as well.