Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Sven Schnelle <svens@linux.ibm.com>
To:     Andy Lutomirski <luto@kernel.org>
Cc:     Vasily Gorbik <gor@linux.ibm.com>, X86 ML <x86@kernel.org>,
        linux-arch <linux-arch@vger.kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: Is s390's new generic-using syscall code actually correct?
References: <CALCETrUx10uHeD7bckVjL9x7S3LEdH3ZfzUbCMWj9j-=nYp8Wg@mail.gmail.com>
Date:   Mon, 29 Mar 2021 13:49:05 +0200
In-Reply-To: <CALCETrUx10uHeD7bckVjL9x7S3LEdH3ZfzUbCMWj9j-=nYp8Wg@mail.gmail.com>
        (Andy Lutomirski's message of "Sat, 20 Mar 2021 20:48:34 -0700")
Message-ID: <yt9dblb2w572.fsf@linux.ibm.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Hi Andy,

sorry for the late reply, i was away from kernel development for a few
weeks.

Andy Lutomirski <luto@kernel.org> writes:

> Hi all-
>
> I'm working on my kentry patchset, and I encountered:
>
> commit 56e62a73702836017564eaacd5212e4d0fa1c01d
> Author: Sven Schnelle <svens@linux.ibm.com>
> Date:   Sat Nov 21 11:14:56 2020 +0100
>
>     s390: convert to generic entry
>
> As part of this work, I was cleaning up the generic syscall helpers,
> and I encountered the goodies in do_syscall() and __do_syscall().
>
> I'm trying to wrap my head around the current code, and I'm rather confused.
>
> 1. syscall_exit_to_user_mode_work() does *all* the exit work, not just
> the syscall exit work.  So a do_syscall() that gets called twice will
> do the loopy part of the exit work (e.g. signal handling) twice.  Is
> this intentional?  If so, why?

Not really intentional, but i decided to accept the overhead for now and
fix that later by splitting up the generic entry code. Otherwise the
patch would have had even more dependencies. I have not looked yet into
your kentry branch, but will do that later. Maybe there is already a
better way to do it or we can work something out.

> 2. I don't understand how this PIF_SYSCALL_RESTART thing is supposed
> to work.  Looking at the code in Linus' tree, if a signal is pending
> and a syscall returns -ERESTARTSYS, the syscall will return back to
> do_syscall().  The work (as in (1)) gets run, calling do_signal(),
> which will notice -ERESTARTSYS and set PIF_SYSCALL_RESTART.
> Presumably it will also push the signal frame onto the stack and aim
> the return address at the svc instruction mentioned in the commit
> message from "s390: convert to generic entry".  Then __do_syscall()
> will turn interrupts back on and loop right back into do_syscall().
> That seems incorrect.

That sounds indeed broken. My understanding is that x86 is always
rewinding the pc in the restart case, and is exiting to user mode. That
would than also work with signal frames.

However, on s390 we cannot simply rewind the pc as the syscall number
might be encoded in the system call instruction. If a user would have
rewritten the system call number (i.e. with seccomp) it would still
execute the original syscall number.

That makes me wonder why i have not seen problems with signals and system
call restarting so far. Have to read the code again.

Regards
Sven