Improvement to linux/hotplug scripts

Improvement to linux/hotplug scripts

[Sylvain Munaut]

This patch series contains several improvement to the linux hotplug network
script.

The first one is a bug fix to properly set the rules for HVM domains tap
interfaces. See the discussion in xen-users about that.

The second one is a workaround for some common issue. I need to use it and
think it can be useful to others, but if people don't think it should be 
merged, feel free to drop it.

The last 3 are mainly about IPv6 support.

I tested the first 4 on a recent Xen 4.4 system. The last one I use on
a fairly old Xen 4.1 system (don't have any system running routing config
here), but the vif-route script didn't change recently so it should work
fine for 4.4 too. Of course, it'd be good if other people tested on their
config to see if I overlooked anything ...


Cheers,

    Sylvain

[PATCH 1/5] hotplug/linux: Fix the vif script to handle_iptable for tap interfaces

[Sylvain Munaut]

The TAP interfaces need the same iptables rules as the VIF, without it,
traffic will not be forwarded to/from them is the default FORWARD policy
is DROP/REJECT

Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com>
---
 tools/hotplug/Linux/vif-bridge    |    2 +-
 tools/hotplug/Linux/vif-common.sh |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/hotplug/Linux/vif-bridge b/tools/hotplug/Linux/vif-bridge
index b7dcbd6..87279df 100644
--- a/tools/hotplug/Linux/vif-bridge
+++ b/tools/hotplug/Linux/vif-bridge
@@ -97,7 +97,7 @@ case "$command" in
         ;;
 esac
 
-if [ "$type_if" = vif ]; then
+if [ "$type_if" = vif -o "$type_if" = tap ]; then
     handle_iptable
 fi
 
diff --git a/tools/hotplug/Linux/vif-common.sh b/tools/hotplug/Linux/vif-common.sh
index 73ee241..28ddae5 100644
--- a/tools/hotplug/Linux/vif-common.sh
+++ b/tools/hotplug/Linux/vif-common.sh
@@ -123,7 +123,7 @@ ip=$(xenstore_read_default "$XENBUS_PATH/ip" "$ip")
 
 frob_iptable()
 {
-  if [ "$command" == "online" ]
+  if [ "$command" == "online" -o "$command" == "add" ]
   then
     local c="-I"
   else
@@ -135,7 +135,7 @@ frob_iptable()
   iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-out "$dev" \
     -j ACCEPT 2>/dev/null
 
-  if [ "$command" == "online" -a $? -ne 0 ]
+  if [ \( "$command" == "online" -o "$command" == "add" \) -a $? -ne 0 ]
   then
     log err "iptables setup failed. This may affect guest networking."
   fi
-- 
1.7.10.4

[PATCH 2/5] hotplug/linux: Add an option to disable acceleration on VIF

[Sylvain Munaut]

If a kernel driver in dom0 tries to access a network service
running in a domU, strange things happen if the acceleration
is not disabled. This offers an easy option to do it.

This has been observed with a CEPH RBD kernel driver trying
to access cluster with some OSDs running as domUs.

Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com>
---
 tools/hotplug/Linux/vif-bridge |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/hotplug/Linux/vif-bridge b/tools/hotplug/Linux/vif-bridge
index 87279df..e0aa55d 100644
--- a/tools/hotplug/Linux/vif-bridge
+++ b/tools/hotplug/Linux/vif-bridge
@@ -35,6 +35,8 @@ dir=$(dirname "$0")
 bridge=${bridge:-}
 bridge=$(xenstore_read_default "$XENBUS_PATH/bridge" "$bridge")
 
+accel=$(xenstore_read_default "$XENBUS_PATH/accel" "on")
+
 if [ -z "$bridge" ]
 then
   bridge=$(brctl show | awk 'NR==2{print$1}')
@@ -82,6 +84,10 @@ case "$command" in
     online)
         setup_virtual_bridge_port "$dev"
         set_mtu $bridge $dev
+        if [ "${accel}" = "off" ]
+        then
+            ethtool -K "$dev" tx off 1>/dev/null 2>&1
+        fi
         add_to_bridge "$bridge" "$dev"
         ;;
 
-- 
1.7.10.4

[PATCH 3/5] hotplug/linux: Improve iptables logic

[Sylvain Munaut]

The main goal of this is to pave the way for IPv6 support, but it
also improves the rules by preventing duplicate incoming packets
rules to be added.

frob_iptables now takes a list of address to handle as parameter
and creates the rules as needed. Any 'common' rule is no longer
repeated.

Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com>
---
 tools/hotplug/Linux/vif-common.sh |   53 +++++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 14 deletions(-)

diff --git a/tools/hotplug/Linux/vif-common.sh b/tools/hotplug/Linux/vif-common.sh
index 28ddae5..e477c81 100644
--- a/tools/hotplug/Linux/vif-common.sh
+++ b/tools/hotplug/Linux/vif-common.sh
@@ -123,6 +123,7 @@ ip=$(xenstore_read_default "$XENBUS_PATH/ip" "$ip")
 
 frob_iptable()
 {
+  # Add or remove
   if [ "$command" == "online" -o "$command" == "add" ]
   then
     local c="-I"
@@ -130,15 +131,36 @@ frob_iptable()
     local c="-D"
   fi
 
-  iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
-    "$@" -j ACCEPT 2>/dev/null &&
+  # Always Allow all packets _to_ the domain
   iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-out "$dev" \
-    -j ACCEPT 2>/dev/null
+    -j ACCEPT 2>/dev/null &&
+
+  # Always allow the domain to talk to a DHCP server.
+  iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
+    -p udp --sport 68 --dport 67 -j ACCEPT 2>/dev/null
 
   if [ \( "$command" == "online" -o "$command" == "add" \) -a $? -ne 0 ]
   then
     log err "iptables setup failed. This may affect guest networking."
   fi
+
+  # Add rules for each address
+  local addr
+
+  for addr in $@; do
+    if [ "$addr" = "any" ]; then
+      addr="0.0.0.0/0"
+    fi
+
+    iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
+      -s "$addr" -j ACCEPT 2>/dev/null
+
+    if [ \( "$command" == "online" -o "$command" == "add" \) -a $? -ne 0 ]
+    then
+      log err "iptables setup failed. This may affect guest networking."
+    fi
+  done
+}
 }
 
 
@@ -160,23 +182,26 @@ handle_iptable()
     return
   fi
 
-  claim_lock "iptables"
+  # Scan through the addresses
+  local ipv4_addrs
 
   if [ "$ip" != "" ]
   then
-      local addr
-      for addr in $ip
-      do
-        frob_iptable -s "$addr"
-      done
-
-      # Always allow the domain to talk to a DHCP server.
-      frob_iptable -p udp --sport 68 --dport 67
+    local addr
+    for addr in $ip
+    do
+        ipv4_addrs="$addr $ipv4_addrs"
+    done
   else
-      # No IP addresses have been specified, so allow anything.
-      frob_iptable
+    # No IP addresses have been specified, so allow anything.
+    ipv4_addrs="any"
   fi
 
+  # Actually add the rules
+  claim_lock "iptables"
+
+  [ "$ipv4_addrs" != "" ] && frob_iptable "$ipv4_addrs"
+
   release_lock "iptables"
 }
 
-- 
1.7.10.4

[PATCH 4/5] hotplug/linux: Add IPv6 support to the iptables logic

[Sylvain Munaut]

This adds the same functions for ip6tables as the one for iptables.
The 'ip' variable can now contain ipv6s for the domain and add
appropriate rules

 - If the 'ip' var is empty then both full IPv4 and IPv6 are allowed.
 - If only IPv4 ips are present, then IPv6 will be completely disallowed.
 - If only IPv6 ips are present, then IPv4 will be completely disallowed.
 - You can use ::0/0 or 0.0.0.0/0 to allow v6 or v4 globally but filter
   the other one.

This gracefully handles if the dom0 doesn't have IPv6. If
the call to ip6tables doesn't succeed, it just ignores any
IPv6 stuff.

By default, domains aren't allows to send Router Advertisement
or DHCP responses, see the ipv6_allow_ra to enable them.

Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com>
---
 tools/hotplug/Linux/vif-common.sh |   86 +++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/tools/hotplug/Linux/vif-common.sh b/tools/hotplug/Linux/vif-common.sh
index e477c81..2f24274 100644
--- a/tools/hotplug/Linux/vif-common.sh
+++ b/tools/hotplug/Linux/vif-common.sh
@@ -121,6 +121,8 @@ fi
 ip=${ip:-}
 ip=$(xenstore_read_default "$XENBUS_PATH/ip" "$ip")
 
+ipv6_allow_ra=$(xenstore_read_default "$XENBUS_PATH/ipv6_allow_ra" "false")
+
 frob_iptable()
 {
   # Add or remove
@@ -161,6 +163,74 @@ frob_iptable()
     fi
   done
 }
+
+frob_ip6table()
+{
+  # Add or remove
+  if [ "$command" == "online" -o "$command" == "add" ]
+  then
+    local c="-I"
+  else
+    local c="-D"
+  fi
+
+  # Always Allow all packets _to_ the domain
+  ip6tables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-out "$dev" \
+    -j ACCEPT 2>/dev/null &&
+
+  # Always allow ICMP messages from link-local addresses (for ND)
+  ip6tables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
+    -s fe80::/64 -j ACCEPT 2>/dev/null &&
+
+  # Always allow the domain to talk to a DHCP server
+  ip6tables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
+    -p udp --sport 546 --dport 547 -j ACCEPT 2>/dev/null
+
+  if [ \( "$command" == "online" -o "$command" == "add" \) -a $? -ne 0 ]
+  then
+    log err "ip6tables setup failed. This may affect guest networking."
+  fi
+
+  # Add rules for each address
+  local addr
+
+  for addr in $@; do
+    if [ "$addr" = "any" ]; then
+      addr="::0/0"
+    fi
+
+    ip6tables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
+      -s "$addr" -j ACCEPT 2>/dev/null
+
+    if [ \( "$command" == "online" -o "$command" == "add" \) -a $? -ne 0 ]
+    then
+      log err "ip6tables setup failed. This may affect guest networking."
+    fi
+  done
+
+  # Filter out RA and DHCP server responses if not explicitely allowed
+  if [ "$ipv6_allow_ra" != "true" ]
+  then
+    ip6tables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
+      -p icmpv6 --icmpv6-type router-advertisement -j DROP 2>/dev/null &&
+
+    ip6tables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
+      -p udp --sport 547 --dport 546 -j DROP 2>/dev/null
+
+    if [ \( "$command" == "online" -o "$command" == "add" \) -a $? -ne 0 ]
+    then
+      log err "ip6tables setup failed. This may affect guest networking."
+    fi
+  fi
+}
+
+
+##
+# Check if the given IP is IPv6 or not
+#
+is_ipv6()
+{
+        echo "$1" | perl -wane '/:/ && print "yes"'
 }
 
 
@@ -182,25 +252,41 @@ handle_iptable()
     return
   fi
 
+  # User has a working IPv4 iptables, but maybe no IPv6 support ...
+  local do_ipv6="yes"
+
+  if ! ip6tables -L -n >&/dev/null
+  then
+    do_ipv6="no"
+  fi
+
   # Scan through the addresses
   local ipv4_addrs
+  local ipv6_addrs
 
   if [ "$ip" != "" ]
   then
     local addr
     for addr in $ip
     do
+      result=$(is_ipv6 "$addr")
+      if [ -z "$result" ] ; then
         ipv4_addrs="$addr $ipv4_addrs"
+      else
+        ipv6_addrs="$addr $ipv6_addrs"
+      fi
     done
   else
     # No IP addresses have been specified, so allow anything.
     ipv4_addrs="any"
+    ipv6_addrs="any"
   fi
 
   # Actually add the rules
   claim_lock "iptables"
 
   [ "$ipv4_addrs" != "" ] && frob_iptable "$ipv4_addrs"
+  [ "$ipv6_addrs" != "" -a "$do_ipv6" = "yes" ] && frob_ip6table "$ipv6_addrs"
 
   release_lock "iptables"
 }
-- 
1.7.10.4

[PATCH 5/5] hotplug/linux: Add IPv6 support to vif-route

[Sylvain Munaut]

Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com>
---
 tools/hotplug/Linux/vif-common.sh |   33 +++++++++++++++++++++++++++++++++
 tools/hotplug/Linux/vif-route     |   20 +++++++++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/tools/hotplug/Linux/vif-common.sh b/tools/hotplug/Linux/vif-common.sh
index 2f24274..cd341a33 100644
--- a/tools/hotplug/Linux/vif-common.sh
+++ b/tools/hotplug/Linux/vif-common.sh
@@ -324,3 +324,36 @@ dom0_ip()
   fi
   echo "$result"
 }
+
+
+##
+# ip6_of interface
+#
+# Print the first IPv6 address currently in use at the given interface, or nothing if
+# the interface is not up.
+#
+ip6_of()
+{
+        ip -6 addr show primary dev "$1" | perl -wane '/scope global/ && /inet6 (([0-9a-f]+:*)+)/ && print $1;'
+}
+
+
+##
+# dom0_ip6
+#
+# Print the IPv6 address of the interface in dom0 through which we are routing.
+# This is the IP address on the interface specified as "netdev" as a parameter
+# to these scripts, or eth0 by default.  This function will call fatal if no
+# such interface could be found.
+#
+dom0_ip6()
+{
+  local nd=${netdev:-eth0}
+  local result=$(ip6_of "$nd")
+  if [ -z "$result" ]
+  then
+        ""
+  else
+        echo "$result"
+  fi
+}
diff --git a/tools/hotplug/Linux/vif-route b/tools/hotplug/Linux/vif-route
index 02f1403..8cf80d5 100644
--- a/tools/hotplug/Linux/vif-route
+++ b/tools/hotplug/Linux/vif-route
@@ -24,11 +24,21 @@ dir=$(dirname "$0")
 . "${dir}/vif-common.sh"
 
 main_ip=$(dom0_ip)
+main_ip6=$(dom0_ip6)
+
+proxy_ndp=$(xenstore_read_default "$XENBUS_PATH/proxy_ndp" "off")
+
 
 case "${command}" in
     online)
         ifconfig ${dev} ${main_ip} netmask 255.255.255.255 up
         echo 1 >/proc/sys/net/ipv4/conf/${dev}/proxy_arp
+        if [ ! -z "${main_ip6}" ]; then
+            ip -6 addr add ${main_ip6} dev ${vif}
+            if [ "${proxy_ndp}" != "off" ]; then
+                echo 1 >/proc/sys/net/ipv6/conf/${vif}/proxy_ndp
+            fi
+        fi
         ipcmd='add'
         cmdprefix=''
         ;;
@@ -43,7 +53,15 @@ if [ "${ip}" ] ; then
     # If we've been given a list of IP addresses, then add routes from dom0 to
     # the guest using those addresses.
     for addr in ${ip} ; do
-      ${cmdprefix} ip route ${ipcmd} ${addr} dev ${dev} src ${main_ip}
+        result=$(is_ipv6 "${addr}")
+        if [ -z "${result}" ] ; then
+            ${cmdprefix} ip route ${ipcmd} ${addr} dev ${dev} src ${main_ip}
+        else
+            ${cmdprefix} ip -6 route ${ipcmd} ${addr} dev ${vif} src ${main_ip6}
+            if [ "${proxy_ndp}" != "off" ]; then
+                ${cmdprefix} ip -6 neighbor ${ipcmd} proxy ${addr} dev ${netdev:-eth0}
+            fi
+        fi
     done
 fi
 
-- 
1.7.10.4

Re: [PATCH 1/5] hotplug/linux: Fix the vif script to handle_iptable for tap interfaces

[Roger Pau Monné]

On 14/05/14 11:23, Sylvain Munaut wrote:
> The TAP interfaces need the same iptables rules as the VIF, without it,
> traffic will not be forwarded to/from them is the default FORWARD policy
> is DROP/REJECT
> 
> Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com>
> ---
>  tools/hotplug/Linux/vif-bridge    |    2 +-
>  tools/hotplug/Linux/vif-common.sh |    4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/hotplug/Linux/vif-bridge b/tools/hotplug/Linux/vif-bridge
> index b7dcbd6..87279df 100644
> --- a/tools/hotplug/Linux/vif-bridge
> +++ b/tools/hotplug/Linux/vif-bridge
> @@ -97,7 +97,7 @@ case "$command" in
>          ;;
>  esac
>  
> -if [ "$type_if" = vif ]; then
> +if [ "$type_if" = vif -o "$type_if" = tap ]; then

type_if can only be "vif" or "tap" so AFAICT you can drop the if
altogether, the rest looks fine.

Roger.

Re: [PATCH 1/5] hotplug/linux: Fix the vif script to handle_iptable for tap interfaces

[Ian Campbell]

On Wed, 2014-05-14 at 13:08 -0400, Roger Pau Monné wrote:
> On 14/05/14 11:23, Sylvain Munaut wrote:
> > The TAP interfaces need the same iptables rules as the VIF, without it,
> > traffic will not be forwarded to/from them is the default FORWARD policy
> > is DROP/REJECT
> > 
> > Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com>
> > ---
> >  tools/hotplug/Linux/vif-bridge    |    2 +-
> >  tools/hotplug/Linux/vif-common.sh |    4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/hotplug/Linux/vif-bridge b/tools/hotplug/Linux/vif-bridge
> > index b7dcbd6..87279df 100644
> > --- a/tools/hotplug/Linux/vif-bridge
> > +++ b/tools/hotplug/Linux/vif-bridge
> > @@ -97,7 +97,7 @@ case "$command" in
> >          ;;
> >  esac
> >  
> > -if [ "$type_if" = vif ]; then
> > +if [ "$type_if" = vif -o "$type_if" = tap ]; then
> 
> type_if can only be "vif" or "tap" so AFAICT you can drop the if
> altogether, the rest looks fine.

I agree, apart from that:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks!
Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/5] hotplug/linux: Add an option to disable acceleration on VIF

[Ian Campbell]

On Wed, 2014-05-14 at 17:23 +0200, Sylvain Munaut wrote:
> If a kernel driver in dom0 tries to access a network service
> running in a domU, strange things happen if the acceleration
> is not disabled. This offers an easy option to do it.

By acceleration here you mean tx checksum offload I think?

How does one go about arranging for this xenstore key to be set?

The XCP vif script[0] handles a wide variety of keys via keys like
ethtool-tx ethtool-rx etc.

FWIW I think this could also be achieved locally with a script
in /etc/xen/scripts/vif-post.d.

[0] https://github.com/xapi-project/xen-api/blob/master/scripts/vif

> This has been observed with a CEPH RBD kernel driver trying
> to access cluster with some OSDs running as domUs.
> 
> Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com>
> ---
>  tools/hotplug/Linux/vif-bridge |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/hotplug/Linux/vif-bridge b/tools/hotplug/Linux/vif-bridge
> index 87279df..e0aa55d 100644
> --- a/tools/hotplug/Linux/vif-bridge
> +++ b/tools/hotplug/Linux/vif-bridge
> @@ -35,6 +35,8 @@ dir=$(dirname "$0")
>  bridge=${bridge:-}
>  bridge=$(xenstore_read_default "$XENBUS_PATH/bridge" "$bridge")
>  
> +accel=$(xenstore_read_default "$XENBUS_PATH/accel" "on")
> +
>  if [ -z "$bridge" ]
>  then
>    bridge=$(brctl show | awk 'NR==2{print$1}')
> @@ -82,6 +84,10 @@ case "$command" in
>      online)
>          setup_virtual_bridge_port "$dev"
>          set_mtu $bridge $dev
> +        if [ "${accel}" = "off" ]
> +        then
> +            ethtool -K "$dev" tx off 1>/dev/null 2>&1
> +        fi
>          add_to_bridge "$bridge" "$dev"
>          ;;
>  

Re: [PATCH 3/5] hotplug/linux: Improve iptables logic

[Ian Campbell]

On Wed, 2014-05-14 at 17:23 +0200, Sylvain Munaut wrote:
> The main goal of this is to pave the way for IPv6 support, but it
> also improves the rules by preventing duplicate incoming packets
> rules to be added.
> 
> frob_iptables now takes a list of address to handle as parameter
> and creates the rules as needed. Any 'common' rule is no longer
> repeated.

What do you mean by a "common" rule? Does this mean the physdev-out rule
which would previously have been in added (identically) in both the "-s
$addr" and DHCP cases?

I see you now create a DHCP rule even if no ip was given (the "any"
case), and in the any case you create an explicit rule for "-s
0.0.0.0/0" rather than just saying nothing.

I'm not an iptables expert but I think this is probably all OK, but for
my peace of mind could you show example of the before and after rules
for the any and ip=1.2.3.4 case?

> 
> Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com>
> ---
>  tools/hotplug/Linux/vif-common.sh |   53 +++++++++++++++++++++++++++----------
>  1 file changed, 39 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/hotplug/Linux/vif-common.sh b/tools/hotplug/Linux/vif-common.sh
> index 28ddae5..e477c81 100644
> --- a/tools/hotplug/Linux/vif-common.sh
> +++ b/tools/hotplug/Linux/vif-common.sh
> @@ -123,6 +123,7 @@ ip=$(xenstore_read_default "$XENBUS_PATH/ip" "$ip")
>  
>  frob_iptable()
>  {
> +  # Add or remove
>    if [ "$command" == "online" -o "$command" == "add" ]
>    then
>      local c="-I"
> @@ -130,15 +131,36 @@ frob_iptable()
>      local c="-D"
>    fi
>  
> -  iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
> -    "$@" -j ACCEPT 2>/dev/null &&
> +  # Always Allow all packets _to_ the domain
>    iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-out "$dev" \
> -    -j ACCEPT 2>/dev/null
> +    -j ACCEPT 2>/dev/null &&

is this && intentional?

I think this is running under set -e so if the first one fails the whole
script will exit.

> +
> +  # Always allow the domain to talk to a DHCP server.
> +  iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
> +    -p udp --sport 68 --dport 67 -j ACCEPT 2>/dev/null
>  
>    if [ \( "$command" == "online" -o "$command" == "add" \) -a $? -ne 0 ]
>    then
>      log err "iptables setup failed. This may affect guest networking."
>    fi
> +
> +  # Add rules for each address
> +  local addr
> +
> +  for addr in $@; do
> +    if [ "$addr" = "any" ]; then
> +      addr="0.0.0.0/0"
> +    fi
> +
> +    iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
> +      -s "$addr" -j ACCEPT 2>/dev/null
> +
> +    if [ \( "$command" == "online" -o "$command" == "add" \) -a $? -ne 0 ]

"$? -ne 0" here makes me wonder if set -e isn't actually in force. I
thought it came via xen-script-common.sh. Am I confused?

> +    then
> +      log err "iptables setup failed. This may affect guest networking."
> +    fi
> +  done
> +}
>  }
>  
> 
> @@ -160,23 +182,26 @@ handle_iptable()
>      return
>    fi
>  
> -  claim_lock "iptables"
> +  # Scan through the addresses
> +  local ipv4_addrs
>  
>    if [ "$ip" != "" ]
>    then
> -      local addr
> -      for addr in $ip
> -      do
> -        frob_iptable -s "$addr"
> -      done
> -
> -      # Always allow the domain to talk to a DHCP server.
> -      frob_iptable -p udp --sport 68 --dport 67
> +    local addr
> +    for addr in $ip
> +    do
> +        ipv4_addrs="$addr $ipv4_addrs"
> +    done

I think this loop is equivalent to
	ipv4_addrs="$ip"
isn't it?

>    else
> -      # No IP addresses have been specified, so allow anything.
> -      frob_iptable
> +    # No IP addresses have been specified, so allow anything.
> +    ipv4_addrs="any"
>    fi
>  
> +  # Actually add the rules
> +  claim_lock "iptables"
> +
> +  [ "$ipv4_addrs" != "" ] && frob_iptable "$ipv4_addrs"

Given that it is a list destined to be used as $@ in frob_iptable do you
want $ipv4_addrs to be unquoted here (or perhaps to use $* In the
function, this aspect of shell always escapes me...).

Is "[ xxx ] && do_something" safe under set -e if xxx evaluates to
false? I think it might be but using an explicit if would definitely be
safe.

Ian.

Re: [PATCH 4/5] hotplug/linux: Add IPv6 support to the iptables logic

[Ian Campbell]

On Wed, 2014-05-14 at 17:23 +0200, Sylvain Munaut wrote:
> This adds the same functions for ip6tables as the one for iptables.
> The 'ip' variable can now contain ipv6s for the domain and add
> appropriate rules
> 
>  - If the 'ip' var is empty then both full IPv4 and IPv6 are allowed.
>  - If only IPv4 ips are present, then IPv6 will be completely disallowed.
>  - If only IPv6 ips are present, then IPv4 will be completely disallowed.
>  - You can use ::0/0 or 0.0.0.0/0 to allow v6 or v4 globally but filter
>    the other one.

Sounds sensible. Can you give examples of the rulesets create in each
case?

> This gracefully handles if the dom0 doesn't have IPv6. If
> the call to ip6tables doesn't succeed, it just ignores any
> IPv6 stuff.
> 
> By default, domains aren't allows to send Router Advertisement
> or DHCP responses, see the ipv6_allow_ra to enable them.

How does one go about setting this?

> +##
> +# Check if the given IP is IPv6 or not
> +#
> +is_ipv6()
> +{
> +        echo "$1" | perl -wane '/:/ && print "yes"'

Annoyingly I don't think we currently require Perl in the runtime
environment (it is used at build time). Luckily I think you can
implement this as
	case $addr in
		*:*) ipv6_addrs="$addrs $ipv6_addrs";;
		*) ipv4.... ;;
	esac

(probably inline in the handle_iptable function, no need for this helper
in that case) 

> @@ -182,25 +252,41 @@ handle_iptable()
>      return
>    fi
>  
> +  # User has a working IPv4 iptables, but maybe no IPv6 support ...
> +  local do_ipv6="yes"
> +
> +  if ! ip6tables -L -n >&/dev/null
> +  then
> +    do_ipv6="no"
> +  fi
> +
>    # Scan through the addresses
>    local ipv4_addrs
> +  local ipv6_addrs
>  
>    if [ "$ip" != "" ]
>    then
>      local addr
>      for addr in $ip

I see now why you had a loop in the previous patch.

>      do
> +      result=$(is_ipv6 "$addr")
> +      if [ -z "$result" ] ; then
>          ipv4_addrs="$addr $ipv4_addrs"
> +      else
> +        ipv6_addrs="$addr $ipv6_addrs"
> +      fi
>      done
>    else
>      # No IP addresses have been specified, so allow anything.
>      ipv4_addrs="any"
> +    ipv6_addrs="any"
>    fi
>  
>    # Actually add the rules
>    claim_lock "iptables"
>  
>    [ "$ipv4_addrs" != "" ] && frob_iptable "$ipv4_addrs"
> +  [ "$ipv6_addrs" != "" -a "$do_ipv6" = "yes" ] && frob_ip6table "$ipv6_addrs"
>  
>    release_lock "iptables"
>  }

Re: [PATCH 5/5] hotplug/linux: Add IPv6 support to vif-route

[Ian Campbell]

On Wed, 2014-05-14 at 17:23 +0200, Sylvain Munaut wrote:
> Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com>
> ---
>  tools/hotplug/Linux/vif-common.sh |   33 +++++++++++++++++++++++++++++++++
>  tools/hotplug/Linux/vif-route     |   20 +++++++++++++++++++-
>  2 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/hotplug/Linux/vif-common.sh b/tools/hotplug/Linux/vif-common.sh
> index 2f24274..cd341a33 100644
> --- a/tools/hotplug/Linux/vif-common.sh
> +++ b/tools/hotplug/Linux/vif-common.sh
> @@ -324,3 +324,36 @@ dom0_ip()
>    fi
>    echo "$result"
>  }
> +
> +
> +##
> +# ip6_of interface
> +#
> +# Print the first IPv6 address currently in use at the given interface, or nothing if
> +# the interface is not up.
> +#
> +ip6_of()
> +{
> +        ip -6 addr show primary dev "$1" | perl -wane '/scope global/ && /inet6 (([0-9a-f]+:*)+)/ && print $1;'

Hrm, Perl again. Can this be done with awk? Looks tricky though.

Ian

Re: [PATCH 4/5] hotplug/linux: Add IPv6 support to the iptables logic

[Ian Campbell]

On Thu, 2014-05-15 at 17:19 +0100, Ian Campbell wrote:
> On Wed, 2014-05-14 at 17:23 +0200, Sylvain Munaut wrote:
> > +        echo "$1" | perl -wane '/:/ && print "yes"'
> 
> Annoyingly I don't think we currently require Perl in the runtime
> environment (it is used at build time).

Hrm, this might be a lie -- we seem to use it in locking.sh. As you
were, sorry for the noise!

Ian/

Re: [PATCH 1/5] hotplug/linux: Fix the vif script to handle_iptable for tap interfaces

[Sylvain Munaut]

Thanks for the comments.

I will answer the comments and prepare a new version of the series on monday.


Cheers,

   Sylvain

Re: [PATCH 4/5] hotplug/linux: Add IPv6 support to the iptables logic

[Jacek Konieczny]

On 2014-05-14 17:23, Sylvain Munaut wrote:
> This adds the same functions for ip6tables as the one for iptables.
> The 'ip' variable can now contain ipv6s for the domain and add
> appropriate rules
> 
>  - If the 'ip' var is empty then both full IPv4 and IPv6 are allowed.
>  - If only IPv4 ips are present, then IPv6 will be completely disallowed.
>  - If only IPv6 ips are present, then IPv4 will be completely disallowed.
>  - You can use ::0/0 or 0.0.0.0/0 to allow v6 or v4 globally but filter
>    the other one.
> 
> This gracefully handles if the dom0 doesn't have IPv6. If
> the call to ip6tables doesn't succeed, it just ignores any
> IPv6 stuff.

I think it would be a good idea to allow autoconfigured IPv6 addresses.
These have the lower 64-bit of the address set to a value based on the
interface MAC address (EUI-64), which is known in the vif script.

Unfortunately it is not easy to compute that suffix in a shell script.
In my setup I use a helper Python script, but guess this might not be
the perfect solution for the standard scripts.

> +  # Always allow ICMP messages from link-local addresses (for ND)
> +  ip6tables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
> +    -s fe80::/64 -j ACCEPT 2>/dev/null &&

I wonder if checking this addresses against the MAC address may be
desirable, especially when bridging. This would be assured by the same
rule as the other auto-configured addresses.

Greets,
	Jacek

Re: [PATCH 5/5] hotplug/linux: Add IPv6 support to vif-route

[Jason Andryuk]

On 5/15/2014 12:20 PM, Ian Campbell wrote:
> On Wed, 2014-05-14 at 17:23 +0200, Sylvain Munaut wrote:
>> Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com>
>> ---
>>   tools/hotplug/Linux/vif-common.sh |   33 +++++++++++++++++++++++++++++++++
>>   tools/hotplug/Linux/vif-route     |   20 +++++++++++++++++++-
>>   2 files changed, 52 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/hotplug/Linux/vif-common.sh b/tools/hotplug/Linux/vif-common.sh
>> index 2f24274..cd341a33 100644
>> --- a/tools/hotplug/Linux/vif-common.sh
>> +++ b/tools/hotplug/Linux/vif-common.sh
>> @@ -324,3 +324,36 @@ dom0_ip()
>>     fi
>>     echo "$result"
>>   }
>> +
>> +
>> +##
>> +# ip6_of interface
>> +#
>> +# Print the first IPv6 address currently in use at the given interface, or nothing if
>> +# the interface is not up.
>> +#
>> +ip6_of()
>> +{
>> +        ip -6 addr show primary dev "$1" | perl -wane '/scope global/ && /inet6 (([0-9a-f]+:*)+)/ && print $1;'
> 
> Hrm, Perl again. Can this be done with awk? Looks tricky though.

Something like:
awk '/inet6 .* scope global/ { split($2, v6, "/"); print v6[1] }'

(Removing perl from locking.sh would be nice, since it's a large requirement for the one use.)

Regards,

Jason

Re: [PATCH 5/5] hotplug/linux: Add IPv6 support to vif-route

[Sylvain Munaut]

>>> +ip6_of()
>>> +{
>>> +        ip -6 addr show primary dev "$1" | perl -wane '/scope global/ && /inet6 (([0-9a-f]+:*)+)/ && print $1;'
>>
>> Hrm, Perl again. Can this be done with awk? Looks tricky though.
>
> Something like:
> awk '/inet6 .* scope global/ { split($2, v6, "/"); print v6[1] }'
>

Yes, I already found substitute for all the perl in my patch using
awk. Will be resubmitted on monday.

Cheers,

 Sylvain

Re: [PATCH 4/5] hotplug/linux: Add IPv6 support to the iptables logic

[Sylvain Munaut]

Hi,


> I think it would be a good idea to allow autoconfigured IPv6 addresses.
> These have the lower 64-bit of the address set to a value based on the
> interface MAC address (EUI-64), which is known in the vif script.
>
> Unfortunately it is not easy to compute that suffix in a shell script.
> In my setup I use a helper Python script, but guess this might not be
> the perfect solution for the standard scripts.

The issue is how do you get the prefix ?

Or add a special eui64:AAAA:BBBB:CCCC:DDDD  address that's
automaticaly transformed into a EUI64 address ?


>> +  # Always allow ICMP messages from link-local addresses (for ND)
>> +  ip6tables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
>> +    -s fe80::/64 -j ACCEPT 2>/dev/null &&
>
> I wonder if checking this addresses against the MAC address may be
> desirable, especially when bridging. This would be assured by the same
> rule as the other auto-configured addresses.

Yes, might be a good idea.

echo $mac | awk '{split($1,i,":"); print "fe80::" i[1]^2 i[2] ":" i[3]
"ff:fe" i[4] ":" i[5] i[6] }'

should work.


Cheers,

    Sylvain

-- 
Sylvain Munaut
Whatever s.a.
Rue Fond Cattelain 5
1435 Mont-Saint-Guibert
Fixed line: +32 10 23.59.30

Re: [PATCH 4/5] hotplug/linux: Add IPv6 support to the iptables logic

[Jacek Konieczny]

On 2014-05-16 21:33, Sylvain Munaut wrote:
>> I think it would be a good idea to allow autoconfigured IPv6 addresses.
>> These have the lower 64-bit of the address set to a value based on the
>> interface MAC address (EUI-64), which is known in the vif script.
>>
>> Unfortunately it is not easy to compute that suffix in a shell script.
>> In my setup I use a helper Python script, but guess this might not be
>> the perfect solution for the standard scripts.
> 
> The issue is how do you get the prefix ?

The prefix doesn't really matter if the goal is to prevent spoofing
other hosts' addresses. And ip6tables allows to match the lower half of
an IPv6 address: '-d ::1111:2222:3333:4444/::ffff:ffff:ffff:ffff'

This itself won't prevent spoofing the network part (and some policies
in the network may rely of the network part of addresses), but this hole
can be sealed with a single ip6tables added, with no need of Xen hotplug
scripts cooperation. I don't know if that is acceptable.

Greets,
	Jacek

Re: [PATCH 2/5] hotplug/linux: Add an option to disable acceleration on VIF

[Sylvain Munaut]

Hi,

>> If a kernel driver in dom0 tries to access a network service
>> running in a domU, strange things happen if the acceleration
>> is not disabled. This offers an easy option to do it.
>
> By acceleration here you mean tx checksum offload I think?

Yes.

> How does one go about arranging for this xenstore key to be set?

Just like 'ip' for eg. just put it in the vif config key like :
vif = ['mac=00:16:3e:00:00:01,ip=192.168.1.254,accel=off']


> The XCP vif script[0] handles a wide variety of keys via keys like
> ethtool-tx ethtool-rx etc.

Ah yes indeed. I don't use XCP, just plain xen.


> FWIW I think this could also be achieved locally with a script
> in /etc/xen/scripts/vif-post.d.

Probably. I think this patch is indeed too specific to my purpose so
I'll just remove it from the series.


Cheers,

    Sylvain

Re: [PATCH 3/5] hotplug/linux: Improve iptables logic

[Sylvain Munaut]

On Thu, May 15, 2014 at 6:15 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-05-14 at 17:23 +0200, Sylvain Munaut wrote:
>> The main goal of this is to pave the way for IPv6 support, but it
>> also improves the rules by preventing duplicate incoming packets
>> rules to be added.
>>
>> frob_iptables now takes a list of address to handle as parameter
>> and creates the rules as needed. Any 'common' rule is no longer
>> repeated.
>
> What do you mean by a "common" rule? Does this mean the physdev-out rule
> which would previously have been in added (identically) in both the "-s
> $addr" and DHCP cases?

Yes.


> I see you now create a DHCP rule even if no ip was given (the "any"
> case), and in the any case you create an explicit rule for "-s
> 0.0.0.0/0" rather than just saying nothing.

I fixed this in a new version of the patch. (I'll post the updated
series in a few hours)


> I'm not an iptables expert but I think this is probably all OK, but for
> my peace of mind could you show example of the before and after rules
> for the any and ip=1.2.3.4 case?


For the case where 'ip' is empty or not given at all:

Previous:

 ACCEPT  all  0.0.0.0/0     0.0.0.0/0  PHYSDEV match --physdev-out
vif87.0 --physdev-is-bridged
 ACCEPT  all  0.0.0.0/0     0.0.0.0/0  PHYSDEV match --physdev-in
vif87.0 --physdev-is-bridged

New:

 ACCEPT  all  0.0.0.0/0     0.0.0.0/0  PHYSDEV match --physdev-in
vif88.0 --physdev-is-bridged
 ACCEPT  all  0.0.0.0/0     0.0.0.0/0  PHYSDEV match --physdev-out
vif88.0 --physdev-is-bridged



For the case where 'ip' is set to "192.168.0.254 192.168.0.141" (as an
example) :

Previous:

 ACCEPT  all  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-out
vif86.0 --physdev-is-bridged
 ACCEPT  udp  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-in
vif86.0 --physdev-is-bridged udp spt:68 dpt:67
 ACCEPT  all  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-out
vif86.0 --physdev-is-bridged
 ACCEPT  all  192.168.0.141  0.0.0.0/0  PHYSDEV match --physdev-in
vif86.0 --physdev-is-bridged
 ACCEPT  all  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-out
vif86.0 --physdev-is-bridged
 ACCEPT  all  192.168.0.254  0.0.0.0/0  PHYSDEV match --physdev-in
vif86.0 --physdev-is-bridged

New:

 ACCEPT  all  192.168.0.141  0.0.0.0/0  PHYSDEV match --physdev-in
vif89.0 --physdev-is-bridged
 ACCEPT  all  192.168.0.254  0.0.0.0/0  PHYSDEV match --physdev-in
vif89.0 --physdev-is-bridged
 ACCEPT  all  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-out
vif89.0 --physdev-is-bridged
 ACCEPT  udp  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-in
vif89.0 --physdev-is-bridged udp spt:68 dpt:67



>> -  iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
>> -    "$@" -j ACCEPT 2>/dev/null &&
>> +  # Always Allow all packets _to_ the domain
>>    iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-out "$dev" \
>> -    -j ACCEPT 2>/dev/null
>> +    -j ACCEPT 2>/dev/null &&
>
> is this && intentional?
>
> I think this is running under set -e so if the first one fails the whole
> script will exit.

It was intentional, but I changed the error logic in the new version
of the patch. The version before the change already had some error
handling which led me to believe set -e wasn't enforced.

I don't know if set -e is really effective or not when the script
executes but in the new version it doesn't matter so much.
The general idea is to try to add all the rules and report any error
for the 'online/add' case. And for the 'offline/remove' case, we try
to remove all the rules, even if one of them fails it continues and
tries to remove the other one.


>> +  # Actually add the rules
>> +  claim_lock "iptables"
>> +
>> +  [ "$ipv4_addrs" != "" ] && frob_iptable "$ipv4_addrs"
>
> Given that it is a list destined to be used as $@ in frob_iptable do you
> want $ipv4_addrs to be unquoted here (or perhaps to use $* In the
> function, this aspect of shell always escapes me...).

I removed the quotes. Indeed it makes more sense, although it does
work either way.


> Is "[ xxx ] && do_something" safe under set -e if xxx evaluates to
> false? I think it might be but using an explicit if would definitely be
> safe.

It is safe, but I nonetheless changed to an explicit 'if'.


Cheers,

     Sylvain Munaut

Re: [PATCH 5/5] hotplug/linux: Add IPv6 support to vif-route

[Sylvain Munaut]

>> +##
>> +# ip6_of interface
>> +#
>> +# Print the first IPv6 address currently in use at the given interface, or nothing if
>> +# the interface is not up.
>> +#
>> +ip6_of()
>> +{
>> +        ip -6 addr show primary dev "$1" | perl -wane '/scope global/ && /inet6 (([0-9a-f]+:*)+)/ && print $1;'
>
> Hrm, Perl again. Can this be done with awk? Looks tricky though.
>

Changed to :

ip -6 -o addr show primary dev "$1" scope global | awk '$3 == "inet6"
{split($4,i,"/"); print i[1]; exit}'

which is now very similar to the ip_of() function.


Cheers,

    Sylvain Munaut

Re: [PATCH 2/5] hotplug/linux: Add an option to disable acceleration on VIF

[Ian Campbell]

On Tue, 2014-05-20 at 09:55 +0200, Sylvain Munaut wrote:
> Hi,
> 
> >> If a kernel driver in dom0 tries to access a network service
> >> running in a domU, strange things happen if the acceleration
> >> is not disabled. This offers an easy option to do it.
> >
> > By acceleration here you mean tx checksum offload I think?
> 
> Yes.
> 
> > How does one go about arranging for this xenstore key to be set?
> 
> Just like 'ip' for eg. just put it in the vif config key like :
> vif = ['mac=00:16:3e:00:00:01,ip=192.168.1.254,accel=off']

Which toolstack is this with? xl won't do anything with that (I don't
think...)

> > The XCP vif script[0] handles a wide variety of keys via keys like
> > ethtool-tx ethtool-rx etc.
> 
> Ah yes indeed. I don't use XCP, just plain xen.

Right, I was mostly trying to suggest that if we were to do something in
plain Xen we should consider handling the full set as well. But...

> > FWIW I think this could also be achieved locally with a script
> > in /etc/xen/scripts/vif-post.d.
> 
> Probably. I think this patch is indeed too specific to my purpose so
> I'll just remove it from the series.

... fair enough!

Thanks,
Ian.

Re: [PATCH 3/5] hotplug/linux: Improve iptables logic

[Ian Campbell]

On Tue, 2014-05-20 at 10:02 +0200, Sylvain Munaut wrote:
> > I'm not an iptables expert but I think this is probably all OK, but for
> > my peace of mind could you show example of the before and after rules
> > for the any and ip=1.2.3.4 case?
> 
> 
> For the case where 'ip' is empty or not given at all:
> 
> Previous:
> 
>  ACCEPT  all  0.0.0.0/0     0.0.0.0/0  PHYSDEV match --physdev-out
> vif87.0 --physdev-is-bridged
>  ACCEPT  all  0.0.0.0/0     0.0.0.0/0  PHYSDEV match --physdev-in
> vif87.0 --physdev-is-bridged
> 
> New:
> 
>  ACCEPT  all  0.0.0.0/0     0.0.0.0/0  PHYSDEV match --physdev-in
> vif88.0 --physdev-is-bridged
>  ACCEPT  all  0.0.0.0/0     0.0.0.0/0  PHYSDEV match --physdev-out
> vif88.0 --physdev-is-bridged

So the in and out rules are reversed, but I suppose that is safe.

> For the case where 'ip' is set to "192.168.0.254 192.168.0.141" (as an
> example) :
> 
> Previous:
> 
>  ACCEPT  all  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-out
> vif86.0 --physdev-is-bridged
>  ACCEPT  udp  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-in
> vif86.0 --physdev-is-bridged udp spt:68 dpt:67
>  ACCEPT  all  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-out
> vif86.0 --physdev-is-bridged
>  ACCEPT  all  192.168.0.141  0.0.0.0/0  PHYSDEV match --physdev-in
> vif86.0 --physdev-is-bridged
>  ACCEPT  all  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-out
> vif86.0 --physdev-is-bridged
>  ACCEPT  all  192.168.0.254  0.0.0.0/0  PHYSDEV match --physdev-in
> vif86.0 --physdev-is-bridged
> 
> New:
> 
>  ACCEPT  all  192.168.0.141  0.0.0.0/0  PHYSDEV match --physdev-in
> vif89.0 --physdev-is-bridged
>  ACCEPT  all  192.168.0.254  0.0.0.0/0  PHYSDEV match --physdev-in
> vif89.0 --physdev-is-bridged
>  ACCEPT  all  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-out
> vif89.0 --physdev-is-bridged
>  ACCEPT  udp  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-in
> vif89.0 --physdev-is-bridged udp spt:68 dpt:67

And here the redundant rules are dropped and the rest are reordered
slightly, which again I think/suppose is safe.

Thanks for gathering this info. Please could you include it in the
commit message for future reference.

> >> -  iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
> >> -    "$@" -j ACCEPT 2>/dev/null &&
> >> +  # Always Allow all packets _to_ the domain
> >>    iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-out "$dev" \
> >> -    -j ACCEPT 2>/dev/null
> >> +    -j ACCEPT 2>/dev/null &&
> >
> > is this && intentional?
> >
> > I think this is running under set -e so if the first one fails the whole
> > script will exit.
> 
> It was intentional, but I changed the error logic in the new version
> of the patch. The version before the change already had some error
> handling which led me to believe set -e wasn't enforced.

OK. Don't assume to much about the correctness of the existing
script ;-)

> I don't know if set -e is really effective or not when the script
> executes but in the new version it doesn't matter so much.
> The general idea is to try to add all the rules and report any error
> for the 'online/add' case. And for the 'offline/remove' case, we try
> to remove all the rules, even if one of them fails it continues and
> tries to remove the other one.

Sounds sensible.

> > Is "[ xxx ] && do_something" safe under set -e if xxx evaluates to
> > false? I think it might be but using an explicit if would definitely be
> > safe.
> 
> It is safe, but I nonetheless changed to an explicit 'if'.

Thanks, much less cognitive load for the reader that way too.

Ian.

Re: [PATCH 5/5] hotplug/linux: Add IPv6 support to vif-route

[Ian Campbell]

On Tue, 2014-05-20 at 10:06 +0200, Sylvain Munaut wrote:
> >> +##
> >> +# ip6_of interface
> >> +#
> >> +# Print the first IPv6 address currently in use at the given interface, or nothing if
> >> +# the interface is not up.
> >> +#
> >> +ip6_of()
> >> +{
> >> +        ip -6 addr show primary dev "$1" | perl -wane '/scope global/ && /inet6 (([0-9a-f]+:*)+)/ && print $1;'
> >
> > Hrm, Perl again. Can this be done with awk? Looks tricky though.
> >
> 
> Changed to :
> 
> ip -6 -o addr show primary dev "$1" scope global | awk '$3 == "inet6"
> {split($4,i,"/"); print i[1]; exit}'
> 
> which is now very similar to the ip_of() function.

thanks.

In theory I think this could be combined into a single helper which took
$2 == '4' or '6' and "ip -$2 | ... inet${2%4}.." (then ip_of and ip6_of
become invocations of that)

But perhaps the ${2%4} bit is overly cryptic?

Ian.

Re: [PATCH 5/5] hotplug/linux: Add IPv6 support to vif-route

[Sylvain Munaut]

Hi,

> In theory I think this could be combined into a single helper which took
> $2 == '4' or '6' and "ip -$2 | ... inet${2%4}.." (then ip_of and ip6_of
> become invocations of that)

It could. However this would also add a filter on 'scope global' for
ipv4 which isn't present at the moment. Not sure if this would affect
anything in practice ...

However since dom0_ip and dom0_ip6 need to stay separate (ipv4 is
mandatory and 'fatal' out if not found, but ipv6 is optional), I'm not
sure how useful it is to really combine there.


> But perhaps the ${2%4} bit is overly cryptic?

Yeah, I had to re-read the doc :)


Cheers,

    Sylvain

Re: [PATCH 3/5] hotplug/linux: Improve iptables logic

[Sylvain Munaut]

Hi,


> And here the redundant rules are dropped and the rest are reordered
> slightly, which again I think/suppose is safe.

Yes, it is. Because the rules have no overlap (i.e if one matches, the
other won't).

I use -A instead of -I (to append rather than insert).

_HOWEVER_ I just realized that it had another impact which is to add
the rules at the end of the chain rather than at the beginning which
will significantly change things if people have other rules in there.
So I'll change back to using -I and change the logic a bit to handle
this properly.

Damn, I almost missed that ...


Cheers,

    Sylvain

Re: [PATCH 5/5] hotplug/linux: Add IPv6 support to vif-route

[Ian Campbell]

On Tue, 2014-05-20 at 13:17 +0200, Sylvain Munaut wrote:
> Hi,
> 
> > In theory I think this could be combined into a single helper which took
> > $2 == '4' or '6' and "ip -$2 | ... inet${2%4}.." (then ip_of and ip6_of
> > become invocations of that)
> 
> It could. However this would also add a filter on 'scope global' for
> ipv4 which isn't present at the moment. Not sure if this would affect
> anything in practice ...

True, I missed that aspect.

> However since dom0_ip and dom0_ip6 need to stay separate (ipv4 is
> mandatory and 'fatal' out if not found, but ipv6 is optional), I'm not
> sure how useful it is to really combine there.

Indeed, I'm happy not to do so if it doesn't seem worthwhile to you.

> > But perhaps the ${2%4} bit is overly cryptic?
> 
> Yeah, I had to re-read the doc :)

I just tried the various options I know (%, %%, #, ##) till I found the
one which did what I expect :-)

Ian.

Re: [PATCH 3/5] hotplug/linux: Improve iptables logic

[Ian Campbell]

On Tue, 2014-05-20 at 13:23 +0200, Sylvain Munaut wrote:
> Hi,
> 
> 
> > And here the redundant rules are dropped and the rest are reordered
> > slightly, which again I think/suppose is safe.
> 
> Yes, it is. Because the rules have no overlap (i.e if one matches, the
> other won't).
> 
> I use -A instead of -I (to append rather than insert).
> 
> _HOWEVER_ I just realized that it had another impact which is to add
> the rules at the end of the chain rather than at the beginning which
> will significantly change things if people have other rules in there.
> So I'll change back to using -I and change the logic a bit to handle
> this properly.
> 
> Damn, I almost missed that ...

I totally missed it too. I suspect the reordering is due to this as well
BTW.

Ian.

Re: [PATCH 4/5] hotplug/linux: Add IPv6 support to the iptables logic

[Sylvain Munaut]

Hi,


>> This adds the same functions for ip6tables as the one for iptables.
>> The 'ip' variable can now contain ipv6s for the domain and add
>> appropriate rules
>>
>>  - If the 'ip' var is empty then both full IPv4 and IPv6 are allowed.
>>  - If only IPv4 ips are present, then IPv6 will be completely disallowed.
>>  - If only IPv6 ips are present, then IPv4 will be completely disallowed.
>>  - You can use ::0/0 or 0.0.0.0/0 to allow v6 or v4 globally but filter
>>    the other one.
>
> Sounds sensible. Can you give examples of the rulesets create in each
> case?

Yes, see below.

I also added the eui64 idea from jacek:
 - The ICMP Link Local messages are locked to the LL address derived
from the mac address
 - The 'ip' parameters also allow the special 'eui64' token to allow
any address with the lower 64 bits set to the EUI64 corresponding to
the MAC. (Filtering on the network part is not done and must be done
globally by the user if needed, or just manually specify the address
completely).


* ip=""

iptables:
 ACCEPT  all  0.0.0.0/0  0.0.0.0/0  PHYSDEV match --physdev-out
vif91.0 --physdev-is-bridged
 ACCEPT  all  0.0.0.0/0  0.0.0.0/0  PHYSDEV match --physdev-in
vif91.0 --physdev-is-bridged

ip6tables:
 DROP    udp    ::/0  ::/0  PHYSDEV match --physdev-in  vif91.0
--physdev-is-bridged udp spt:547 dpt:546
 DROP    icmpv6 ::/0  ::/0  PHYSDEV match --physdev-in  vif91.0
--physdev-is-bridged ipv6-icmptype 134
 ACCEPT  all    ::/0  ::/0  PHYSDEV match --physdev-out vif91.0
--physdev-is-bridged
 ACCEPT  all    ::/0  ::/0  PHYSDEV match --physdev-in  vif91.0
--physdev-is-bridged


* ip="192.168.0.254"

iptables:
 ACCEPT  udp  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-in
vif92.0 --physdev-is-bridged udp spt:68 dpt:67
 ACCEPT  all  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-out
vif92.0 --physdev-is-bridged
 ACCEPT  all  192.168.0.254  0.0.0.0/0  PHYSDEV match --physdev-in
vif92.0 --physdev-is-bridged

ip6tables:
 [nothing]


* ip="2001:aaaa:bbbb:cccc::1 eui64"

iptables:
 [nothing]

ip6tables:
 DROP    udp    ::/0                 ::/0
PHYSDEV match --physdev-in  vif94.0 --physdev-is-bridged udp spt:547
dpt:546
 DROP    icmpv6 ::/0                 ::/0
PHYSDEV match --physdev-in  vif94.0 --physdev-is-bridged ipv6-icmptype
134
 ACCEPT  udp    ::/0                 ::/0
PHYSDEV match --physdev-in  vif94.0 --physdev-is-bridged udp spt:546
dpt:547
 ACCEPT  all    fe80::216:3eff:fed0:da2d/128  ::/0
PHYSDEV match --physdev-in  vif94.0 --physdev-is-bridged
 ACCEPT  all    ::/0                 ::/0
PHYSDEV match --physdev-out vif94.0 --physdev-is-bridged
 ACCEPT  all    2001:aaaa:bbbb:cccc::1/128  ::/0
PHYSDEV match --physdev-in  vif94.0 --physdev-is-bridged
 ACCEPT  all    ::216:3eff:fed0:da2d/::ffff:ffff:ffff:ffff  ::/0
PHYSDEV match --physdev-in  vif94.0 --physdev-is-bridged


* ip="192.168.0.254 2001:aaaa:bbbb:cccc::1" (either ipv4 or ipv6 can
be replaced by the 0.0.0.0/0 or ::0/0 address to allow any, the
dhcp/nd rules might be redudant then).

iptables:
 ACCEPT  udp  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-in
vif95.0 --physdev-is-bridged udp spt:68 dpt:67
 ACCEPT  all  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-out
vif95.0 --physdev-is-bridged
 ACCEPT  all  192.168.0.254  0.0.0.0/0  PHYSDEV match --physdev-in
vif95.0 --physdev-is-bridged

ip6tables:
 DROP    udp    ::/0                 ::/0           PHYSDEV match
--physdev-in  vif95.0 --physdev-is-bridged udp spt:547 dpt:546
 DROP    icmpv6 ::/0                 ::/0           PHYSDEV match
--physdev-in  vif95.0 --physdev-is-bridged ipv6-icmptype 134
 ACCEPT  udp    ::/0                 ::/0           PHYSDEV match
--physdev-in  vif95.0 --physdev-is-bridged udp spt:546 dpt:547
 ACCEPT  all    fe80::216:3eff:fed0:da2d/128  ::/0  PHYSDEV match
--physdev-in  vif95.0 --physdev-is-bridged
 ACCEPT  all    ::/0                 ::/0           PHYSDEV match
--physdev-out vif95.0 --physdev-is-bridged




>> By default, domains aren't allows to send Router Advertisement
>> or DHCP responses, see the ipv6_allow_ra to enable them.
>
> How does one go about setting this?

Well ... I thought it would work just like  accel=  (which I'm using
in prod but that's with 'xm' on 4.1).
Turns out after a bit of googling that 'accel' might work just by luck
because somehow at some point passing that option was added to 'xm' in
2007 ...

I just retried now with 'xl' under 4.4 and indeed that doesn't work :(

So if I want to pass custom parameters to the vif-script, I have to
add it to libxl ?
And if yes, is that acceptable ? What would be the best way : add each
parameter independently, or add a single 'hotplug_extra' parameter
that would need to be parsed in the hotplug scripts themselves ?

(I'll wait until I fixed this before reposting obviously)


>> +##
>> +# Check if the given IP is IPv6 or not
>> +#
>> +is_ipv6()
>> +{
>> +        echo "$1" | perl -wane '/:/ && print "yes"'
>
> Annoyingly I don't think we currently require Perl in the runtime
> environment (it is used at build time). Luckily I think you can
> implement this as
>         case $addr in
>                 *:*) ipv6_addrs="$addrs $ipv6_addrs";;
>                 *) ipv4.... ;;
>         esac
>
> (probably inline in the handle_iptable function, no need for this helper
> in that case)


I fixed that using only 'awk'. I left it as a helper because it's used
in vif-route as well later and I find it nicer to have the test in a
single place.


Cheers,

    Sylvain

Re: [PATCH 4/5] hotplug/linux: Add IPv6 support to the iptables logic

[Ian Campbell]

On Tue, 2014-05-20 at 13:58 +0200, Sylvain Munaut wrote:
> Hi,
> 
> 
> >> This adds the same functions for ip6tables as the one for iptables.
> >> The 'ip' variable can now contain ipv6s for the domain and add
> >> appropriate rules
> >>
> >>  - If the 'ip' var is empty then both full IPv4 and IPv6 are allowed.
> >>  - If only IPv4 ips are present, then IPv6 will be completely disallowed.
> >>  - If only IPv6 ips are present, then IPv4 will be completely disallowed.
> >>  - You can use ::0/0 or 0.0.0.0/0 to allow v6 or v4 globally but filter
> >>    the other one.
> >
> > Sounds sensible. Can you give examples of the rulesets create in each
> > case?
> 
> Yes, see below.

Thanks. Like with the v4 ones could you also include these in the commit
log.

> 
> I also added the eui64 idea from jacek:
>  - The ICMP Link Local messages are locked to the LL address derived
> from the mac address
>  - The 'ip' parameters also allow the special 'eui64' token to allow
> any address with the lower 64 bits set to the EUI64 corresponding to
> the MAC. (Filtering on the network part is not done and must be done
> globally by the user if needed, or just manually specify the address
> completely).
> 
> 
> * ip=""
> 
> iptables:
>  ACCEPT  all  0.0.0.0/0  0.0.0.0/0  PHYSDEV match --physdev-out
> vif91.0 --physdev-is-bridged
>  ACCEPT  all  0.0.0.0/0  0.0.0.0/0  PHYSDEV match --physdev-in
> vif91.0 --physdev-is-bridged

> ip6tables:
>  DROP    udp    ::/0  ::/0  PHYSDEV match --physdev-in  vif91.0
> --physdev-is-bridged udp spt:547 dpt:546

So here we deny DHCP whereas for v4 we don't? Why is that? And in other
cases for v4 we explicitly allow it? I see you called this out in the
commit message, but I must confess I don't know v6 well enough to guess
why. Is allowing a guest to send DHCP responses more dangerous for v6
than with v4?


>  DROP    icmpv6 ::/0  ::/0  PHYSDEV match --physdev-in  vif91.0
> --physdev-is-bridged ipv6-icmptype 134
>  ACCEPT  all    ::/0  ::/0  PHYSDEV match --physdev-out vif91.0
> --physdev-is-bridged
>  ACCEPT  all    ::/0  ::/0  PHYSDEV match --physdev-in  vif91.0
> --physdev-is-bridged
> 
> 
> * ip="192.168.0.254"
> 
> iptables:
>  ACCEPT  udp  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-in
> vif92.0 --physdev-is-bridged udp spt:68 dpt:67
>  ACCEPT  all  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-out
> vif92.0 --physdev-is-bridged
>  ACCEPT  all  192.168.0.254  0.0.0.0/0  PHYSDEV match --physdev-in
> vif92.0 --physdev-is-bridged
> 
> ip6tables:
>  [nothing]
> 
> 
> * ip="2001:aaaa:bbbb:cccc::1 eui64"
> 
> iptables:
>  [nothing]
> 
> ip6tables:
>  DROP    udp    ::/0                 ::/0
> PHYSDEV match --physdev-in  vif94.0 --physdev-is-bridged udp spt:547
> dpt:546
>  DROP    icmpv6 ::/0                 ::/0
> PHYSDEV match --physdev-in  vif94.0 --physdev-is-bridged ipv6-icmptype
> 134
>  ACCEPT  udp    ::/0                 ::/0
> PHYSDEV match --physdev-in  vif94.0 --physdev-is-bridged udp spt:546
> dpt:547
>  ACCEPT  all    fe80::216:3eff:fed0:da2d/128  ::/0
> PHYSDEV match --physdev-in  vif94.0 --physdev-is-bridged
>  ACCEPT  all    ::/0                 ::/0
> PHYSDEV match --physdev-out vif94.0 --physdev-is-bridged
>  ACCEPT  all    2001:aaaa:bbbb:cccc::1/128  ::/0
> PHYSDEV match --physdev-in  vif94.0 --physdev-is-bridged
>  ACCEPT  all    ::216:3eff:fed0:da2d/::ffff:ffff:ffff:ffff  ::/0
> PHYSDEV mat

216:3eff:fed0:da2d here is related to the mac address and therefore to
the eui64 option?

BTW, please can you update docs/misc/xl-network-configuration.markdown
to reflect the ipv6 behaviour.

> ch --physdev-in  vif94.0 --physdev-is-bridged
> 
> 
> * ip="192.168.0.254 2001:aaaa:bbbb:cccc::1" (either ipv4 or ipv6 can
> be replaced by the 0.0.0.0/0 or ::0/0 address to allow any, the
> dhcp/nd rules might be redudant then).
> 
> iptables:
>  ACCEPT  udp  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-in
> vif95.0 --physdev-is-bridged udp spt:68 dpt:67
>  ACCEPT  all  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-out
> vif95.0 --physdev-is-bridged
>  ACCEPT  all  192.168.0.254  0.0.0.0/0  PHYSDEV match --physdev-in
> vif95.0 --physdev-is-bridged
> 
> ip6tables:
>  DROP    udp    ::/0                 ::/0           PHYSDEV match
> --physdev-in  vif95.0 --physdev-is-bridged udp spt:547 dpt:546
>  DROP    icmpv6 ::/0                 ::/0           PHYSDEV match
> --physdev-in  vif95.0 --physdev-is-bridged ipv6-icmptype 134
>  ACCEPT  udp    ::/0                 ::/0           PHYSDEV match
> --physdev-in  vif95.0 --physdev-is-bridged udp spt:546 dpt:547
>  ACCEPT  all    fe80::216:3eff:fed0:da2d/128  ::/0  PHYSDEV match
> --physdev-in  vif95.0 --physdev-is-bridged
>  ACCEPT  all    ::/0                 ::/0           PHYSDEV match
> --physdev-out vif95.0 --physdev-is-bridged
> 
> 
> 
> 
> >> By default, domains aren't allows to send Router Advertisement
> >> or DHCP responses, see the ipv6_allow_ra to enable them.
> >
> > How does one go about setting this?
> 
> Well ... I thought it would work just like  accel=  (which I'm using
> in prod but that's with 'xm' on 4.1).
> Turns out after a bit of googling that 'accel' might work just by luck
> because somehow at some point passing that option was added to 'xm' in
> 2007 ...
> 
> I just retried now with 'xl' under 4.4 and indeed that doesn't work :(
> 
> So if I want to pass custom parameters to the vif-script, I have to
> add it to libxl ?

At the moment, yes.

> And if yes, is that acceptable ? What would be the best way : add each
> parameter independently, or add a single 'hotplug_extra' parameter
> that would need to be parsed in the hotplug scripts themselves ?

TBH I'm not sure. I'll reply separately retitling this to be more
obvious and ccing a few folks.

> (I'll wait until I fixed this before reposting obviously)

I think if you note it in the commit message that it is pending a
resolution to this then it would be reasonable to repost. I *think* that
the implementation of consuming these options from xenstored should be
largely orthogonal to the mechanism of how they get there.

BTW, as a hack/workaround for testing you could create vif-sylvain which
hacks things into the right places and then execs the real script.

> 
> >> +##
> >> +# Check if the given IP is IPv6 or not
> >> +#
> >> +is_ipv6()
> >> +{
> >> +        echo "$1" | perl -wane '/:/ && print "yes"'
> >
> > Annoyingly I don't think we currently require Perl in the runtime
> > environment (it is used at build time). Luckily I think you can
> > implement this as
> >         case $addr in
> >                 *:*) ipv6_addrs="$addrs $ipv6_addrs";;
> >                 *) ipv4.... ;;
> >         esac
> >
> > (probably inline in the handle_iptable function, no need for this helper
> > in that case)
> 
> 
> I fixed that using only 'awk'. I left it as a helper because it's used
> in vif-route as well later and I find it nicer to have the test in a
> single place.

Ack, thanks.

configuring vif options to be consumed by hotplug scripts (Was: Re: [PATCH 4/5] hotplug/linux: Add IPv6 support to the iptables logic)

[Ian Campbell]

Pulling this out into a new subthread. CCing Ian as the other tools
maintainer.

On Tue, 2014-05-20 at 13:58 +0200, Sylvain Munaut wrote:
> >> By default, domains aren't allows to send Router Advertisement
> >> or DHCP responses, see the ipv6_allow_ra to enable them.
> >
> > How does one go about setting this?
> 
> Well ... I thought it would work just like  accel=  (which I'm using
> in prod but that's with 'xm' on 4.1).
> Turns out after a bit of googling that 'accel' might work just by luck
> because somehow at some point passing that option was added to 'xm' in
> 2007 ...
> 
> I just retried now with 'xl' under 4.4 and indeed that doesn't work :(
> 
> So if I want to pass custom parameters to the vif-script, I have to
> add it to libxl ?
> And if yes, is that acceptable ? What would be the best way : add each
> parameter independently, or add a single 'hotplug_extra' parameter
> that would need to be parsed in the hotplug scripts themselves ?

TBH I'm not sure.

Adding the individual options is probably The Right Way(tm), since it
provides a meaningful API for libxl and allows for error checking and
configuring defaults etc. In general adding the ability to pass down an
arbitrary string etc does not lead to a good API.

But, as you've seen, it does have the shortcoming that it does make it
somewhat harder to deploy new configuration items or to do deployment
specific special stuff by adding custom code to vif scripts etc. Often
in cases like this we like to provide the proper way (individual
options) as well as some sort of escape hatch to allow workarounds/local
cfg etc.

Perhaps the existing ability to specify script=my-local-foo is already a
sufficient hatch though? On the the hand if you have some local
configuration option which takes e.g. an integer creating
my-local-foo-1, my-local-foo-2 etc is pretty tedious. Perhaps we could
arrange for it to be possible to pass parameters to the script somehow?

Ian, what do you think? 

Cheers,
Ian.

Re: [PATCH 4/5] hotplug/linux: Add IPv6 support to the iptables logic

[Sylvain Munaut]

Hi,


> So here we deny DHCP whereas for v4 we don't? Why is that? And in other
> cases for v4 we explicitly allow it?

Both for v4 and v6 it allows the VM to make DHCP requests (be a
client) in the case not everything is allowed.


> I see you called this out in the
> commit message, but I must confess I don't know v6 well enough to guess
> why. Is allowing a guest to send DHCP responses more dangerous for v6
> than with v4?

I'm not sure about "more dangerous" in absolute terms, but the fact
many distribs comes with v6 enabled but people don't always take care
to configure it properly by default, I thought it would be a good idea
to be "safe by default".

For v6, the Router Announcement can contain a flag compelling the
client to not only do the classic autoconfig but to also make a DHCPv6
requests to obtains missing parameters (like DNS which isn't part of
RA). A lot of distribs have v6 autoconfig enabled by default, I'm not
sure how many will actually obey this flag though.

Honestly maybe blocking dhcp server response for v4 by default would
make sense, but this could break some existing config and I tried to
stay away from implementing changes that would require people to do
stuff for it to work again.

All the changes here should have no breaking impact if people don't
change their config. They could however allow more stuff than before.
If they had before a working ip6tables setup that was set to FORWARD
policy DROP, and no 'ip' sets in the VIF config, their VMs wouldn't be
able to exchange ipv6 before and they will be able to afterwards.


>> ip6tables:
>>  ACCEPT  all    ::216:3eff:fed0:da2d/::ffff:ffff:ffff:ffff  ::/0
>> PHYSDEV mat
>
> 216:3eff:fed0:da2d here is related to the mac address and therefore to
> the eui64 option?

yes.


> BTW, please can you update docs/misc/xl-network-configuration.markdown
> to reflect the ipv6 behaviour.

Ok, I will.


Cheers,

    Sylvain

Re: [PATCH 4/5] hotplug/linux: Add IPv6 support to the iptables logic

[Ian Campbell]

On Tue, 2014-05-20 at 15:11 +0200, Sylvain Munaut wrote:
> Hi,
> 
> 
> > So here we deny DHCP whereas for v4 we don't? Why is that? And in other
> > cases for v4 we explicitly allow it?
> 
> Both for v4 and v6 it allows the VM to make DHCP requests (be a
> client) in the case not everything is allowed.

Ah, so it was blocking (from the guest PoV) outgoing responses and
incoming requests? But the guest could send a request and receive a
response? That makes sense.

> > I see you called this out in the
> > commit message, but I must confess I don't know v6 well enough to guess
> > why. Is allowing a guest to send DHCP responses more dangerous for v6
> > than with v4?
> 
> I'm not sure about "more dangerous" in absolute terms, but the fact
> many distribs comes with v6 enabled but people don't always take care
> to configure it properly by default, I thought it would be a good idea
> to be "safe by default".

OK. Can you include the rationale in the commit log please.

I'm not 100% sure about the argument that distros/admins might not
configure things properly and so we should help them to not hurt
themselves. Historically when we've gone down that path it's turned out
to be more rather than less confusing (essentially because Xen become
"special" in some way or another). For example this is why we no longer
try and setup networking and point people instead towards the distro
networking configuration tools.

IOW perhaps we should just write on
http://wiki.xen.org/wiki/HostConfiguration/Networking that people should
take care to setup IPv6 properly, or something.

> For v6, the Router Announcement can contain a flag compelling the
> client to not only do the classic autoconfig but to also make a DHCPv6
> requests to obtains missing parameters (like DNS which isn't part of
> RA). A lot of distribs have v6 autoconfig enabled by default, I'm not
> sure how many will actually obey this flag though.
> 
> Honestly maybe blocking dhcp server response for v4 by default would
> make sense, but this could break some existing config and I tried to
> stay away from implementing changes that would require people to do
> stuff for it to work again.

Yes, avoiding breaking changes is certainly a good idea.

> All the changes here should have no breaking impact if people don't
> change their config. They could however allow more stuff than before.
> If they had before a working ip6tables setup that was set to FORWARD
> policy DROP, and no 'ip' sets in the VIF config, their VMs wouldn't be
> able to exchange ipv6 before and they will be able to afterwards.

I think that's OK. Might be one for the release notes I suppose.

Ian.

Re: [PATCH 5/5] hotplug/linux: Add IPv6 support to vif-route

[Sylvain Munaut]

Hi,

> I'm afraid I don't really understand IPv6 neighbour discovery
> etc. well enough to review this.
>
> Can you point me to an explanation ?  (I understand IPv4 quite well.)

It's pretty much that same as IPv4 except instead of being a different
protocol (ARP), it's done over ICMPv6 with special multicast
addresses.

https://en.wikipedia.org/wiki/Neighbor_Discovery_Protocol
http://www.cisco.com/c/dam/en/us/td/i/000001-100000/50001-55000/52501-53000/52673.ps/_jcr_content/renditions/52673.jpg

The kernel has a 'proxy_ndp' mode that's similar to 'proxy_arp' except
you need to manually specify which address you want it to proxy and on
which interface.


Cheers,

   Sylvain Munaut

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 5/5] hotplug/linux: Add IPv6 support to vif-route

[Ian Jackson]

Sylvain Munaut writes ("[PATCH 5/5] hotplug/linux: Add IPv6 support to vif-route"):
> Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com>
> ---
>  docs/man/xl-network-configuration.markdown.5 |  9 ++++++++
>  tools/hotplug/Linux/vif-common.sh            | 33 ++++++++++++++++++++++++++++
>  tools/hotplug/Linux/vif-route                | 20 ++++++++++++++++-
>  tools/libxl/libxl_nic.c                      |  5 +++++
>  tools/libxl/libxl_types.idl                  |  1 +
>  tools/libxl/xl_cmdimpl.c                     |  7 ++++++
>  6 files changed, 74 insertions(+), 1 deletion(-)

I'm afraid I don't really understand IPv6 neighbour discovery
etc. well enough to review this.

Can you point me to an explanation ?  (I understand IPv4 quite well.)

> diff --git a/docs/man/xl-network-configuration.markdown.5 b/docs/man/xl-network-configuration.markdown.5
> index 5b86974..ce0da42 100644
> --- a/docs/man/xl-network-configuration.markdown.5
> +++ b/docs/man/xl-network-configuration.markdown.5
> @@ -143,6 +143,15 @@ address globally if necessary. This is of course only usable for the
>  vif-bridge script as the vif-route will require a fully defined address
>  in the 'ip' field.
>  
> +### ipv6_proxy_ndp
> +
> +Enabling this option will cause the Dom0 to answer the IPv6 Neighbor
> +Sollicitation messages it sees on 'gatewaydev' for the VM IPs.

 Solicitation

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 5/5] hotplug/linux: Add IPv6 support to vif-route

[Sylvain Munaut]

Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com>
---
 docs/man/xl-network-configuration.markdown.5 |  9 ++++++++
 tools/hotplug/Linux/vif-common.sh            | 33 ++++++++++++++++++++++++++++
 tools/hotplug/Linux/vif-route                | 20 ++++++++++++++++-
 tools/libxl/libxl_nic.c                      |  5 +++++
 tools/libxl/libxl_types.idl                  |  1 +
 tools/libxl/xl_cmdimpl.c                     |  7 ++++++
 6 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/docs/man/xl-network-configuration.markdown.5 b/docs/man/xl-network-configuration.markdown.5
index 5b86974..ce0da42 100644
--- a/docs/man/xl-network-configuration.markdown.5
+++ b/docs/man/xl-network-configuration.markdown.5
@@ -143,6 +143,15 @@ address globally if necessary. This is of course only usable for the
 vif-bridge script as the vif-route will require a fully defined address
 in the 'ip' field.
 
+### ipv6_proxy_ndp
+
+Enabling this option will cause the Dom0 to answer the IPv6 Neighbor
+Sollicitation messages it sees on 'gatewaydev' for the VM IPs.
+
+Valid values are 'no' & 'yes'. Defaults to 'no'.
+
+This feature is currently only supported in the linux hotplug script.
+
 ### backend
 
 Specifies the backend domain which this device should attach to. This
diff --git a/tools/hotplug/Linux/vif-common.sh b/tools/hotplug/Linux/vif-common.sh
index b92c36c..d5d98a7 100644
--- a/tools/hotplug/Linux/vif-common.sh
+++ b/tools/hotplug/Linux/vif-common.sh
@@ -346,3 +346,36 @@ dom0_ip()
   fi
   echo "$result"
 }
+
+
+##
+# ip6_of interface
+#
+# Print the first IPv6 address currently in use at the given interface, or nothing if
+# the interface is not up.
+#
+ip6_of()
+{
+  ip -6 -o addr show primary dev "$1" scope global | awk '$3 == "inet6" {split($4,i,"/"); print i[1]; exit}'
+}
+
+
+##
+# dom0_ip6
+#
+# Print the IPv6 address of the interface in dom0 through which we are routing.
+# This is the IP address on the interface specified as "netdev" as a parameter
+# to these scripts, or eth0 by default.  This function will return nothing if no
+# such interface could be found.
+#
+dom0_ip6()
+{
+  local nd=${netdev:-eth0}
+  local result=$(ip6_of "$nd")
+  if [ -z "$result" ]
+  then
+        ""
+  else
+        echo "$result"
+  fi
+}
diff --git a/tools/hotplug/Linux/vif-route b/tools/hotplug/Linux/vif-route
index c149ffc..f07cd75 100644
--- a/tools/hotplug/Linux/vif-route
+++ b/tools/hotplug/Linux/vif-route
@@ -20,11 +20,21 @@ dir=$(dirname "$0")
 . "${dir}/vif-common.sh"
 
 main_ip=$(dom0_ip)
+main_ip6=$(dom0_ip6)
+
+ipv6_proxy_ndp=$(xenstore_read_default "$XENBUS_PATH/ipv6_proxy_ndp" "0")
+
 
 case "${command}" in
     online)
         ifconfig ${dev} ${main_ip} netmask 255.255.255.255 up
         echo 1 >/proc/sys/net/ipv4/conf/${dev}/proxy_arp
+        if [ ! -z "${main_ip6}" ]; then
+            ip -6 addr add ${main_ip6} dev ${dev}
+            if [ "${ipv6_proxy_ndp}" != "0" ]; then
+                echo 1 >/proc/sys/net/ipv6/conf/${dev}/proxy_ndp
+            fi
+        fi
         ipcmd='add'
         cmdprefix=''
         ;;
@@ -39,7 +49,15 @@ if [ "${ip}" ] ; then
     # If we've been given a list of IP addresses, then add routes from dom0 to
     # the guest using those addresses.
     for addr in ${ip} ; do
-      ${cmdprefix} ip route ${ipcmd} ${addr} dev ${dev} src ${main_ip}
+        result=$(is_ipv6 "${addr}")
+        if [ -z "${result}" ] ; then
+            ${cmdprefix} ip route ${ipcmd} ${addr} dev ${dev} src ${main_ip}
+        else
+            ${cmdprefix} ip -6 route ${ipcmd} ${addr} dev ${dev} src ${main_ip6}
+            if [ "${ipv6_proxy_ndp}" != "0" ]; then
+                ${cmdprefix} ip -6 neighbor ${ipcmd} proxy ${addr} dev ${netdev:-eth0}
+            fi
+        fi
     done
 fi
 
diff --git a/tools/libxl/libxl_nic.c b/tools/libxl/libxl_nic.c
index 61b55ca..47b0a63 100644
--- a/tools/libxl/libxl_nic.c
+++ b/tools/libxl/libxl_nic.c
@@ -214,6 +214,11 @@ static void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
                             nic->rate_interval_usecs));
     }
 
+    if (!libxl_defbool_is_default(nic->ipv6_proxy_ndp)) {
+        flexarray_append_pair(back,"ipv6_proxy_ndp",
+            libxl_defbool_val(nic->ipv6_proxy_ndp) ? "1" : "0");
+    }
+
     flexarray_append(back, "bridge");
     flexarray_append(back, libxl__strdup(gc, nic->bridge));
     flexarray_append(back, "handle");
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index a612d1f..628b102 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -626,6 +626,7 @@ libxl_device_nic = Struct("device_nic", [
     ("rate_bytes_per_interval", uint64),
     ("rate_interval_usecs", uint32),
     ("gatewaydev", string),
+    ("ipv6_proxy_ndp", libxl_defbool),
     # Note that the COLO configuration settings should be considered unstable.
     # They may change incompatibly in future versions of Xen.
     ("coloft_forwarddev", string)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 7e8a8ae..9996c61 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1059,6 +1059,13 @@ static int parse_nic_config(libxl_device_nic *nic, XLU_Config **config, char *to
         parse_vif_rate(config, oparg, nic);
     } else if (MATCH_OPTION("forwarddev", token, oparg)) {
         replace_string(&nic->coloft_forwarddev, oparg);
+    } else if (MATCH_OPTION("ipv6_proxy_ndp", token, oparg)) {
+	if (!strcasecmp(oparg, "on") || !strcasecmp(oparg, "1"))
+		libxl_defbool_set(&nic->ipv6_proxy_ndp, true);
+	else if (!strcasecmp(oparg, "off") || !strcasecmp(oparg, "0"))
+		libxl_defbool_set(&nic->ipv6_proxy_ndp, false);
+	else
+		fprintf(stderr, "Invalid value for 'ipv6_proxy_ndp' parameter\n");
     } else if (MATCH_OPTION("accel", token, oparg)) {
         fprintf(stderr, "the accel parameter for vifs is currently not supported\n");
     } else {
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel