xt_recent fails with kernel 3.19.0

xt_recent fails with kernel 3.19.0

[Chris Vine]

Loading 'recent' xtables match support for iptables fails where the
following sample rule is appended with SSH_TRIES set to 4:

  iptables -A SSH_CHAIN -m conntrack --ctstate NEW \
    -m recent --update --seconds $SSH_LOGIN_PERIOD --hitcount $SSH_TRIES -j DROP

It fails with this message:

  kernel: xt_recent: hitcount (4) is larger than packets to be
  remembered (4) for table DEFAULT

This appears to be due to an off-by-one error in testing the hit count in
recent_mt_check().  This occurs because nstamp_mask is set to one less than
the value of ip_pkt_list_tot (if any) or of hit_count rounded up to a power
of two value.  When that hit count boundary is actually reached nstamp_mask
is therefore exceeded by one.

I can't say I fully understand the heuristics of nstamp_mask, but the
patch below deals with this and works for me(TM).

Signed-of-by: Chris Vine <vine.chris@gmail.com>

--- linux-3.19.0/net/netfilter/xt_recent.c~	2015-02-10 09:18:44.657376355 +0000
+++ linux-3.19.0/net/netfilter/xt_recent.c	2015-02-11 17:58:33.311608835 +0000
@@ -378,7 +378,7 @@
 	mutex_lock(&recent_mutex);
 	t = recent_table_lookup(recent_net, info->name);
 	if (t != NULL) {
-		if (info->hit_count > t->nstamps_max_mask) {
+		if (info->hit_count > t->nstamps_max_mask + 1) {
 			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);

Re: xt_recent fails with kernel 3.19.0

[Chris Vine]

On Thu, 12 Feb 2015 10:25:53 +0000
Chris Vine <chris@cvine.freeserve.co.uk> wrote:
> Loading 'recent' xtables match support for iptables fails where the
> following sample rule is appended with SSH_TRIES set to 4:
> 
>   iptables -A SSH_CHAIN -m conntrack --ctstate NEW \
>     -m recent --update --seconds $SSH_LOGIN_PERIOD --hitcount
> $SSH_TRIES -j DROP
> 
> It fails with this message:
> 
>   kernel: xt_recent: hitcount (4) is larger than packets to be
>   remembered (4) for table DEFAULT
> 
> This appears to be due to an off-by-one error in testing the hit
> count in recent_mt_check().  This occurs because nstamp_mask is set
> to one less than the value of ip_pkt_list_tot (if any) or of
> hit_count rounded up to a power of two value.  When that hit count
> boundary is actually reached nstamp_mask is therefore exceeded by one.
> 
> I can't say I fully understand the heuristics of nstamp_mask, but the
> patch below deals with this and works for me(TM).
> 
> Signed-of-by: Chris Vine <vine.chris@gmail.com>
> 
> --- linux-3.19.0/net/netfilter/xt_recent.c~	2015-02-10
> 09:18:44.657376355 +0000 +++
> linux-3.19.0/net/netfilter/xt_recent.c	2015-02-11
> 17:58:33.311608835 +0000 @@ -378,7 +378,7 @@
> mutex_lock(&recent_mutex); t = recent_table_lookup(recent_net,
> info->name); if (t != NULL) {
> -		if (info->hit_count > t->nstamps_max_mask) {
> +		if (info->hit_count > t->nstamps_max_mask + 1) {
>  			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);

Scrub that.  This now fails when SSH_TRIES is set to other than a power
of two boundary.  There seems to be something fundamentally wrong with
the heuristic employed here.

Chris
the

Re: xt_recent fails with kernel 3.19.0

[Chris Vine]

On Thu, 12 Feb 2015 10:51:45 +0000
Chris Vine <chris@cvine.freeserve.co.uk> wrote:

> On Thu, 12 Feb 2015 10:25:53 +0000
> Chris Vine <chris@cvine.freeserve.co.uk> wrote:
> > Loading 'recent' xtables match support for iptables fails where the
> > following sample rule is appended with SSH_TRIES set to 4:
> > 
> >   iptables -A SSH_CHAIN -m conntrack --ctstate NEW \
> >     -m recent --update --seconds $SSH_LOGIN_PERIOD --hitcount
> > $SSH_TRIES -j DROP
> > 
> > It fails with this message:
> > 
> >   kernel: xt_recent: hitcount (4) is larger than packets to be
> >   remembered (4) for table DEFAULT
> > 
> > This appears to be due to an off-by-one error in testing the hit
> > count in recent_mt_check().  This occurs because nstamp_mask is set
> > to one less than the value of ip_pkt_list_tot (if any) or of
> > hit_count rounded up to a power of two value.  When that hit count
> > boundary is actually reached nstamp_mask is therefore exceeded by
> > one.
> > 
> > I can't say I fully understand the heuristics of nstamp_mask, but
> > the patch below deals with this and works for me(TM).
> > 
> > Signed-of-by: Chris Vine <vine.chris@gmail.com>
> > 
> > --- linux-3.19.0/net/netfilter/xt_recent.c~	2015-02-10
> > 09:18:44.657376355 +0000 +++
> > linux-3.19.0/net/netfilter/xt_recent.c	2015-02-11
> > 17:58:33.311608835 +0000 @@ -378,7 +378,7 @@
> > mutex_lock(&recent_mutex); t = recent_table_lookup(recent_net,
> > info->name); if (t != NULL) {
> > -		if (info->hit_count > t->nstamps_max_mask) {
> > +		if (info->hit_count > t->nstamps_max_mask + 1) {
> >  			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);
> 
> Scrub that.  This now fails when SSH_TRIES is set to other than a
> power of two boundary.  There seems to be something fundamentally
> wrong with the heuristic employed here.

On more testing I am wrong about that.  You seem to need to rmmod
xt_recent to get it to flush the previous setting.  With that done, the
patch does indeed seem to work with any values of SSH_TRIES.

Chris

Re: xt_recent fails with kernel 3.19.0

[Florian Westphal]

Chris Vine <chris@cvine.freeserve.co.uk> wrote:
> > > info->name); if (t != NULL) {
> > > -		if (info->hit_count > t->nstamps_max_mask) {
> > > +		if (info->hit_count > t->nstamps_max_mask + 1) {
> > >  			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);
> > 
> > Scrub that.  This now fails when SSH_TRIES is set to other than a
> > power of two boundary.  There seems to be something fundamentally
> > wrong with the heuristic employed here.
> 
> On more testing I am wrong about that.  You seem to need to rmmod
> xt_recent to get it to flush the previous setting.  With that done, the
> patch does indeed seem to work with any values of SSH_TRIES.

Grrr.  Right.  This is because if you have single

-m recent  --name DEFAULT ..

iptables-save > foo
then edit foo to bump the hitcount, then run
iptables-restore < foo

we'll find the existing DEFAULT entry with the old hitcount.

It works for something like 11 -> 13 since we're internally
tracking a count of 16 (mask 15).

I don't see a simple fix except your patch above plus

-static unsigned int ip_pkt_list_tot __read_mostly;
+static unsigned int ip_pkt_list_tot __read_mostly = 32;

To work around this.
This causes us to ignore hitcount in the check completely, at additional
memory cost.

I'll see if we can fix this in a better way.

Re: xt_recent fails with kernel 3.19.0

[Florian Westphal]

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++;

Re: xt_recent fails with kernel 3.19.0

[Chris Vine]

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

Re: xt_recent fails with kernel 3.19.0

[Florian Westphal]

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).

Re: xt_recent fails with kernel 3.19.0

[Chris Vine]

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

Re: xt_recent fails with kernel 3.19.0

[Florian Westphal]

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.

Re: xt_recent fails with kernel 3.19.0

[Chris Vine]

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