Skip to content

Commit

Permalink
xfrm: policy: fix infinite loop when merging src-nodes
Browse files Browse the repository at this point in the history
With very small change to test script we can trigger softlockup due to
bogus assignment of 'p' (policy to be examined) on restart.

Previously the two to-be-merged nodes had same address/prefixlength pair,
so no erase/reinsert was necessary, we only had to append the list from
node a to b.

If prefix lengths are different, the node has to be deleted and re-inserted
into the tree, with the updated prefix length.  This was broken; due to
bogus update to 'p' this loops forever.

Add a 'restart' label and use that instead.

While at it, don't perform the unneeded reinserts of the policies that
are already sorted into the 'new' node.

A previous patch in this series made xfrm_policy_inexact_list_reinsert()
use the relative position indicator to sort policies according to age in
case priorities are identical.

Fixes: 6ac098b ("xfrm: policy: add 2nd-level saddr trees for inexact policies")
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
  • Loading branch information
Florian Westphal authored and Steffen Klassert committed Jan 9, 2019
1 parent fcf86f5 commit 12750ab
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 10 deletions.
15 changes: 7 additions & 8 deletions net/xfrm/xfrm_policy.c
Original file line number Diff line number Diff line change
Expand Up @@ -886,12 +886,13 @@ static void xfrm_policy_inexact_node_reinsert(struct net *net,
struct rb_root *new,
u16 family)
{
struct rb_node **p, *parent = NULL;
struct xfrm_pol_inexact_node *node;
struct rb_node **p, *parent;

/* we should not have another subtree here */
WARN_ON_ONCE(!RB_EMPTY_ROOT(&n->root));

restart:
parent = NULL;
p = &new->rb_node;
while (*p) {
u8 prefixlen;
Expand All @@ -911,12 +912,11 @@ static void xfrm_policy_inexact_node_reinsert(struct net *net,
} else {
struct xfrm_policy *tmp;

hlist_for_each_entry(tmp, &node->hhead, bydst)
tmp->bydst_reinsert = true;
hlist_for_each_entry(tmp, &n->hhead, bydst)
hlist_for_each_entry(tmp, &n->hhead, bydst) {
tmp->bydst_reinsert = true;
hlist_del_rcu(&tmp->bydst);
}

INIT_HLIST_HEAD(&node->hhead);
xfrm_policy_inexact_list_reinsert(net, node, family);

if (node->prefixlen == n->prefixlen) {
Expand All @@ -928,8 +928,7 @@ static void xfrm_policy_inexact_node_reinsert(struct net *net,
kfree_rcu(n, rcu);
n = node;
n->prefixlen = prefixlen;
*p = new->rb_node;
parent = NULL;
goto restart;
}
}

Expand Down
4 changes: 2 additions & 2 deletions tools/testing/selftests/net/xfrm_policy.sh
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ do_overlap()
# adds a new node in the 10.0.0.0/24 tree (dst node exists).
ip -net $ns xfrm policy add src 10.2.0.0/24 dst 10.0.0.0/24 dir fwd priority 200 action block

# adds a 10.2.0.0/24 node, but for different dst.
ip -net $ns xfrm policy add src 10.2.0.0/24 dst 10.0.1.0/24 dir fwd priority 200 action block
# adds a 10.2.0.0/23 node, but for different dst.
ip -net $ns xfrm policy add src 10.2.0.0/23 dst 10.0.1.0/24 dir fwd priority 200 action block

# dst now overlaps with the 10.0.1.0/24 ESP policy in fwd.
# kernel must 'promote' existing one (10.0.0.0/24) to 10.0.0.0/23.
Expand Down

0 comments on commit 12750ab

Please sign in to comment.