On Tue, Jul 28, 2020 at 1:22 PM Gabriel Krisman Bertazi <kris...@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 <kris...@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