Re: [RFC] entry: Untangle the return value of syscall_enter_from_user_mode from syscall NR

["Michal Suchánek" <msuchanek@suse.de>]

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