Re: [PATCH] ptrace: add PTRACE_GET_RSEQ_CONFIGURATION request

[Dmitry V. Levin]

On Mon, Feb 22, 2021 at 09:53:10AM -0500, Mathieu Desnoyers wrote:
> ----- On Feb 22, 2021, at 6:57 AM, Dmitry V. Levin ldv@xxxxxxxxxxxx wrote:
> > On Mon, Feb 22, 2021 at 11:04:43AM +0100, Piotr Figiel wrote:
[...]
> >> +#ifdef CONFIG_RSEQ
> >> +static long ptrace_get_rseq_configuration(struct task_struct *task,
> >> +					  unsigned long size, void __user *data)
> >> +{
> >> +	struct ptrace_rseq_configuration conf = {
> >> +		.rseq_abi_pointer = (u64)(uintptr_t)task->rseq,
> >> +		.signature = task->rseq_sig,
> >> +	};
> >> +
> >> +	size = min_t(unsigned long, size, sizeof(conf));
> >> +	if (copy_to_user(data, &conf, size))
> >> +		return -EFAULT;
> >> +	return size;
> >> +}
> >> +#endif
> > 
> > From API perspective I suggest for such interfaces to return the amount of
> > data that could have been written if there was enough room specified, e.g.
> > in this case it's sizeof(conf) instead of size.
> 
> Looking at the ptrace(2) man page:
> 
> RETURN VALUE
>        On success, the PTRACE_PEEK* requests return the  requested  data  (but
>        see NOTES), the PTRACE_SECCOMP_GET_FILTER request returns the number of
>        instructions in the BPF program, and other requests return zero.

PTRACE_GET_SYSCALL_INFO returns "the number of bytes available to be
written by the kernel".

It's written in the "DESCRIPTION" section, needs to be mirrored
to "RETURN VALUE" section, thanks for reporting the inconsistency.

>        On error, all requests return  -1,  and  errno  is  set  appropriately.
>        Since  the  value  returned by a successful PTRACE_PEEK* request may be
>        -1, the caller must clear errno before the call, and then check it  af‐
>        terward to determine whether or not an error occurred.
> 
> It looks like the usual behavior for ptrace requests would be to return 0 when everything
> is OK. Unless there a strong motivation for doing different for this new request, I
> would be tempted to use the same expected behavior than other requests on success:
> return 0.
> 
> Unless there is a strong motivation for returning either size or sizeof(conf) ? If we
> return sizeof(conf) to user-space, it means it should check it and deal with the
> size mismatch. Is that size ever expected to change ?

When adding new interfaces, it's generally a good idea to allow for
future extensions.
If some day in the future the structure is extended, the return value
would be the way to tell userspace what's actually supported by the kernel.


-- 
ldv