linux-kernel - Re: [PATCH 2/3] ARM: Move thread_info into task

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 05 Sep 2021 23:14:09 -0700
From:   Keith Packard <keithp@...thp.com>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Abbott Liu <liuwenliang@...wei.com>,
        Alexander Sverdlin <alexander.sverdlin@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Anshuman Khandual <anshuman.khandual@....com>,
        Arnd Bergmann <arnd@...db.de>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Hartley Sweeten <hsweeten@...ionengravers.com>,
        Jens Axboe <axboe@...nel.dk>, Jian Cai <jiancai@...gle.com>,
        Joe Perches <joe@...ches.com>,
        Kees Cook <keescook@...omium.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Manivannan Sadhasivam <mani@...nel.org>,
        Marc Zyngier <maz@...nel.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Miguel Ojeda <ojeda@...nel.org>,
        Mike Rapoport <rppt@...nel.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Nicolas Pitre <nico@...xnic.net>,
        Rob Herring <robh@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Thomas Gleixner <tglx@...utronix.de>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Valentin Schneider <valentin.schneider@....com>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        "Wolfram Sang (Renesas)" <wsa+renesas@...g-engineering.com>,
        YiFei Zhu <yifeifz2@...inois.edu>
Subject: Re: [PATCH 2/3] ARM: Move thread_info into task_struct (v7 only)

Ard Biesheuvel <ardb@...nel.org> 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.

>> +#ifdef CONFIG_THREAD_INFO_IN_TASK
>
> No need for this #ifdef - it only guards macros that will have no
> effect if they are never instantiated (another case below)

Sure, the resulting code is a bit less noisy, which seems good.

>> +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?
>
> 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. 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?

-- 
-keith

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)