[CRIU] [PATCH 3/4] vdso: Rework vdso processing files

[Laurent Dufour]

On 15/09/2015 15:54, Christopher Covington wrote:
> Hi Laurent,
> 
> On 09/03/2015 10:26 AM, Laurent Dufour wrote:
>> There were multiple copy of the same code spread over the different
>> architectures handling the vDSO.
>>
>> This patch is merging the duplicated code in arch/*/vdso-pie.c and
>> arch/*/include/asm/vdso.h in the common files and let only the architecture
>> specific part in the arch/*/* files.
> 
> Thanks for doing this! I've finally taken these changes for a spin and found
> the following issue.

Hi Christopher,

Thanks for debugging my stuff :)

> 
>> diff --git a/arch/aarch64/vdso-pie.c b/arch/aarch64/vdso-pie.c
>> index c6558378db1d..0f06c2d191d1 100644
>> --- a/arch/aarch64/vdso-pie.c
>> +++ b/arch/aarch64/vdso-pie.c
>> @@ -1,23 +1,10 @@
>> -#include <stdlib.h>
>> -#include <stdio.h>
>>  #include <unistd.h>
>> -#include <string.h>
>> -#include <elf.h>
>> -#include <fcntl.h>
>> -#include <errno.h>
>> -
>> -#include <sys/types.h>
>> -#include <sys/stat.h>
>> -#include <sys/mman.h>
>>  
>>  #include "asm/string.h"
>>  #include "asm/types.h"
>>  
>> -#include "compiler.h"
>>  #include "syscall.h"
>> -#include "image.h"
>> -#include "vdso.h"
>> -#include "vma.h"
>> +#include "parasite-vdso.h"
>>  #include "log.h"
>>  #include "bug.h"
>>  
>> @@ -26,7 +13,7 @@
>>  #endif
>>  #define LOG_PREFIX "vdso: "
>>  
>> -int vdso_redirect_calls(void *base_to, void *base_from,
>> +int vdso_redirect_calls(unsigned long base_to, unsigned long base_from,
>>  			struct vdso_symtable *to,
>>  			struct vdso_symtable *from)
>>  {
>> @@ -37,8 +24,8 @@ int vdso_redirect_calls(void *base_to, void *base_from,
>>  			continue;
>>  
>>  		pr_debug("br: %lx/%lx -> %lx/%lx (index %d)\n",
>> -			 (unsigned long)base_from, from->symbols[i].offset,
>> -			 (unsigned long)base_to, to->symbols[i].offset, i);
>> +			 base_from, from->symbols[i].offset,
>> +			 base_to, to->symbols[i].offset, i);
>>  
>>  		write_intraprocedure_branch(base_to + to->symbols[i].offset,
>>  					    base_from + from->symbols[i].offset);
>> @@ -46,383 +33,3 @@ int vdso_redirect_calls(void *base_to, void *base_from,
>>  
>>  	return 0;
>>  }
>> -
>> -
>> -/* Check if pointer is out-of-bound */
>> -static bool __ptr_oob(void *ptr, void *start, size_t size)
>> -{
>> -	void *end = (void *)((unsigned long)start + size);
>> -	return ptr > end || ptr < start;
>> -}
>> -
>> -/*
>> - * Elf hash, see format specification.
>> - */
>> -static unsigned long elf_hash(const unsigned char *name)
>> -{
>> -	unsigned long h = 0, g;
>> -
>> -	while (*name) {
>> -		h = (h << 4) + *name++;
>> -		g = h & 0xf0000000ul;
>> -		if (g)
>> -			h ^= g >> 24;
>> -		h &= ~g;
>> -	}
>> -	return h;
>> -}
>> -
>> -int vdso_fill_symtable(char *mem, size_t size, struct vdso_symtable *t)
>> -{
>> -	Elf64_Phdr *dynamic = NULL, *load = NULL;
>> -	Elf64_Ehdr *ehdr = (void *)mem;
>> -	Elf64_Dyn *dyn_strtab = NULL;
>> -	Elf64_Dyn *dyn_symtab = NULL;
>> -	Elf64_Dyn *dyn_strsz = NULL;
>> -	Elf64_Dyn *dyn_syment = NULL;
>> -	Elf64_Dyn *dyn_hash = NULL;
>> -	Elf64_Word *hash = NULL;
>> -	Elf64_Phdr *phdr;
>> -	Elf64_Dyn *d;
>> -
>> -	Elf64_Word *bucket, *chain;
>> -	Elf64_Word nbucket, nchain;
>> -
>> -	/*
>> -	 * See Elf specification for this magic values.
>> -	 */
>> -	const char elf_ident[] = {
>> -		0x7f, 0x45, 0x4c, 0x46, 0x02, 0x01, 0x01, 0x00,
>> -		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> -	};
>> -
>> -	const char *vdso_symbols[VDSO_SYMBOL_MAX] = {
>> -		[VDSO_SYMBOL_CLOCK_GETRES]	= VDSO_SYMBOL_CLOCK_GETRES_NAME,
>> -		[VDSO_SYMBOL_CLOCK_GETTIME]	= VDSO_SYMBOL_CLOCK_GETTIME_NAME,
>> -		[VDSO_SYMBOL_GETTIMEOFDAY]	= VDSO_SYMBOL_GETTIMEOFDAY_NAME,
>> -		[VDSO_SYMBOL_RT_SIGRETURN]	= VDSO_SYMBOL_RT_SIGRETURN_NAME,
>> -	};
> 
> I need to debug further, but I think the code that replaced this is somehow
> not properly position independent when I build for AArch64 and run. I'm
> getting a segfault. The faulting address (for example 0x000055d8) is
> vdso_symbols[0].

The function vdso_fill_symtable has been moved to pie/util-vdso.c which
is built twice, for criu (not requiring position independent code), for
the parasite code in position independent code.

I took a look to the pie/Makefile file and the -fpie GCC's option should
be raised for aarch64:

ifneq ($(filter-out i386 ia32, $(ARCH)),)
cflags-y += -DCR_NOGLIBC -fpie -Wa,--noexecstack -fno-stack-protector
else
...

This being said, my understanding is that the restarted process is
getting a segfault when it tries to access the vDSO, in this case, this
may be due to the trampoline code, in arch/aarch64/intraprocedure.S, but
that part has not been touched.

The only change I see in this area, which may impact, is the type of the
vdso_redirect_calls()'s parameters base_to and base_from, changed from
void* to unsigned long. This was required since these parameters are
used to compute pointers using the + operator.

Regarding vdso_redirect_calls() it is built only once (in PIE mode)
since it is only used in the parasite's context.

> 
> Some corresponding debug output I added:
> 
> pie: vdso: pc = 0x13b58
> pie: vdso: vdso_symbols = 0x146e8
> pie: vdso: vdso_symbols[0] = 0x55d8

What about the output printed in vdso_redirect_calls() ?

		pr_debug("br: %lx/%lx -> %lx/%lx (index %d)\n",
			 base_from, from->symbols[i].offset,
			 base_to, to->symbols[i].offset, i);


> I'll keep investigating, but any suggested anyone has would be appreciated.

I'm available on the irc channel #criu on freenode, nickame ldu4, if you
want more interactive help here...

By the way, did you patch the kernel to track the vDSO remap operation ?

Cheers,
Laurent.

> 
> I see this with both 4k and 64k pages (4k version above).
> 
> Thanks,
> Christopher Covington
>