Re: [PATCH v2 nf] netfilter: x_tables: speed up jump target validation

[Florian Westphal]

Florian Westphal  wrote:
> The dummy ruleset I used to test the original validation change was broken,
> most rules were unreachable and were not tested by mark_source_chains().

...

I will send a v3 to also include arptables.
I thought arptables was irrelevant since arptable rulesets are usually
very small but I forgot about DoS angle (we use single mutext for all
net nsamespaces).
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 nf] netfilter: x_tables: speed up jump target validation

[Florian Westphal]

The dummy ruleset I used to test the original validation change was broken,
most rules were unreachable and were not tested by mark_source_chains().

In some cases rulesets that used to load in a few seconds now require
several minutes.

sample ruleset that shows the behaviour:

echo "*filter"
for i in $(seq 0 10);do
printf ":chain_%06x - [0:0]\n" $i
done
for i in $(seq 0 10);do
   printf -- "-A INPUT -j chain_%06x\n" $i
   printf -- "-A INPUT -j chain_%06x\n" $i
   printf -- "-A INPUT -j chain_%06x\n" $i
done
echo COMMIT

[ pipe result into iptables-restore ]

 78401416, count 59
This ruleset will be about 74mbyte in size, with ~500k searches
though all 500k[1] rule entries. iptables-restore will take forever
(gave up after 10 minutes)

Instead of always searching the entire blob for a match, fill an
array with the start offsets of every single ipt_entry struct,
then do a binary search to check if the jump target is present or not.

After this change ruleset restore times get again close to what one
gets when reverting 36472341017529e (~3 seconds on my workstation).

[1] every user-defined rule gets an implicit RETURN, so we get
300k jumps + 100k userchains + 100k returns -> 500k rule entries

Fixes: 36472341017529e ("netfilter: x_tables: validate targets of jumps")
Reported-by: Jeff Wu 
Tested-by: Jeff Wu 
Signed-off-by: Florian Westphal 
---
 Changes since v2:
  - get rid of unneeded 'offsets &&' clause in if-test
  - add Tested-by Tag from Jeff Wu
  - fixup some parts of commit message wrt. size of example ruleset

 include/linux/netfilter/x_tables.h |  4 +++
 net/ipv4/netfilter/ip_tables.c | 45 ++
 net/ipv6/netfilter/ip6_tables.c| 45 ++
 net/netfilter/x_tables.c   | 50 ++
 4 files changed, 102 insertions(+), 42 deletions(-)

diff --git a/include/linux/netfilter/x_tables.h 
b/include/linux/netfilter/x_tables.h
index dc4f58a..5f968a3 100644
--- a/include/linux/netfilter/x_tables.h
+++ b/include/linux/netfilter/x_tables.h
@@ -246,6 +246,10 @@ int xt_check_entry_offsets(const void *base, const char 
*elems,
   unsigned int target_offset,
   unsigned int next_offset);
 
+unsigned int *xt_alloc_entry_offsets(unsigned int size);
+bool xt_find_jump_offset(const unsigned int *offsets,
+unsigned int target, unsigned int size);
+
 int xt_check_match(struct xt_mtchk_param *, unsigned int size, u_int8_t proto,
   bool inv_proto);
 int xt_check_target(struct xt_tgchk_param *, unsigned int size, u_int8_t proto,
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 54906e0..1c909f3 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -375,23 +375,12 @@ ipt_do_table(struct sk_buff *skb,
else return verdict;
 }
 
-static bool find_jump_target(const struct xt_table_info *t,
-const struct ipt_entry *target)
-{
-   struct ipt_entry *iter;
-
-   xt_entry_foreach(iter, t->entries, t->size) {
-if (iter == target)
-   return true;
-   }
-   return false;
-}
-
 /* Figures out from what hook each rule can be called: returns 0 if
there are loops.  Puts hook bitmask in comefrom. */
 static int
 mark_source_chains(const struct xt_table_info *newinfo,
-  unsigned int valid_hooks, void *entry0)
+  unsigned int valid_hooks, void *entry0,
+  unsigned int *offsets)
 {
unsigned int hook;
 
@@ -460,10 +449,11 @@ mark_source_chains(const struct xt_table_info *newinfo,
   XT_STANDARD_TARGET) == 0 &&
newpos >= 0) {
/* This a jump; chase it. */
+   if (!xt_find_jump_offset(offsets, 
newpos,
+
newinfo->number))
+   return 0;
e = (struct ipt_entry *)
(entry0 + newpos);
-   if (!find_jump_target(newinfo, e))
-   return 0;
} else {
/* ... this is a fallthru */
newpos = pos + e->next_offset;
@@ -696,6 +686,7 @@ translate_table(struct net *net, struct xt_table_info 
*newinfo, void *entry0,
const struct ipt_replace *repl)
 {
struct ipt_entry *iter;
+   unsigned int *offsets;
unsigned int i;
int ret = 0;
 
@@ -708,6 +699,9 @@ translate_table(struct net *net, struct xt_table_info 
*newinfo, void