[CRIU] Re: [PATCH cr 08/16] restore: don't unmap premmapped private vma-s

[Andrew Vagin]

On Tue, Oct 30, 2012 at 08:32:07PM +0400, Pavel Emelyanov wrote:
> > @@ -231,6 +233,23 @@ static int map_private_vma(pid_t pid, struct vma_area *vma, void *tgt_addr,
> >  	return 0;
> >  }
> >  
> > +/* Prevent merging vma-s outside and inside the premmapped region */
> 
> Why? Besides, having two guard pages doesn't prevent vmas from being merged.
This guard pages will be unmapped, then cr-restore reads
/proc/self/maps.

        ret = parse_smaps(pid, &self_vma_list, false);

In self_vma_list will will have a separate vma for a private space

restorer.c:
        for (vma_entry = args->self_vmas; vma_entry->start != 0; vma_entry++) {
                if (!vma_entry_is(vma_entry, VMA_AREA_REGULAR))
                        continue;

                if (args->premmapped_addr == vma_entry->start)
                        continue;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
we have only this line, if we use guard pages.

                if (sys_munmap((void *)vma_entry->start, vma_entry_len(vma_entry))) {
                        pr_err("Munmap fail for %lx\n", vma_entry->start);
                        goto core_restore_end;
                }
        }

Do you want this version or I should remove guards and handle merged vma-s.

The second variant will be faster and a bit more complex.

> 
> > +static int unmap_priv_guard_pages()
> > +{
> > +	if (munmap(premmapped_addr, PAGE_SIZE)) {
> > +		pr_perror("Could not unmap a guard page %p", premmapped_addr);
> > +		return -1;
> > +	}
> > +
> > +	if (munmap(premmapped_addr + premmapped_len - PAGE_SIZE, PAGE_SIZE)) {
> > +		pr_perror("Could not unmap a guard page %p",
> > +				premmapped_addr + premmapped_len);
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int read_vmas(int pid)
> >  {
> >  	int fd, ret = 0;
> > @@ -280,13 +299,26 @@ static int read_vmas(int pid)
> >  		priv_size += vma_area_len(vma);
> >  	}
> >  
> > -	/* Reserve a place for mapping private vma-s one by one */
> > -	addr = mmap(NULL, priv_size, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
> > +	/*
> > +	 * Reserve a place for mapping private vma-s one by one.
> > +	 *
> > +	 * Add two guard pages from both sides. This pages will be
> > +	 * unmaped before reading a current vmas for this process.
> > +	 * In this case we can be sure, that vmas outside and inside
> > +	 * of the premmapped region are not merged to each other.
> > +	 */
> > +	addr = mmap(NULL, priv_size + 2 * PAGE_SIZE,
> > +			PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
> 
> If we DO need guard pages then implement this in separate patch with proper description.
> 
> >  	if (addr == MAP_FAILED) {
> >  		pr_perror("Unable to reserve memory");
> >  		return -1;
> >  	}
> >  
> > +	premmapped_addr = addr;
> > +	premmapped_len = priv_size + 2 * PAGE_SIZE;
> > +
> > +	addr += PAGE_SIZE;
> > +
> >  	list_for_each_entry(vma, &vma_list, list) {
> >  		if (!vma_priv(&vma->vma))
> >  			continue;
> > @@ -321,6 +321,10 @@ long __export_restore_task(struct task_restore_core_args *args)
> >  		if (!vma_entry_is(vma_entry, VMA_AREA_REGULAR))
> >  			continue;
> >  
> > +		if (args->premmapped_addr <= vma_entry->start &&
> > +		    vma_entry->end <= args->premmapped_addr + args->premmapped_len)
> > +			continue;
> 
> Why so complex? Isn't it easier just to remove this are from self_vma_list in cr-restore?
> 
> > +
> >  		if (sys_munmap((void *)vma_entry->start, vma_entry_len(vma_entry))) {
> >  			pr_err("Munmap fail for %lx\n", vma_entry->start);
> >  			goto core_restore_end;
> > 
> 
>