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

[Sylvain Munaut <s.munaut@xxxxxxxxxxxxxxxxxxxx>]

On Thu, May 15, 2014 at 6:15 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 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

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