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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.


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