netdev - Re: [PATCHv3 net] xfrm: move xfrm_garbage_collect out of xfrm_policy

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]

Message-ID: <CADvbK_cxFyx0VWOzJyoxUc+FfnSJy0H+Y2enUaGmvHb4ZKDr7Q@mail.gmail.com>
Date:   Sun, 11 Jun 2017 18:51:29 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Steffen Klassert <steffen.klassert@...unet.com>
Subject: Re: [PATCHv3 net] xfrm: move xfrm_garbage_collect out of xfrm_policy_flush

On Sun, Jun 11, 2017 at 9:44 AM, Hangbin Liu <liuhangbin@...il.com> wrote:
> Now we will force to do garbage collection if any policy removed in
> xfrm_policy_flush(). But during xfrm_net_exit(). We call flow_cache_fini()
> first and set set fc->percpu to NULL. Then after we call xfrm_policy_fini()
> -> frxm_policy_flush() -> flow_cache_flush(), we will get NULL pointer
> dereference when check percpu_empty. The code path looks like:
>
> flow_cache_fini()
>   - fc->percpu = NULL
> xfrm_policy_fini()
>   - xfrm_policy_flush()
>     - xfrm_garbage_collect()
>       - flow_cache_flush()
>         - flow_cache_percpu_empty()
>           - fcp = per_cpu_ptr(fc->percpu, cpu)
>
> To reproduce, just add ipsec in netns and then remove the netns.
>
> v2:
> As Xin Long suggested, since only two other places need to call it. move
> xfrm_garbage_collect() outside xfrm_policy_flush().
>
> v3:
> Fix subject mismatch after v2 fix.
>
> Fixes: 35db06912189 ("xfrm: do the garbage collection after flushing policy")
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> ---
>  net/key/af_key.c       | 2 ++
>  net/xfrm/xfrm_policy.c | 4 ----
>  net/xfrm/xfrm_user.c   | 1 +
>  3 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/net/key/af_key.c b/net/key/af_key.c
> index 512dc43..5103f92 100644
> --- a/net/key/af_key.c
> +++ b/net/key/af_key.c
> @@ -2755,6 +2755,8 @@ static int pfkey_spdflush(struct sock *sk, struct sk_buff *skb, const struct sad
>         int err, err2;
>
>         err = xfrm_policy_flush(net, XFRM_POLICY_TYPE_MAIN, true);
> +       if (!err)
> +               xfrm_garbage_collect(net);
>         err2 = unicast_flush_resp(sk, hdr);
>         if (err || err2) {
>                 if (err == -ESRCH) /* empty table - old silent behavior */
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index ed4e52d..643a18f 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -1006,10 +1006,6 @@ int xfrm_policy_flush(struct net *net, u8 type, bool task_valid)
>                 err = -ESRCH;
>  out:
>         spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
> -
> -       if (cnt)
> -               xfrm_garbage_collect(net);
> -
>         return err;
>  }
>  EXPORT_SYMBOL(xfrm_policy_flush);
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 38614df..86116e9 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -2027,6 +2027,7 @@ static int xfrm_flush_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
>                         return 0;
>                 return err;
>         }
> +       xfrm_garbage_collect(net);
>
>         c.data.type = type;
>         c.event = nlh->nlmsg_type;
> --
> 2.5.5
>
Reviewed-by: Xin Long <lucien.xin@...il.com>

though I could still see some typo err in changelog.