| Message ID | 20150212115202.GD22887@breakpoint.cc |
|---|---|
| State | RFC |
| Delegated to: | Pablo Neira |
| Headers | show |
On Thu, 12 Feb 2015 12:52:02 +0100 Florian Westphal <fw@strlen.de> wrote: > Florian Westphal <fw@strlen.de> wrote: > > I'll see if we can fix this in a better way. > > What about this, it will transparently grow the table as needed, > we simply have to take the lock and make sure we zap all existing > entries (needed since those entries don't have enough room for > the larger nstamp_mask entry count)? > > diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c > --- a/net/netfilter/xt_recent.c > +++ b/net/netfilter/xt_recent.c > @@ -378,12 +378,11 @@ static int recent_mt_check(const struct > xt_mtchk_param *par, mutex_lock(&recent_mutex); > t = recent_table_lookup(recent_net, info->name); > if (t != NULL) { > - if (info->hit_count > t->nstamps_max_mask) { > - pr_info("hitcount (%u) is larger than packets to be remembered (%u) for table %s\n", > - info->hit_count, t->nstamps_max_mask + 1, > - info->name); > - ret = -EINVAL; > - goto out; > + if (nstamp_mask > t->nstamps_max_mask) { > + spin_lock_bh(&recent_lock); > + recent_table_flush(t); > + t->nstamps_max_mask = nstamp_mask; > + spin_unlock_bh(&recent_lock); > } > > t->refcnt++; I don't know your code but forgive me for asking one thing. The previous versions of this code (both in the 3.18 and 3.19 kernels) checked the value of hit_count for sanity. This patch seems to be doing something different, and I note that nstamps_max_mask is unconditionally set later in recent_mt_check() anyway. Can the check for the value of hit_count simply be omitted? In what circumstances can it be anything other than true? Chris -- 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
Chris Vine <chris@cvine.freeserve.co.uk> wrote: > On Thu, 12 Feb 2015 12:52:02 +0100 > Florian Westphal <fw@strlen.de> wrote: > > Florian Westphal <fw@strlen.de> wrote: > > > I'll see if we can fix this in a better way. > > > > What about this, it will transparently grow the table as needed, > > we simply have to take the lock and make sure we zap all existing > > entries (needed since those entries don't have enough room for > > the larger nstamp_mask entry count)? > > > > diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c > > --- a/net/netfilter/xt_recent.c > > +++ b/net/netfilter/xt_recent.c > > @@ -378,12 +378,11 @@ static int recent_mt_check(const struct > > xt_mtchk_param *par, mutex_lock(&recent_mutex); > > t = recent_table_lookup(recent_net, info->name); > > if (t != NULL) { > > - if (info->hit_count > t->nstamps_max_mask) { > > - pr_info("hitcount (%u) is larger than packets to be remembered (%u) for table %s\n", > > - info->hit_count, t->nstamps_max_mask + 1, > > - info->name); > > - ret = -EINVAL; > > - goto out; > > + if (nstamp_mask > t->nstamps_max_mask) { > > + spin_lock_bh(&recent_lock); > > + recent_table_flush(t); > > + t->nstamps_max_mask = nstamp_mask; > > + spin_unlock_bh(&recent_lock); > > } > > > > t->refcnt++; > > I don't know your code but forgive me for asking one thing. The > previous versions of this code (both in the 3.18 and 3.19 kernels) > checked the value of hit_count for sanity. nstamp_mask is computed based on hitcount. > This patch seems to be doing > something different, and I note that nstamps_max_mask is > unconditionally set later in recent_mt_check() anyway. No, its only set if recent_table_lookup returns NULL. We return soon after we bump the refcnt when we take this branch. > Can the check for the value of hit_count simply be omitted? In what > circumstances can it be anything other than true? You mean when nstamp_mask > t->nstamps_max_mask is false? e.g. iptables -A foo -m recent --hitcount 5 iptables -A foo -m recent --hitcount 4 (2nd rule finds existing table with mask 7). -- 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 Thu, 12 Feb 2015 18:09:31 +0100 Florian Westphal <fw@strlen.de> wrote: [snip] > > This patch seems to be doing > > something different, and I note that nstamps_max_mask is > > unconditionally set later in recent_mt_check() anyway. > > No, its only set if recent_table_lookup returns NULL. > We return soon after we bump the refcnt when we take this branch. You probably are working on a more up-to-date branch. Your patch assigning to nstamps_max_mask is only executed if recent_table_lookup() does not return NULL. In the 3.19.0 kernel, the assignment to nstamps_max_mask in line 404 also only occurs if recent_table_lookup() does not return NULL. > > Can the check for the value of hit_count simply be omitted? In what > > circumstances can it be anything other than true? > > You mean when nstamp_mask > t->nstamps_max_mask is false? > > e.g. > iptables -A foo -m recent --hitcount 5 > iptables -A foo -m recent --hitcount 4 > > (2nd rule finds existing table with mask 7). There's the rub I suspect, but as I say, I don't know your code. Let's leave it at that: if I apply the off-by-one patch it works for me (provided I don't change settings, which I don't in ordinary usage). I will wait for whatever you and/or others come up with in due course to solve it. Chris -- 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
Chris Vine <chris@cvine.freeserve.co.uk> wrote: > On Thu, 12 Feb 2015 18:09:31 +0100 > Florian Westphal <fw@strlen.de> wrote: > [snip] > > > This patch seems to be doing > > > something different, and I note that nstamps_max_mask is > > > unconditionally set later in recent_mt_check() anyway. > > > > No, its only set if recent_table_lookup returns NULL. > > We return soon after we bump the refcnt when we take this branch. > > You probably are working on a more up-to-date branch. Your patch > assigning to nstamps_max_mask is only executed if recent_table_lookup() > does not return NULL. In the 3.19.0 kernel, the assignment to > nstamps_max_mask in line 404 also only occurs if recent_table_lookup() > does not return NULL. Thats what I meant -- line 404 is ONLY executed if the table doesn't exist, so we need to assign it in case we have a table and we want to increase the upper limit of the _existing_ table. Unless someone spots an issue with this approach i'll submit this formally tomorrow. -- 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 Thu, 12 Feb 2015 22:40:30 +0100 Florian Westphal <fw@strlen.de> wrote: [snip] > Thats what I meant -- line 404 is ONLY executed if the table doesn't > exist, so we need to assign it in case we have a table and we want > to increase the upper limit of the _existing_ table. Ah yes, I missed line 391 and the subsequent assignments to t. Easily done - one rarely comes across goto outside the kernel. (Not that there is anything wrong with goto when used as C's version of finally, once you get used to it.) Chris -- 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_recent.c b/net/netfilter/xt_recent.c --- a/net/netfilter/xt_recent.c +++ b/net/netfilter/xt_recent.c @@ -378,12 +378,11 @@ static int recent_mt_check(const struct xt_mtchk_param *par, mutex_lock(&recent_mutex); t = recent_table_lookup(recent_net, info->name); if (t != NULL) { - if (info->hit_count > t->nstamps_max_mask) { - pr_info("hitcount (%u) is larger than packets to be remembered (%u) for table %s\n", - info->hit_count, t->nstamps_max_mask + 1, - info->name); - ret = -EINVAL; - goto out; + if (nstamp_mask > t->nstamps_max_mask) { + spin_lock_bh(&recent_lock); + recent_table_flush(t); + t->nstamps_max_mask = nstamp_mask; + spin_unlock_bh(&recent_lock); } t->refcnt++;
Florian Westphal <fw@strlen.de> wrote: > I'll see if we can fix this in a better way. What about this, it will transparently grow the table as needed, we simply have to take the lock and make sure we zap all existing entries (needed since those entries don't have enough room for the larger nstamp_mask entry count)? -- 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