[CRIU] [PATCHv5 3/3] crtools: cr_service() meat and a few fixes to properly dump cr_service socket

[Pavel Emelyanov]

On 09/09/2013 04:30 PM, Ruslan Kuprieiev wrote:
> On 09/09/2013 04:14 PM, Ruslan Kuprieiev wrote:
>> Signed-off-by: Ruslan Kuprieiev kupruser at gmail.com
>>
> I'm sorry, there was a whitespace. Here is fixed one.
> 


OK, this is almost perfect from the coding point of view :)
A couple of comments inline, after fixing them we'll concentrate
on RPC messages themselves.

> +static int treat_req(CriuDumpReq *req, CriuDumpResp *resp)
> +{
> +	struct ucred ids;
> +	struct stat st;
> +	socklen_t ids_len = sizeof(struct ucred);
> +
> +	if (getsockopt(client_fd, SOL_SOCKET, SO_PEERCRED, &ids, &ids_len)) {
> +		pr_perror("Can't get socket options.");
> +		goto err;
> +	}
> +
> +	if (req->pid == -1) {
> +		req->pid = ids.pid;
> +	} else if (req->pid != ids.pid) {
> +		/*
> +		 * FIXME Add some flag to resp for this case.

FIXME -- add proper uids checks here

> +		 */
> +		if (ids.uid != 0) {
> +			pr_perror("Client has no permissions to dump task %d",
> +								     req->pid);
> +			goto err;
> +		}
> +	}
> +
> +	/* going to dir, where to place images*/
> +	if (req->images_dir == NULL) {
> +		pr_perror("No images dir");
> +		goto err;
> +	}
> +
> +	if (stat(req->images_dir, &st) == -1) {

This stat and chdir below should be fixed. If a task lives in another
mount namespace or in chroot the path it issued to you would not refer
to the proper directory from criu.

Instead, you should open task's root via /proc/pid/root and then do
fstatat and openat + fchdir relative to his root.

> +		pr_perror("Can't stat images direcrtory");
> +		goto err;
> +	}
> +
> +	if (ids.uid == st.st_uid || ids.gid == st.st_gid || ids.uid == 0) {

This check is pointless. Kernel would do it itself properly.

> +		if (chdir(req->images_dir)) {
> +			pr_perror("Can't change dir");
> +			goto err;
> +		}
> +	} else {
> +		pr_perror("Client has no permissions to use images directory");
> +		goto err;
> +	}
> +
> +	/* initiate log file in imgs dir */
> +	opts.output = xmalloc(sizeof(char)*PATH_MAX);
> +	strcpy(opts.output, getcwd(NULL, PATH_MAX));
> +	strcat(opts.output, "/dump.log");

Don't use absolute paths for log file. log_init works fine with relative.

> +
> +	if (open_image_dir() < 0) {
> +		pr_perror("Can't open current directory.");
> +		goto err;
> +	}
> +
> +	/* setting loglevel */
> +	log_set_loglevel(req->log_level);
> +	if (log_init(opts.output) == -1) {
> +		pr_perror("Can't initiate log.");
> +		goto err;
> +	}
> +
> +	/* checking dump flags from client */
> +	if (req->leave_running)
> +		opts.final_state = TASK_ALIVE;
> +
> +	opts.ext_unix_sk	= req->ext_unix_sk;
> +	opts.tcp_established_ok	= req->tcp_established;
> +	opts.evasive_devices	= req->evasive_devices;
> +	opts.shell_job		= req->shell_job;
> +	opts.handle_file_locks	= req->file_locks;
> +
> +	return 0;
> +
> +err:	resp->bad_args	= true;

err:
	resp->bad_args = true;

> +	return -1;
> +}
> +
> +int cr_service(bool daemon_mode)
> +{
> +	int server_fd;
> +	int child_pid;
> +	struct sockaddr_un server_addr;
> +	struct sockaddr_un client_addr;
> +	socklen_t server_addr_len;
> +	socklen_t client_addr_len;
> +	CriuDumpReq *req;
> +	CriuDumpResp resp = CRIU_DUMP_RESP__INIT;
> +
> +	server_fd = socket(AF_LOCAL, SOCK_STREAM, 0);
> +	if (server_fd == -1) {
> +		pr_perror("Can't initialize service socket.");
> +		return -1;
> +	}
> +
> +	memset(&server_addr, 0, sizeof(server_addr));
> +	memset(&client_addr, 0, sizeof(client_addr));
> +	server_addr.sun_family = AF_LOCAL;
> +
> +	if (opts.addr == NULL)
> +		opts.addr = CR_DEFAULT_SERVICE_ADDRESS;
> +
> +	strcpy(server_addr.sun_path, opts.addr);
> +
> +	server_addr_len = strlen(server_addr.sun_path)
> +			+ sizeof(server_addr.sun_family);
> +	client_addr_len = sizeof(client_addr);
> +
> +	unlink(server_addr.sun_path);
> +
> +	if (bind(server_fd, (struct sockaddr *) &server_addr,
> +					server_addr_len) == -1) {
> +		pr_perror("Can't bind.");
> +		return -1;
> +	}

Some message in log about where we bound to would be nice.

> +
> +	/* change service socket permissions, so anyone can connect to it */
> +	if (chmod(server_addr.sun_path, 0666)) {
> +		pr_perror("Can't change permissions of the service socket.");
> +		return -1;
> +	}
> +
> +	if (daemon_mode) {
> +		if (daemon(0, 0) == -1) {
> +			pr_perror("Can't run service server in the background");
> +			return -errno;
> +		}
> +	}

Going daemon should be _after_ switching socket into listen state.

> +
> +	if (listen(server_fd, 16) == -1) {
> +		pr_perror("Can't listen for socket connections.");
> +		return -1;
> +	}
> +
> +	/* FIXME Do not ignore children's return values */
> +	signal(SIGCHLD, SIG_IGN);
> +
> +	while (1) {
> +		pr_info("Waiting for connection...\n");
> +
> +		client_fd = accept(server_fd, &client_addr, &client_addr_len);
> +		if (client_fd == -1) {
> +			pr_perror("Can't accept connection.");
> +			continue;
> +		}
> +
> +		pr_info("Connected.\n");
> +
> +		switch (child_pid = fork()) {
> +		case -1:
> +			pr_perror("Can't fork a child.");
> +			continue;
> +		case 0:
> +
> +			if (pb_read_one_eof(client_fd,

I think in RPC we should use raw pb messages w/o u32 size-s pb_foo engine adds.

> +						&req, PB_CRIU_DUMP_REQ) == -1) {
> +				pr_perror("Can't recv request");
> +				resp.bad_args = true;
> +				goto exit;
> +			}
> +
> +			if (treat_req(req, &resp) == -1) {
> +				pr_perror("Arguments treating fail");
> +				goto exit;
> +			}
> +
> +			if (cr_dump_tasks(req->pid) == -1) {
> +				pr_perror("Dump fail");
> +				goto exit;
> +			}
> +
> +			resp.success = true;
> +exit:			if (pb_write_one(client_fd,

smth strange with formatting

Other than this -- if the tasks' final state is "dead" and dump succeeded
then we might want not to write the response back.

> +					 &resp, PB_CRIU_DUMP_RESP) == -1) {
> +				pr_perror("Can't send response");
> +				resp.success = false;
> +			}
> +
> +			close(client_fd);
> +
> +			if (resp.success)
> +				exit(0);
> +			else
> +				exit(1);

exit(resp.success ? 0 : 1);

> +		default:
> +			close(client_fd);
> +		}
> +	}
> +
>  	return 0;
>  }