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

[Cyrill Gorcunov]

On Fri, Dec 18, 2015 at 05:14:34PM +0300, Pavel Emelyanov wrote:
...
> > +static struct thread_creds_args *
> > +rst_prep_creds_args(struct thread_creds_args *prev, CredsEntry *ce)
> > +{
> > +	unsigned long this_pos = rst_mem_cpos(RM_PRIVATE);
> > +	struct thread_creds_args *args;
> > +
> > +	if (!verify_cap_size(ce)) {
> > +		pr_err("Caps size mismatch %d %d %d %d\n",
> > +		       (int)ce->n_cap_inh, (int)ce->n_cap_eff,
> > +		       (int)ce->n_cap_prm, (int)ce->n_cap_bnd);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	if (!may_restore(ce))
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	args = rst_mem_alloc(sizeof(*args), RM_PRIVATE);
> > +	if (!args)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	args->cap_last_cap = kdat.last_cap;
> 
> This is per-thread constant, leave it on task_args.

What for? I use this variable inside pie/restorer, where arguments
are only _thread_ data, not task, so I need a copy here. Sure I can
add some references to access this variable from task args but I think
it makes code even more vague.

...
> > +static int rst_prep_creds(pid_t pid, CoreEntry *core, unsigned long *creds_pos)
> > +{
> > +	size_t i;
> > +
> > +	/*
> > +	 * This is _really_ very old image
> > +	 * format where @thread_core were not
> > +	 * present. It means we don't have
> > +	 * creds either, just ignore and exit
> > +	 * early.
> > +	 */
> > +	if (unlikely(!core->thread_core)) {
> > +		*creds_pos = 0;
> > +		return 0;
> > +	}
> > +
> > +	*creds_pos = rst_mem_cpos(RM_PRIVATE);
> > +
> > +	/*
> > +	 * Old format: one Creds per task carried in own image file.
> > +	 */
> > +	if (!core->thread_core->creds)
> > +		return rst_prep_creds_from_img(pid);
> 
> This would produce only one creds object at creds_cpos, while the loop
> below would produce several of them. But the threads_args code ... more
> below scans (should scan) creds_pos as array which would only work for
> "new" case.

Yes, for legacy case we create one object which get used for task only,
the ->next field always null. Note the legacy case gonna be out of
date pretty soon so consider it rather as exception than a regular
basis.

> > +
> > +	for (i = 0; i < current->nr_threads; i++) {
> > +		CredsEntry *ce = current->core[i]->thread_core->creds;
> > +		struct thread_creds_args *args = NULL;
> > +
> > +		args = rst_prep_creds_args(args, ce);
> 
> The args as a function argument is always NULL here, so it is in the legacy
> function above. Is this intended behavior?

Nope, just non refreshed sources, thanks! I'll update.

> > @@ -3157,6 +3218,8 @@ static int sigreturn_restore(pid_t pid, CoreEntry *core)
> >  		thread_args[i].clear_tid_addr	= CORE_THREAD_ARCH_INFO(tcore)->clear_tid_addr;
> >  		core_get_tls(tcore, &thread_args[i].tls);
> >  
> > +		rst_reloc_creds(&thread_args[i], &creds_pos_next);
> 
> The creds_pos_next is write-only constant variable here. Why do threads get
> different creds after all?

Same here, forgot to refresh. I'm preparing new bundle now.

> > +
> >  		if (tcore->thread_core) {
> >  			thread_args[i].has_futex	= true;
> >  			thread_args[i].futex_rla	= tcore->thread_core->futex_rla;
> 
> > @@ -884,7 +895,7 @@ long __export_restore_task(struct task_restore_args *args)
> >  	log_set_fd(args->logfd);
> >  	log_set_loglevel(args->loglevel);
> >  
> > -	cap_last_cap = args->cap_last_cap;
> > +	cap_last_cap = args->t->creds_args->cap_last_cap;
> 
> The global cap_last_cap becomes unused after this patch.

Yes, thanks, dropped.