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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx>
> ---
>  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.


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