[ipsec,5/7] xfrm: policy: fix reinsertion on node merge

"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 <fw@strlen.de>
---
 net/xfrm/xfrm_policy.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

On Fri, Jan 4, 2019 at 5:19 AM Florian Westphal <fw@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 <fw@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
>

Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > -       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()?

Good question.  Its not necessary from a technical point of view
because tmp isn't free'd and hlist_del_rcu leaves tmp->next alone.

But perhaps its still better to use _safe variant.

I'll let Steffen decide.