Linux-m68k Archive on lore.kernel.org
 help / color / Atom feed
* syscall_trace_enter and syscall_trace_leave for m68k w/MMU
@ 2020-07-23 10:03 John Paul Adrian Glaubitz
  2020-07-23 22:56 ` Michael Schmitz
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: John Paul Adrian Glaubitz @ 2020-07-23 10:03 UTC (permalink / raw)
  To: linux-m68k; +Cc: Michael Karcher

Hello!

Michael and I have been working on SECCOMP in SuperH and successfully added
the missing pieces yesterday [1].

We're now looking into what needs to be done for m68k and I have noticed that
syscall_trace_enter and syscall_trace_leave - which needs to be touched for
SECCOMP support - are available on ColdFire and non-MMU targets for m68k.

Is there a particular reason for that? Looking at 68000/entry.S, I can see
that both functions are invoked [2] so I'm not sure why they are built
for CONFIG_COLDFIRE and !CONFIG_MMM only.

Any ideas?

Adrian

> [1] https://marc.info/?l=linux-sh&m=159546012829989&w=2
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/m68k/68000/entry.S

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: syscall_trace_enter and syscall_trace_leave for m68k w/MMU
  2020-07-23 10:03 syscall_trace_enter and syscall_trace_leave for m68k w/MMU John Paul Adrian Glaubitz
@ 2020-07-23 22:56 ` Michael Schmitz
  2020-07-25  1:48 ` [PATCH RFC] m68k/kernel - wire up syscall_trace_enter/leave for m68k Michael Schmitz
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Michael Schmitz @ 2020-07-23 22:56 UTC (permalink / raw)
  To: John Paul Adrian Glaubitz, linux-m68k; +Cc: Michael Karcher, Andreas Schwab

Adrian,

m68k/mm only has syscall_trace() (in kernel/ptrace.c) which is called 
from the system call entry in kernel/entry.S (look for do_trace_* there).

syscall_trace() is called before and after syscall execution (if the 
syscall trace flag is set). Looking at the sparc 32 and 64 bit trace 
code for comparison, you might try replacing these calls by 
syscall_trace_enter() and syscall_trace_leave(). The current code in 
syscall_trace replicates the code in ptrace_report_syscall which will be 
called from syscall_trace_enter() and syscall_trace_leave(), so things 
ought to continue to function as normal.

syscall_trace() is also called from the ret_from_signal entry - no idea 
what should happen to that one. My guess is to treat this as syscall 
trace exit.

CC to Andreas - maybe there are subtleties with strace or gdb that I'm 
missing.

Cheers,

     Michael


On 23/07/20 10:03 PM, John Paul Adrian Glaubitz wrote:
> Hello!
>
> Michael and I have been working on SECCOMP in SuperH and successfully added
> the missing pieces yesterday [1].
>
> We're now looking into what needs to be done for m68k and I have noticed that
> syscall_trace_enter and syscall_trace_leave - which needs to be touched for
> SECCOMP support - are available on ColdFire and non-MMU targets for m68k.
>
> Is there a particular reason for that? Looking at 68000/entry.S, I can see
> that both functions are invoked [2] so I'm not sure why they are built
> for CONFIG_COLDFIRE and !CONFIG_MMM only.
>
> Any ideas?
>
> Adrian
>
>> [1] https://marc.info/?l=linux-sh&m=159546012829989&w=2
>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/m68k/68000/entry.S

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH RFC] m68k/kernel - wire up syscall_trace_enter/leave for m68k
  2020-07-23 10:03 syscall_trace_enter and syscall_trace_leave for m68k w/MMU John Paul Adrian Glaubitz
  2020-07-23 22:56 ` Michael Schmitz
@ 2020-07-25  1:48 ` Michael Schmitz
  2020-07-26  1:28 ` [PATCH RFC v2] " Michael Schmitz
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Michael Schmitz @ 2020-07-25  1:48 UTC (permalink / raw)
  To: glaubitz, geert; +Cc: linux-m68k, Michael Schmitz

m68k (other than Coldfire) uses syscall_trace for both trace entry
and trace exit. Seccomp support requires separate entry points for
trace entry and exit which are already provided for Coldfire.

Replace syscall_trace by syscall_trace_enter and syscall_trace_leave
in preparation for seccomp support.

No regression seen in testing with strace on ARAnyM.

Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 arch/m68k/kernel/entry.S  |    6 +++---
 arch/m68k/kernel/ptrace.c |   17 -----------------
 2 files changed, 3 insertions(+), 20 deletions(-)

diff --git a/arch/m68k/kernel/entry.S b/arch/m68k/kernel/entry.S
index 9dd76fb..76badc4 100644
--- a/arch/m68k/kernel/entry.S
+++ b/arch/m68k/kernel/entry.S
@@ -164,7 +164,7 @@ do_trace_entry:
 	movel	#-ENOSYS,%sp@(PT_OFF_D0)| needed for strace
 	subql	#4,%sp
 	SAVE_SWITCH_STACK
-	jbsr	syscall_trace
+	jbsr	syscall_trace_enter
 	RESTORE_SWITCH_STACK
 	addql	#4,%sp
 	movel	%sp@(PT_OFF_ORIG_D0),%d0
@@ -177,7 +177,7 @@ badsys:
 do_trace_exit:
 	subql	#4,%sp
 	SAVE_SWITCH_STACK
-	jbsr	syscall_trace
+	jbsr	syscall_trace_leave
 	RESTORE_SWITCH_STACK
 	addql	#4,%sp
 	jra	.Lret_from_exception
@@ -186,7 +186,7 @@ ENTRY(ret_from_signal)
 	movel	%curptr@(TASK_STACK),%a1
 	tstb	%a1@(TINFO_FLAGS+2)
 	jge	1f
-	jbsr	syscall_trace
+	jbsr	syscall_trace_leave
 1:	RESTORE_SWITCH_STACK
 	addql	#4,%sp
 /* on 68040 complete pending writebacks if any */
diff --git a/arch/m68k/kernel/ptrace.c b/arch/m68k/kernel/ptrace.c
index 748c63b..b747a86 100644
--- a/arch/m68k/kernel/ptrace.c
+++ b/arch/m68k/kernel/ptrace.c
@@ -272,22 +272,6 @@ long arch_ptrace(struct task_struct *child, long request,
 	return -EIO;
 }
 
-asmlinkage void syscall_trace(void)
-{
-	ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD)
-				 ? 0x80 : 0));
-	/*
-	 * this isn't the same as continuing with a signal, but it will do
-	 * for normal use.  strace only continues with a signal if the
-	 * stopping signal is not SIGTRAP.  -brl
-	 */
-	if (current->exit_code) {
-		send_sig(current->exit_code, current, 1);
-		current->exit_code = 0;
-	}
-}
-
-#if defined(CONFIG_COLDFIRE) || !defined(CONFIG_MMU)
 asmlinkage int syscall_trace_enter(void)
 {
 	int ret = 0;
@@ -302,4 +286,3 @@ asmlinkage void syscall_trace_leave(void)
 	if (test_thread_flag(TIF_SYSCALL_TRACE))
 		tracehook_report_syscall_exit(task_pt_regs(current), 0);
 }
-#endif /* CONFIG_COLDFIRE */
-- 
1.7.0.4


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH RFC v2] m68k/kernel - wire up syscall_trace_enter/leave for m68k
  2020-07-23 10:03 syscall_trace_enter and syscall_trace_leave for m68k w/MMU John Paul Adrian Glaubitz
  2020-07-23 22:56 ` Michael Schmitz
  2020-07-25  1:48 ` [PATCH RFC] m68k/kernel - wire up syscall_trace_enter/leave for m68k Michael Schmitz
@ 2020-07-26  1:28 ` Michael Schmitz
  2020-07-27  4:19 ` [PATCH] " Michael Schmitz
  2020-08-05 12:23 ` syscall_trace_enter and syscall_trace_leave for m68k w/MMU Greg Ungerer
  4 siblings, 0 replies; 10+ messages in thread
From: Michael Schmitz @ 2020-07-26  1:28 UTC (permalink / raw)
  To: glaubitz, geert; +Cc: linux-m68k, schwab, Michael Schmitz

m68k (other than Coldfire) uses syscall_trace for both trace entry
and trace exit. Seccomp support requires separate entry points for
trace entry and exit which are already provided for Coldfire.

Replace syscall_trace by syscall_trace_enter and syscall_trace_leave
in preparation for seccomp support. Check return code of
syscall_trace_enter(), and skip syscall if nonzero. Return code
will be set to -EPERM in that case.

No regression seen in testing with strace on ARAnyM.

Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---

Changes from v1:

- add return code check in do_trace_entry branch to enable syscall
  filtering (will return -EPERM)
- change to use testl for return code check (suggested by Andreas Schwab)
 
---
 arch/m68k/kernel/entry.S  |   11 ++++++++---
 arch/m68k/kernel/ptrace.c |   17 -----------------
 2 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/arch/m68k/kernel/entry.S b/arch/m68k/kernel/entry.S
index 9dd76fb..fbd2222 100644
--- a/arch/m68k/kernel/entry.S
+++ b/arch/m68k/kernel/entry.S
@@ -164,20 +164,25 @@ do_trace_entry:
 	movel	#-ENOSYS,%sp@(PT_OFF_D0)| needed for strace
 	subql	#4,%sp
 	SAVE_SWITCH_STACK
-	jbsr	syscall_trace
+	jbsr	syscall_trace_enter
 	RESTORE_SWITCH_STACK
 	addql	#4,%sp
+	tstl	%d0
+	jne	denied
 	movel	%sp@(PT_OFF_ORIG_D0),%d0
 	cmpl	#NR_syscalls,%d0
 	jcs	syscall
 badsys:
 	movel	#-ENOSYS,%sp@(PT_OFF_D0)
 	jra	ret_from_syscall
+denied:
+	movel	#-EPERM,%sp@(PT_OFF_D0)
+	jra	ret_from_syscall
 
 do_trace_exit:
 	subql	#4,%sp
 	SAVE_SWITCH_STACK
-	jbsr	syscall_trace
+	jbsr	syscall_trace_leave
 	RESTORE_SWITCH_STACK
 	addql	#4,%sp
 	jra	.Lret_from_exception
@@ -186,7 +191,7 @@ ENTRY(ret_from_signal)
 	movel	%curptr@(TASK_STACK),%a1
 	tstb	%a1@(TINFO_FLAGS+2)
 	jge	1f
-	jbsr	syscall_trace
+	jbsr	syscall_trace_leave
 1:	RESTORE_SWITCH_STACK
 	addql	#4,%sp
 /* on 68040 complete pending writebacks if any */
diff --git a/arch/m68k/kernel/ptrace.c b/arch/m68k/kernel/ptrace.c
index 748c63b..b747a86 100644
--- a/arch/m68k/kernel/ptrace.c
+++ b/arch/m68k/kernel/ptrace.c
@@ -272,22 +272,6 @@ long arch_ptrace(struct task_struct *child, long request,
 	return -EIO;
 }
 
-asmlinkage void syscall_trace(void)
-{
-	ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD)
-				 ? 0x80 : 0));
-	/*
-	 * this isn't the same as continuing with a signal, but it will do
-	 * for normal use.  strace only continues with a signal if the
-	 * stopping signal is not SIGTRAP.  -brl
-	 */
-	if (current->exit_code) {
-		send_sig(current->exit_code, current, 1);
-		current->exit_code = 0;
-	}
-}
-
-#if defined(CONFIG_COLDFIRE) || !defined(CONFIG_MMU)
 asmlinkage int syscall_trace_enter(void)
 {
 	int ret = 0;
@@ -302,4 +286,3 @@ asmlinkage void syscall_trace_leave(void)
 	if (test_thread_flag(TIF_SYSCALL_TRACE))
 		tracehook_report_syscall_exit(task_pt_regs(current), 0);
 }
-#endif /* CONFIG_COLDFIRE */
-- 
1.7.0.4


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] m68k/kernel - wire up syscall_trace_enter/leave for m68k
  2020-07-23 10:03 syscall_trace_enter and syscall_trace_leave for m68k w/MMU John Paul Adrian Glaubitz
                   ` (2 preceding siblings ...)
  2020-07-26  1:28 ` [PATCH RFC v2] " Michael Schmitz
@ 2020-07-27  4:19 ` Michael Schmitz
  2020-07-27 10:03   ` John Paul Adrian Glaubitz
  2020-08-05 12:23 ` syscall_trace_enter and syscall_trace_leave for m68k w/MMU Greg Ungerer
  4 siblings, 1 reply; 10+ messages in thread
From: Michael Schmitz @ 2020-07-27  4:19 UTC (permalink / raw)
  To: glaubitz, geert; +Cc: linux-m68k, schwab, Michael Schmitz

m68k (other than Coldfire) uses syscall_trace for both trace entry
and trace exit. Seccomp support requires separate entry points for
trace entry and exit which are already provided for Coldfire.

Replace syscall_trace by syscall_trace_enter and syscall_trace_leave
in preparation for seccomp support. Check return code of
syscall_trace_enter(), and skip syscall if nonzero. Return code
will be left at what had been set by by ptrace or seccomp.

No regression seen in testing with strace on ARAnyM.

Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---

Changes from RFC v1:

- add return code check in do_trace_entry branch to enable syscall
  filtering (will return -EPERM)
- change to use testl for return code check (suggested by Andreas Schwab)

Changes from RFC v2:

- don't set return code of filtered syscall - seccomp may want to set
  that for use by calling process.

---
 arch/m68k/kernel/entry.S  |    8 +++++---
 arch/m68k/kernel/ptrace.c |   17 -----------------
 2 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/arch/m68k/kernel/entry.S b/arch/m68k/kernel/entry.S
index 9dd76fb..de3d54a 100644
--- a/arch/m68k/kernel/entry.S
+++ b/arch/m68k/kernel/entry.S
@@ -164,9 +164,11 @@ do_trace_entry:
 	movel	#-ENOSYS,%sp@(PT_OFF_D0)| needed for strace
 	subql	#4,%sp
 	SAVE_SWITCH_STACK
-	jbsr	syscall_trace
+	jbsr	syscall_trace_enter
 	RESTORE_SWITCH_STACK
 	addql	#4,%sp
+	tstl	%d0
+	jne	ret_from_syscall
 	movel	%sp@(PT_OFF_ORIG_D0),%d0
 	cmpl	#NR_syscalls,%d0
 	jcs	syscall
@@ -177,7 +179,7 @@ badsys:
 do_trace_exit:
 	subql	#4,%sp
 	SAVE_SWITCH_STACK
-	jbsr	syscall_trace
+	jbsr	syscall_trace_leave
 	RESTORE_SWITCH_STACK
 	addql	#4,%sp
 	jra	.Lret_from_exception
@@ -186,7 +188,7 @@ ENTRY(ret_from_signal)
 	movel	%curptr@(TASK_STACK),%a1
 	tstb	%a1@(TINFO_FLAGS+2)
 	jge	1f
-	jbsr	syscall_trace
+	jbsr	syscall_trace_leave
 1:	RESTORE_SWITCH_STACK
 	addql	#4,%sp
 /* on 68040 complete pending writebacks if any */
diff --git a/arch/m68k/kernel/ptrace.c b/arch/m68k/kernel/ptrace.c
index 748c63b..b747a86 100644
--- a/arch/m68k/kernel/ptrace.c
+++ b/arch/m68k/kernel/ptrace.c
@@ -272,22 +272,6 @@ long arch_ptrace(struct task_struct *child, long request,
 	return -EIO;
 }
 
-asmlinkage void syscall_trace(void)
-{
-	ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD)
-				 ? 0x80 : 0));
-	/*
-	 * this isn't the same as continuing with a signal, but it will do
-	 * for normal use.  strace only continues with a signal if the
-	 * stopping signal is not SIGTRAP.  -brl
-	 */
-	if (current->exit_code) {
-		send_sig(current->exit_code, current, 1);
-		current->exit_code = 0;
-	}
-}
-
-#if defined(CONFIG_COLDFIRE) || !defined(CONFIG_MMU)
 asmlinkage int syscall_trace_enter(void)
 {
 	int ret = 0;
@@ -302,4 +286,3 @@ asmlinkage void syscall_trace_leave(void)
 	if (test_thread_flag(TIF_SYSCALL_TRACE))
 		tracehook_report_syscall_exit(task_pt_regs(current), 0);
 }
-#endif /* CONFIG_COLDFIRE */
-- 
1.7.0.4


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] m68k/kernel - wire up syscall_trace_enter/leave for m68k
  2020-07-27  4:19 ` [PATCH] " Michael Schmitz
@ 2020-07-27 10:03   ` John Paul Adrian Glaubitz
  2020-07-27 20:48     ` Michael Schmitz
  0 siblings, 1 reply; 10+ messages in thread
From: John Paul Adrian Glaubitz @ 2020-07-27 10:03 UTC (permalink / raw)
  To: Michael Schmitz, geert; +Cc: linux-m68k, schwab

Hi Michael!

On 7/27/20 6:19 AM, Michael Schmitz wrote:
> m68k (other than Coldfire) uses syscall_trace for both trace entry
> and trace exit. Seccomp support requires separate entry points for
> trace entry and exit which are already provided for Coldfire.
> 
> Replace syscall_trace by syscall_trace_enter and syscall_trace_leave
> in preparation for seccomp support. Check return code of
> syscall_trace_enter(), and skip syscall if nonzero. Return code
> will be left at what had been set by by ptrace or seccomp.

Correct me if I'm wrong, but shouldn't the skip happen when the return
code is -1? At least that's what we're doing on SuperH and that seems
to be what other architectures are doing as well.

Also, shouldn't that part of the change not be part of the patch that
adds support for SECCOMP filter like in [1]? I don't think it makes
sense to add the return code check unless the rest of SECCOMP filter
has been implemented.

Adrian

> [1] https://github.com/glaubitz/linux/commit/2fa1e7b9ba5150bc12adaddc017d5a6b79f363e7

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] m68k/kernel - wire up syscall_trace_enter/leave for m68k
  2020-07-27 10:03   ` John Paul Adrian Glaubitz
@ 2020-07-27 20:48     ` Michael Schmitz
  2020-07-27 21:09       ` John Paul Adrian Glaubitz
  0 siblings, 1 reply; 10+ messages in thread
From: Michael Schmitz @ 2020-07-27 20:48 UTC (permalink / raw)
  To: John Paul Adrian Glaubitz, geert; +Cc: linux-m68k, schwab, Greg Ungerer

Hi Adrian,

On 27/07/20 10:03 PM, John Paul Adrian Glaubitz wrote:
> Hi Michael!
>
> On 7/27/20 6:19 AM, Michael Schmitz wrote:
>> m68k (other than Coldfire) uses syscall_trace for both trace entry
>> and trace exit. Seccomp support requires separate entry points for
>> trace entry and exit which are already provided for Coldfire.
>>
>> Replace syscall_trace by syscall_trace_enter and syscall_trace_leave
>> in preparation for seccomp support. Check return code of
>> syscall_trace_enter(), and skip syscall if nonzero. Return code
>> will be left at what had been set by by ptrace or seccomp.
> Correct me if I'm wrong, but shouldn't the skip happen when the return
> code is -1? At least that's what we're doing on SuperH and that seems
> to be what other architectures are doing as well.

What other non-zero return codes do you expect syscall_trace_enter() to 
set, and what should the action in entry.S be?

(Note that according to my reading, your SH patch does not actually do 
what your description says. If syscall_trace_enter() returns a positive 
non-zero value, that value is _not_ used as changed syscall number. SH 
uses r3 for the syscall number, not r0).

As far as I can see, any non-zero return code should abort the syscall, 
so I just test for that (for simplicity). Our use of the 
tracehook_report_syscall_entry() return code (directly passed back from 
syscall_trace_enter()) doesn't leave much choice there, see comment in 
include/linux/tracehook.h.

If later on seccomp needs any specific action, it should be easy enough 
to change the syscall number (offset PT_OFF_ORIG_D0 on the stack) or 
syscall return code (offset PT_OFF_D0). There's code in kernel/ptrace.c 
to do that AFAICS.

Changing the behaviour of syscall_trace_enter() to match what other 
architectures do (return changed syscall number, not error code) is 
beyond the scope of this patch. I suspect the capability to change 
syscall numbers from ptrace code does predate the seccomp filter 
approach, and as m68k has never used it in the past, I don't see a point 
to add this now.

> Also, shouldn't that part of the change not be part of the patch that
> adds support for SECCOMP filter like in [1]? I don't think it makes
> sense to add the return code check unless the rest of SECCOMP filter
> has been implemented.

After replacing syscall_trace() by syscall_trace_enter() and 
syscall_trace_leave(), there is a return code provided by 
syscall_trace_enter() which we must check, hence I added the check at 
the same time as replacing syscall_trace() for non-Coldfire m68k.

(I note that the same check should probably be added to coldfire/entry.S.)

I can't test any of the later seccomp related stuff, so I'd rather leave 
that bit to someone else who can.

Cheers,

     Michael

>
> Adrian
>
>> [1] https://github.com/glaubitz/linux/commit/2fa1e7b9ba5150bc12adaddc017d5a6b79f363e7

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] m68k/kernel - wire up syscall_trace_enter/leave for m68k
  2020-07-27 20:48     ` Michael Schmitz
@ 2020-07-27 21:09       ` John Paul Adrian Glaubitz
  0 siblings, 0 replies; 10+ messages in thread
From: John Paul Adrian Glaubitz @ 2020-07-27 21:09 UTC (permalink / raw)
  To: Michael Schmitz, geert; +Cc: linux-m68k, schwab, Greg Ungerer

On 7/27/20 10:48 PM, Michael Schmitz wrote:
> On 27/07/20 10:03 PM, John Paul Adrian Glaubitz wrote:
>> Hi Michael!
>>
>> On 7/27/20 6:19 AM, Michael Schmitz wrote:
>>> m68k (other than Coldfire) uses syscall_trace for both trace entry
>>> and trace exit. Seccomp support requires separate entry points for
>>> trace entry and exit which are already provided for Coldfire.
>>>
>>> Replace syscall_trace by syscall_trace_enter and syscall_trace_leave
>>> in preparation for seccomp support. Check return code of
>>> syscall_trace_enter(), and skip syscall if nonzero. Return code
>>> will be left at what had been set by by ptrace or seccomp.
>> Correct me if I'm wrong, but shouldn't the skip happen when the return
>> code is -1? At least that's what we're doing on SuperH and that seems
>> to be what other architectures are doing as well.
> 
> What other non-zero return codes do you expect syscall_trace_enter() to set, and what should the action in entry.S be?

I just don't think we should do it any different than the other architectures
which explicitly compare the return value against -1, i.e. RISC-V and PA-RISC.

> (Note that according to my reading, your SH patch does not actually do what your description says. If syscall_trace_enter() returns a positive non-zero value, that value is _not_ used as changed syscall number. SH uses r3 for the syscall number, not r0).

You are right, of course. I somehow mixed that up. You're right, it checks
the return value of syscall_trace_enter and it should skip the syscall
if syscall_trace_enter returns -1.

> As far as I can see, any non-zero return code should abort the syscall, so I just test for that (for simplicity). Our use of the tracehook_report_syscall_entry() return code (directly passed back from syscall_trace_enter()) doesn't leave much choice there, see comment in include/linux/tracehook.h.

My point is to stay consistent with the other architectures.

> If later on seccomp needs any specific action, it should be easy enough to change the syscall number (offset PT_OFF_ORIG_D0 on the stack) or syscall return code (offset PT_OFF_D0). There's code in kernel/ptrace.c to do that AFAICS.
> 
> Changing the behaviour of syscall_trace_enter() to match what other architectures do (return changed syscall number, not error code) is beyond the scope of this patch. I suspect the capability to change syscall numbers from ptrace code does predate the seccomp filter approach, and as m68k has never used it in the past, I don't see a point to add this now.

Yes, I agree and my previous comment in this regard was wrong.

>> Also, shouldn't that part of the change not be part of the patch that
>> adds support for SECCOMP filter like in [1]? I don't think it makes
>> sense to add the return code check unless the rest of SECCOMP filter
>> has been implemented.
> 
> After replacing syscall_trace() by syscall_trace_enter() and syscall_trace_leave(), there is a return code provided by syscall_trace_enter() which we must check, hence I added the check at the same time as replacing syscall_trace() for non-Coldfire m68k.
> 
> (I note that the same check should probably be added to coldfire/entry.S.)
Do we actually need to check the return value unless we implement SECCOMP_FILTER?
(Which no doubt we will do ;-)). 

> I can't test any of the later seccomp related stuff, so I'd rather leave that bit to someone else who can.

I will work on the necessary changes for libseccomp this week, so that we can test
whether the libseccomp live tests pass correctly on a patched kernel.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: syscall_trace_enter and syscall_trace_leave for m68k w/MMU
  2020-07-23 10:03 syscall_trace_enter and syscall_trace_leave for m68k w/MMU John Paul Adrian Glaubitz
                   ` (3 preceding siblings ...)
  2020-07-27  4:19 ` [PATCH] " Michael Schmitz
@ 2020-08-05 12:23 ` Greg Ungerer
  2020-08-05 12:36   ` John Paul Adrian Glaubitz
  4 siblings, 1 reply; 10+ messages in thread
From: Greg Ungerer @ 2020-08-05 12:23 UTC (permalink / raw)
  To: John Paul Adrian Glaubitz, linux-m68k; +Cc: Michael Karcher

Hi Adrian,

On 23/7/20 8:03 pm, John Paul Adrian Glaubitz wrote:
> Hello!
> 
> Michael and I have been working on SECCOMP in SuperH and successfully added
> the missing pieces yesterday [1].
> 
> We're now looking into what needs to be done for m68k and I have noticed that
> syscall_trace_enter and syscall_trace_leave - which needs to be touched for
> SECCOMP support - are available on ColdFire and non-MMU targets for m68k.
> 
> Is there a particular reason for that? Looking at 68000/entry.S, I can see
> that both functions are invoked [2] so I'm not sure why they are built
> for CONFIG_COLDFIRE and !CONFIG_MMM only.
> 
> Any ideas?

I am late to the party on this.

But the reason is historical more than anything else.
The time that m68knommu, which included ColdFire, spent as its own architecture
directory (arch/m68knommu) led to a few things like this diverging.
There is not technical reason.

Regards
Greg


> Adrian
> 
>> [1] https://marc.info/?l=linux-sh&m=159546012829989&w=2
>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/m68k/68000/entry.S
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: syscall_trace_enter and syscall_trace_leave for m68k w/MMU
  2020-08-05 12:23 ` syscall_trace_enter and syscall_trace_leave for m68k w/MMU Greg Ungerer
@ 2020-08-05 12:36   ` John Paul Adrian Glaubitz
  0 siblings, 0 replies; 10+ messages in thread
From: John Paul Adrian Glaubitz @ 2020-08-05 12:36 UTC (permalink / raw)
  To: Greg Ungerer, linux-m68k; +Cc: Michael Karcher

Hi Greg!

On 8/5/20 2:23 PM, Greg Ungerer wrote:
> But the reason is historical more than anything else.
> The time that m68knommu, which included ColdFire, spent as its own architecture
> directory (arch/m68knommu) led to a few things like this diverging.
> There is not technical reason.

That's what I was already suspecting after finding the commit that merged
both into one directory :-).

Thanks for the confirmation.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-23 10:03 syscall_trace_enter and syscall_trace_leave for m68k w/MMU John Paul Adrian Glaubitz
2020-07-23 22:56 ` Michael Schmitz
2020-07-25  1:48 ` [PATCH RFC] m68k/kernel - wire up syscall_trace_enter/leave for m68k Michael Schmitz
2020-07-26  1:28 ` [PATCH RFC v2] " Michael Schmitz
2020-07-27  4:19 ` [PATCH] " Michael Schmitz
2020-07-27 10:03   ` John Paul Adrian Glaubitz
2020-07-27 20:48     ` Michael Schmitz
2020-07-27 21:09       ` John Paul Adrian Glaubitz
2020-08-05 12:23 ` syscall_trace_enter and syscall_trace_leave for m68k w/MMU Greg Ungerer
2020-08-05 12:36   ` John Paul Adrian Glaubitz

Linux-m68k Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-m68k/0 linux-m68k/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-m68k linux-m68k/ https://lore.kernel.org/linux-m68k \
		linux-m68k@vger.kernel.org linux-m68k@lists.linux-m68k.org
	public-inbox-index linux-m68k

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-m68k


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git