| Message ID | 1421009571-5279-3-git-send-email-richard@nod.at |
|---|---|
| State | Not Applicable |
| Delegated to: | Pablo Neira |
| Headers | show |
On Sun, 2015-01-11 at 21:52 +0100, Richard Weinberger wrote: > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > include/linux/netfilter/x_tables.h | 22 ++++++++++++++++++++++ > net/ipv4/netfilter/arp_tables.c | 28 +++++++++++++++++----------- > net/ipv4/netfilter/ip_tables.c | 15 +++++---------- > net/ipv6/netfilter/ip6_tables.c | 18 +++++++----------- > net/netfilter/xt_physdev.c | 9 ++------- > 5 files changed, 53 insertions(+), 39 deletions(-) Richard, I dislike this, sorry. iptables is already horribly expensive, you add another expensive step for every rule. device aliasing can be done from user space. iptables should have used ifindex, its sad we allowed the substring match in first place. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 12.01.2015 um 17:04 schrieb Eric Dumazet: > On Sun, 2015-01-11 at 21:52 +0100, Richard Weinberger wrote: >> Signed-off-by: Richard Weinberger <richard@nod.at> >> --- >> include/linux/netfilter/x_tables.h | 22 ++++++++++++++++++++++ >> net/ipv4/netfilter/arp_tables.c | 28 +++++++++++++++++----------- >> net/ipv4/netfilter/ip_tables.c | 15 +++++---------- >> net/ipv6/netfilter/ip6_tables.c | 18 +++++++----------- >> net/netfilter/xt_physdev.c | 9 ++------- >> 5 files changed, 53 insertions(+), 39 deletions(-) > > Richard, I dislike this, sorry. That's fine, the series carries the "RFC" burning mark for a reason. > iptables is already horribly expensive, you add another expensive step > for every rule. Yeah, you mean the extra unlikey() check? > device aliasing can be done from user space. How? I did this series because I'm not so happy with the device renaming what udev does. The idea was to offer udev a better kernel interface to deal with aliases. Such that one can use the regular names form the kernel and the predictable names generated from udev. For block devices it was easy, we have the good old symlink. For network interface the kernel does not offer an API. > iptables should have used ifindex, its sad we allowed the substring > match in first place. Maybe nftables can do better. :-) Thanks, //richard -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 2015-01-12 17:04, Eric Dumazet wrote: > >iptables should have used ifindex [for interface matching], >it[']s sad we allowed the substring match in first place. How would you solve interface name wildcards with ifindices? (They come in handy if you have something like lots of tun+/veth+ interfaces from openvpn/lxc.) -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-01-12 at 17:32 +0100, Jan Engelhardt wrote: > On Monday 2015-01-12 17:04, Eric Dumazet wrote: > > > >iptables should have used ifindex [for interface matching], > >it[']s sad we allowed the substring match in first place. > > How would you solve interface name wildcards with ifindices? > (They come in handy if you have something like lots of tun+/veth+ > interfaces from openvpn/lxc.) This is what I said : "it[']s sad we allowed the substring match in first place." This obviously referred to wildcards, in the in/out interface match for every _single_ rule, consuming 64 bytes of memory per rule and per cpu ! Which is absolutely crazy in term of memory usage. Matching tun+ or whatever could easily be done by a match (-m ...), because you can factorize this quite easily (called once for a group of rules) -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-01-12 at 17:39 +0100, Patrick Schaaf wrote: > > iptables should have used ifindex, its sad we allowed the substring > > > match in first place. > > > > Not to comment on the ifalias thing, which I think is unneccessary, > too, but matching on interface names instead of only ifindex, is > definitely needed, so that one can establish a full ruleset before > interfaces even exist. That's good practise at boottime, but also > needed for dynamic interface creation during runtime. > > > > A pure ifindex-during-packet-inspection approach might still work, but > the ruleset must IMO keep the interface names. Maybe register them in > a hash, keyed by name, with values an ifindex or ifindex set (for > wildcard names), plus a reverse mapping from active ifindices to all > places in these hash values where an ifindex has been remembered. On > interface creation / destruction that structure could then be updated, > and active packet filtering rules would refer to (and keep a refcount > on) specific hash elements. > Please do not send html messages : Your reply did not reach the lists. Then, all you mention could have been solved by proper userspace support. Every time you add an interface or change device name, you could change firewalls rules if needed. Nothing shocking here. The ruleset can indeed mention interface names, but the kernel part really should not care about names, which are a 'human' convenient way to represent things. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 12 January 2015 08:51:54 Eric Dumazet wrote: > On Mon, 2015-01-12 at 17:39 +0100, Patrick Schaaf wrote: > > > > Not to comment on the ifalias thing, which I think is unneccessary, > > too, but matching on interface names instead of only ifindex, is > > definitely needed, so that one can establish a full ruleset before > > interfaces even exist. That's good practise at boottime, but also > > needed for dynamic interface creation during runtime. > > Please do not send html messages : Your reply did not reach the lists. Sigh. Sorry... > Then, all you mention could have been solved by proper userspace > support. > > Every time you add an interface or change device name, you could change > firewalls rules if needed. Nothing shocking here. That is totally impractical, IMO. Interfaces come and go through many different actions. There's the admin downing and upping stuff like bridges or bonds. There's stuff like libvirt / KVM / qemu creating and destroying interfaces. In all these cases, in my practise, I give the interfaces useful names to that I can prefix-match them in iptables rules. Dynamically modifying the ruleset for each such creation and destruction, would be a huge burden. The base ruleset would need suitable "hooks" where these rules were inserted (ordering matters!). The addition would hardly be atomic (with traditional iptables, unless done by generating a whole new ruleset and restoring). The programs (e.g. libvirt) would need to be able to call out to these specially crafted rule generator scripts. The admin would need to add them as pre/post actions to their static (manual) interface configuration. Loading and looking at the ruleset before bringing up the interface would be impossible. Note that I do fully agree that it's sad that iptables rules waste all that memory for each and every rule! I remember musing about improving that in talks with Harald Welte back in the 90ies. A simple match would be perfectly fine for me. Only having ifindex support, isn't. best regards Patrick -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12.01, Patrick Schaaf wrote: > On Monday 12 January 2015 08:51:54 Eric Dumazet wrote: > > On Mon, 2015-01-12 at 17:39 +0100, Patrick Schaaf wrote: > > > > > > Not to comment on the ifalias thing, which I think is unneccessary, > > > too, but matching on interface names instead of only ifindex, is > > > definitely needed, so that one can establish a full ruleset before > > > interfaces even exist. That's good practise at boottime, but also > > > needed for dynamic interface creation during runtime. > > > > Please do not send html messages : Your reply did not reach the lists. > > Sigh. Sorry... > > > Then, all you mention could have been solved by proper userspace > > support. > > > > Every time you add an interface or change device name, you could change > > firewalls rules if needed. Nothing shocking here. > > That is totally impractical, IMO. > > Interfaces come and go through many different actions. There's the admin > downing and upping stuff like bridges or bonds. There's stuff like libvirt / > KVM / qemu creating and destroying interfaces. In all these cases, in my > practise, I give the interfaces useful names to that I can prefix-match them > in iptables rules. > > Dynamically modifying the ruleset for each such creation and destruction, > would be a huge burden. The base ruleset would need suitable "hooks" where > these rules were inserted (ordering matters!). The addition would hardly be > atomic (with traditional iptables, unless done by generating a whole new > ruleset and restoring). The programs (e.g. libvirt) would need to be able to > call out to these specially crafted rule generator scripts. The admin would > need to add them as pre/post actions to their static (manual) interface > configuration. Loading and looking at the ruleset before bringing up the > interface would be impossible. devgroups seem like the best solution for this. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 12 January 2015 17:22:57 Patrick McHardy wrote: > On 12.01, Patrick Schaaf wrote: > > > > Interfaces come and go through many different actions. There's the admin > > downing and upping stuff like bridges or bonds. There's stuff like libvirt > > / KVM / qemu creating and destroying interfaces. In all these cases, in > > my practise, I give the interfaces useful names to that I can > > prefix-match them in iptables rules. > > > > Dynamically modifying the ruleset for each such creation and destruction, > > would be a huge burden. The base ruleset would need suitable "hooks" where > > these rules were inserted (ordering matters!). The addition would hardly > > be > > atomic (with traditional iptables, unless done by generating a whole new > > ruleset and restoring). The programs (e.g. libvirt) would need to be able > > to call out to these specially crafted rule generator scripts. The admin > > would need to add them as pre/post actions to their static (manual) > > interface configuration. Loading and looking at the ruleset before > > bringing up the interface would be impossible. > > devgroups seem like the best solution for this. Could be, technically. Is there devgroup support in libvirt, ifcfg, whatever other distros use for their static interface configuration? Or, do I again have to write pre/post scripts to set devgroups? Wouldn't bother me too much nowadays, I've automated that for ifcfg style stuff in my production environment a year ago, but it's something an admin must actively manage... There is other stuff, apart from libvirt, that creates and destroys interfaces on the fly. From my production environment, there's at least keepalived, which creates macvlan interfaces on the fly for VRRP VMAC support. I can configure the name for that, but nothing else, nor can I call a script pre/post for that. And my iptables rules on that boxen _do_ match specially on these interfaces. Gooling a bit around does not immediately turn up any good documentation on it at all (four year old iproute2 commits, once I give that as a search term too?). Looks very sketchy (although the fundamental idea is clear to me. I'm looking through the normal admin practise lens....) best regards Patrick -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h index a3e215b..15bda23 100644 --- a/include/linux/netfilter/x_tables.h +++ b/include/linux/netfilter/x_tables.h @@ -351,6 +351,28 @@ static inline unsigned long ifname_compare_aligned(const char *_a, return ret; } +/* + * A wrapper around ifname_compare_aligned() to match against dev->name and + * dev->ifalias. + */ +static inline unsigned long ifname_compare_all(const struct net_device *dev, + const char *name, + const char *mask) +{ + unsigned long res = 0; + + if (!dev) + goto out; + + res = ifname_compare_aligned(dev->name, name, mask); + if (unlikely(dev->ifalias && res)) + res = ifname_compare_aligned(dev->ifalias, name, mask); + +out: + return res; +} + + struct nf_hook_ops *xt_hook_link(const struct xt_table *, nf_hookfn *); void xt_hook_unlink(const struct xt_table *, struct nf_hook_ops *); diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c index f95b6f9..457d4ed 100644 --- a/net/ipv4/netfilter/arp_tables.c +++ b/net/ipv4/netfilter/arp_tables.c @@ -81,19 +81,30 @@ static inline int arp_devaddr_compare(const struct arpt_devaddr_info *ap, * Some arches dont care, unrolling the loop is a win on them. * For other arches, we only have a 16bit alignement. */ -static unsigned long ifname_compare(const char *_a, const char *_b, const char *_mask) +static unsigned long ifname_compare(const struct net_device *dev, + const char *_b, const char *_mask) { #ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS - unsigned long ret = ifname_compare_aligned(_a, _b, _mask); + unsigned long ret = ifname_compare_all(dev, _b, _mask); #else unsigned long ret = 0; - const u16 *a = (const u16 *)_a; + const u16 *a = (const u16 *)dev->name; const u16 *b = (const u16 *)_b; const u16 *mask = (const u16 *)_mask; int i; for (i = 0; i < IFNAMSIZ/sizeof(u16); i++) ret |= (a[i] ^ b[i]) & mask[i]; + + if (likely(!(dev->ifalias && ret))) + goto out; + + ret = 0; + a = (const u16 *)dev->ifalias; + for (i = 0; i < IFNAMSIZ/sizeof(u16); i++) + ret |= (a[i] ^ b[i]) & mask[i]; + +out: #endif return ret; } @@ -101,8 +112,8 @@ static unsigned long ifname_compare(const char *_a, const char *_b, const char * /* Returns whether packet matches rule or not. */ static inline int arp_packet_match(const struct arphdr *arphdr, struct net_device *dev, - const char *indev, - const char *outdev, + const struct net_device *indev, + const struct net_device *outdev, const struct arpt_arp *arpinfo) { const char *arpptr = (char *)(arphdr + 1); @@ -252,11 +263,9 @@ unsigned int arpt_do_table(struct sk_buff *skb, const struct net_device *out, struct xt_table *table) { - static const char nulldevname[IFNAMSIZ] __attribute__((aligned(sizeof(long)))); unsigned int verdict = NF_DROP; const struct arphdr *arp; struct arpt_entry *e, *back; - const char *indev, *outdev; void *table_base; const struct xt_table_info *private; struct xt_action_param acpar; @@ -265,9 +274,6 @@ unsigned int arpt_do_table(struct sk_buff *skb, if (!pskb_may_pull(skb, arp_hdr_len(skb->dev))) return NF_DROP; - indev = in ? in->name : nulldevname; - outdev = out ? out->name : nulldevname; - local_bh_disable(); addend = xt_write_recseq_begin(); private = table->private; @@ -291,7 +297,7 @@ unsigned int arpt_do_table(struct sk_buff *skb, do { const struct xt_entry_target *t; - if (!arp_packet_match(arp, skb->dev, indev, outdev, &e->arp)) { + if (!arp_packet_match(arp, skb->dev, in, out, &e->arp)) { e = arpt_next_entry(e); continue; } diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c index 99e810f..87df9ef 100644 --- a/net/ipv4/netfilter/ip_tables.c +++ b/net/ipv4/netfilter/ip_tables.c @@ -73,8 +73,8 @@ EXPORT_SYMBOL_GPL(ipt_alloc_initial_table); /* Performance critical - called for every packet */ static inline bool ip_packet_match(const struct iphdr *ip, - const char *indev, - const char *outdev, + const struct net_device *indev, + const struct net_device *outdev, const struct ipt_ip *ipinfo, int isfrag) { @@ -97,7 +97,7 @@ ip_packet_match(const struct iphdr *ip, return false; } - ret = ifname_compare_aligned(indev, ipinfo->iniface, ipinfo->iniface_mask); + ret = ifname_compare_all(indev, ipinfo->iniface, ipinfo->iniface_mask); if (FWINV(ret != 0, IPT_INV_VIA_IN)) { dprintf("VIA in mismatch (%s vs %s).%s\n", @@ -106,7 +106,7 @@ ip_packet_match(const struct iphdr *ip, return false; } - ret = ifname_compare_aligned(outdev, ipinfo->outiface, ipinfo->outiface_mask); + ret = ifname_compare_all(outdev, ipinfo->outiface, ipinfo->outiface_mask); if (FWINV(ret != 0, IPT_INV_VIA_OUT)) { dprintf("VIA out mismatch (%s vs %s).%s\n", @@ -292,11 +292,9 @@ ipt_do_table(struct sk_buff *skb, const struct net_device *out, struct xt_table *table) { - static const char nulldevname[IFNAMSIZ] __attribute__((aligned(sizeof(long)))); const struct iphdr *ip; /* Initializing verdict to NF_DROP keeps gcc happy. */ unsigned int verdict = NF_DROP; - const char *indev, *outdev; const void *table_base; struct ipt_entry *e, **jumpstack; unsigned int *stackptr, origptr, cpu; @@ -306,8 +304,6 @@ ipt_do_table(struct sk_buff *skb, /* Initialization */ ip = ip_hdr(skb); - indev = in ? in->name : nulldevname; - outdev = out ? out->name : nulldevname; /* We handle fragments by dealing with the first fragment as * if it was a normal packet. All other fragments are treated * normally, except that they will NEVER match rules that ask @@ -348,8 +344,7 @@ ipt_do_table(struct sk_buff *skb, const struct xt_entry_match *ematch; IP_NF_ASSERT(e); - if (!ip_packet_match(ip, indev, outdev, - &e->ip, acpar.fragoff)) { + if (!ip_packet_match(ip, in, out, &e->ip, acpar.fragoff)) { no_match: e = ipt_next_entry(e); continue; diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c index e080fbb..9ed5d70 100644 --- a/net/ipv6/netfilter/ip6_tables.c +++ b/net/ipv6/netfilter/ip6_tables.c @@ -83,8 +83,8 @@ EXPORT_SYMBOL_GPL(ip6t_alloc_initial_table); /* Performance critical - called for every packet */ static inline bool ip6_packet_match(const struct sk_buff *skb, - const char *indev, - const char *outdev, + const struct net_device *indev, + const struct net_device *outdev, const struct ip6t_ip6 *ip6info, unsigned int *protoff, int *fragoff, bool *hotdrop) @@ -109,7 +109,7 @@ ip6_packet_match(const struct sk_buff *skb, return false; } - ret = ifname_compare_aligned(indev, ip6info->iniface, ip6info->iniface_mask); + ret = ifname_compare_all(indev, ip6info->iniface, ip6info->iniface_mask); if (FWINV(ret != 0, IP6T_INV_VIA_IN)) { dprintf("VIA in mismatch (%s vs %s).%s\n", @@ -118,7 +118,7 @@ ip6_packet_match(const struct sk_buff *skb, return false; } - ret = ifname_compare_aligned(outdev, ip6info->outiface, ip6info->outiface_mask); + ret = ifname_compare_all(outdev, ip6info->outiface, ip6info->outiface_mask); if (FWINV(ret != 0, IP6T_INV_VIA_OUT)) { dprintf("VIA out mismatch (%s vs %s).%s\n", @@ -318,10 +318,8 @@ ip6t_do_table(struct sk_buff *skb, const struct net_device *out, struct xt_table *table) { - static const char nulldevname[IFNAMSIZ] __attribute__((aligned(sizeof(long)))); /* Initializing verdict to NF_DROP keeps gcc happy. */ unsigned int verdict = NF_DROP; - const char *indev, *outdev; const void *table_base; struct ip6t_entry *e, **jumpstack; unsigned int *stackptr, origptr, cpu; @@ -329,10 +327,8 @@ ip6t_do_table(struct sk_buff *skb, struct xt_action_param acpar; unsigned int addend; - /* Initialization */ - indev = in ? in->name : nulldevname; - outdev = out ? out->name : nulldevname; - /* We handle fragments by dealing with the first fragment as + /* Initialization: + * We handle fragments by dealing with the first fragment as * if it was a normal packet. All other fragments are treated * normally, except that they will NEVER match rules that ask * things we don't know, ie. tcp syn flag or ports). If the @@ -368,7 +364,7 @@ ip6t_do_table(struct sk_buff *skb, IP_NF_ASSERT(e); acpar.thoff = 0; - if (!ip6_packet_match(skb, indev, outdev, &e->ipv6, + if (!ip6_packet_match(skb, in, out, &e->ipv6, &acpar.thoff, &acpar.fragoff, &acpar.hotdrop)) { no_match: e = ip6t_next_entry(e); diff --git a/net/netfilter/xt_physdev.c b/net/netfilter/xt_physdev.c index f440f57..8d2ee7d 100644 --- a/net/netfilter/xt_physdev.c +++ b/net/netfilter/xt_physdev.c @@ -25,10 +25,8 @@ MODULE_ALIAS("ip6t_physdev"); static bool physdev_mt(const struct sk_buff *skb, struct xt_action_param *par) { - static const char nulldevname[IFNAMSIZ] __attribute__((aligned(sizeof(long)))); const struct xt_physdev_info *info = par->matchinfo; unsigned long ret; - const char *indev, *outdev; const struct nf_bridge_info *nf_bridge; /* Not a bridged IP packet or no info available yet: @@ -68,8 +66,7 @@ physdev_mt(const struct sk_buff *skb, struct xt_action_param *par) if (!(info->bitmask & XT_PHYSDEV_OP_IN)) goto match_outdev; - indev = nf_bridge->physindev ? nf_bridge->physindev->name : nulldevname; - ret = ifname_compare_aligned(indev, info->physindev, info->in_mask); + ret = ifname_compare_all(nf_bridge->physindev, info->physindev, info->in_mask); if (!ret ^ !(info->invert & XT_PHYSDEV_OP_IN)) return false; @@ -77,9 +74,7 @@ physdev_mt(const struct sk_buff *skb, struct xt_action_param *par) match_outdev: if (!(info->bitmask & XT_PHYSDEV_OP_OUT)) return true; - outdev = nf_bridge->physoutdev ? - nf_bridge->physoutdev->name : nulldevname; - ret = ifname_compare_aligned(outdev, info->physoutdev, info->out_mask); + ret = ifname_compare_all(nf_bridge->physoutdev, info->physoutdev, info->out_mask); return (!!ret ^ !(info->invert & XT_PHYSDEV_OP_OUT)); }
Signed-off-by: Richard Weinberger <richard@nod.at> --- include/linux/netfilter/x_tables.h | 22 ++++++++++++++++++++++ net/ipv4/netfilter/arp_tables.c | 28 +++++++++++++++++----------- net/ipv4/netfilter/ip_tables.c | 15 +++++---------- net/ipv6/netfilter/ip6_tables.c | 18 +++++++----------- net/netfilter/xt_physdev.c | 9 ++------- 5 files changed, 53 insertions(+), 39 deletions(-)