On Thu, Jul 02, 2026 at 01:24:57PM +0200, Thomas Gleixner wrote:
> On Wed, Jul 01 2026 at 19:42, Michal Suchánek wrote:
> > The return value of syscall_enter_from_user_mode is used both for the
> > adjusted syscall number and the indicator that a syscall should be
> > skipped.
> >
> > As seccomp can be invoked on any syscall, including invalid ones this
> > somewhat undermines seccomp.
> >
> > While the seccomp variants that terminate the process do not need to
> > care about this for the filter that sets the syscall return value this
> > disctinction is required.
>
> You completely fail to explain why and what actual problem you are
> trying to solve. At least I can't figure it out from the above word
> salad.
syscall_enter_from_user_mode returns the new syscall number after doing
something arbitrarry with it, including running seccomp.
Wehn the syscall is already handled, eg. by seccomp filtering it returns
-1 as the new syscall number. -1 is an invalid syscall number but it can
still be filtered by seccomp. When the syscall number was -1 to start
with it's not possible to determine if the syscall was fileterd from the
return value. s390 returns the filtered state in a flag it sets on the
regs structure, avoiding this problem.
However, the API should be specified in a way that does not require
everyone implementing such flag.
>
> > Pass the syscall number as a pointer to the inline entry functions, and
> > use the return value exclusively for the indication that the syscall is
> > already handled.
> >
> > This should avoid the need for the s390 PIF_SYSCALL_RET_SET which is the
> > workaround for exactly this deficiency.
> >
> > If this is desirable the patch could be split into some series that
> > adjusts the code flow where needed so that the final change is mostly
> > mechanical.
>
> That's not a matter of desire. That's mandatory.
So long as it's desirable to implement an API change in this direction,
it's not clear to me so far.
> > - instrumentation_begin();
> > - if (!invoke_syscall(regs, nr) && nr != -1)
> > - result_reg(regs) = __sys_ni_syscall(regs);
> > - instrumentation_end();
> > + /* Skip syscall when -1 is returned */
> > + if (!syscall_enter_from_user_mode(regs, &nr)) {
>
> Seriously?
>
> If we go and separate the syscall number from the return value, then the
> return value 0 means success and anything else fail. Which in other
> words is a boolean. So instead of tastelessly adding a completely
> nonsensical comment about -1 here, syscall_enter_from_user_mode() wants
> to have the return value type bool with a proper boolean logic: true =
> success, false = abort.
We have that very same API down to __secure_computing() which returns
boolean represented as -1 and 0 values. That does not mean it's not
tasteless.
>
> > @@ -168,8 +168,7 @@ __visible noinstr void do_int80_emulation(struct
> > pt_regs *regs)
> > nr = syscall_32_enter(regs);
> >
> > local_irq_enable();
> > - nr = syscall_enter_from_user_mode_work(regs, nr);
> > - do_syscall_32_irqs_on(regs, nr);
> > + syscall_enter_from_user_mode_work(regs, &nr);
>
> How exactly is this ever going to invoke a valid syscall?
That's one of the problems with giant all-in-one patch, things like this
easily slip in. However, it is in cluded mostly for illustration, I
don't expect anyone to merge this as-is.
>
> > + if (!syscall_enter_from_user_mode_work(regs, &nr)) {
> > + nr &= GENMASK(31, 0);
> > + do_syscall_32_irqs_on(regs, nr);
>
> do_syscall_32_irqs_on(regs, (int)nr);
>
> would be too simple, right?
Also way less explicit.
Thanks
Michal
