Re: [PATCH iptables 3/3] libxt_hashlimit: iptables-restore does not work as expected with xt_hashlimit

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi,

On Fri, Jun 24, 2016 at 02:24:18PM -0400, Vishwanath Pai wrote:
> On 06/23/2016 06:25 AM, Pablo Neira Ayuso wrote:
> > On Wed, Jun 01, 2016 at 08:17:59PM -0400, Vishwanath Pai wrote:
> >> libxt_hashlimit: iptables-restore does not work as expected with xt_hashlimit
> >>
> >> Add the following iptables rule.
> >>
> >> $ iptables -A INPUT -m hashlimit --hashlimit-above 200/sec \
> >>   --hashlimit-burst 5 --hashlimit-mode srcip --hashlimit-name hashlimit1 \
> >>   --hashlimit-htable-expire 30000 -j DROP
> >>
> >> $ iptables-save > save.txt
> >>
> >> Edit save.txt and change the value of --hashlimit-above to 300:
> >>
> >> -A INPUT -m hashlimit --hashlimit-above 300/sec --hashlimit-burst 5 \
> >> --hashlimit-mode srcip --hashlimit-name hashlimit2 \
> >> --hashlimit-htable-expire 30000 -j DROP
> >>
> >> Now restore save.txt
> >>
> >> $ iptables-restore < save.txt
> > 
> > In this case, we don't end up with two rules, we actually get one
> > single hashlimit rule, given the sequence you provide.
> > 
> >         $ iptables-save > save.txt
> >         ... edit save.txt
> >         $ iptables-restore < save.txt
> > 
> 
> Yes, we end up with just one rule, but the kernel data structure is not
> updated. Userspace thinks the value is 300/s but in the kernel it is
> still 200/s.

Right, but the main point of this is to honor the new rule
configuration, ie. to update the internal hashlimit configuration of
the previous rules.

> >> Now userspace thinks that the value of --hashlimit-above is 300 but it is
> >> actually 200 in the kernel. This happens because when we add multiple
> >> hash-limit rules with the same name they will share the same hashtable
> >> internally. The kernel module tries to re-use the old hashtable without
> >> updating the values.
> >>
> >> There are multiple problems here:
> >> 1) We can add two iptables rules with the same name, but kernel does not
> >>    handle this well, one procfs file cannot work with two rules
> >> 2) If the second rule has no effect because the hashtable has values from
> >>    rule 1
> >> 3) hashtable-restore does not work (as described above)
> >>
> >> To fix this I have made the following design change:
> >> 1) If a second rule is added with the same name as an existing rule,
> >>    append a number when we create the procfs, for example hashlimit_1,
> >>    hashlimit_2 etc
> >> 2) Two rules will not share the same hashtable unless they are similar in
> >>    every possible way
> >> 3) This behavior has to be forced with a new userspace flag:
> >>    --hashlimit-ehanced-procfs, if this flag is not passed we default to
> >>    the old behavior. This is to make sure we do not break existing scripts
> >>    that rely on the existing behavior.
> > 
> > We discussed this in netdev0.1, and I think we agreed on adding a new
> > option, something like --hashlimit-update that would force an update
> > to the existing hashlimit internal state (that is identified by the
> > hashlimit name).
> > 
> > I think the problem here is that you may want to update the internal
> > state of an existing hashlimit object, and currently this is not
> > actually happening.
> > 
> > With the explicit --hashlimit-update flag, from the kernel we really
> > know that the user wants an update.
> > 
> > Let me know, thanks.
> 
> Yes, I believe you had a discussion about this with Josh Hunt. This
> patch does add a new option, but it is called -enhanced-procfs instead.
> I am open to renaming this to something else. I chose this name because
> this patch will affect the names of the procfs files when multiple rules
> with the same name exist. This generally does not happen, but is a side
> effect of the way we create these files. In the case of restore example
> above - we get the call to "hashlimit_mt_check" for the new rule before
> the old rule is deleted, so there is a short window where we have two
> rules in the kernel with the same name.

I see, but I'm not convinced about this /proc rename feature.

I think the main point of this, as well as other entries in bugzilla
related to this, is ability to update an existing hashlimit state.

So, I'm not proposing to rename --enhanced-procfs to something else,
I think that a different approach consisting on adding a new option
like --hashlimit-update that will update the internal state of an
existing hashlimit object is just fine for your usecase, right?

> Other than that, we are doing exactly what you said, but creating a new
> entry in the hashtable instead of updating it. The previous entry will
> automatically be removed when the old rule is flushed/deleted.

What I'm missing is why we need this /proc rename at all.