[CRIU] [PATCH 08/15] restore: don't unmap premmapped private vma-s (v2)

[Andrew Vagin]

On Mon, Nov 12, 2012 at 03:38:45PM +0400, Pavel Emelyanov wrote:
> On 11/02/2012 05:32 PM, Andrey Vagin wrote:
> > Private vma-s are mapped before forking children, then they are
> > remapped to corrected places in restorer.c.
> > 
> > In restorer all unneeded vma-s are unmaped. VMA-s from premmapped
> > regions should not be unmaped.
> > 
> > v2: replace guard pages on arithmetic in restorer
> > 
> > Signed-off-by: Andrey Vagin <avagin at openvz.org>
> > ---
> >  cr-restore.c       |  2 ++
> >  include/restorer.h |  2 ++
> >  restorer.c         | 28 +++++++++++++++++++++++++---
> >  3 files changed, 29 insertions(+), 3 deletions(-)
> > 
> > diff --git a/cr-restore.c b/cr-restore.c
> > index 0597e6b..7d05c31 100644
> > --- a/cr-restore.c
> > +++ b/cr-restore.c
> > @@ -1603,6 +1603,8 @@ static int sigreturn_restore(pid_t pid, CoreEntry *core)
> >  
> >  	mem += self_vmas_len;
> >  	task_args->tgt_vmas = vma_list_remap(mem, vmas_len, &rst_vma_list);
> > +	task_args->premmapped_addr = (unsigned long) premmapped_addr;
> > +	task_args->premmapped_len = premmapped_len;
> >  	if (!task_args->tgt_vmas)
> >  		goto err;
> >  
> > diff --git a/include/restorer.h b/include/restorer.h
> > index 8fa6c1d..377afff 100644
> > --- a/include/restorer.h
> > +++ b/include/restorer.h
> > @@ -99,6 +99,8 @@ struct task_restore_core_args {
> >  	struct task_entries		*task_entries;
> >  	VmaEntry			*self_vmas;
> >  	VmaEntry			*tgt_vmas;
> > +	unsigned long			premmapped_addr;
> > +	unsigned long			premmapped_len;
> >  	rt_sigaction_t			sigchld_act;
> >  
> >  	struct itimerval		itimers[3];
> > diff --git a/restorer.c b/restorer.c
> > index 369adb9..6c0d888 100644
> > --- a/restorer.c
> > +++ b/restorer.c
> > @@ -327,6 +327,7 @@ long __export_restore_task(struct task_restore_core_args *args)
> >  	long ret = -1;
> >  	VmaEntry *vma_entry;
> >  	u64 va;
> > +	unsigned long premmapped_end = args->premmapped_addr + args->premmapped_len;
> >  
> >  	struct rt_sigframe *rt_sigframe;
> >  	unsigned long new_sp;
> > @@ -344,12 +345,33 @@ long __export_restore_task(struct task_restore_core_args *args)
> >  	pr_info("Switched to the restorer %d\n", my_pid);
> >  
> >  	for (vma_entry = args->self_vmas; vma_entry->start != 0; vma_entry++) {
> > +		unsigned long addr = vma_entry->start;
> > +		unsigned long len;
> > +
> >  		if (!vma_entry_is(vma_entry, VMA_AREA_REGULAR))
> >  			continue;
> >  
> > -		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;
> > +		pr_debug("Examine %lx-%lx\n", vma_entry->start, vma_entry->end);
> > +
> > +		if (addr < args->premmapped_addr) {
> > +			if (vma_entry->end >= args->premmapped_addr)
> > +				len = args->premmapped_addr - addr;
> > +			else
> > +				len = vma_entry->end - vma_entry->start;
> > +			if (sys_munmap((void *) addr, len)) {
> > +				pr_err("munmap fail for %lx - %lx\n", addr, addr + len);
> > +				goto core_restore_end;
> > +			}
> > +		}
> > +
> > +		if (vma_entry->end > premmapped_end) {
> > +			if (vma_entry->start < premmapped_end)
> > +				addr = premmapped_end;
> > +			len = vma_entry->end - addr;
> > +			if (sys_munmap((void *) addr, len)) {
> > +				pr_err("munmap fail for %lx - %lx\n", addr, addr + len);
> > +				goto core_restore_end;
> > +			}
> 
> This isn't nice. Plz, rewrite it to look like
> 
> for () {
> 	if (/* skip_this_map */)
> 		continue;
> 	sys_munmap()
> }
We may need to call sys_munmap twice
For example we have three merged VMA-s:

|vma1|tgt vma|vma2|

vma1 and vma2 should be unmapped and tgt vma shoud remain.

> 
> not
> 
> for () {
> 	if (/* map is OK left */)
> 		sys_munmap();
> 	if (/* map is OK right */)
> 		sys_munmap();
> }
> 
> >  		}
> >  	}
> >  
> > 
> 
>