[CRIU] Re: [PATCH] rst: Fix creds vs threads restoration

[Andrey Vagin]

On Tue, Oct 30, 2012 at 10:37:17AM +0400, Pavel Emelyanov wrote:
> Writing to last_pid sysctl is CAP_SYS_ADMIN potected. Thus restoring
> creds before it won't work in all the cases.
> 
> Fix this by making all threads restore creds themselves, and the
> thread group leader -- after all of them.
> 
> #2410
> 
> Signed-off-by: Pavel Emelyanov <xemul at parallels.com>

Probably we should drop CAP_SYS_ADMIN in zdtm tests (futex*, pthread*)

Acked-by: Andrey Vagin <avagin at parallels.com>
> 
> ---
> 
> diff --git a/cr-restore.c b/cr-restore.c
> index e930b49..d47e34f 100644
> --- a/cr-restore.c
> +++ b/cr-restore.c
> @@ -1495,7 +1495,7 @@ static int sigreturn_restore(pid_t pid, CoreEntry *core, struct list_head *tgt_v
>  	if (ret < 0)
>  		goto err;
>  
> -	mutex_init(&task_args->t._rst_lock);
> +	mutex_init(&task_args->rst_lock);
>  
>  	/*
>  	 * Now prepare run-time data for threads restore.
> @@ -1537,7 +1537,7 @@ static int sigreturn_restore(pid_t pid, CoreEntry *core, struct list_head *tgt_v
>  			goto err;
>  		}
>  
> -		thread_args[i].rst_lock		= &task_args->t._rst_lock;
> +		thread_args[i].ta		= task_args;
>  		thread_args[i].gpregs		= *core->thread_info->gpregs;
>  		thread_args[i].clear_tid_addr	= core->thread_info->clear_tid_addr;
>  
> diff --git a/include/restorer.h b/include/restorer.h
> index 7c74a29..11750e4 100644
> --- a/include/restorer.h
> +++ b/include/restorer.h
> @@ -62,6 +62,8 @@ struct rst_sched_param {
>  	int prio;
>  };
>  
> +struct task_restore_core_args;
> +
>  /* Make sure it's pow2 in size */
>  struct thread_restore_args {
>  	struct restore_mem_zone		mem_zone;
> @@ -76,10 +78,7 @@ struct thread_restore_args {
>  
>  	struct rst_sched_param		sp;
>  
> -	union {
> -		mutex_t			_rst_lock;
> -		mutex_t			*rst_lock;
> -	};
> +	struct task_restore_core_args	*ta;
>  } __aligned(sizeof(long));
>  
>  struct task_restore_core_args {
> @@ -90,6 +89,8 @@ struct task_restore_core_args {
>  	int				logfd;
>  	unsigned int			loglevel;
>  
> +	mutex_t				rst_lock;
> +
>  	/* threads restoration */
>  	int				nr_threads;		/* number of threads */
>  	thread_restore_fcall_t		clone_restore_fn;	/* helper address for clone() call */
> diff --git a/restorer.c b/restorer.c
> index 2dd3733..369adb9 100644
> --- a/restorer.c
> +++ b/restorer.c
> @@ -224,7 +224,9 @@ long __export_restore_thread(struct thread_restore_args *args)
>  	if (restore_thread_common(rt_sigframe, args))
>  		goto core_restore_end;
>  
> -	mutex_unlock(args->rst_lock);
> +	mutex_unlock(&args->ta->rst_lock);
> +
> +	restore_creds(&args->ta->creds);
>  
>  	futex_dec_and_wake(&task_entries->nr_in_progress);
>  
> @@ -529,14 +531,6 @@ long __export_restore_task(struct task_restore_core_args *args)
>  			goto core_restore_end;
>  		}
>  
> -		/* 
> -		 * last-pid is CAP_SYS_ADMIN protected, thus restore creds
> -		 * _after_ opening that file, but before fork to make threads
> -		 * inherit them properly
> -		 */
> -
> -		restore_creds(&args->creds);
> -
>  		ret = sys_flock(fd, LOCK_EX);
>  		if (ret) {
>  			pr_err("Can't lock last_pid %d\n", fd);
> @@ -550,7 +544,7 @@ long __export_restore_task(struct task_restore_core_args *args)
>  			if (thread_args[i].pid == args->t.pid)
>  				continue;
>  
> -			mutex_lock(&args->t._rst_lock);
> +			mutex_lock(&args->rst_lock);
>  
>  			new_sp =
>  				RESTORE_ALIGN_STACK((long)thread_args[i].mem_zone.stack,
> @@ -613,8 +607,14 @@ long __export_restore_task(struct task_restore_core_args *args)
>  		}
>  
>  		sys_close(fd);
> -	} else
> -		restore_creds(&args->creds);
> +	}
> +
> +	/* 
> +	 * Writing to last-pid is CAP_SYS_ADMIN protected, thus restore
> +	 * creds _after_ all threads creation.
> +	 */
> +
> +	restore_creds(&args->creds);
>  
>  	futex_dec_and_wake(&args->task_entries->nr_in_progress);
>