Re: [PATCH] rseq: update kernel fields in lockstep with CONFIG_DEBUG_RSEQ

[Mathieu Desnoyers]

On 2025-02-22 08:27, Ingo Molnar wrote:
> * Michael Jeanson <mjeanson@xxxxxxxxxxxx> wrote:
>
> > With CONFIG_DEBUG_RSEQ an in-kernel copy of the read-only fields is
> > kept synchronized with the user-space fields. Ensure the updates
> > are done in lockstep in case we error out on a write to user-space.
> >
> > Fixes: 7d5265ffcd8b ("rseq: Validate read-only fields under DEBUG_RSEQ config")
> > Signed-off-by: Michael Jeanson <mjeanson@xxxxxxxxxxxx>
> > Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> > ---
> >   kernel/rseq.c | 85 +++++++++++++++++++++++++++------------------------
> >   1 file changed, 45 insertions(+), 40 deletions(-)
> >
> > diff --git a/kernel/rseq.c b/kernel/rseq.c
> > index 2cb16091ec0a..5bdb96944e1f 100644
> > --- a/kernel/rseq.c
> > +++ b/kernel/rseq.c
> > @@ -26,6 +26,11 @@
> >   				  RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL | \
> >   				  RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE)
> >   
> > +static struct rseq __user *rseq_user_fields(struct task_struct *t)
> > +{
> > +	return (struct rseq __user *) t->rseq;
> > +}
> > +
> >   #ifdef CONFIG_DEBUG_RSEQ
> >   static struct rseq *rseq_kernel_fields(struct task_struct *t)
> >   {
> > @@ -78,24 +83,24 @@ static int rseq_validate_ro_fields(struct task_struct *t)
> >   	return -EFAULT;
> >   }
> >   
> > -static void rseq_set_ro_fields(struct task_struct *t, u32 cpu_id_start, u32 cpu_id,
> > -			       u32 node_id, u32 mm_cid)
> > -{
> > -	rseq_kernel_fields(t)->cpu_id_start = cpu_id;
> > -	rseq_kernel_fields(t)->cpu_id = cpu_id;
> > -	rseq_kernel_fields(t)->node_id = node_id;
> > -	rseq_kernel_fields(t)->mm_cid = mm_cid;
> > -}
> > +/*
> > + * Update an rseq field and its in-kernel copy in lock-step to keep a coherent
> > + * state.
> > + */
> > +#define unsafe_rseq_set_field(t, field, value, error_label)		\
> > +	do {								\
> > +		unsafe_put_user(value, &rseq_user_fields(t)->field, error_label);	\
> > +		rseq_kernel_fields(t)->field = value;			\
> > +	} while (0)
> > +
> >   #else
> >   static int rseq_validate_ro_fields(struct task_struct *t)
> >   {
> >   	return 0;
> >   }
> >   
> > -static void rseq_set_ro_fields(struct task_struct *t, u32 cpu_id_start, u32 cpu_id,
> > -			       u32 node_id, u32 mm_cid)
> > -{
> > -}
> > +#define unsafe_rseq_set_field(t, field, value, error_label)		\
> > +	unsafe_put_user(value, &rseq_user_fields(t)->field, error_label)
> >   #endif
> >   
> >   /*
> > @@ -173,17 +178,18 @@ static int rseq_update_cpu_node_id(struct task_struct *t)
> >   	WARN_ON_ONCE((int) mm_cid < 0);
> >   	if (!user_write_access_begin(rseq, t->rseq_len))
> >   		goto efault;
> > -	unsafe_put_user(cpu_id, &rseq->cpu_id_start, efault_end);
> > -	unsafe_put_user(cpu_id, &rseq->cpu_id, efault_end);
> > -	unsafe_put_user(node_id, &rseq->node_id, efault_end);
> > -	unsafe_put_user(mm_cid, &rseq->mm_cid, efault_end);
> > +
> > +	unsafe_rseq_set_field(t, cpu_id_start, cpu_id, efault_end);
> > +	unsafe_rseq_set_field(t, cpu_id, cpu_id, efault_end);
> > +	unsafe_rseq_set_field(t, node_id, node_id, efault_end);
> > +	unsafe_rseq_set_field(t, mm_cid, mm_cid, efault_end);
>
> Could we please name the new wrapper rseq_unsafe_put_user(), to make it
> clear it's a wrapper around unsafe_put_user()?

If we do this then need to make sure the order of arguments becomes
similar to unsafe_put_user for consistency. If we look at its macro:

#define unsafe_put_user(x,p,e) unsafe_op_wrap(__put_user(x,p),e),
we have the following argument order:

1) value (x),
2) pointer (p),
3) error label (e).

So far unsafe_rseq_set_field has:

1) task struct pointer,
2) rseq field name,
3) value,
4) error label.

I always find it odd that the "source" argument comes first and
the "destination" argument comes second in all put_user() APIs,
compared to memcpy, WRITE_ONCE() and all assignments (e.g.
operator "=" LHS vs RHS). Choosing a different argument order
therefore made sense with a naming different from "*put_user",
but not so much if we use a derived naming.

This argument order oddness was actually one motivation for using
a naming *different* from put_user.

We can consider "task" a context, "field" as destination
argument, and "value" a source argument. So if we use a
similar approach as put_user, we'd have "task" as first
argument (context), "value" as second argument, followed
by "field", then error label.

Thoughts ?

Thanks,

Mathieu

> Thanks,
>
> 	Ingo


--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com