From: Steven Rostedt <[email protected]>
task_current_syscall() has a single user that passes in 6 for maxargs, which
is the maximum arguments that can be used to get system calls from
syscall_get_arguments(). Instead of passing in a number of arguments to
grab, just get 6 arguments. The args argument even specifies that it's an
array of 6 items.
This will also allow changing syscall_get_arguments() to not get a variable
number of arguments, but always grab 6.
Signed-off-by: Steven Rostedt <[email protected]>
---
fs/proc/base.c | 2 +-
include/linux/ptrace.h | 4 ++--
lib/syscall.c | 22 ++++++++--------------
3 files changed, 11 insertions(+), 17 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 8e654468ab67..25cd58bd7236 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -650,7 +650,7 @@ static int proc_pid_syscall(struct seq_file *m, struct pid_namespace *ns,
if (res)
return res;
- if (task_current_syscall(task, &nr, args, 6, &sp, &pc))
+ if (task_current_syscall(task, &nr, args, &sp, &pc))
seq_puts(m, "running\n");
else if (nr < 0)
seq_printf(m, "%ld 0x%lx 0x%lx\n", nr, sp, pc);
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 504c98a278d4..8af5226d2ee6 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -403,7 +403,7 @@ static inline void user_single_step_siginfo(struct task_struct *tsk,
#endif
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);
#endif
diff --git a/lib/syscall.c b/lib/syscall.c
index 63239e097b13..cbd376c66bbc 100644
--- a/lib/syscall.c
+++ b/lib/syscall.c
@@ -4,8 +4,8 @@
#include <asm/syscall.h>
static int collect_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)
{
struct pt_regs *regs;
@@ -25,8 +25,8 @@ static int collect_syscall(struct task_struct *target, long *callno,
*pc = instruction_pointer(regs);
*callno = syscall_get_nr(target, regs);
- if (*callno != -1L && maxargs > 0)
- syscall_get_arguments(target, regs, 0, maxargs, args);
+ if (*callno != -1L)
+ syscall_get_arguments(target, regs, 0, 6, args);
put_task_stack(target);
return 0;
@@ -37,7 +37,6 @@ static int collect_syscall(struct task_struct *target, long *callno,
* @target: thread to examine
* @callno: filled with system call number or -1
* @args: filled with @maxargs system call arguments
- * @maxargs: number of elements in @args to fill
* @sp: filled with user stack pointer
* @pc: filled with user PC
*
@@ -55,21 +54,16 @@ static int collect_syscall(struct task_struct *target, long *callno,
* get() calls as long as we're sure @target won't return to user mode.
*
* Returns -%EAGAIN if @target does not remain blocked.
- *
- * Returns -%EINVAL if @maxargs is too large (maximum is six).
*/
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)
{
long state;
unsigned long ncsw;
- if (unlikely(maxargs > 6))
- return -EINVAL;
-
if (target == current)
- return collect_syscall(target, callno, args, maxargs, sp, pc);
+ return collect_syscall(target, callno, args, sp, pc);
state = target->state;
if (unlikely(!state))
@@ -77,7 +71,7 @@ int task_current_syscall(struct task_struct *target, long *callno,
ncsw = wait_task_inactive(target, state);
if (unlikely(!ncsw) ||
- unlikely(collect_syscall(target, callno, args, maxargs, sp, pc)) ||
+ unlikely(collect_syscall(target, callno, args, sp, pc)) ||
unlikely(wait_task_inactive(target, state) != ncsw))
return -EAGAIN;
--
2.9.3
On Mon, Nov 7, 2016 at 1:26 PM, Steven Rostedt <[email protected]> wrote:
> From: Steven Rostedt <[email protected]>
>
> task_current_syscall() has a single user that passes in 6 for maxargs, which
> is the maximum arguments that can be used to get system calls from
> syscall_get_arguments(). Instead of passing in a number of arguments to
> grab, just get 6 arguments. The args argument even specifies that it's an
> array of 6 items.
>
> This will also allow changing syscall_get_arguments() to not get a variable
> number of arguments, but always grab 6.
Reviewed-by: Andy Lutomirski <[email protected]>
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 <[email protected]> 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
On Tue, Nov 8, 2016 at 8:16 AM, Linus Torvalds
<[email protected]> wrote:
> 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 <[email protected]> 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.
I would propose calling this 'struct seccomp_data'.
>
> 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);
It's a bit hard to tell from seccomp.c, but x86 carefully arranges for
that code to never get run -- instead the entry code supplies a struct
seccomp_data. Other arches could follow suit for a nice speedup.
--Andy
On Tue, 8 Nov 2016 08:20:48 -0800
Andy Lutomirski <[email protected]> wrote:
> On Tue, Nov 8, 2016 at 8:16 AM, Linus Torvalds
> <[email protected]> wrote:
> > 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 <[email protected]> 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.
>
> I would propose calling this 'struct seccomp_data'.
I'm assuming you mean to use the existing seccomp_data? But isn't that
already defined as a user structure? Thus, we can't add sp and pc to it.
I can change syscall_get_arguments() to take the seccomp_data as an
input, and just fill in the arguments directly.
-- Steve
On Nov 8, 2016 11:48 AM, "Steven Rostedt" <[email protected]> wrote:
>
> On Tue, 8 Nov 2016 08:20:48 -0800
> Andy Lutomirski <[email protected]> wrote:
>
> > On Tue, Nov 8, 2016 at 8:16 AM, Linus Torvalds
> > <[email protected]> wrote:
> > > 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 <[email protected]> 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.
> >
> > I would propose calling this 'struct seccomp_data'.
>
> I'm assuming you mean to use the existing seccomp_data? But isn't that
> already defined as a user structure? Thus, we can't add sp and pc to it.
pc is there. sp isn't, but that could be separate. Or you could
embed seccomp_data in a bigger structure.
On Tue, 8 Nov 2016 13:06:35 -0800
Andy Lutomirski <[email protected]> wrote:
> Or you could
> embed seccomp_data in a bigger structure.
I was thinking about doing just that.
But for now, I may just use seccomp_data and when we want to add more,
we can do the embedding.
-- Steve