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