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

[Nickolai Zeldovich <nickolai@csail.mit.edu>]

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

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