Celeste Liu coelacanthushex@gmail.com writes:
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c index 51ebfd23e0076447518081d137102a9a11ff2e45..3125fab8ee4af468ace9f692dd34e1797555cce3 100644 --- a/arch/riscv/kernel/traps.c +++ b/arch/riscv/kernel/traps.c @@ -316,18 +316,25 @@ void do_trap_ecall_u(struct pt_regs *regs) { if (user_mode(regs)) { long syscall = regs->a7;
long res;regs->epc += 4; regs->orig_a0 = regs->a0;
regs->a0 = -ENOSYS;riscv_v_vstate_discard(regs);
syscall = syscall_enter_from_user_mode(regs, syscall);
res = syscall_enter_from_user_mode(regs, syscall);/** Call syscall_get_nr() again because syscall_enter_from_user_mode()* may change a7 register.*/syscall = syscall_get_nr(current, regs);add_random_kstack_offset();
if (syscall >= 0 && syscall < NR_syscalls)
if (syscall < 0 || syscall >= NR_syscalls)regs->a0 = -ENOSYS;else if (res != -1) syscall_handler(regs, syscall);Here we can perform the syscall, even if res is -1. E.g., if this path [2] is taken, we might have a valid syscall number in a7, but the syscall should not be performed.
I may misunderstand what you said, but I can't see the issue you pointed. A syscall is performed iff
- syscall number in a7 must be valid, so it can reach "else if" branch.
- res != -1, so syscall_enter_from_user_mode() doesn't return -1 to inform the syscall should be skipped.
Ah, indeed. Apologies, that'll work!
Related, now wont this reintroduce the seccomp filtering problem? Say, res is -1 *and* syscall invalid, then a0 updated by seccomp will be overwritten here?
Also, one reason for the generic entry is so that it should be less work. Here, you pull (IMO) details that belong to the common entry implementation/API up the common entry user. Wdyt about pushing it down to common entry? Something like:
Yeah, we can. But I pull it out of common entry to get more simple API semantic:
- syscall_enter_from_user_mode() will do two things:
- the return value is only to inform whether the syscall should be skipped.
- regs will be modified by filters (seccomp or ptrace and so on).
- for common entry user, there is two informations: syscall number and the return value of syscall_enter_from_user_mode() (called is_skipped below). so there is three situations:
- if syscall number is invalid, the syscall should not be performed, and we set a0 to -ENOSYS to inform userspace the syscall doesn't exist.
- if syscall number is valid, is_skipped will be used: a) if is_skipped is -1, which means there are some filters reject this syscall, so the syscall should not performed. (Of course, we can use bool instead to get better semantic) b) if is_skipped != -1, which means the filters approved this syscall, so we invoke syscall handler with modified regs.
In your design, the logical condition is not obvious. Why syscall_enter_from_user_mode() informed the syscall will be skipped but the syscall handler will be called when syscall number is invalid? The users need to think two things to get result: a) -1 means skip b) -1 < 0 in signed integer, so the skip condition is always a invalid syscall number.
In may way, the users only need to think one thing: The syscall_enter_from_user_mode() said -1 means the syscall should not be performed, so use it as a condition of reject directly. They just need to combine the informations that they get from API as the condition of control flow.
I'm all-in for simpler API usage! Maybe massage the syscall_enter_from_user_mode() (or a new one), so that additional syscall_get_nr() call is not needed?
Björn