[CRIU] [PATCH 9/9] creds: restore -- Implement per-thread dump of credentials

[Cyrill Gorcunov]

On Fri, Dec 18, 2015 at 05:21:14PM +0300, Pavel Emelyanov wrote:
...
> >  
> > -static int dump_task_creds(struct parasite_ctl *ctl,
> > -			   const struct cr_imgset *fds)
> > +static int dump_task_creds(struct parasite_ctl *ctl, pid_t pid, CredsEntry *creds)
> >  {
> > -	CredsEntry ce = CREDS_ENTRY__INIT;
> > +	pr_info("Dumping creds for %d\n", pid);
> >  
> > -	pr_info("\n");
> > -	pr_info("Dumping creds for %d)\n", ctl->pid.real);
> > -	pr_info("----------------------------------------\n");
> > -
> > -	if (parasite_dump_creds(ctl, &ce) < 0)
> > -		return -1;
> > -
> > -	if (collect_lsm_profile(ctl->pid.real, &ce) < 0)
> > +	if (parasite_dump_creds(ctl, creds) < 0 ||
> > +	    collect_lsm_profile(ctl->pid.real, creds) < 0) {
> > +		pr_err("Dump creds (pid: %d) failed\n",pid);
> >  		return -1;
> > +	}
> 
> What's the reason for screwing up this code? Why two if-s were bad? Please,
> if you don't like the codeflow, fix it in separate patch, don't mix functional
> changes with cosmetic ones.

Because this is rather new function but with same name (note even
number of arguments is changed) so I wanted it be short, it doen't
make review harder neither it screw anything. I'm fine with original
flow but since I'm remaking the function anyway merging strings
should not cause any harm.

The former pr_info("---") and etc has very different meaning --
it was a separate stage of dumping but now it's a part of
"Core" dumpe so I threw out unneeded pr_info.

But fine, I'll redo.
...

> >  struct parasite_dump_creds {
> >  	unsigned int		cap_last_cap;
> > @@ -214,6 +188,30 @@ struct parasite_dump_creds {
> >  	unsigned int		groups[0];
> >  };
> >  
> > +struct parasite_dump_thread {
> > +	unsigned int			*tid_addr;
> > +	pid_t				tid;
> > +	tls_t				tls;
> > +	stack_t				sas;
> > +	int				pdeath_sig;
> > +	struct parasite_dump_creds	creds[0];
> > +};
> > +
> > +/*
> > + * Misc sfuff, that is too small for separate file, but cannot
> > + * be read w/o using parasite
> > + */
> > +
> > +struct parasite_dump_misc {
> > +	unsigned long			brk;
> > +	u32				pid;
> > +	u32				sid;
> > +	u32				pgid;
> > +	u32				umask;
> > +	int				dumpable;
> > +	struct parasite_dump_thread	ti;
> > +};
> 
> What's the reason for code move?

Obviously because I parasite_dump_thread now
carries parasite_dump_creds structure.

...

> >  int parasite_dump_thread_seized(struct parasite_ctl *ctl, int id,
> >  				struct pid *tid, CoreEntry *core)
> >  {
> >  	struct parasite_dump_thread *args;
> >  	pid_t pid = tid->real;
> >  	ThreadCoreEntry *tc = core->thread_core;
> > +	CredsEntry *creds = tc->creds;
> > +	struct parasite_dump_creds *pc;
> >  	int ret;
> >  	struct thread_ctx octx;
> >  
> > @@ -545,6 +582,9 @@ int parasite_dump_thread_seized(struct parasite_ctl *ctl, int id,
> >  
> >  	args = parasite_args(ctl, struct parasite_dump_thread);
> >  
> > +	pc = args->creds;
> > +	pc->cap_last_cap = kdat.last_cap;
> 
> This is per-thread constant, leave it in per-task.

Please, lets keep this data close to threads. It's just
really small one and makes code a way more conveniten
to work with -- the number of bits is bound close to
caps keepers itselves.


> > +
> >  	ret = get_thread_ctx(pid, &octx);
> >  	if (ret)
> >  		return -1;
> > @@ -552,6 +592,7 @@ int parasite_dump_thread_seized(struct parasite_ctl *ctl, int id,
> >  	tc->has_blk_sigset = true;
> >  	memcpy(&tc->blk_sigset, &octx.sigmask, sizeof(k_rtsigset_t));
> >  
> > +
> 
> Trash.

Да ладно :) Yah, thanks, will drop.

> > +static int dump_misc(struct parasite_dump_misc *args)
> > +{
> > +	args->brk = sys_brk(0);
> > +
> > +	args->pid = sys_getpid();
> > +	args->sid = sys_getsid();
> > +	args->pgid = sys_getpgid(0);
> > +	args->umask = sys_umask(0);
> > +	sys_umask(args->umask); /* never fails */
> > +	args->dumpable = sys_prctl(PR_GET_DUMPABLE, 0, 0, 0, 0);
> > +
> > +	return dump_thread_common(&args->ti, false);
> > +}
> 
> What's the reason for code move?

Because dump_thread_common not only fetches misc params but
creds as well. Note we call for "task" this routine via
parasite daemon, while for threads -- via do-trap stop.

dump_misc is called from criu main code to fetch misc params
without creds, in turn when second argument is true, the
creds are fetched as well.

> > +
> >  static int drain_fds(struct parasite_drain_fd *args)
> >  {
> >  	int ret;
> > @@ -274,7 +279,7 @@ static int drain_fds(struct parasite_drain_fd *args)
> >  static int dump_thread(struct parasite_dump_thread *args)
> >  {
> >  	args->tid = sys_gettid();
> > -	return dump_thread_common(args);
> > +	return dump_thread_common(args, true);
> 
> Why true here and false above?

Because for threads we dump creds together with misc, unlike to
group leader.

Look, here is a big picture

dump_one_task
 parasite_infect_seized
 parasite_dump_misc_seized 	(calls for daemon to fetch misc params)
 dump_task_core_all 		(calls for daemon to fetch creds for the group leader)
 parasite_stop_daemon
 dump_task_threads 		(calls for parasite with trap mode to fecth creds)

Sure we might need to cleanup the sequence, but
at moment I prefer smaller change. The patch
is huge already :(

> > +bool proc_status_creds_dumpable(struct proc_status_creds *parent,
> > +				struct proc_status_creds *child)
> >  {
> > -	/* FIXME: this is a little too strict, we should do semantic comparison
> > -	 * of seccomp filters instead of forcing them to be exactly identical.
> > -	 * It's not unsafe, though, so let's be lazy for now.
> > +	const size_t size = sizeof(struct proc_status_creds) -
> > +			offsetof(struct proc_status_creds, cap_inh);
> > +
> > +	/*
> > +	 * The comparision rules are the following
> > +	 *
> > +	 *  - CAPs can be different
> > +	 *  - seccomp filters should be passed via
> > +	 *    semantic comparision (FIXME) but for
> > +	 *    now we require them to be exactly
> > +	 *    identical
> 
> I don't get this part of comment. You _just_ force filters to match (btw,
> can different threads have different filters?).

Yes, filters can be different but here FIXME stands for it.
Ie I need to address this later.

	Cyrill