[CRIU] [PATCH] cr-super: Initial commit

[Pavel Emelyanov]

On 09/16/2015 04:43 PM, Cyrill Gorcunov wrote:
> On Wed, Sep 16, 2015 at 02:55:09PM +0300, Andrew Vagin wrote:
>>
>> In addition, we need to check that we are able to attache to a process
>> by ptrace.
>>
>> I think for that we need to drop CAP_SYS_PTRACE from the effective set,
>> try to call PTRACE_SEIZE and if this operation was success, we can read
>> map_files.
> 
> What for? It's already checked by criu itself. Seizing process is not
> that cheap operation, which actually changes internal state of a task
> we're poking while this wrapper is supposed to be very fast and
> "client" filtering should be based on groups (say group criu)
> or any other auth mechanism. Moreover, in the kernel there
> is a check for PTRACE_MODE_READ not for PTRACE_MODE_ATTACH
> (as happens for ptrace-seizing) so it's not 1:1 map of
> what map-files reading does. Am I missing something obvious?
> 
>>> +++ b/super/cr-super.h
>>> @@ -0,0 +1,59 @@
>>> +#ifndef __CR_SUPER_H__
>>> +#define __CR_SUPER_H__
>>> +
>>> +#include <limits.h>
>>> +
>>> +enum {
>>> +	SUPER_RSP_OK				= 1,
>>> +	SUPER_RSP_ERR				= 2,
>>> +
>>> +	SUPER_REQ_EXIT				= 3,
>>> +
>>> +	SUPER_REQ_MFD_OPEN_DIR			= 4,
>>
>> Why do we need to have a separate command to open a directory?
>>
>> We can cache a directory desriptor and open a directory only
>> if prev_pid isn't equal to req->pid.
>>
>>> +	SUPER_REQ_MFD_FILL_STAT			= 5,
>>> +	SUPER_REQ_MFD_FILL_LINK			= 6,
>>> +	SUPER_REQ_MFD_FILL_MNT_ID		= 7,
>>
>> Why we can't have one only one command?
> 
> Look, there is one problem -- aufs, it might ask for

Let's forget the aufs case for non-root usage. It was invented
for Docker to workaround the fs's problem. Now aufs is not used
by it, it's replaced with overlayfs.

> reading link, so we have to pass the complete PATH_MAX
> size via net. But I wanted to minimize net usage impact
> so the packets are small in size (ie there usually gonna
> be the following sequence
> 
> 	-> open-dir
> 	<- ok
> 	-> get-stat
> 	<- ok
> 
> and that's all. Probably merging open-dir with get-stat
> is a good idea. Thanks for the note.
> 
> Same time for auif it might endup in
> 
> 	-> opendir
> 	<- ok
> 	-> get-stat
> 	<- ok
> 	-> read-link (huge size)
> 	<- ok
> 	-> parse mnt-id
> 	<- ok
> 
>>> +
>>> +#define __check_opened(__r)					\
>>> +	do {							\
>>> +		if (mfd_dir_fd < 0) {				\
>>> +			pr_err("dir is not yet opened\n");	\
>>> +			__RSP_ERR(__r, -EINVAL);		\
>>> +			return -EINVAL;				\
>>> +		}						\
>>> +	} while (0)
>>
>> I don't like macroses which calls "return"
> 
> ok, i'll make it inline
> 
>>> +
>>> +	pr_debug("Listening\n");
>>> +	for (;;) {
>>> +		recv_size = recv(socket_fd, NULL, 0, MSG_TRUNC | MSG_PEEK);
>>> +		if (recv_size < sizeof(super_req_hdr_t)) {
>>> +			pr_perror("Can't read request");
>>> +			goto out;
>>> +		}
>>> +
>>> +		reqcv_buf = xmalloc(recv_size);
>>> +		if (!reqcv_buf)
>>> +			goto out;
>>> +
>>> +		recv_size = recv(socket_fd, reqcv_buf, recv_size, MSG_TRUNC);
>>> +		if (recv_size < 0) {
>>> +			pr_perror("Can't read request");
>>> +			goto out;
>>> +		} else if (recv_size == 0)
>>> +			goto out;
>>> +
>>> +		__TRACE_RCV_PKT(reqcv_buf);
>>> +
>>> +		ret = handle_request(socket_fd, &ids, reqcv_buf, recv_size);
>>> +		if (ret < 0)
>>> +			goto out;
>>> +
>>> +		__TRACE_SND_PKT(reqcv_buf);
>>> +
>>> +		send_size = send(socket_fd, reqcv_buf, recv_size, 0);
>>> +		if (send_size < recv_size) {
>>> +			pr_perror("Can't send request");
>>
>> Don't mix ret and the function exit code.
>> Here ret is zero, so you return zero code for the error case
> 
> yes, thanks, need to setup @ret here, missed.
> 
>>> +
>>> +usage:
>>> +	printf("%s: --fd <num>\n", argv[0][1]);
>>
>> This should be printed on stderr
> 
> ok
> .
>