[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3/5] hotplug/linux: Improve iptables logic
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |