From mboxrd@z Thu Jan 1 00:00:00 1970 From: Oliver Subject: Re: [PATCH v3] netfilter: ipset: Fix serious failure in CIDR tracking Date: Wed, 11 Sep 2013 17:32:18 +0200 Message-ID: <1613281.TIUUNvYVSf@gentoovm> References: <1378894613-37279-1-git-send-email-oliver@8.c.9.b.0.7.4.0.1.0.0.2.ip6.arpa> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: netfilter-devel@vger.kernel.org To: Jozsef Kadlecsik Return-path: Received: from mail.uptheinter.net ([77.74.196.236]:34887 "EHLO mail.uptheinter.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755268Ab3IKPdT (ORCPT ); Wed, 11 Sep 2013 11:33:19 -0400 In-Reply-To: Sender: netfilter-devel-owner@vger.kernel.org List-ID: On Wednesday 11 September 2013 16:01:18 Jozsef Kadlecsik wrote: > Hi Oliver, > > On Wed, 11 Sep 2013, Oliver wrote: > > From: Oliver Smith > > > > This fixes a serious bug affecting all hash types with a net element - > > specifically, if a CIDR value is deleted such that none of the same size > > exist any more, all larger (less-specific) values will then fail to > > match. Adding back any prefix with a CIDR equal to or more specific than > > the one deleted will fix it. > > > > Steps to reproduce: > > ipset -N test hash:net > > ipset -A test 1.1.0.0/16 > > ipset -A test 2.2.2.0/24 > > ipset -T test 1.1.1.1 #1.1.1.1 IS in set > > ipset -D test 2.2.2.0/24 > > ipset -T test 1.1.1.1 #1.1.1.1 IS NOT in set > > > > This is due to the fact that the nets counter was unconditionally > > decremented prior to the iteration that shifts up the entries. Now, we > > first check if there is a proceeding entry and if not, decrement it and > > return. Otherwise, we proceed to iterate and then clean up the last > > element afterwards. > > > > Signed-off-by: Oliver Smith > > --- > > > > kernel/net/netfilter/ipset/ip_set_hash_gen.h | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/kernel/net/netfilter/ipset/ip_set_hash_gen.h > > b/kernel/net/netfilter/ipset/ip_set_hash_gen.h index 7a5b776..6599ea8 > > 100644 > > --- a/kernel/net/netfilter/ipset/ip_set_hash_gen.h > > +++ b/kernel/net/netfilter/ipset/ip_set_hash_gen.h > > @@ -307,19 +307,20 @@ mtype_add_cidr(struct htype *h, u8 cidr, u8 > > nets_length, u8 n)> > > static void > > mtype_del_cidr(struct htype *h, u8 cidr, u8 nets_length, u8 n) > > { > > > > - u8 i, j; > > + u8 i, j, net_end = nets_length - 1; > > > > - for (i = 0; i < nets_length - 1 && h->nets[i].cidr[n] != cidr; i++) > > + for (i = 0; i < net_end && h->nets[i].cidr[n] != cidr; i++) > > > > ; > > > > - h->nets[i].nets[n]--; > > - > > - if (h->nets[i].nets[n] != 0) > > + if (h->nets[i].nets[n] > 1 || i == net_end || !h->nets[i + 1].nets[n]) { > > + h->nets[i].nets[n]--; > > > > return; > > > > + } > > If you restored the original code in the part above, then the whole thing > could be collapsed into starting the next loop from j = i + 1 (and > changing the indices in the for body accordingly). I'm not sure I follow; the whole problem is that the proceeding loop fails because the element at j gets set to zero, causing it to be skipped when we do our shift-up below - why would we want to bother with looping when we can just decrement and return immediately? > > > - for (j = i; j < nets_length - 1 && h->nets[j].nets[n]; j++) { > > + for (j = i; j < net_end && h->nets[j].nets[n]; j++) { > > > > h->nets[j].cidr[n] = h->nets[j + 1].cidr[n]; > > h->nets[j].nets[n] = h->nets[j + 1].nets[n]; > > > > } > > > > + h->nets[j].nets[n] = 0; > > Here the checking of the value "j" is missing. I suppose we can check this, but what's the point really? j is always going to be nets_length - 1, so setting that element to zero is either necessary for cleanup, or won't do anything of use (nor harm) - why add in a memory read to see if we need to do a memory write when there's no harm in just doing it regardless? Kind Regards, Oliver.