Re: [patch V4 23/36] rseq: Provide and use rseq_set_ids()

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, Sep 11 2025 at 09:40, Mathieu Desnoyers wrote:
> On 2025-09-08 17:32, Thomas Gleixner wrote:
>> +/*
>> + * On debug kernels validate that user space did not mess with it if
>> + * DEBUG_RSEQ is enabled, but don't on the first exit to user space. In
>> + * that case cpu_cid is ~0. See fork/execve.
>> + */
>> +bool rseq_debug_validate_ids(struct task_struct *t)
>> +{
>> +	struct rseq __user *rseq = t->rseq.usrptr;
>
> Why not check on NULL rseq user pointer rather than skip using
> the ~0ULL cpu_cid ?

It's not about the NULL rseq pointer.

At the point of a freshly installed RSEQ the cached values are invalid
when the task goes out to user space for the first time because the
write out obviously never happened before that.

We could write out some defined value when RSEQ is installed and cache
that, but I did not think about that and this is debug code so I did not
care about the performance that much.

>> +	u32 cpu_id, uval, node_id;
>> +
>> +	if (t->rseq.ids.cpu_cid == ~0)
>
> Here we are using ~0 and where cpu_cid is set in rseq_set() ~0ULL is
> used. Now I understand that because of sign-extend and type promotion
> this happens to provide all bits set on a 64-bit, but can we please just
> pick one, e.g. ~0ULL ?

Sure. Trivial enough

> Also why does rseq_fork() set it to ~0ULL rather than keep the value
> from the parent when forking a new process ?
>
> Whatever was present in the parent process in the rseq area should
> still be in the child, including the rseq registration.
>
> Or am missing something ?

Not really. I just made all those cases (install, fork, exec...) behave
the same way. It's harmless enough, no?

Thanks,

        tglx