Date:   Mon, 13 Feb 2023 16:00:17 -0500
From:   Gregory Price <gregory.price@memverge.com>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Gregory Price <gourry.memverge@gmail.com>,
        linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
        oleg@redhat.com, avagin@gmail.com, peterz@infradead.org,
        luto@kernel.org, krisman@collabora.com, corbet@lwn.net,
        shuah@kernel.org
Subject: Re: [PATCH v9 1/1] ptrace,syscall_user_dispatch: checkpoint/restore
 support for SUD
Message-ID: <Y+qk4fkIph40KyDh@memverge.com>
References: <20230210072503.1808-1-gregory.price@memverge.com>
 <20230210072503.1808-2-gregory.price@memverge.com>
 <871qmttiqa.ffs@tglx>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <871qmttiqa.ffs@tglx>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?XCGWdvCbzZ6rALvcWcI9tjG/XX7B62GwTI7rz4Q8IRbuDBWz3t9VCWFQB/ek?=
 =?us-ascii?Q?DmGkIrJSFE2vNhiXLA/RB+HM1tQscJS6q9c41SiX4ZGgaICIj5rWMyU0ykXN?=
 =?us-ascii?Q?W1qELwDkCIpYpUTXxsPkz7SzVlVXbYWYhhM9vgc92v5+1ho818rBNxLJ5NJ+?=
 =?us-ascii?Q?DIZ0NPxwA68lDDanEsxZ6X0FprSZmcZaWKDQJwQakX5b67Pwz3VSOS2gC5nk?=
 =?us-ascii?Q?59Yz+2Rmv3y6ruEheJYl7uLT6/Nb89Hkp/8nHemK2wj6dNEzlyFqRKjPmvP7?=
 =?us-ascii?Q?bi+1orXG97DvtpcJZkPMRQvZFOlvBh1b1jWPi0urOSLToQAHHT5klPO61zfm?=
 =?us-ascii?Q?2AVgouDgaNu7ykTx6kVAn+ajgBs/62vtUK+clbYTB5LRG2eQlcKCdatICxn2?=
 =?us-ascii?Q?WVEAZq4b6XSVdVmHAEK+H2izkX/CO5N3iw75O48O7djC8PCrghadRogWCCs/?=
 =?us-ascii?Q?iDVVY0ec3QkEvvwhnixuFjWnXlAgqkAJ+iBvxnFUcdEn9GhUc3P6dFtFt/vf?=
 =?us-ascii?Q?d+VfVlN1bmbZRE1R0BDUtAGnsgATlh1kyYok5l7YjONKbwW2MKTkxZ3Odqkq?=
 =?us-ascii?Q?0chZ+dmsmJGUqYN9AGY+bdSaLid1v3EgGDVlkEdfuYbHg/EWPaeNuIMdR7TN?=
 =?us-ascii?Q?/6OBVmFAcAvbLCLVkkgyxiyw73HLb08ACoD1w1QPFS9KmUI+ZixPsKNTUv/y?=
 =?us-ascii?Q?pBdD81AatTBQg5Kq63RvtQsi3l7248skkrRwCvRmX9cDQYtO5Eb7+1NpdZ1G?=
 =?us-ascii?Q?DMBCE6GZOiCSDLUPmVvIzfCSzSBR7w1PLK30+2Y2p4jTjOL5F3ZLqFMlmllP?=
 =?us-ascii?Q?7rTO+C+ru8cmWBNDzXsgcdciRH5YDZKDDPq8dEfW36AvqVcJVAUka+16pRNO?=
 =?us-ascii?Q?F+f793pxkO7aKqh7mr+22C3vYnf7y8rb/TTgVnbx88d/q5pWhyN4IjKSVIgP?=
 =?us-ascii?Q?fKpnPxpyvkclgMP3MBYGCpgcUwl/g3H3VQgEjnfGvzric61mQK6wLAp12Vbw?=
 =?us-ascii?Q?BgDeq+UPFkLKUNaDQSHvRapD2mXlGL1BTSAjK34iE1EWndq6QY6Uo+E+HIOq?=
 =?us-ascii?Q?wvrneCE5uGTD9bXUi4fqhRss7QDPXoO5zDpoJRc46mctbVAfnnKPJZHhhrMz?=
 =?us-ascii?Q?5IJsp7osoKEHR//hQj4vWCo7l7o/nPYH3rJp4G/cXAGfGtXOvMx49dDm/c5r?=
 =?us-ascii?Q?gbxp0Pm+DdSPlxuSpoS1lh5BHT9OMo0tUXleHyCEhjIe+KiHtYari0+QWc8U?=
 =?us-ascii?Q?+F3C5Zid0bzyNIj4OjbCN24TWcx3Q6VelOZ8XMARyZXD03KPy5eYuH9ytADf?=
 =?us-ascii?Q?GyBct6i5wkb9Y5VgQa+KIEJaf0v0qZEFKT1/EGGldLAYJypyysiXBuXXM8OM?=
 =?us-ascii?Q?1SB3WOOCgfR6MA2y2v24OJZ9sLGY8zwx36a4DU0SZh4CrqiGPRHpHBCRX1tv?=
 =?us-ascii?Q?tQZLRiKXoc1wVMO2oEYPJ3Rn2GHAp2XmfpGnQtggPIabV3BtXklQkvXv4jh0?=
 =?us-ascii?Q?CLJl+otZIkf3UKULKGMlNxCvqAZUmvXKXrou3tgrjqgpqGnj4VeD3is3ythO?=
 =?us-ascii?Q?eAQ3jsIIfD/uV6oVHGdT2TseaBCgxKGnA2KB2R1I9FVejn0HZf7mmBH9MCHL?=
 =?us-ascii?Q?Xg=3D=3D?=
X-OriginatorOrg: memverge.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dfec24b8-684c-4b34-27cc-08db0e0555d5
X-MS-Exchange-CrossTenant-AuthSource: BN6PR17MB3121.namprd17.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Feb 2023 21:00:28.3527
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 5c90cb59-37e7-4c81-9c07-00473d5fb682
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: jGyInsZjTfR25kZPbTBhQFOGJ3f3psAAN/15mKUqOg9eZp+V9ctJMqV7OOne/LBgMRo3lazPeaX1WthhV/yctzouvw/+XOCiDYIM8TNpX5c=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR17MB4843
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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