xt_recent fails with kernel 3.19.0

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>

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