Re: [patch V2 20/37] rseq: Replace the debug crud

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

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