On Fri, Apr 02, 2010 at 11:39:45AM -0400, Stefan Berger wrote:
> Since v1, I have made the following changes:
>
> - remove unneeded include files
> - created a new ATTRIBUTE_PACKED in internal.h (please check)
> - added libpcap to rpm spec file (not built via rpm)
> - added 2 flags for the type of traffic to be used to detect the IP
> address of a VM: DETECT_DHCP & DETECT_STATIC. Now if DETECT_DHCP is
> chosen, temporary filtering rules are applied on the VM's interface that
> allow the VM to only send and receive DHCP traffic (optionally
> restricted to a certain DHCP server) until the actual rules are then
> applied. In that case the libpcap uses a filter that restricts packets
> the thread received to DHCP Response traffic. The function to request
> the learning of the IP address is currently invoked with both flags set,
> but either one by itself works. A user should consciously configure the
> VM that triggers that for example DETECT_DHCP is to be used alone since
> in that case a statically configured interface in the VM will not be
> able to communicate at all.
> Now the following ways for learning the IP address parameter are
> available:
> - explicitly provided IP address by user as filter parameter
> - snooping of DHCP OFFERs from a specific DHCP server
> - snooping of DHCP OFFERs from any server
> - snooping of any traffic from the VM indicating its IP address
>
> This patch implements support for learning a VM's IP address. It uses
> the pcap library to listen on the VM's backend network interface (tap)
> or the physical ethernet device (macvtap) and tries to capture packets
> with source or destination MAC address of the VM and learn from DHCP
> Offers, ARP traffic, or first-sent IPv4 packet what the IP address of
> the VM's interface is. This then allows to instantiate the network
> traffic filtering rules without the user having to provide the IP
> parameter somewhere in the filter description or in the interface
> description as a parameter. This only supports to detect the parameter
> IP, which is for the assumed single IPv4 address of a VM. There is not
> support for interfaces that may have multiple IP addresses (IP
> aliasing) or IPv6 that may then require more than one valid IP address
> to be detected. A VM can have multiple independent interfaces that each
> uses a different IP address and in that case it will be attempted to
> detect each one of the address independently.
>
> So, when for example an interface description in the domain XML has
> looked like this up to now:
>
> <interface type='bridge'>
> <source bridge='mybridge'/>
> <model type='virtio'/>
> <filterref filter='clean-traffic'>
> <parameter name='IP' value='10.2.3.4'/>
> </filterref>
> </interface>
>
> you may omit the IP parameter:
>
> <interface type='bridge'>
> <source bridge='mybridge'/>
> <model type='virtio'/>
> <filterref filter='clean-traffic'/>
> </interface>
>
> Internally I am walking the 'tree' of a VM's referenced network filters
> and determine with the given variables which variables are missing. Now,
> the above IP parameter may be missing and this causes a libvirt-internal
> thread to be started that uses the pcap library's API to listen to the
> backend interface (in case of macvtap to the physical interface) in an
> attempt to determine the missing IP parameter. If the backend interface
> disappears the thread terminates assuming the VM was brought down. In
> case of a macvtap device a timeout is being used to wait for packets
> from the given VM (filtering by VM's interface MAC address). If the VM's
> macvtap device disappeared the thread also terminates. In all other
> cases it tries to determine the IP address of the VM and will then apply
> the rules late on the given interface, which would have happened
> immediately if the IP parameter had been explicitly given. In case an
> error happens while the firewall rules are applied, the VM's backend
> interface is 'down'ed preventing it to communicate. Reasons for failure
> for applying the network firewall rules may that an ebtables/iptables
> command failes or OOM errors. Essentially the same failure reasons may
> occur as when the firewall rules are applied immediately on VM start,
> except that due to the late application of the filtering rules the VM
> now is already running and cannot be hindered anymore from starting.
> Bringing down the whole VM would probably be considered too drastic.
> While a VM's IP address is attempted to be determined only limited
> updates to network filters are allowed. In particular it is prevented
> that filters are modified in such a way that they would introduce new
> variables.
>
> A caveat: The algorithm does not know which one is the appropriate IP
> address of a VM. If the VM spoofs an IP address in its first ARP traffic
> or IPv4 packets its filtering rules will be instantiated for this IP
> address, thus 'locking' it to the found IP address. So, it's still
> 'safer' to explicitly provide the IP address of a VM's interface in the
> filter description if it is known beforehand.
>
> Signed-off-by: Stefan Berger <stef...@us.ibm.com>
In general the code looks nice to me, just a few things to point out
> Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c
[...]
> +/* structure of an ARP request/reply message */
> +struct f_arphdr {
> + struct arphdr arphdr;
> + uint8_t ar_sha[ETH_ALEN];
> + uint32_t ar_sip;
> + uint8_t ar_tha[ETH_ALEN];
> + uint32_t ar_tip;
> +} ATTRIBUTE_PACKED;
> +
> +
> +/* structure representing DHCP message */
> +struct dhcp {
> + uint8_t op;
> + uint8_t htype;
> + uint8_t hlen;
> + uint8_t hops;
> + uint32_t xid;
> + uint16_t secs;
> + uint16_t flags;
> + uint32_t ciaddr;
> + uint32_t yiaddr;
> + uint32_t siaddr;
> + uint32_t giaddr;
> + uint8_t chaddr[16];
> + /* omitted */
> +} ATTRIBUTE_PACKED;
> +
> +
> +struct ether_vlan_header
> +{
> + uint8_t dhost[ETH_ALEN];
> + uint8_t shost[ETH_ALEN];
> + uint16_t vlan_type;
> + uint16_t vlan_flags;
> + uint16_t ether_type;
> +} ATTRIBUTE_PACKED;
> +
the 3 structures requiring ATTRIBUTE_PACKED, actually f_arphdr might be
the only one really requiring the packing, I don't know how the
compilers would work withthe 2 * 8 uint8_t arrays of
ether_vlan_header...
[...]
> +// FIXME: move chgIfFlags, ifUp, checkIf into common file & share w/
> macvtap.c
> +
> +/*
> + * chgIfFlags: Change flags on an interface
> + * @ifname : name of the interface
> + * @flagclear : the flags to clear
> + * @flagset : the flags to set
> + *
> + * The new flags of the interface will be calculated as
> + * flagmask = (~0 ^ flagclear)
> + * newflags = (curflags & flagmask) | flagset;
> + *
> + * Returns 0 on success, errno on failure.
> + */
> +static int chgIfFlags(const char *ifname, short flagclear, short
> flagset) {
[...]
> +/*
> + * ifUp
> + * @name: name of the interface
> + * @up: 1 for up, 0 for down
> + *
> + * Function to control if an interface is activated (up, 1) or not
> (down, 0)
> + *
> + * Returns 0 in case of success or an errno code in case of failure.
> + */
> +static int
> +ifUp(const char *name, int up)
> +{
[...]
> +
> +/**
> + * checkIf
> + *
> + * @ifname: Name of the interface
> + * @macaddr: expected MAC address of the interface
> + *
> + * FIXME: the interface's index is another good parameter to check
> + *
> + * Determine whether a given interface is still available. If so,
> + * it must have the given MAC address.
> + *
> + * Returns an error code ENODEV in case the interface does not exist
> + * anymore or its MAC address is different, 0 otherwise.
> + */
> +int
> +checkIf(const char *ifname, const unsigned char *macaddr)
> +{
[...]
>
> +int checkIf(const char *ifname, const unsigned char *macaddr);
> +
> #endif
I think those 3 should really be cleaned up, either merged with
another funcion as suggested, or at least be prefixed to avoid name
pollution. Not extremely urgent especially if there is some merging
with macvtap.c code needed.
> Index: libvirt-acl/src/libvirt_private.syms
> ===================================================================
> --- libvirt-acl.orig/src/libvirt_private.syms
> +++ libvirt-acl/src/libvirt_private.syms
> @@ -488,6 +488,8 @@ virNWFilterRegisterCallbackDriver;
> virNWFilterTestUnassignDef;
> virNWFilterConfLayerInit;
> virNWFilterConfLayerShutdown;
> +virNWFilterLockFilterUpdates;
> +virNWFilterUnlockFilterUpdates;
>
>
> #nwfilter_params.h
> @@ -503,6 +505,16 @@ virNWFilterInstantiateFilter;
> virNWFilterTeardownFilter;
>
>
> +#nwfilter_learnipaddr.h
> +ipAddressMap;
> +ipAddressMapLock;
> +pendingLearnReq;
> +pendingLearnReqLock;
as far as I can tell those 4 don't need to be exported as they are not
used in any other module, and sine they don't have a virNWFilter prefix
I think it's cleaner that way. They are actually static in your latest
version.
> +virNWFilterGetIpAddrForIfname;
> +virNWFilterDelIpAddrForIfname;
> +virNWFilterLookupLearnReq;
> +
> +
> # pci.h
> pciGetDevice;
> pciFreeDevice;
[...]
> Index: libvirt-acl/configure.ac
> ===================================================================
> --- libvirt-acl.orig/configure.ac
> +++ libvirt-acl/configure.ac
> @@ -41,6 +41,7 @@ XMLRPC_REQUIRED=1.14.0
> HAL_REQUIRED=0.5.0
> DEVMAPPER_REQUIRED=1.0.0
> LIBCURL_REQUIRED="7.18.0"
> +LIBPCAP_REQUIRED="1.0.0"
>
> dnl Checks for C compiler.
> AC_PROG_CC
> @@ -1045,6 +1046,39 @@ AC_SUBST([NUMACTL_CFLAGS])
> AC_SUBST([NUMACTL_LIBS])
>
>
> +dnl pcap lib
> +LIBPCAP_CONFIG="pcap-config"
> +LIBPCAP_CFLAGS=""
> +LIBPCAP_LIBS=""
> +LIBPCAP_FOUND="no"
> +
> +AC_ARG_WITH([libpcap], AC_HELP_STRING([--with-libpcap=@<:@PFX@:>@],
> [libpcap location]))
> +if test "$with_qemu" = "yes"; then
> + if test "x$with_libpcap" != "xno" ; then
Somehow we are binding this completely to the compilation of qemu,
in the detection and in the spec file, it's fine for now but maybe
should be relaxed at some point.
> + if test "x$with_libpcap" != "x" ; then
> + LIBPCAP_CONFIG=$with_libpcap/bin/$LIBPCAP_CONFIG
> + fi
> + AC_MSG_CHECKING(libpcap $LIBPCAP_CONFIG >= $LIBPCAP_REQUIRED )
> + if ! $LIBPCAP_CONFIG --libs > /dev/null 2>&1 ; then
> + AC_MSG_RESULT(no)
> + else
> + LIBPCAP_LIBS="`$LIBPCAP_CONFIG --libs`"
> + LIBPCAP_CFLAGS="`$LIBPCAP_CONFIG --cflags`"
> + LIBPCAP_FOUND="yes"
> + AC_MSG_RESULT(yes)
> + fi
> + fi
> +fi
> +
> +if test "x$LIBPCAP_FOUND" = "xyes"; then
> + AC_DEFINE_UNQUOTED([HAVE_LIBPCAP], 1, [whether libpcap can be used])
> +fi
> +
> +AC_SUBST([LIBPCAP_CFLAGS])
> +AC_SUBST([LIBPCAP_LIBS])
> +
> +
> +
> dnl
> dnl Checks for the UML driver
> dnl
> @@ -2129,6 +2163,11 @@ AC_MSG_NOTICE([ xmlrpc: $XMLRPC_CFLAGS
> else
> AC_MSG_NOTICE([ xmlrpc: no])
> fi
> +if test "$with_qemu" = "yes" ; then
> +AC_MSG_NOTICE([ pcap: $LIBPCAP_CFLAGS $LIBPCAP_LIBS])
> +else
> +AC_MSG_NOTICE([ pcap: no])
> +fi
> AC_MSG_NOTICE([])
> AC_MSG_NOTICE([Test suite])
> Index: libvirt-acl/src/internal.h
> ===================================================================
> --- libvirt-acl.orig/src/internal.h
> +++ libvirt-acl/src/internal.h
> @@ -138,6 +138,14 @@
> # endif
> # endif
I added an extra comment there since we currently fail on non gcc
due to this:
+/**
+ * ATTRIBUTE_PACKED
+ *
+ * force a structure to be packed, i.e. not following architecture and
+ * compiler best alignments for its sub components. It's needed for example
+ * for the network filetering code when defining the content of raw
+ * ethernet packets.
+ * Others compiler than gcc may use something different e.g. #pragma pack(1)
+ */
> +# ifndef ATTRIBUTE_PACKED
> +# if __GNUC_PREREQ (3, 3)
> +# define ATTRIBUTE_PACKED __attribute__((packed))
> +# else
> +# error "Need an __attribute__((packed)) equivalent"
> +# endif
> +# endif
> +
> # ifndef ATTRIBUTE_NONNULL
> # if __GNUC_PREREQ (3, 3)
> # define ATTRIBUTE_NONNULL(m) __attribute__((__nonnull__(m)))
I made the couple of changes above which were looking needed and
pushed this. This is really related to nwfilter which is new in this
version anyway.
thanks !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
