Re: [PATCH v4 10/10] tools/proctrace: add proctrace tool

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 30/06/2020 13:33, Michał Leszczyński wrote:
> diff --git a/tools/proctrace/COPYING b/tools/proctrace/COPYING
> new file mode 100644
> index 0000000000..c0a841112c
> --- /dev/null
> +++ b/tools/proctrace/COPYING

The top-level COPYING file is GPL2.  There shouldn't be any need to
include a second copy here.

> diff --git a/tools/proctrace/Makefile b/tools/proctrace/Makefile
> new file mode 100644
> index 0000000000..2983c477fe
> --- /dev/null
> +++ b/tools/proctrace/Makefile
> @@ -0,0 +1,48 @@
> +# Copyright (C) CERT Polska - NASK PIB
> +# Author: Michał Leszczyński <michal.leszczynski@cert.pl>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; under version 2 of the License.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +
> +XEN_ROOT=$(CURDIR)/../..
> +include $(XEN_ROOT)/tools/Rules.mk
> +
> +CFLAGS  += -Werror
> +CFLAGS  += $(CFLAGS_libxenevtchn)
> +CFLAGS  += $(CFLAGS_libxenctrl)
> +LDLIBS  += $(LDLIBS_libxenctrl)
> +LDLIBS  += $(LDLIBS_libxenevtchn)
> +LDLIBS  += $(LDLIBS_libxenforeignmemory)
> +
> +.PHONY: all
> +all: build
> +
> +.PHONY: build
> +build: proctrace
> +
> +.PHONY: install
> +install: build
> +	$(INSTALL_DIR) $(DESTDIR)$(sbindir)
> +	$(INSTALL_PROG) proctrace $(DESTDIR)$(sbindir)/proctrace
> +
> +.PHONY: uninstall
> +uninstall:
> +	rm -f $(DESTDIR)$(sbindir)/proctrace
> +
> +.PHONY: clean
> +clean:
> +	$(RM) -f $(DEPS_RM)

You need to remove proctrace as well, for `make clean` to have the
intended semantics.

> +
> +.PHONY: distclean
> +distclean: clean
> +
> +iptlive: iptlive.o Makefile
> +	$(CC) $(LDFLAGS) $< -o $@ $(LDLIBS) $(APPEND_LDFLAGS)

This rule looks to be totally unused?

> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <sys/mman.h>
> +#include <signal.h>
> +
> +#include <xenctrl.h>
> +#include <xen/xen.h>
> +#include <xenforeignmemory.h>
> +
> +#define BUF_SIZE (16384 * XC_PAGE_SIZE)

This hardcodes the size of the buffer which is configurable per VM. 
Mapping the buffer fails when it is smaller than this.

It appears there is still outstanding bug from the acquire_resource work
which never got fixed.  The guest_handle_is_null(xmar.frame_list) path
in Xen is supposed to report the size of the resource, not the size of
Xen's local buffer, so userspace can ask "how large is this resource".

I'll try and find some time to fix this and arrange for backports, but
the current behaviour is nonsense, and problematic for new users.

~Andrew