netdev - Re: [PATCH ipsec 5/7] xfrm: policy: fix reinsertion on node merge

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 4 Jan 2019 20:48:16 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Florian Westphal <fw@...len.de>
Cc:     Steffen Klassert <steffen.klassert@...unet.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH ipsec 5/7] xfrm: policy: fix reinsertion on node merge

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