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

[Thomas Gleixner <tglx@kernel.org>]

On Thu, Jul 02 2026 at 13:45, Michal Suchánek wrote:
> 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.

Once syscall_enter_from_user_mode() returns -1 nothing can filter it
anymore.

> 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.

What needs to determine whether the syscall was filtered or not?

> However, the API should be specified in a way that does not require
> everyone implementing such flag.

Which exact problem does the flag solve?

>> > -	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.

Hahahahaha.

We have a lot of functions which have a boolean return value but a
int/long return type for historical reasons.

We've added bool because it's not ambiguous and allows the compiler to
optimize better. It also makes the code more clear. Modern code uses a
non-boolean return type only when there is an actual reason for it,
e.g. propagating an error code all the way back through the call chain.
The historical 0=success <0 = errorcode model really want's to be
restricted to such cases.

Just for the record:

  https://lore.kernel.org/all/67c3ae5c-d88b-4172-9996-4e2046b7e0dc@huawei.com/
  https://lore.kernel.org/all/20260629130616.642022-2-ruanjinjie@huawei.com/

So I stand with my comment that it is sloppy and tasteless to slap an
argument into a pile of functions, claim separation of return value and
syscall number and leave the return value in an ill-defined state.

>> > @@ -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.

It's a problem with hastily cobbled together slop. Even RFC patches
should at least be functional.
 
>> > +	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.

Now you care about explicit, but the return value mess can be left
ambiguous, right?

Aside of that, the cast is very much explicit for people who can read C.

It would be great if you could sit back and come up with a very explicit
and comprehensible explanation for the problem you are trying to solve.

Thanks,

        tglx