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

[Gregory Price <gregory.price@memverge.com>]

On Mon, Feb 13, 2023 at 09:26:21PM +0100, Thomas Gleixner wrote:
> On Fri, Feb 10 2023 at 02:25, Gregory Price wrote:
> >  
> >  As the ABI of these intercepted syscalls is unknown to Linux, these
> > -syscalls are not instrumentable via ptrace or the syscall tracepoints.
> > +syscalls are not instrumentable via ptrace or the syscall tracepoints,
> > +however an interfaces to suspend, checkpoint, and restore syscall user
> > +dispatch configuration has been added to ptrace to assist userland
> > +checkpoint/restart software.
> 
> The above is incomprehensible word salad to me. Once the ptrace
> functions are there then they can be used independent of CRIU, no?
> 

The verbiage here is half-baked, I'll just break out a separate
paragraph to explain better (or drop entirely, if that's preferable).

Since SUD isn't really designed for anything other than syscall
emulation, there's not much of a use for these get/set interfaces
outside the context of checkpoint/restart.  GDB and friends are already
perfectly happy and capable of debugging SUD enabled software in the
absense of these interfaces and have no need to disable the feature.

> > + * struct ptrace_sud_config - Per-task configuration for SUD
> > + * @mode:	One of PR_SYS_DISPATCH_ON or PR_SYS_DISPATCH_OFF
> > + * @selector:	Tracee's user virtual address of SUD selector
> > + * @offset:	SUD exclusion area (virtual address)
> > + * @len:	Length of SUD exclusion area
> > + *
> > + * Used to get/set the syscall user dispatch configuration for tracee.
> > + * process.  Selector is optional (may be NULL), and if invalid will produce
> > + * a SIGSEGV in the tracee upon first access.
> > + *
> > + * If mode is PR_SYS_DISPATCH_ON, syscall dispatch will be enabled. If
> > + * PR_SYS_DISPATCH_OFF, syscall dispatch will be disabled and all other
> > + * parameters must be 0.  The value in *selector (if not null), also determines
> > + * whether syscall dispatch will occur.
> > + *
> > + * The SUD Exclusion area described by offset/len is the virtual address space
> > + * from which syscalls will not produce a user dispatch.
> > + */
> > +struct ptrace_sud_config {
> > +	__u64 mode;
> > +	__s8 *selector;
> 
> How is this correct for a 32bit ptracer running on a 64bit kernel? Aside
> of not wiring up the compat syscall without any argumentation in the
> changelog.
> 

you're right, these would need to be unsigned long/pointers, i will
take a closer look at how ptrace manages this elsewhere and come back.

> 
> > --- a/kernel/entry/syscall_user_dispatch.c
> > +++ b/kernel/entry/syscall_user_dispatch.c
> 
> This section:
> 
> > -int set_syscall_user_dispatch(unsigned long mode, unsigned long offset,
> > -			      unsigned long len, char __user *selector)
> > +static int task_set_syscall_user_dispatch(struct task_struct *task, unsigned long mode,
> > +		                          unsigned long offset, unsigned long len,
> > +		                          char __user *selector)
> >  {
> >  	switch (mode) {
> >  	case PR_SYS_DISPATCH_OFF:
> > @@ -94,15 +96,60 @@ int set_syscall_user_dispatch(unsigned long mode, unsigned long offset,
> >  		return -EINVAL;
> >  	}
> >  
> > -	current->syscall_dispatch.selector = selector;
> > -	current->syscall_dispatch.offset = offset;
> > -	current->syscall_dispatch.len = len;
> > -	current->syscall_dispatch.on_dispatch = false;
> > +	task->syscall_dispatch.selector = selector;
> > +	task->syscall_dispatch.offset = offset;
> > +	task->syscall_dispatch.len = len;
> > +	task->syscall_dispatch.on_dispatch = false;
> >  
> >  	if (mode == PR_SYS_DISPATCH_ON)
> > -		set_syscall_work(SYSCALL_USER_DISPATCH);
> > +		set_task_syscall_work(task, SYSCALL_USER_DISPATCH);
> >  	else
> > -		clear_syscall_work(SYSCALL_USER_DISPATCH);
> > +		clear_task_syscall_work(task, SYSCALL_USER_DISPATCH);
> >  
> >  	return 0;
> >  }
> > +
> > +int set_syscall_user_dispatch(unsigned long mode, unsigned long offset,
> > +		              unsigned long len, char __user *selector)
> > +{
> > +	return task_set_syscall_user_dispatch(current, mode, offset, len, selector);
> > +}
> 
> until here want's to be a seperate preparatory patch.
> 

I had considered this, but didn't know what was preferable, given that
there's not much reason to create the functions outside the context of
this patch.

Will do.

> > +++ b/tools/testing/selftests/ptrace/get_set_sud.c
> > +	child = fork();
> > +	ASSERT_GE(child, 0);
> > +	if (child == 0) {
> > +		ASSERT_EQ(0, sys_ptrace(PTRACE_TRACEME, 0, 0, 0)) {
> > +			TH_LOG("PTRACE_TRACEME: %m");
> > +		}
> > +		kill(getpid(), SIGSTOP);
> > +		sleep(2);
> 
> The purpose of this sleep is what?
> 

artifact of taking other tests as an outline, will drop it and rerun.

> > +		_exit(1);
> > +	}
> > +
> > +	waitpid(child, &status, 0);
> > +
> > +	config.mode = PR_SYS_DISPATCH_ON;
> > +	config.selector = (void*)0xDEADBEEF;
> > +	config.offset = 0x12345678;
> > +	config.len = 0x87654321;
> 
> What's the purpose of these magic numbers? memset(&config, 0xff,...) is
> sufficient, no?
> 
> Thanks,
> 
>         tglx

Nothing, will drop.

~Gregory