[CRIU] [PATCH 1/4] vdso: Introduce vdso engine

[Pavel Emelyanov]

Split the patch. It contains too many independent pieces of code.

> +++ b/arch/x86/vdso.c

> +static const char vdso_ident[] = {
> +	0x7f, 0x45, 0x4c, 0x46, 0x02, 0x01, 0x01, 0x00,
> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +};

What is this? Where did these numbers come from?

> +/*
> + * FIXME Usually vDSO memory area has predefined structure (ie section
> + *       indices, thus we can optimize this routine and don't walk
> + *       over all sections but probe predefined first and if fail do
> + *       a complete walk over sections, this will speedup procedure
> + *       significantly.
> + *
> + *       Also we need more strict checking for section sizes and addresses
> + *       we obtain here -- there is no guarantee that data is always valid
> + *       (as it's assumed now).
> + */
> +int arch_parse_vdso(char *mem, size_t size, symtable_t *t)
> +{

What does "parse" wrt VDSO mean?

> +#define PM_PSHIFT_BITS      (6)
> +#define PM_STATUS_BITS      (3)
> +#define PM_STATUS_OFFSET    (64 - PM_STATUS_BITS)
> +#define PM_PSHIFT_OFFSET    (PM_STATUS_OFFSET - PM_PSHIFT_BITS)
> +#define PM_PFRAME_MASK      ((1ULL << PM_PSHIFT_OFFSET) - 1)
> +#define PM_PFRAME(x)        ((x) & PM_PFRAME_MASK)

We already have pagemap-related constants in mem.c. All should be in one place.

> +++ b/include/vdso.h

> +enum {
> +	VDSO_SYMBOL_GETTIMEOFDAY	= 0,

Doesn't enum auto-assign zero to the 1st member?

> +	VDSO_SYMBOL_GETCPU,
> +	VDSO_SYMBOL_CLOCK_GETTIME,
> +	VDSO_SYMBOL_TIME,
> +
> +	VDSO_SYMBOL_MAX
> +};

> +static inline bool vdso_is_symtable_empty(symtable_t *s)
> +{

Make the name more English, please.


Now concerning the below flood of symbols:

> +typedef struct vdso_mark_s {
> +	union {
> +		unsigned char	mag[SELFMAG];
> +		u32		_mag;

Why do you need this union?

> +	};
> +	union {
> +		unsigned char	sig[VDSO_MARK_SIG_SIZE];
> +		u64		_sig;

And this? What if sizeof(sig) != sizeof(sig)? Will it crash?

> +	};
> +	unsigned long		ref;

What is this field for?

> +} __packed vdso_mark_t;
> +
> +#define VDSO_MARK_INIT						\
> +	{							\
> +		.mag	= {					\
> +			0x7f, 0x45, 0x4c, 0x46,			\
> +		},						\
> +								\
> +		/* This is VDSO_PROXY_SIG string */		\
> +		.sig	= {					\
> +			0x63, 0x72, 0x69, 0x75,			\
> +			0x76, 0x64, 0x73, 0x6f,			\

That's not nice to have the same string coded twice, one time in
letters, the other one in digits.

> +		},						\
> +		.ref	= VDSO_BAD_ADDR,			\
> +	}
> +
> +#define INIT_VDSO_MARK(m)					\
> +	*(m) = (vdso_mark_t)VDSO_MARK_INIT
> +
> +static inline int is_vdso_mark(void *addr)
> +{
> +	vdso_mark_t s = VDSO_MARK_INIT;
> +	vdso_mark_t *d = addr;
> +
> +	BUILD_BUG_ON(sizeof(s._mag) != sizeof(s.mag));
> +	BUILD_BUG_ON(sizeof(s._sig) != sizeof(s.sig));
> +
> +	return s._mag == d->_mag && s._sig == d->_sig;

So all of the above are just to compare two 64-bit integers?

> +}

Summary: This is horrible.


> +
> +int vdso_init(void)
> +{
> +	if (arch_parse_self_vdso(&vdso_proxy.sym_rt))
> +		return -1;
> +	if (arch_get_vdso_pfn(getpid(), vdso_proxy.sym_rt.vma_start, &vdso_pfn))
> +		return -1;
> +	return 0;
> +}
> +

The arch_get_vdso_pfn should be in kerndat. Parsing self-vdso presumably there as well.

> +int vdso_analyse_vmas(struct list_head *vma_list)
> +{

Analyze for what?

> +int vdso_analyse_vmas(struct list_head *vma_list)
> +{
> +	bool has_vdso = false;
> +	struct vma_area *vma;
> +	unsigned int i;
> +
> +	pr_debug("Analysing dumpee VMAs\n");
> +
> +	list_for_each_entry(vma, vma_list, list) {
> +		if (!vma_entry_is(&vma->vma, VMA_AREA_VDSO))
> +			continue;
> +
> +		if (has_vdso) {
> +			pr_err("Got second vDSO area while only one expected\n");
> +			return -1;
> +		} else
> +			has_vdso = true;

Obfuscating else.

> +
> +		INIT_SYMTABLE(&vdso_proxy.sym_dumpee);
> +		vdso_proxy.remap_rt_at = 0;
> +		vdso_proxy.proxify = false;

You set it to true several lines of code below.

> +		vdso_proxy.directly = true;
> +
> +		vdso_proxy.premmaped_at = (unsigned long)vma_premmaped_start(&(vma->vma));
> +		vdso_proxy.sym_dumpee.vma_start = vma->vma.start;
> +		vdso_proxy.sym_dumpee.vma_end = vma->vma.end;
> +
> +		if (arch_parse_vdso((void *)vdso_proxy.premmaped_at,
> +				    symtable_vma_size(&vdso_proxy.sym_dumpee),
> +				    &vdso_proxy.sym_dumpee))
> +			return -1;
> +
> +		if (symtable_vma_size(&vdso_proxy.sym_dumpee) ==
> +		    symtable_vma_size(&vdso_proxy.sym_rt)) {
> +			for (i = 0; i < ARRAY_SIZE(vdso_proxy.sym_dumpee.sym); i++) {
> +				if (vdso_proxy.sym_dumpee.sym[i].offset !=
> +				    vdso_proxy.sym_rt.sym[i].offset) {
> +					vdso_proxy.directly = false;
> +					break;
> +				}
> +			}
> +		}
> +
> +		vdso_proxy.proxify = true;
> +	}
> +
> +	return 0;
> +}