Re: [PATCH v1] riscv: signal: check the full syscall return value for a restart

[Jisheng Zhang <jszhang@kernel.org>]

On Sat, Oct 17, 2026 at 10:42:15PM -0400, Nickolai Zeldovich wrote:
> A system call that returns a 64-bit value whose low 32 bits happen to
> equal one of the -ERESTART* codes is restarted or turned into -EINTR
> when a signal is pending on the way back to user space. lseek() returns
> the new file offset, so lseek(fd, 0x1fffffe00, SEEK_CUR) followed by a
> signal with an SA_RESTART handler runs the lseek() a second time and
> leaves the offset at 0x3fffffc00; without SA_RESTART the call fails with
> EINTR after it has moved the offset. With a SIGALRM interval timer of
> 20 us, 2337 out of 2000000 such lseek() calls moved the offset twice and
> 2695 out of 2000000 returned EINTR. brk() can return an arbitrary
> address the same way, and a value ending in 0xfffffdfc goes through
> restart_syscall() with whatever restart_block.fn the task last set.
> 
> arch_do_signal_or_restart() reads regs->a0 into an int, which drops the
> upper half of the return value before it is compared with -ERESTARTSYS
> and friends. The code before the restart handling was moved out of
> handle_signal() switched on the unsigned long a0 and was not affected.
> 
> Keep the return value in a long. Compat tasks see only the low 32 bits
> of a0, and a 32-bit tracer writes it zero-extended, so sign-extend it
> from 32 bits for them as before.
> 
> Tested with the program in the notes on QEMU virt (riscv64 defconfig):
> the offset is moved exactly once and no EINTR is returned with the patch.
> 
> Fixes: ce4f78f1b53d ("riscv: signal: handle syscall restart before get_signal")
> Assisted-by: LLM
> Signed-off-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
Good catch!

Reviewed-by: Jisheng Zhang <jszhang@kernel.org>

> ---
> 
> Notes:
>     Reproducer (static riscv64 binary run as /init's child on QEMU virt, -smp 2;
>     riscv64-linux-gnu-gcc -O2 -static -o lseek_restart lseek_restart.c):
>     
>     #define _GNU_SOURCE
>     #include <errno.h>
>     #include <fcntl.h>
>     #include <signal.h>
>     #include <stdio.h>
>     #include <string.h>
>     #include <sys/time.h>
>     #include <unistd.h>
>     
>     #define OFF 0x1fffffe00LL	/* low 32 bits read as -ERESTARTSYS */
>     
>     static volatile long nsig;
>     
>     static void handler(int sig)
>     {
>     	nsig++;
>     }
>     
>     static void run(int fd, int flags, const char *name)
>     {
>     	struct sigaction sa;
>     	struct itimerval it = { { 0, 20 }, { 0, 20 } };
>     	long i, moved = 0, eintr = 0;
>     
>     	memset(&sa, 0, sizeof(sa));
>     	sa.sa_handler = handler;
>     	sa.sa_flags = flags;
>     	nsig = 0;
>     	sigaction(SIGALRM, &sa, NULL);
>     	setitimer(ITIMER_REAL, &it, NULL);
>     
>     	for (i = 0; i < 2000000; i++) {
>     		off_t r;
>     
>     		lseek(fd, 0, SEEK_SET);
>     		r = lseek(fd, OFF, SEEK_CUR);
>     		if (r == OFF)
>     			continue;
>     		if (r == -1 && errno == EINTR)
>     			eintr++;
>     		else if (r == 2 * OFF)
>     			moved++;
>     		else
>     			printf("%s: unexpected %#llx\n", name, (long long)r);
>     	}
>     	memset(&it, 0, sizeof(it));
>     	setitimer(ITIMER_REAL, &it, NULL);
>     	printf("%s: %ld signals, %ld calls moved the offset twice, %ld returned EINTR\n",
>     	       name, nsig, moved, eintr);
>     }
>     
>     int main(void)
>     {
>     	int fd = open("/lseek-test", O_RDWR | O_CREAT, 0600);
>     
>     	if (fd < 0) {
>     		perror("open");
>     		return 2;
>     	}
>     	run(fd, SA_RESTART, "SA_RESTART");
>     	run(fd, 0, "no SA_RESTART");
>     	return 0;
>     }
>     
>     Before:
>     SA_RESTART: 283700 signals, 2337 calls moved the offset twice, 0 returned EINTR
>     no SA_RESTART: 246693 signals, 0 calls moved the offset twice, 2695 returned EINTR
>     After:
>     SA_RESTART: 306854 signals, 0 calls moved the offset twice, 0 returned EINTR
>     no SA_RESTART: 278090 signals, 0 calls moved the offset twice, 0 returned EINTR
> 
>  arch/riscv/kernel/signal.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
> index 59784dc117e4..b1e92b969d96 100644
> --- a/arch/riscv/kernel/signal.c
> +++ b/arch/riscv/kernel/signal.c
> @@ -499,7 +499,7 @@ static void handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>  void arch_do_signal_or_restart(struct pt_regs *regs)
>  {
>  	unsigned long continue_addr = 0, restart_addr = 0;
> -	int retval = 0;
> +	long retval = 0;
>  	struct ksignal ksig;
>  	bool syscall = (regs->cause == EXC_SYSCALL);
>  
> @@ -508,6 +508,8 @@ void arch_do_signal_or_restart(struct pt_regs *regs)
>  		continue_addr = regs->epc;
>  		restart_addr = continue_addr - 4;
>  		retval = regs->a0;
> +		if (is_compat_task())
> +			retval = (int)retval;
>  
>  		/* Avoid additional syscall restarting via ret_from_exception */
>  		regs->cause = -1UL;
> 
> base-commit: 58161ead29e50d70a317ea26b472e0c7508e69da
> -- 
> 2.53.0
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv