[PATCH net] net/flow: fix fc->percpu NULL pointer dereference

[PATCH net] net/flow: fix fc->percpu NULL pointer dereference

[Hangbin Liu]

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.

Fixes: 35db06912189 ("xfrm: do the garbage collection after flushing policy")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/core/flow.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/core/flow.c b/net/core/flow.c
index f7f5d19..321fc53 100644
--- a/net/core/flow.c
+++ b/net/core/flow.c
@@ -332,10 +332,13 @@ static int flow_cache_percpu_empty(struct flow_cache *fc, int cpu)
 	struct flow_cache_percpu *fcp;
 	unsigned int i;
 
-	fcp = per_cpu_ptr(fc->percpu, cpu);
-	for (i = 0; i < flow_cache_hash_size(fc); i++)
-		if (!hlist_empty(&fcp->hash_table[i]))
-			return 0;
+	if (fc->percpu) {
+		fcp = per_cpu_ptr(fc->percpu, cpu);
+		for (i = 0; i < flow_cache_hash_size(fc); i++)
+			if (!hlist_empty(&fcp->hash_table[i]))
+				return 0;
+	}
+
 	return 1;
 }
 
-- 
2.5.5

Re: [PATCH net] net/flow: fix fc->percpu NULL pointer dereference

[Hangbin Liu]

Hi Steffen,

BTW, If we put the check in xfrm_policy_flush(), we can prevent it earlier.
But If we put the check in flow_cache_percpu_empty(), we can prevent
other functions set fc->percpu to NULL, although not much possible : )

So I'm not quite sure whether we should put the check in
flow_cache_percpu_empty() or in xfrm_policy_flush().

Do you have any suggestion?

Thanks
Hangbin

2017-06-09 16:13 GMT+08:00 Hangbin Liu <liuhangbin@gmail.com>:
> 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.
>
> Fixes: 35db06912189 ("xfrm: do the garbage collection after flushing policy")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  net/core/flow.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/net/core/flow.c b/net/core/flow.c
> index f7f5d19..321fc53 100644
> --- a/net/core/flow.c
> +++ b/net/core/flow.c
> @@ -332,10 +332,13 @@ static int flow_cache_percpu_empty(struct flow_cache *fc, int cpu)
>         struct flow_cache_percpu *fcp;
>         unsigned int i;
>
> -       fcp = per_cpu_ptr(fc->percpu, cpu);
> -       for (i = 0; i < flow_cache_hash_size(fc); i++)
> -               if (!hlist_empty(&fcp->hash_table[i]))
> -                       return 0;
> +       if (fc->percpu) {
> +               fcp = per_cpu_ptr(fc->percpu, cpu);
> +               for (i = 0; i < flow_cache_hash_size(fc); i++)
> +                       if (!hlist_empty(&fcp->hash_table[i]))
> +                               return 0;
> +       }
> +
>         return 1;
>  }
>
> --
> 2.5.5
>

Re: [PATCH net] net/flow: fix fc->percpu NULL pointer dereference

[Steffen Klassert]

On Fri, Jun 09, 2017 at 04:23:01PM +0800, Hangbin Liu wrote:
> Hi Steffen,
> 
> BTW, If we put the check in xfrm_policy_flush(), we can prevent it earlier.
> But If we put the check in flow_cache_percpu_empty(), we can prevent
> other functions set fc->percpu to NULL, although not much possible : )
> 
> So I'm not quite sure whether we should put the check in
> flow_cache_percpu_empty() or in xfrm_policy_flush().

Can't we just call xfrm_policy_fini() first and then flow_cache_fini()?

Re: [PATCH net] net/flow: fix fc->percpu NULL pointer dereference

[Xin Long]

On Fri, Jun 9, 2017 at 4:32 PM, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> On Fri, Jun 09, 2017 at 04:23:01PM +0800, Hangbin Liu wrote:
>> Hi Steffen,
>>
>> BTW, If we put the check in xfrm_policy_flush(), we can prevent it earlier.
>> But If we put the check in flow_cache_percpu_empty(), we can prevent
>> other functions set fc->percpu to NULL, although not much possible : )
>>
>> So I'm not quite sure whether we should put the check in
>> flow_cache_percpu_empty() or in xfrm_policy_flush().
>
> Can't we just call xfrm_policy_fini() first and then flow_cache_fini()?
>
That would be a better fix. seems safe as what flow_cache_fini does
is only to free fcp->hash_table and stop timer, I didn't see  it has
any dependence on xfrm_policy stuff.

Re: [PATCH net] net/flow: fix fc->percpu NULL pointer dereference

[Hangbin Liu]

2017-06-09 16:43 GMT+08:00 Xin Long <lucien.xin@gmail.com>:
> On Fri, Jun 9, 2017 at 4:32 PM, Steffen Klassert
> <steffen.klassert@secunet.com> wrote:
>> On Fri, Jun 09, 2017 at 04:23:01PM +0800, Hangbin Liu wrote:
>>> Hi Steffen,
>>>
>>> BTW, If we put the check in xfrm_policy_flush(), we can prevent it earlier.
>>> But If we put the check in flow_cache_percpu_empty(), we can prevent
>>> other functions set fc->percpu to NULL, although not much possible : )
>>>
>>> So I'm not quite sure whether we should put the check in
>>> flow_cache_percpu_empty() or in xfrm_policy_flush().
>>
>> Can't we just call xfrm_policy_fini() first and then flow_cache_fini()?

Yes, that would be easy fix. I have been thinking about that.  But if we change
 the order in xfrm_net_exit(), do we also need to change the order in
 xfrm_net_init()? That would change a lot.

If no need, that would be good.

>>
> That would be a better fix. seems safe as what flow_cache_fini does
> is only to free fcp->hash_table and stop timer, I didn't see  it has
> any dependence on xfrm_policy stuff.

I'm not familiar about this part. So not sure about the influence if we free
the flow cache after xfrm_policy_fini(). I need do some test first.

I would also be appreciate if you or some one could make sure if it doesn't
influence anything.

Thanks
Hangbin

Re: [PATCH net] net/flow: fix fc->percpu NULL pointer dereference

[Xin Long]

On Fri, Jun 9, 2017 at 5:06 PM, Hangbin Liu <liuhangbin@gmail.com> wrote:
> 2017-06-09 16:43 GMT+08:00 Xin Long <lucien.xin@gmail.com>:
>> On Fri, Jun 9, 2017 at 4:32 PM, Steffen Klassert
>> <steffen.klassert@secunet.com> wrote:
>>> On Fri, Jun 09, 2017 at 04:23:01PM +0800, Hangbin Liu wrote:
>>>> Hi Steffen,
>>>>
>>>> BTW, If we put the check in xfrm_policy_flush(), we can prevent it earlier.
>>>> But If we put the check in flow_cache_percpu_empty(), we can prevent
>>>> other functions set fc->percpu to NULL, although not much possible : )
>>>>
>>>> So I'm not quite sure whether we should put the check in
>>>> flow_cache_percpu_empty() or in xfrm_policy_flush().
>>>
>>> Can't we just call xfrm_policy_fini() first and then flow_cache_fini()?
>
> Yes, that would be easy fix. I have been thinking about that.  But if we change
>  the order in xfrm_net_exit(), do we also need to change the order in
>  xfrm_net_init()? That would change a lot.
>
> If no need, that would be good.
>
>>>
>> That would be a better fix. seems safe as what flow_cache_fini does
>> is only to free fcp->hash_table and stop timer, I didn't see  it has
>> any dependence on xfrm_policy stuff.
>
> I'm not familiar about this part. So not sure about the influence if we free
> the flow cache after xfrm_policy_fini(). I need do some test first.
>
> I would also be appreciate if you or some one could make sure if it doesn't
> influence anything.
>
another fix is to move xfrm_garbage_collect out of xfrm_policy_flush.
I could only see two places need to call it.
something like:

--- 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..89343a3 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1007,9 +1007,6 @@ int xfrm_policy_flush(struct net *net, u8 type,
bool task_valid)
 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);

Re: [PATCH net] net/flow: fix fc->percpu NULL pointer dereference

[Hangbin Liu]

On Fri, Jun 09, 2017 at 05:49:50PM +0800, Xin Long wrote:
> another fix is to move xfrm_garbage_collect out of xfrm_policy_flush.
> I could only see two places need to call it.
> something like:
> 
> --- 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 */

Hmm, that would be better, just the xfrm_garbage_collect() need to be after
err || err2 check

+          xfrm_garbage_collect(net);

I will send v2 patch.

Thanks
Hangbin

Re: [PATCH net] net/flow: fix fc->percpu NULL pointer dereference

[Hangbin Liu]

2017-06-09 20:29 GMT+08:00 Hangbin Liu <liuhangbin@gmail.com>:
> On Fri, Jun 09, 2017 at 05:49:50PM +0800, Xin Long wrote:
>> another fix is to move xfrm_garbage_collect out of xfrm_policy_flush.
>> I could only see two places need to call it.
>> something like:
>>
>> --- 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 */
>
> Hmm, that would be better, just the xfrm_garbage_collect() need to be after
> err || err2 check
>
> +          xfrm_garbage_collect(net);

Ah, my mistake.  we should check err first. Sorry.

>
> I will send v2 patch.
>
> Thanks
> Hangbin

[PATCHv2 net] net/flow: fix fc->percpu NULL pointer dereference

[Hangbin Liu]

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().

Fixes: 35db06912189 ("xfrm: do the garbage collection after flushing policy")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.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

Re: [PATCHv2 net] net/flow: fix fc->percpu NULL pointer dereference

[Xin Long]

On Fri, Jun 9, 2017 at 9:09 PM, Hangbin Liu <liuhangbin@gmail.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().
>
> Fixes: 35db06912189 ("xfrm: do the garbage collection after flushing policy")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.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
>
 It's a xfrm fix, pls also fix the title, like:
   xfrm: move xfrm_garbage_collect out of xfrm_policy_flush
or
   xfrm: fix ...

Re: [PATCHv2 net] net/flow: fix fc->percpu NULL pointer dereference

[Hangbin Liu]

On Sat, Jun 10, 2017 at 04:29:23PM +0800, Xin Long wrote:
>  It's a xfrm fix, pls also fix the title, like:
>    xfrm: move xfrm_garbage_collect out of xfrm_policy_flush
> or
>    xfrm: fix ...
Opps, sorry forgot that.

[PATCHv3 net] xfrm: move xfrm_garbage_collect out of xfrm_policy_flush

[Hangbin Liu]

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@gmail.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

Re: [PATCHv3 net] xfrm: move xfrm_garbage_collect out of xfrm_policy_flush

[Xin Long]

On Sun, Jun 11, 2017 at 9:44 AM, Hangbin Liu <liuhangbin@gmail.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@gmail.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@gmail.com>

though I could still see some typo err in changelog.

Re: [PATCHv3 net] xfrm: move xfrm_garbage_collect out of xfrm_policy_flush

[Steffen Klassert]

On Sun, Jun 11, 2017 at 09:44:20AM +0800, Hangbin Liu 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@gmail.com>

Patch applied, thanks eveyone!