[CRIU] [PATCH 3/4] vdso: Escape double dumping of rt-vdso if proxy present

[Pavel Emelyanov]

On 05/22/2013 12:37 PM, Cyrill Gorcunov wrote:
> 
> In case if we have created vdso proxy the rt-vdso should
> not be dumped because it will be re-created on next restore
> anyway. Thus with help of parasite service routine find
> the rt-vdso and tear it off from dumping.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov at openvz.org>
> ---
>  cr-dump.c                  | 10 ++++++
>  include/parasite-syscall.h |  3 ++
>  include/parasite.h         |  8 +++++
>  parasite-syscall.c         | 76 ++++++++++++++++++++++++++++++++++++++++++++++
>  pie/parasite.c             | 18 +++++++++++
>  5 files changed, 115 insertions(+)
> 


Why is this patch #3? We don't yet know what the proxy is.

> +int parasite_handle_special_vmas(struct parasite_ctl *ctl, pid_t pid,
> +				 struct vm_area_list *vma_area_list)

Bad fn name.

> +{
> +	struct parasite_vdso_vma_entry *args;
> +	struct vma_area *backref = NULL;
> +	struct vma_area *vma, *k, *t;
> +	u64 pfn;
> +	int ret;
> +
> +	args = parasite_args(ctl, struct parasite_vdso_vma_entry);
> +
> +	list_for_each_entry_safe(vma, k, &vma_area_list->h, list) {
> +		if (!vma_area_is(vma, VMA_AREA_REGULAR))
> +			continue;
> +		if ((vma->vma.prot & VDSO_PROT) != VDSO_PROT)
> +			continue;
> +
> +		args->start = vma->vma.start;
> +		args->len = vma_area_len(vma);
> +
> +		ret = parasite_execute(PARASITE_CMD_PARSE_VDSO_MARK, ctl);
> +		if (ret) {
> +			pr_err("vdso: Parasite failed to poke for mark\n");
> +			return -1;
> +		}
> +
> +		ret = arch_get_vdso_pfn(pid, vma->vma.start, &pfn);
> +		if (ret)
> +			return -1;
> +		BUG_ON(!VDSO_PAGE_PRESENT(pfn));
> +
> +		if (args->is_marked) {

Is marked with what? You use too many generic words for exact things.

> +			pr_debug("vdso: Found marked at %lx (backref at %lx)\n",
> +				 (long)vma->vma.start, (long)args->ref);
> +
> +			/*
> +			 * Don't forget to restore the former vDSO status.
> +			 */

This comment explains nothing.

> +			BUG_ON(args->ref == VDSO_BAD_ADDR);
> +			BUG_ON(args->ref == (unsigned long)vma->vma.start);
> +
> +			list_for_each_entry(t, &vma_area_list->h, list) {
> +				if (t->vma.start == args->ref) {
> +					t->vma.status |= VMA_AREA_REGULAR | VMA_AREA_VDSO;
> +					backref = t;
> +					pr_debug("vdso: Restore status by backref at %lx\n",
> +						 (long)vma->vma.start);
> +					break;
> +				}
> +			}
> +
> +			pr_debug("vdso: Droppping vDSO at %lx\n", (long)vma->vma.start);
> +			list_del(&vma->list);
> +			xfree(vma);
> +
> +			break;
> +		} else {
> +			if (vdso_pfn_match(pfn)) {

Why is it nice to have a wrapper for "a == b" on every case? Like VDSO_PAGE_PRESENT
and this...

> +				if (!vma_area_is(vma, VMA_AREA_VDSO)) {
> +					pr_debug("vdso: Restore status by pfn at %lx\n",
> +						 (long)vma->vma.start);
> +					vma->vma.status |= VMA_AREA_REGULAR | VMA_AREA_VDSO;
> +				}
> +			} else {
> +				if (vma_area_is(vma, VMA_AREA_VDSO) && vma != backref) {
> +					pr_debug("vdso: Drop mishinted status at %lx\n",
> +						 (long)vma->vma.start);
> +					vma->vma.status &= ~VMA_AREA_VDSO;
> +				}

Huh? In this case we continue "handling" another vma. Is it OK? Why?

> +			}
> +		}
> +	}
> +	return 0;
> +}
> +