Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200728202229.1195682-1-krisman@collabora.com> <20200728202229.1195682-3-krisman@collabora.com>
In-Reply-To: <20200728202229.1195682-3-krisman@collabora.com>
From:   Andy Lutomirski <luto@kernel.org>
Date:   Tue, 28 Jul 2020 20:43:27 -0700
Message-ID: <CALCETrV_HJCrDLCKLrqNbLiOsoEcC9M7zn-v_hcVMvDgnWW8yw@mail.gmail.com>
Subject: Re: [PATCH 2/6] arch: x86: Wrap TIF_IA32 checks
To:     Gabriel Krisman Bertazi <krisman@collabora.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Christoph Hellwig <hch@lst.de>
Cc:     Andrew Lutomirski <luto@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Kees Cook <keescook@chromium.org>, X86 ML <x86@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, kernel@collabora.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jul 28, 2020 at 1:22 PM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> In preparation to remove TIF_IA32, add wrapper that check the process
> has IA32 ABI without using the flag directly.

Thank you for doing this, but let's please do it right.  There is,
fundamentally, no such thing as a "process with IA32 ABI".

>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> ---
>  arch/x86/events/core.c             | 2 +-
>  arch/x86/events/intel/ds.c         | 2 +-
>  arch/x86/events/intel/lbr.c        | 2 +-
>  arch/x86/include/asm/compat.h      | 2 +-
>  arch/x86/include/asm/thread_info.h | 2 ++
>  arch/x86/kernel/perf_regs.c        | 2 +-
>  arch/x86/oprofile/backtrace.c      | 2 +-
>  7 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 4103665c6e03..42dff74c6197 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2491,7 +2491,7 @@ perf_callchain_user32(struct pt_regs *regs, struct perf_callchain_entry_ctx *ent
>         struct stack_frame_ia32 frame;
>         const struct stack_frame_ia32 __user *fp;
>
> -       if (!test_thread_flag(TIF_IA32))
> +       if (!TASK_IA32(current))
>                 return 0;

if (user_64bit_mode(regs))
  return 0;

>
>         cs_base = get_segment_base(regs->cs);
> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index dc43cc124e09..27d1cc1f3d05 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -1261,7 +1261,7 @@ static int intel_pmu_pebs_fixup_ip(struct pt_regs *regs)
>                 old_to = to;
>
>  #ifdef CONFIG_X86_64
> -               is_64bit = kernel_ip(to) || !test_thread_flag(TIF_IA32);
> +               is_64bit = kernel_ip(to) || !TASK_IA32(current);

PeterZ, does PEBS not give us a CPL?  Is it really just IP?

Anyway, this should probably be:

is_64bit = kernel_ip(to) || user_64bit_mode(regs) || !user_mode(regs);


>  #ifdef CONFIG_X86_64
> -       is64 = kernel_ip((unsigned long)addr) || !test_thread_flag(TIF_IA32);
> +       is64 = kernel_ip((unsigned long)addr) || !TASK_IA32(current);

Same as above.

> diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h
> index d4edf281fff4..d39f9b3ae683 100644
> --- a/arch/x86/include/asm/compat.h
> +++ b/arch/x86/include/asm/compat.h
> @@ -181,7 +181,7 @@ static inline void __user *arch_compat_alloc_user_space(long len)
>  {
>         compat_uptr_t sp;
>
> -       if (test_thread_flag(TIF_IA32)) {
> +       if (TASK_IA32(current)) {
>                 sp = task_pt_regs(current)->sp;

Christoph, you spend a *lot* more time looking at this stuff lately
than I do, but this looks totally wrong.  Shouldn't this be either:

sp = task_pt_regs(current)->sp;

/* This might be a compat syscall issued via int $0x80 from 64-bit-ABI code. */
if (user_64bit_mode(task_pt_regs(current))
  sp -= 128;

Or perhaps the same thing without the user_64bit_mode() check at all?
There shouldn't be much if any harm done by respecting the redzone
unnecessarily.

> --- a/arch/x86/kernel/perf_regs.c
> +++ b/arch/x86/kernel/perf_regs.c
> @@ -123,7 +123,7 @@ int perf_reg_validate(u64 mask)
>
>  u64 perf_reg_abi(struct task_struct *task)
>  {
> -       if (test_tsk_thread_flag(task, TIF_IA32))
> +       if (TASK_IA32(task))
>                 return PERF_SAMPLE_REGS_ABI_32;
>         else
>                 return PERF_SAMPLE_REGS_ABI_64;

Surely this should be:

if (user_64bit_mode(task_pt_regs(regs))
  return PERF_SAMPLE_REGS_ABI_64;
else
  return PERF_SAMPLE_REGS_ABI_32;

> diff --git a/arch/x86/oprofile/backtrace.c b/arch/x86/oprofile/backtrace.c
> index a2488b6e27d6..3f1086afa297 100644
> --- a/arch/x86/oprofile/backtrace.c
> +++ b/arch/x86/oprofile/backtrace.c
> @@ -49,7 +49,7 @@ x86_backtrace_32(struct pt_regs * const regs, unsigned int depth)
>         struct stack_frame_ia32 *head;
>
>         /* User process is IA32 */
> -       if (!current || !test_thread_flag(TIF_IA32))
> +       if (!current || !TASK_IA32(current))
>                 return 0;

if (user_64bit_mode(regs))
  return 0;


And now you don't need the TASK_IA32 macro :)

All of the above being said, I'm wondering how many of these profiling
users remember to check whether the task is a kernel thread.  And I
have no idea what task_pt_regs(current) contains in a kernel thread.

--Andy