On Fri, Jun 9, 2017 at 5:06 PM, Hangbin Liu <liuhang...@gmail.com> wrote:
> 2017-06-09 16:43 GMT+08:00 Xin Long <lucien....@gmail.com>:
>> On Fri, Jun 9, 2017 at 4:32 PM, Steffen Klassert
>> <steffen.klass...@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);
