| Message ID | 20181218211451.16413-1-fw@strlen.de |
|---|---|
| State | Superseded |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [iptables] libxtables: work around unwanted kernel module load | expand |
On Tue, Dec 18, 2018 at 10:14:51PM +0100, Florian Westphal wrote: > Following command: > iptables -D FORWARD -m physdev ... > causes connectivity loss in some setups. So, scenario is: User calls this where there is no rule at all with -m physdev, right? > This is caused indirectly by commit 3b2530ce7a0d6aa3bee687bf0167bb490 > ("xtables: Do not register matches/targets with incompatible revision"). > > With this change, libtables queries the kernel for the match revision. > This causes the "phydev" module to be loaded, which in turn enables the > "call-iptables" infrastructure. > > bridged packets might then get dropped by the iptables ruleset. > > The better fix would be to change the "call-iptables" defaults to 0 and > enforce explicit setting to 1. > Another fix would be to only probe on rule add, not delete, but this is > a detail that libxtables doesn't know. > > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > libxtables/xtables.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/libxtables/xtables.c b/libxtables/xtables.c > index ea9bb102c8eb..4fdf6554f339 100644 > --- a/libxtables/xtables.c > +++ b/libxtables/xtables.c > @@ -1011,11 +1011,6 @@ static bool xtables_fully_register_pending_match(struct xtables_match *me) > const char *rn; > int compare; > > - /* See if new match can be used. */ > - rn = (me->real_name != NULL) ? me->real_name : me->name; > - if (!compatible_match_revision(rn, me->revision)) > - return false; > - > old = xtables_find_match(me->name, XTF_DURING_LOAD, NULL); > while (old) { > compare = xtables_match_prefer(old, me); > -- > 2.19.2 >
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Tue, Dec 18, 2018 at 10:14:51PM +0100, Florian Westphal wrote: > > Following command: > > iptables -D FORWARD -m physdev ... > > causes connectivity loss in some setups. > > So, scenario is: User calls this where there is no rule at all with -m > physdev, right? Yes, exactly. Its part of some 'delete old/previous rules' after startup cleanup procedure, where iptables -F/X can't be used (as it might contain rules set up by someone else).
On Fri, Dec 21, 2018 at 04:25:48PM +0100, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Tue, Dec 18, 2018 at 10:14:51PM +0100, Florian Westphal wrote: > > > Following command: > > > iptables -D FORWARD -m physdev ... > > > causes connectivity loss in some setups. > > > > So, scenario is: User calls this where there is no rule at all with -m > > physdev, right? > > Yes, exactly. > Its part of some 'delete old/previous rules' after startup cleanup > procedure, where iptables -F/X can't be used (as it might contain > rules set up by someone else). Should we fix this from the kernel? ie. like we do with conntracking, track how many rules with physdev are loaded to register the br_netfilter hooks? That will be a bit more work... not sure it is worth. I'm not sure what are the implications of removing this chunk in userspace, ie. side effects of your workaround. Let me know, thanks!
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > Should we fix this from the kernel? ie. like we do with conntracking, > track how many rules with physdev are loaded to register the > br_netfilter hooks? I don't see how this is possible -- it would require to change the default of the sysctls to 0. And we don't know if br_netfilter gets loaded because of the physdev dependency or because its due to 'modprobe br_netfilter' by the user. In the second case, changing the defaults alters behaviour in an incompatible way. Only solution I see is to get rid of the module dependency in physdev (its artificial) and make checkentry() call request_module() once. Not sure thats better, but it would avoid this problem, as '-D ... -m physdev' doesn't call checkentry. > That will be a bit more work... not sure it is worth. I'm not sure > what are the implications of removing this chunk in userspace, ie. > side effects of your workaround. Yes, it un-does changes that went into 1.8.0 release.
On Sat, Dec 29, 2018 at 02:13:23AM +0100, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > Should we fix this from the kernel? ie. like we do with conntracking, > > track how many rules with physdev are loaded to register the > > br_netfilter hooks? > > I don't see how this is possible -- it would require to change > the default of the sysctls to 0. > > And we don't know if br_netfilter gets loaded because of the > physdev dependency or because its due to 'modprobe br_netfilter' > by the user. > > In the second case, changing the defaults alters behaviour in > an incompatible way. > > Only solution I see is to get rid of the module dependency > in physdev (its artificial) and make checkentry() call > request_module() once. > > Not sure thats better, but it would avoid this problem, as > '-D ... -m physdev' doesn't call checkentry. request_module() should be fine. Long time ago br_netfilter used to be built-in into the bridge module, this was very much defeating the purpose of the nf_hook static_key at that time - that was not good. This symbol stub trick was added to relieve the impact of changing such change in default behaviour time ago, ie. if physdev is used, user wants br_netfilter module. Using request_module() would just relax the dependency a bit - since you could now rmmod br_netfilter - but this still will serve for the purpose of helping users that are jumping from an old Linux kernel to more recent ones.
diff --git a/libxtables/xtables.c b/libxtables/xtables.c index ea9bb102c8eb..4fdf6554f339 100644 --- a/libxtables/xtables.c +++ b/libxtables/xtables.c @@ -1011,11 +1011,6 @@ static bool xtables_fully_register_pending_match(struct xtables_match *me) const char *rn; int compare; - /* See if new match can be used. */ - rn = (me->real_name != NULL) ? me->real_name : me->name; - if (!compatible_match_revision(rn, me->revision)) - return false; - old = xtables_find_match(me->name, XTF_DURING_LOAD, NULL); while (old) { compare = xtables_match_prefer(old, me);
Following command: iptables -D FORWARD -m physdev ... causes connectivity loss in some setups. This is caused indirectly by commit 3b2530ce7a0d6aa3bee687bf0167bb490 ("xtables: Do not register matches/targets with incompatible revision"). With this change, libtables queries the kernel for the match revision. This causes the "phydev" module to be loaded, which in turn enables the "call-iptables" infrastructure. bridged packets might then get dropped by the iptables ruleset. The better fix would be to change the "call-iptables" defaults to 0 and enforce explicit setting to 1. Another fix would be to only probe on rule add, not delete, but this is a detail that libxtables doesn't know. Signed-off-by: Florian Westphal <fw@strlen.de> --- libxtables/xtables.c | 5 ----- 1 file changed, 5 deletions(-)