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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-05-20 at 16:56 +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.
> 
> Here below is a comparaison of the rules before / after.

"comparison"

>  <*> 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-out
>  vif88.0 --physdev-is-bridged
>   ACCEPT  all  0.0.0.0/0     0.0.0.0/0  PHYSDEV match --physdev-in
>  vif88.0 --physdev-is-bridged
> 

i.e. no change?

>  <*> 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  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
>   ACCEPT  all  0.0.0.0/0      0.0.0.0/0  PHYSDEV match --physdev-out
>  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  192.168.0.141  0.0.0.0/0  PHYSDEV match --physdev-in
>  vif89.0 --physdev-is-bridged
> 
> Signed-off-by: Sylvain Munaut <s.munaut@xxxxxxxxxxxxxxxxxxxx>

Looks good to me:
Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

I'm going to give it a day or two for others to comment before I apply.

When I do apply I'll fix the typo, insert the answer to that question
and unwrap the example lines as I go.

Thanks!

> ---
>  tools/hotplug/Linux/vif-common.sh |   64 
> ++++++++++++++++++++++++++++---------
>  1 file changed, 49 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/hotplug/Linux/vif-common.sh 
> b/tools/hotplug/Linux/vif-common.sh
> index 28ddae5..b098630 100644
> --- a/tools/hotplug/Linux/vif-common.sh
> +++ b/tools/hotplug/Linux/vif-common.sh
> @@ -123,6 +123,10 @@ ip=$(xenstore_read_default "$XENBUS_PATH/ip" "$ip")
>  
>  frob_iptable()
>  {
> +  local has_err="no"
> +  local has_any="no"
> +
> +  # Add or remove
>    if [ "$command" == "online" -o "$command" == "add" ]
>    then
>      local c="-I"
> @@ -130,16 +134,40 @@ frob_iptable()
>      local c="-D"
>    fi
>  
> -  iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
> -    "$@" -j ACCEPT 2>/dev/null &&
> +  # Add rules for each address
> +  local addr
> +
> +  for addr in $@; do
> +    if [ "$addr" = "any" ]; then
> +      addr="0.0.0.0/0"
> +      has_any="yes"
> +    fi
> +
> +    iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in 
> "$dev" \
> +      -s "$addr" -j ACCEPT 2>/dev/null || has_err="yes"
> +  done
> +
> +  # 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 || has_err="yes"
>  
> -  if [ \( "$command" == "online" -o "$command" == "add" \) -a $? -ne 0 ]
> +  # If 'any' isn't allowed, we needs to allow a few more things
> +  if [ "$has_any" != "yes" ]
> +  then
> +
> +    # 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 || has_err="yes"
> +
> +  fi
> +
> +  # Error handling
> +  if [ \( "$command" == "online" -o "$command" == "add" \) -a "$has_err" == 
> "yes" ]
>    then
>      log err "iptables setup failed. This may affect guest networking."
>    fi
>  }
> +}
>  
> 
>  ##
> @@ -160,21 +188,27 @@ 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"
> +
> +  if [ "$ipv4_addrs" != "" ]
> +  then
> +    frob_iptable $ipv4_addrs
>    fi
>  
>    release_lock "iptables"



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