On 2025-08-23 12:39, Thomas Gleixner wrote:
> Just utilize the new infrastructure and put the original one to rest.
I would recommend changing the patch subject to e.g.
"rseq: Replace the syscall debug code by the new infrastructure"
since I don't think the pre-existing debug code qualifies as "crud",
even though your proposed changes are certainly improvements.
Other than that:
Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Thanks,
Mathieu
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> kernel/rseq.c | 80 ++++++++--------------------------------------------------
> 1 file changed, 12 insertions(+), 68 deletions(-)
>
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -472,83 +472,27 @@ void __rseq_handle_notify_resume(struct
>
> #ifdef CONFIG_DEBUG_RSEQ
> /*
> - * Unsigned comparison will be true when ip >= start_ip, and when
> - * ip < start_ip + post_commit_offset.
> - */
> -static bool in_rseq_cs(unsigned long ip, struct rseq_cs *rseq_cs)
> -{
> - return ip - rseq_cs->start_ip < rseq_cs->post_commit_offset;
> -}
> -
> -/*
> - * If the rseq_cs field of 'struct rseq' contains a valid pointer to
> - * user-space, copy 'struct rseq_cs' from user-space and validate its fields.
> - */
> -static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
> -{
> - struct rseq_cs __user *urseq_cs;
> - u64 ptr;
> - u32 __user *usig;
> - u32 sig;
> - int ret;
> -
> - if (get_user_masked_u64(&ptr, &t->rseq->rseq_cs))
> - return -EFAULT;
> -
> - /* If the rseq_cs pointer is NULL, return a cleared struct rseq_cs. */
> - if (!ptr) {
> - memset(rseq_cs, 0, sizeof(*rseq_cs));
> - return 0;
> - }
> - /* Check that the pointer value fits in the user-space process space. */
> - if (ptr >= TASK_SIZE)
> - return -EINVAL;
> - urseq_cs = (struct rseq_cs __user *)(unsigned long)ptr;
> - if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)))
> - return -EFAULT;
> -
> - if (rseq_cs->start_ip >= TASK_SIZE ||
> - rseq_cs->start_ip + rseq_cs->post_commit_offset >= TASK_SIZE ||
> - rseq_cs->abort_ip >= TASK_SIZE ||
> - rseq_cs->version > 0)
> - return -EINVAL;
> - /* Check for overflow. */
> - if (rseq_cs->start_ip + rseq_cs->post_commit_offset < rseq_cs->start_ip)
> - return -EINVAL;
> - /* Ensure that abort_ip is not in the critical section. */
> - if (rseq_cs->abort_ip - rseq_cs->start_ip < rseq_cs->post_commit_offset)
> - return -EINVAL;
> -
> - usig = (u32 __user *)(unsigned long)(rseq_cs->abort_ip - sizeof(u32));
> - ret = get_user(sig, usig);
> - if (ret)
> - return ret;
> -
> - if (current->rseq_sig != sig) {
> - printk_ratelimited(KERN_WARNING
> - "Possible attack attempt. Unexpected rseq signature 0x%x, expecting 0x%x (pid=%d, addr=%p).\n",
> - sig, current->rseq_sig, current->pid, usig);
> - return -EINVAL;
> - }
> - return 0;
> -}
> -
> -/*
> * Terminate the process if a syscall is issued within a restartable
> * sequence.
> */
> void rseq_syscall(struct pt_regs *regs)
> {
> - unsigned long ip = instruction_pointer(regs);
> struct task_struct *t = current;
> - struct rseq_cs rseq_cs;
> + u64 csaddr;
>
> - if (!t->rseq)
> + if (!t->rseq_event.has_rseq)
> + return;
> + if (get_user_masked_u64(&csaddr, &t->rseq->rseq_cs))
> + goto fail;
> + if (likely(!csaddr))
> return;
> - if (rseq_get_rseq_cs(t, &rseq_cs) || in_rseq_cs(ip, &rseq_cs))
> - force_sig(SIGSEGV);
> + if (unlikely(csaddr >= TASK_SIZE))
> + goto fail;
> + if (rseq_debug_update_user_cs(t, regs, csaddr))
> + return;
> +fail:
> + force_sig(SIGSEGV);
> }
> -
> #endif
>
> /*
>
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com