On Fri, Jan 4, 2019 at 5:19 AM Florian Westphal <f...@strlen.de> wrote:
>
> "newpos" has wrong scope. It must be NULL on each iteration of the loop.
> Otherwise, when policy is to be inserted at the start, we would instead
> insert at point found by the previous loop-iteration instead.
>
> Also, we need to unlink the policy before we reinsert it to the new node,
> else we can get next-points-to-self loops.
>
> Because policies are only ordered by priority it is irrelevant which policy
> is "more recent" except when two policies have same priority.
> (the more recent one is placed after the older one).
>
> In these cases, we can use the ->pos id number to know which one is the
> 'older': the higher the id, the more recent the policy.
>
> So we only need to unlink all policies from the node that is about to be
> removed, and insert them to the replacement node.
>
> Fixes: 9cf545ebd591da ("xfrm: policy: store inexact policies in a tree
> ordered by destination address")
> Signed-off-by: Florian Westphal <f...@strlen.de>
> ---
> net/xfrm/xfrm_policy.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 24dfd1e47cf0..e691683223ee 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -823,13 +823,13 @@ static void xfrm_policy_inexact_list_reinsert(struct
> net *net,
> u16 family)
> {
> unsigned int matched_s, matched_d;
> - struct hlist_node *newpos = NULL;
> struct xfrm_policy *policy, *p;
>
> matched_s = 0;
> matched_d = 0;
>
> list_for_each_entry_reverse(policy, &net->xfrm.policy_all, walk.all) {
> + struct hlist_node *newpos = NULL;
> bool matches_s, matches_d;
>
> if (!policy->bydst_reinsert)
> @@ -839,7 +839,10 @@ static void xfrm_policy_inexact_list_reinsert(struct net
> *net,
>
> policy->bydst_reinsert = false;
> hlist_for_each_entry(p, &n->hhead, bydst) {
> - if (policy->priority >= p->priority)
> + if (policy->priority > p->priority)
> + newpos = &p->bydst;
> + else if (policy->priority == p->priority &&
> + policy->pos > p->pos)
> newpos = &p->bydst;
> else
> break;
> @@ -955,12 +958,11 @@ static void xfrm_policy_inexact_node_merge(struct net
> *net,
> family);
> }
>
> - hlist_for_each_entry(tmp, &v->hhead, bydst)
> - tmp->bydst_reinsert = true;
> - hlist_for_each_entry(tmp, &n->hhead, bydst)
> + hlist_for_each_entry(tmp, &v->hhead, bydst) {
hlist_for_each_entry_safe()?
> tmp->bydst_reinsert = true;
> + hlist_del_rcu(&tmp->bydst);
> + }
>
> - INIT_HLIST_HEAD(&n->hhead);
> xfrm_policy_inexact_list_reinsert(net, n, family);
> }
>
> --
> 2.19.2
>
