| Message ID | 1406004850-31336-1-git-send-email-johunt@akamai.com |
|---|---|
| State | RFC |
| Delegated to: | Pablo Neira |
| Headers | show |
On 07/21/2014 11:54 PM, Josh Hunt wrote: > Below is a first pass attempt at fixing a problem we've come across when > trying to do an iptables-restore where the hashlimit name stays the same, but > one of the hashlimit parameters changes but does not take affect. > > For ex, if you have an existing hashlimit rule, do an iptables-save, change the > rate for that rule, and then do an iptables-restore the new rate will not be > enforced. > > This appears to be due to a problem where hashlimit only checks for existing > hashes by name and family and does not consider any of the other config > parameters. > > I've attempted to fix this by having it check for all hashlimit config params, > this way it doesn't accidentally match just on name. This brought up an issue > of having to make hashlimit aware of how many references there are to its > proc entry. > > I'm not submitting this for inclusion yet, but for feedback. Mainly on the approach > and if there's possibly a better way of resolving this problem. My handling of > the proc "problem" is pretty messy right now and possibly incomplete, but the > patch below allows the case I described above to pass now. I hope to clean up > the proc handling in a v2. I just realized that what I'm doing with the proc stuff isn't going to work, but feedback on the other portion is still appreciated. Thanks Josh -- 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
Josh Hunt <johunt@akamai.com> wrote: > Below is a first pass attempt at fixing a problem we've come across when > trying to do an iptables-restore where the hashlimit name stays the same, but > one of the hashlimit parameters changes but does not take affect. > > For ex, if you have an existing hashlimit rule, do an iptables-save, change the > rate for that rule, and then do an iptables-restore the new rate will not be > enforced. > > This appears to be due to a problem where hashlimit only checks for existing > hashes by name and family and does not consider any of the other config > parameters. > > I've attempted to fix this by having it check for all hashlimit config params, > this way it doesn't accidentally match just on name. This brought up an issue > of having to make hashlimit aware of how many references there are to its > proc entry. > > I'm not submitting this for inclusion yet, but for feedback. Mainly on the approach > and if there's possibly a better way of resolving this problem. My handling of > the proc "problem" is pretty messy right now and possibly incomplete, but the > patch below allows the case I described above to pass now. I hope to clean up > the proc handling in a v2. > > Any feedback is greatly appreciated. [ ignoring proc issue you pointed out ] > from: > -A INPUT -s 10.18.40.44/32 -i eth1 -p tcp -m hashlimit --hashlimit-upto 10/sec --hashlimit-burst 10 --hashlimit-name test -j ACCEPT > > to: > -A INPUT -s 10.18.40.44/32 -i eth1 -p tcp -m hashlimit --hashlimit-upto 1000/sec --hashlimit-burst 10 --hashlimit-name test -j ACCEPT > > Currently when we do do this the new parameters are not enforced. Note that: -A INPUT -m hashlimit --hashlimit-upto 10/sec --hashlimit-burst 10 --hashlimit-name test -A INPUT -m hashlimit --hashlimit-upto 1/sec --hashlimit-burst 10 --hashlimit-name test doesn't work as expected either (rule #2 uses config options of #1). I think is behaviour is so unexpected that I would consider this a bug... Wrt. to the patch, aside from proc issue I only see style/indent nits, f.e. > + hlist_for_each_entry(hinfo, &hashlimit_net->htables, node) { > + if (!strcmp(name, hinfo->name) && > + family == hinfo->family) { 'family' should align with the 'if (', not the body. Other than that, it looks good to me. -- 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 24. Juli 2014 09:49:27 GMT+01:00, Florian Westphal <fw@strlen.de> wrote: >Josh Hunt <johunt@akamai.com> wrote: >> Currently when we do do this the new parameters are not enforced. > >Note that: > >-A INPUT -m hashlimit --hashlimit-upto 10/sec --hashlimit-burst 10 >--hashlimit-name test >-A INPUT -m hashlimit --hashlimit-upto 1/sec --hashlimit-burst 10 >--hashlimit-name test > >doesn't work as expected either (rule #2 uses config options of #1). > >I think is behaviour is so unexpected that I would consider this a >bug... True, but it's a bug that has existed forever and I've seen scripts that actually rely on this. I'm not sure if we can silently change this behaviour. -- 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
Patrick McHardy <kaber@trash.net> wrote: > On 24. Juli 2014 09:49:27 GMT+01:00, Florian Westphal <fw@strlen.de> wrote: > >Josh Hunt <johunt@akamai.com> wrote: > >> Currently when we do do this the new parameters are not enforced. > > > >Note that: > > > >-A INPUT -m hashlimit --hashlimit-upto 10/sec --hashlimit-burst 10 > >--hashlimit-name test > >-A INPUT -m hashlimit --hashlimit-upto 1/sec --hashlimit-burst 10 > >--hashlimit-name test > > > >doesn't work as expected either (rule #2 uses config options of #1). > > > >I think is behaviour is so unexpected that I would consider this a > >bug... > > True, but it's a bug that has existed forever and I've seen scripts that actually rely on this. Too bad. In that case it seems like we really cannot fix this 8-( -- 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 07/24/2014 05:53 AM, Patrick McHardy wrote: > On 24. Juli 2014 09:49:27 GMT+01:00, Florian Westphal <fw@strlen.de> wrote: >> Josh Hunt <johunt@akamai.com> wrote: >>> Currently when we do do this the new parameters are not enforced. >> >> Note that: >> >> -A INPUT -m hashlimit --hashlimit-upto 10/sec --hashlimit-burst 10 >> --hashlimit-name test >> -A INPUT -m hashlimit --hashlimit-upto 1/sec --hashlimit-burst 10 >> --hashlimit-name test >> >> doesn't work as expected either (rule #2 uses config options of #1). >> >> I think is behaviour is so unexpected that I would consider this a >> bug... > > True, but it's a bug that has existed forever and I've seen scripts that actually rely on this. > > I'm not sure if we can silently change this behaviour. Can you elaborate on what behavior they're relying on? It'd be helpful to know in case my approach can't be used we might be able to come up with an alternative. In the case I describe with a restore it gives the user what I would expect to be undesired behavior since they would have explicitly changed the hash's config, but it's not having any affect. In Florian's case the user would have a number of rules where they've explicitly made different hash configurations, but all of the rules with the same hash name would behave the same as the first. Ignoring all of the configs the user input. Thanks Josh -- 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/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c index a3910fc..5518018 100644 --- a/net/netfilter/xt_hashlimit.c +++ b/net/netfilter/xt_hashlimit.c @@ -215,6 +215,39 @@ dsthash_free(struct xt_hashlimit_htable *ht, struct dsthash_ent *ent) } static void htable_gc(unsigned long htlong); +static int htable_count_name(struct net *net, + const char *name, + u_int8_t family) +{ + struct hashlimit_net *hashlimit_net = hashlimit_pernet(net); + struct xt_hashlimit_htable *hinfo; + int count = 0; + + hlist_for_each_entry(hinfo, &hashlimit_net->htables, node) { + if (!strcmp(name, hinfo->name) && + family == hinfo->family) { + count++; + } + } + return count; +} + +static struct proc_dir_entry *htable_get_pde(struct net *net, + const char *name, + u_int8_t family) +{ + struct hashlimit_net *hashlimit_net = hashlimit_pernet(net); + struct xt_hashlimit_htable *hinfo; + + hlist_for_each_entry(hinfo, &hashlimit_net->htables, node) { + if (!strcmp(name, hinfo->name) && + family == hinfo->family) { + return hinfo->pde; + } + } + return NULL; +} + static int htable_create(struct net *net, struct xt_hashlimit_mtinfo1 *minfo, u_int8_t family) { @@ -262,10 +295,13 @@ static int htable_create(struct net *net, struct xt_hashlimit_mtinfo1 *minfo, } spin_lock_init(&hinfo->lock); - hinfo->pde = proc_create_data(minfo->name, 0, - (family == NFPROTO_IPV4) ? - hashlimit_net->ipt_hashlimit : hashlimit_net->ip6t_hashlimit, - &dl_file_ops, hinfo); + hinfo->pde = htable_get_pde(net, minfo->name, family); + if (hinfo->pde == NULL) { + hinfo->pde = proc_create_data(minfo->name, 0, + (family == NFPROTO_IPV4) ? + hashlimit_net->ipt_hashlimit : hashlimit_net->ip6t_hashlimit, + &dl_file_ops, hinfo); + } if (hinfo->pde == NULL) { kfree(hinfo->name); vfree(hinfo); @@ -339,25 +375,50 @@ static void htable_remove_proc_entry(struct xt_hashlimit_htable *hinfo) remove_proc_entry(hinfo->name, parent); } -static void htable_destroy(struct xt_hashlimit_htable *hinfo) +static void htable_destroy(struct xt_hashlimit_htable *hinfo, bool remove_proc) { del_timer_sync(&hinfo->timer); - htable_remove_proc_entry(hinfo); + if (remove_proc) + htable_remove_proc_entry(hinfo); htable_selective_cleanup(hinfo, select_all); kfree(hinfo->name); vfree(hinfo); } +static bool htable_compare(struct xt_hashlimit_mtinfo1 *info, + u_int8_t family, + struct xt_hashlimit_htable *hinfo) +{ + struct hashlimit_cfg1 newcfg = info->cfg; + struct hashlimit_cfg1 oldcfg = hinfo->cfg; + + if (!strcmp(info->name, hinfo->name) && + family == hinfo->family && + newcfg.mode == oldcfg.mode && + newcfg.avg == oldcfg.avg && + newcfg.burst == oldcfg.burst && + newcfg.size == oldcfg.size && + newcfg.max == oldcfg.max && + newcfg.gc_interval == oldcfg.gc_interval && + newcfg.expire == oldcfg.expire && + newcfg.srcmask == oldcfg.srcmask && + newcfg.dstmask == oldcfg.dstmask ) { + + return true; + } + + return false; +} + static struct xt_hashlimit_htable *htable_find_get(struct net *net, - const char *name, + struct xt_hashlimit_mtinfo1 *info, u_int8_t family) { struct hashlimit_net *hashlimit_net = hashlimit_pernet(net); struct xt_hashlimit_htable *hinfo; hlist_for_each_entry(hinfo, &hashlimit_net->htables, node) { - if (!strcmp(name, hinfo->name) && - hinfo->family == family) { + if (htable_compare(info, family, hinfo)) { hinfo->use++; return hinfo; } @@ -367,10 +428,14 @@ static struct xt_hashlimit_htable *htable_find_get(struct net *net, static void htable_put(struct xt_hashlimit_htable *hinfo) { + bool remove_proc = false; + mutex_lock(&hashlimit_mutex); if (--hinfo->use == 0) { + if (htable_count_name(hinfo->net, hinfo->name, hinfo->family) == 1) + remove_proc = true; hlist_del(&hinfo->node); - htable_destroy(hinfo); + htable_destroy(hinfo, remove_proc); } mutex_unlock(&hashlimit_mutex); } @@ -698,7 +763,7 @@ static int hashlimit_mt_check(const struct xt_mtchk_param *par) } mutex_lock(&hashlimit_mutex); - info->hinfo = htable_find_get(net, info->name, par->family); + info->hinfo = htable_find_get(net, info, par->family); if (info->hinfo == NULL) { ret = htable_create(net, info, par->family); if (ret < 0) {
Below is a first pass attempt at fixing a problem we've come across when trying to do an iptables-restore where the hashlimit name stays the same, but one of the hashlimit parameters changes but does not take affect. For ex, if you have an existing hashlimit rule, do an iptables-save, change the rate for that rule, and then do an iptables-restore the new rate will not be enforced. This appears to be due to a problem where hashlimit only checks for existing hashes by name and family and does not consider any of the other config parameters. I've attempted to fix this by having it check for all hashlimit config params, this way it doesn't accidentally match just on name. This brought up an issue of having to make hashlimit aware of how many references there are to its proc entry. I'm not submitting this for inclusion yet, but for feedback. Mainly on the approach and if there's possibly a better way of resolving this problem. My handling of the proc "problem" is pretty messy right now and possibly incomplete, but the patch below allows the case I described above to pass now. I hope to clean up the proc handling in a v2. Any feedback is greatly appreciated. Thanks Josh --- netfilter: xt_hashlimit: handle iptables-restore of hash with same name We encountered a problem when trying to do an iptables-restore of a ruleset which included a hashlimit match where the name stays the same and one of the other config parameters changed, for ex: from: -A INPUT -s 10.18.40.44/32 -i eth1 -p tcp -m hashlimit --hashlimit-upto 10/sec --hashlimit-burst 10 --hashlimit-name test -j ACCEPT to: -A INPUT -s 10.18.40.44/32 -i eth1 -p tcp -m hashlimit --hashlimit-upto 1000/sec --hashlimit-burst 10 --hashlimit-name test -j ACCEPT Currently when we do do this the new parameters are not enforced. The problem stems from how htable_find_get() only checks by name and family. This causes htable_find_get() to return the old hash, instead of creating a new one. I've attempted to resolve this by having htable_find_get() check all config parameters b/f it returns a match. In addition, we have to keep the proc file around since it doesn't change b/c the hashes have the same name. Here's an example of the rules file: *filter :INPUT ACCEPT [1:1108] :FORWARD ACCEPT [0:0] :OUTPUT ACCEPT [0:0] -A INPUT -s 10.18.40.44/32 -i eth1 -p tcp -m hashlimit --hashlimit-upto 10/sec --hashlimit-burst 10 --hashlimit-name test -j ACCEPT COMMIT Signed-off-by: Josh Hunt <johunt@akamai.com> --- net/netfilter/xt_hashlimit.c | 87 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 76 insertions(+), 11 deletions(-)