Re: [PATCH v8 1/1] ptrace,syscall_user_dispatch: checkpoint/restore support for SUD

Thomas Gleixner
2023-02-09T23:45:32Z

On Tue, Jan 31 2023 at 09:44, Gregory Price wrote:
>
> +static inline int syscall_user_dispatch_get_config(struct task_struct *task, unsigned long size,
> + void __user *data)

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#line-breaks

All over the place.

> index 195ae64a8c87..6d2f3b86f932 100644
> --- a/include/uapi/linux/ptrace.h
> +++ b/include/uapi/linux/ptrace.h
> @@ -112,6 +112,15 @@ struct ptrace_rseq_configuration {
> __u32 pad;
> };
>
> +#define PTRACE_SET_SYSCALL_USER_DISPATCH_CONFIG 0x4210
> +#define PTRACE_GET_SYSCALL_USER_DISPATCH_CONFIG 0x4211
> +struct syscall_user_dispatch_config {

Can you please visibly separate the defines from the struct definition
by a newline? Glueing that stuff together is just horrible to read.

> + __u64 mode;
> + __s8 *selector;
> + __u64 offset;
> + __u64 len;

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

Please add proper documentation to this struct. It's user space ABI and
it's not the job of the man page maintainers to figure out what this
actually means.

> +int syscall_user_dispatch_set_config(struct task_struct *task, unsigned long size,
> + void __user *data)
> +{
> + struct syscall_user_dispatch_config config;
> +
> + if (size != sizeof(struct syscall_user_dispatch_config))
> + return -EINVAL;
> +
> + if (copy_from_user(&config, data, sizeof(config)))
> + return -EFAULT;
> +
> + return set_syscall_user_dispatch(config.mode, config.offset, config.len,
> + config.selector);

How is this supposed to work? This is called from the ptracer to set the
user dispatch mode on the ptracee, i.e. on @task.

But set_syscall_user_dispatch() operates on current, which is the
ptracer itself.

Clearly well tested with the non-existant selftest, which is part of
this submission.

So please fix the above issues, add a selftest and proper documentation.

I'm neither impressed by this patch nor by the reviews.

Sigh

tglx