Re: [PATCH v5 24/25] ptrace: add PTRACE_GET_SYSCALL_INFO request

From: Andy Lutomirski <luto@kernel.org>
To: "Dmitry V. Levin" <ldv@altlinux.org>
Cc: paul.burton@mips.com, Ralf Baechle <ralf@linux-mips.org>,
	jhogan@kernel.org, Oleg Nesterov <oleg@redhat.com>,
	Andrew Lutomirski <luto@kernel.org>,
	Elvira Khabirova <lineprinter@altlinux.org>,
	Eugene Syromiatnikov <esyr@redhat.com>,
	Kees Cook <keescook@chromium.org>, Jann Horn <jannh@google.com>,
	Linux API <linux-api@vger.kernel.org>,
	strace-devel@lists.strace.io, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 24/25] ptrace: add PTRACE_GET_SYSCALL_INFO request
Date: Mon, 10 Dec 2018 11:38:17 -0800	[thread overview]
Message-ID: <CALCETrV4J8+_gSHONAcQHt6XFgtuW487b9DHZ8MV-xa6XhZ_Og@mail.gmail.com> (raw)
In-Reply-To: <20181210160940.GF14149@altlinux.org>

> On Dec 10, 2018, at 8:09 AM, Dmitry V. Levin <ldv@altlinux.org> wrote:
>
> Hi, things are getting too complicated and we need some advice how to deal
> with this frame_pointer issue.
>
>> On Mon, Dec 10, 2018 at 10:26:50PM +0800, kbuild test robot wrote:
>> Hi Elvira,
>>
>> Thank you for the patch! Yet something to improve:
>>
>> [auto build test ERROR on linus/master]
>> [also build test ERROR on v4.20-rc6]
>> [cannot apply to next-20181207]
>> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>>
>> url:    https://github.com/0day-ci/linux/commits/Dmitry-V-Levin/ptrace-add-PTRACE_GET_SYSCALL_INFO-request/20181210-174745
>> config: mips-malta_kvm_defconfig (attached as .config)
>> compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
>> reproduce:
>>        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>>        chmod +x ~/bin/make.cross
>>        # save the attached .config to linux build tree
>>        GCC_VERSION=7.2.0 make.cross ARCH=mips
>>
>> All errors (new ones prefixed by >>):
>>
>>   kernel/ptrace.c: In function 'ptrace_get_syscall_info':
>>>> kernel/ptrace.c:942:20: error: implicit declaration of function 'frame_pointer'; did you mean 'trace_printk'? [-Werror=implicit-function-declaration]
>>      .frame_pointer = frame_pointer(regs)
>>                       ^~~~~~~~~~~~~
>>                       trace_printk
>>   cc1: some warnings being treated as errors
>>
>> vim +942 kernel/ptrace.c
>>
>>   931
>>   932    static int
>>   933    ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
>>   934                void __user *datavp)
>>   935    {
>>   936        struct pt_regs *regs = task_pt_regs(child);
>>   937        struct ptrace_syscall_info info = {
>>   938            .op = PTRACE_SYSCALL_INFO_NONE,
>>   939            .arch = syscall_get_arch(child),
>>   940            .instruction_pointer = instruction_pointer(regs),
>>   941            .stack_pointer = user_stack_pointer(regs),
>>> 942            .frame_pointer = frame_pointer(regs)
>>   943        };
>>   944        unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
>>   945        unsigned long write_size;
>>   946
>>   947        /*
>>   948         * This does not need lock_task_sighand() to access
>>   949         * child->last_siginfo because ptrace_freeze_traced()
>>   950         * called earlier by ptrace_check_attach() ensures that
>>   951         * the tracee cannot go away and clear its last_siginfo.
>>   952         */
>>   953        switch (child->last_siginfo ? child->last_siginfo->si_code : 0) {
>>   954        case SIGTRAP | 0x80:
>>   955            switch (child->ptrace_message) {
>>   956            case PTRACE_EVENTMSG_SYSCALL_ENTRY:
>>   957                actual_size = ptrace_get_syscall_info_entry(child, regs,
>>   958                                        &info);
>>   959                break;
>>   960            case PTRACE_EVENTMSG_SYSCALL_EXIT:
>>   961                actual_size = ptrace_get_syscall_info_exit(child, regs,
>>   962                                       &info);
>>   963                break;
>>   964            }
>>   965            break;
>>   966        case SIGTRAP | (PTRACE_EVENT_SECCOMP << 8):
>>   967            actual_size = ptrace_get_syscall_info_seccomp(child, regs,
>>   968                                      &info);
>>   969            break;
>>   970        }
>>   971
>>   972        write_size = min(actual_size, user_size);
>>   973        return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
>>   974    }
>>   975
>
> We decided to add .frame_pointer to struct ptrace_syscall_info just for
> consistency with .instruction_pointer and .stack_pointer; I must have been
> misled by comments in asm-generic/ptrace.h into thinking that
> frame_pointer() is universally available across architectures.
>
> Unlike .instruction_pointer and .stack_pointer that are actually needed
> in strace, .frame_pointer is not used, so from strace PoV we don't really
> need it.
>
> So the question is, does anybody need a
> struct ptrace_syscall_info.frame_pointer?
>
> If yes, how can frame_pointer() be defined on MIPS?
> Or should we just forget about making sense of frame_pointer() and remove
> struct ptrace_syscall_info.frame_pointer from the proposed API?
>

I would suggest getting rid of frame_pointer. Anyone who needs that
degree of debugging can use existing ptrace APIs for it.

>
> --
> ldv