Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753846AbcKHQQM (ORCPT ); Tue, 8 Nov 2016 11:16:12 -0500 Received: from mail-yw0-f195.google.com ([209.85.161.195]:34900 "EHLO mail-yw0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752944AbcKHQQK (ORCPT ); Tue, 8 Nov 2016 11:16:10 -0500 MIME-Version: 1.0 In-Reply-To: <20161107213233.466776454@goodmis.org> References: <20161107212634.529267342@goodmis.org> <20161107213233.466776454@goodmis.org> From: Linus Torvalds Date: Tue, 8 Nov 2016 08:16:08 -0800 X-Google-Sender-Auth: 9kDpThVktDbZUswjoM9P-ZOvgGo Message-ID: Subject: Re: [RFC][ATCH 1/3] ptrace: Remove maxargs from task_current_syscall() To: Steven Rostedt Cc: Linux Kernel Mailing List , Ingo Molnar , Andrew Morton , Andy Lutomirski , Roland McGrath , Oleg Nesterov , "linux-arch@vger.kernel.org" , Peter Zijlstra Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2338 Lines: 57 So I definitely approve of the change, but I wonder if we should go one step further: On Mon, Nov 7, 2016 at 1:26 PM, Steven Rostedt wrote: > > extern int task_current_syscall(struct task_struct *target, long *callno, > - unsigned long args[6], unsigned int maxargs, > - unsigned long *sp, unsigned long *pc); > + unsigned long args[6], unsigned long *sp, > + unsigned long *pc); The thing is, in C, having an array in a function declaration is pretty much exactly the same as just having a pointer, so from a type checking standpoint it doesn't really help all that much (but from a "human documentation" side the "args[6]" is much better than "*args"). However, what would really help type checking is making it a structure. And maybe that structure could just contain "callno", "sp" and "pc" too? That would not only fix the type checking, it would make the calling convention even cleaner. Just have one single structure that contains all the relevant data. That would also allow us (later - don't do it now) to replace the odd collection of "get registers one by one" with a single architecture-specific routine that fills it all in.Right now we do *sp = user_stack_pointer(regs); *pc = instruction_pointer(regs); *callno = syscall_get_nr(target, regs); if (*callno != -1L && maxargs > 0) syscall_get_arguments(target, regs, 0, maxargs, args); and it feels like this could/should just be a single "syscall_get_info()" helper. For example, kernel/seccomp.c does this instead: sd->nr = syscall_get_nr(task, regs); sd->arch = syscall_get_arch(); syscall_get_arguments(task, regs, 0, 6, args); sd->args[0] = args[0]; sd->args[1] = args[1]; sd->args[2] = args[2]; sd->args[3] = args[3]; sd->args[4] = args[4]; sd->args[5] = args[5]; sd->instruction_pointer = KSTK_EIP(task); and notice how it wants "pc" too, but it used a completely different way to get them? So the ad-hoc nature of the current interfaces really does shine through here (ok, so seccomp doesn't need the user stack pointer, but it really won't hurt there either. Hmm? Linus