Re: [PATCH v3] fs/proc: Expose RSEQ configuration

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

----- On Jan 26, 2021, at 1:54 PM, Piotr Figiel figiel@google.com wrote:
[...]
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index a4f86a9d6937..6aea67878065 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -322,8 +322,10 @@ SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32,
> rseq_len,
> 		ret = rseq_reset_rseq_cpu_id(current);
> 		if (ret)
> 			return ret;
> +		task_lock(current);
> 		current->rseq = NULL;
> 		current->rseq_sig = 0;
> +		task_unlock(current);
> 		return 0;
> 	}
> 
> @@ -353,8 +355,10 @@ SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32,
> rseq_len,
> 		return -EINVAL;
> 	if (!access_ok(rseq, rseq_len))
> 		return -EFAULT;
> +	task_lock(current);
> 	current->rseq = rseq;
> 	current->rseq_sig = sig;
> +	task_unlock(current);

So AFAIU, the locks are there to make sure that whenever a user-space thread reads
that state through that new /proc file ABI, it observes coherent "rseq" vs "rseq_sig"
values. However, I'm not convinced this is the right approach to consistency here.

Because if you add locking as done here, you ensure that the /proc file reader
sees coherent values, but between the point where those values are read from
kernel-space, copied to user-space, and then acted upon by user-space, those can
very well have become outdated if the observed process runs concurrently.

So my understanding here is that the only non-racy way to effectively use those
values is to either read them from /proc/self/* (from the thread owning the task struct),
or to ensure that the thread is stopped/frozen while the read is done.

Maybe we should consider validating that the proc file is used from the right context
(from self or when the target thread is stopped/frozen) rather than add dubious locking ?

Thanks,

Mathieu

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