Re: [PATCH v3] libvchan: interdomain communications library

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 08/30/2011 06:32 AM, Ian Campbell wrote:
> On Mon, 2011-08-29 at 19:48 +0100, Daniel De Graaf wrote:
>> diff --git a/tools/libvchan/Makefile b/tools/libvchan/Makefile
>> new file mode 100644
>> index 0000000..6cccf3e
>> --- /dev/null
>> +++ b/tools/libvchan/Makefile
>> @@ -0,0 +1,56 @@
>> +#
>> +# tools/libvchan/Makefile
>> +#
>> +
>> +XEN_ROOT = $(CURDIR)/../..
>> +include $(XEN_ROOT)/tools/Rules.mk
>> +
>> +LIBVCHAN_OBJS = init.o io.o
>> +NODE_OBJS = node.o
>> +NODE2_OBJS = node-select.o
>> +
>> +LIBVCHAN_LIBS = $(LDLIBS_libxenstore)
>> +$(LIBVCHAN_OBJS): CFLAGS += $(CFLAGS_libxenstore)
>> +
>> +MAJOR = 1.0
>> +MINOR = 0
>> +
>> +CFLAGS += -I../include -I. -fPIC
> 
> Can you use foo.opic in your $(*_OBJS) instead of -fPIC? I think that's
> how this is intended to work.

I was copying libxl's Makefile which doesn't use .opic; using .opic for the .so
and not the .a looks like a good idea.

>> +
>> +.PHONY: all
>> +all: libvchan.so vchan-node1 vchan-node2 libvchan.a
>> +
>> +libvchan.so: libvchan.so.$(MAJOR)
>> +       ln -sf $< $@
>> +
>> +libvchan.so.$(MAJOR): libvchan.so.$(MAJOR).$(MINOR)
>> +       ln -sf $< $@
>> +
>> +libvchan.so.$(MAJOR).$(MINOR): $(LIBVCHAN_OBJS)
>> +       $(CC) $(LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libvchan.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $^ $(LIBVCHAN_LIBS)
>> +
>> +libvchan.a: $(LIBVCHAN_OBJS)
>> +       $(AR) rcs libvchan.a $^
>> +
>> +vchan-node1: $(NODE_OBJS) libvchan.so
>> +       $(CC) $(LDFLAGS) -o $@ $(NODE_OBJS) libvchan.so $(LDLIBS_libvchan)
>> +
>> +vchan-node2: $(NODE2_OBJS) libvchan.so
>> +       $(CC) $(LDFLAGS) -o $@ $(NODE2_OBJS) libvchan.so $(LDLIBS_libvchan)
>> +
>> +.PHONY: install
>> +install: all
>> +       $(INSTALL_DIR) $(DESTDIR)$(LIBDIR)
>> +       $(INSTALL_DIR) $(DESTDIR)$(INCLUDEDIR)
>> +       $(INSTALL_PROG) libvchan.so.$(MAJOR).$(MINOR) $(DESTDIR)$(LIBDIR)
> 
> Perhaps the library should be libxenvchan since it is going in /usr/lib
> and vchan is a bit generic?

That seems a sensible name change.

>> diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
>> new file mode 100644
>> index 0000000..b267ca7
>> --- /dev/null
>> +++ b/tools/libvchan/init.c
>> @@ -0,0 +1,464 @@
>> +/**
>> + * @file
>> + * @section AUTHORS
>> + *
>> + * Copyright (C) 2010  Rafal Wojtczuk  <rafal@invisiblethingslab.com>
>> + *
>> + *  Authors:
>> + *       Rafal Wojtczuk  <rafal@invisiblethingslab.com>
>> + *       Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> + *
>> + * @section LICENSE
>> + *
>> + *  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.
> 
> I don't have a problem with GPL rather than LGPL myself but I suppose we
> should consider if it meets the needs of the potential users now while
> the number of people who have touched the library is small enough that
> we can ask them if they are happy to relicense.
> 

I have agreement from Rafal Wojtczuk to relicense under the LGPL, so that
will be done in the next version.

> [...]
>> +static int init_gnt_srv(struct libvchan *ctrl)
>> +{
>> +       int pages_left = ctrl->read.order >= PAGE_SHIFT ? 1 << (ctrl->read.order - PAGE_SHIFT) : 0;
>> +       int pages_right = ctrl->write.order >= PAGE_SHIFT ? 1 << (ctrl->write.order - PAGE_SHIFT) : 0;
>> +       struct ioctl_gntalloc_alloc_gref *gref_info = NULL;
>> +       int ring_fd = open("/dev/xen/gntalloc", O_RDWR);
>> +       int ring_ref = -1;
>> +       int err;
>> +       void *ring, *area;
>> +
>> +       if (ring_fd < 0)
>> +               return -1;
>> +
>> +       gref_info = malloc(sizeof(*gref_info) + max(pages_left, pages_right)*sizeof(uint32_t));
>> +
>> +       gref_info->domid = ctrl->other_domain_id;
>> +       gref_info->flags = GNTALLOC_FLAG_WRITABLE;
>> +       gref_info->count = 1;
>> +
>> +       err = ioctl(ring_fd, IOCTL_GNTALLOC_ALLOC_GREF, gref_info);
> 
> Unless libvchan is going to be the only user of this interface we should
> add helpful wrappers to libxc, like we do for gntdev and evtchn.

Adding the wrappers made the library more complex with no other gains when
it was out-of-tree; for upstreaming, this does make sense. This will result
in a vchan consuming two file descriptors while it is active because the libxc
API does not expose the ability to close descriptors without unmapping memory.
Since that ability is likely to be linux-specific, it's reasonable to stop
relying on it for portability reasons.

>> +       if (err)
>> +               goto out;
>> +
>> +       ring = mmap(NULL, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, ring_fd, gref_info->index);
>> +
>> +       if (ring == MAP_FAILED)
>> +               goto out;
>> +
>> +       ctrl->ring = ring;
>> +       ring_ref = gref_info->gref_ids[0];
>> +
>> +       memset(ring, 0, PAGE_SIZE);
>> +
>> +       ctrl->read.shr = &ctrl->ring->left;
>> +       ctrl->write.shr = &ctrl->ring->right;
>> +       ctrl->ring->left_order = ctrl->read.order;
>> +       ctrl->ring->right_order = ctrl->write.order;
>> +       ctrl->ring->cli_live = 2;
>> +       ctrl->ring->srv_live = 1;
>> +       ctrl->ring->debug = 0xabcd;
> 
> Makes a change from deafbeef I guess ;-)
> 
>> +#ifdef IOCTL_GNTALLOC_SET_UNMAP_NOTIFY
>> +       {
>> +               struct ioctl_gntalloc_unmap_notify arg;
>> +               arg.index = gref_info->index + offsetof(struct vchan_interface, srv_live);
>> +               arg.action = UNMAP_NOTIFY_CLEAR_BYTE | UNMAP_NOTIFY_SEND_EVENT;
>> +               arg.event_channel_port = ctrl->event_port;
>> +               ioctl(ring_fd, IOCTL_GNTALLOC_SET_UNMAP_NOTIFY, &arg);
>> +       }
>> +#endif
> 
> What is the fallback if this isn't available?

The fallback is that the notify is not sent, and the peer cannot detect when
its peer crashes or is killed instead of executing a graceful shutdown.

Adding this functionality to libxc requires yet another wrapper on the grant
mapping functionality. Instead of attempting to pass back the index as is
done in the current version, I am considering adding the functions
xc_gnttab_map_grant_ref_notify(xcg, domid, ref, notify_offset, notify_port) and
xc_gntshr_share_page_notify(xcs, domid, &ref, notify_offset, notify_port);
these would fall back to xc_gnttab_map_grant_ref if notify is not present.

> [...]
>> static int init_xs_srv(struct libvchan *ctrl, int ring_ref)
>> +{
>> +       int ret = -1;
>> +       struct xs_handle *xs;
>> +       struct xs_permissions perms[2];
>> +       char buf[64];
>> +       char ref[16];
>> +       char* domid_str = NULL;
>> +       xs = xs_domain_open();
>> +       if (!xs)
>> +               goto fail;
>> +       domid_str = xs_read(xs, 0, "domid", NULL);
>> +       if (!domid_str)
>> +               goto fail_xs_open;
>> +
>> +       // owner domain is us
>> +       perms[0].id = atoi(domid_str);
> 
> It sucks a bit that xenstore doesn't appear to allow DOMNID_SELF here
> but oh well.

On the client side, we need to look up our own domid to find the path
(if the changes to follow usual xenstore convention are made) so it's
required either way.
 
>> +       // permissions for domains not listed = none
>> +       perms[0].perms = XS_PERM_NONE;
>> +       // other domains
>> +       perms[1].id = ctrl->other_domain_id;
>> +       perms[1].perms = XS_PERM_READ;
>> +
>> +       snprintf(ref, sizeof ref, "%d", ring_ref);
>> +       snprintf(buf, sizeof buf, "data/vchan/%d/ring-ref", ctrl->device_number);
>> +       if (!xs_write(xs, 0, buf, ref, strlen(ref)))
>> +               goto fail_xs_open;
>> +       if (!xs_set_permissions(xs, 0, buf, perms, 2))
>> +               goto fail_xs_open;
>> +
>> +       snprintf(ref, sizeof ref, "%d", ctrl->event_port);
>> +       snprintf(buf, sizeof buf, "data/vchan/%d/event-channel", ctrl->device_number);
>> +       if (!xs_write(xs, 0, buf, ref, strlen(ref)))
>> +               goto fail_xs_open;
>> +       if (!xs_set_permissions(xs, 0, buf, perms, 2))
>> +               goto fail_xs_open;
> 
> Am I right that the intended usage model is that two domains can decide
> to setup a connection without admin or toolstack involvement?
> 
> Do we need to arrange on the toolstack side that a suitable
> vchan-specific directory (or directories) in xenstore exists with
> suitable permissions to allow this to happen exists or do we think data
> is an appropriate location? 

Yes, the intended use is to avoid needing to have management tools involved
in the setup. Of course, that doesn't mean that vchan can't have help from
management tools - but since this help isn't required, adding an unneeded
dependency was pointless and might also imply a level of control that is not
actually present (i.e. restricting the management tools does not actually
restrict the ability to set up a vchan; that requires something like an XSM
policy blocking the grant or event channels). I picked data because it does
not require toolstack modification to use, and no other location jumped out
at me - vchan isn't really a device.

> [...]
>> +static int init_evt_cli(struct libvchan *ctrl)
>> +{
>> +       struct ioctl_evtchn_bind_interdomain bind;
>> +       ctrl->event_fd = open("/dev/xen/evtchn", O_RDWR);
>> +       if (ctrl->event_fd < 0)
>> +               return -1;
>> +
>> +       bind.remote_domain = ctrl->other_domain_id;
>> +       bind.remote_port = ctrl->event_port;
>> +       ctrl->event_port = ioctl(ctrl->event_fd, IOCTL_EVTCHN_BIND_INTERDOMAIN, &bind);
>> +       if (ctrl->event_port < 0)
>> +               return -1;
>> +       return 0;
>> +}
> 
> This appears to be reimplementing xc_evtchn_bind_interdomain. It should
> use the provided infrastructure libraries instead.

At the time of writing, the infrastructure libraries did not work well in domUs;
this appears to be fixed now, so using the libraries should make porting easier.
This will require adding libxc logging to the API.

>> diff --git a/xen/include/public/io/libvchan.h b/xen/include/public/io/libvchan.h
>> new file mode 100644
>> index 0000000..81db0e2
>> --- /dev/null
>> +++ b/xen/include/public/io/libvchan.h
>> @@ -0,0 +1,209 @@
>> +/**
>> + * @file
>> + * @section AUTHORS
>> + *
>> + * Copyright (C) 2010  Rafal Wojtczuk  <rafal@invisiblethingslab.com>
>> + *
>> + *  Authors:
>> + *       Rafal Wojtczuk  <rafal@invisiblethingslab.com>
>> + *       Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> + *
>> + * @section LICENSE
>> + *
>> + *  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.
>> + *
>> + *  You should have received a copy of the GNU General Public License along
>> + *  with this program; if not, write to the Free Software Foundation, Inc.,
>> + *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>> + *
>> + * @section DESCRIPTION
>> + *
>> + *  Originally borrowed from the Qubes OS Project, http://www.qubes-os.org,
>> + *  this code has been substantially rewritten to use the gntdev and gntalloc
>> + *  devices instead of raw MFNs and map_foreign_range.
>> + *
>> + *  This is a library for inter-domain communication.  A standard Xen ring
>> + *  buffer is used, with a datagram-based interface built on top.  The grant
>> + *  reference and event channels are shared in XenStore under the path
>> + *  /local/domain/<domid>/data/vchan/<port>/{ring-ref,event-channel}
> 
> Is the peer's domid just implicit in port and/or the out of band setup?
> 
> Ian.
> 

Yes. Since the server already needs to query its own domid, the client can also
add such a query and use /local/domain/<srv-d>/data/vchan/<cli-id>/<port>/*
which matches the usual xenstore conventions.

-- 
Daniel De Graaf
National Security Agency