Re: [PATCH v2] xfrm: policy: Fix doulbe free in xfrm_policy_timer

[Yuehaibing]

On 2020/3/23 14:53, Timo Teras wrote:
> Hi
> 
> On Mon, 23 Mar 2020 09:41:55 +0800
> YueHaibing <yuehaibing@xxxxxxxxxx> wrote:
> 
>> After xfrm_add_policy add a policy, its ref is 2, then
>>
>>                              xfrm_policy_timer
>>                                read_lock
>>                                xp->walk.dead is 0
>>                                ....
>>                                mod_timer()
>> xfrm_policy_kill
>>   policy->walk.dead = 1
>>   ....
>>   del_timer(&policy->timer)
>>     xfrm_pol_put //ref is 1
>>   xfrm_pol_put  //ref is 0
>>     xfrm_policy_destroy
>>       call_rcu
>>                                  xfrm_pol_hold //ref is 1
>>                                read_unlock
>>                                xfrm_pol_put //ref is 0
>>                                  xfrm_policy_destroy
>>                                   call_rcu
>>
>> xfrm_policy_destroy is called twice, which may leads to
>> double free.
> 
> I believe the timer changes were added later in commit e7d8f6cb2f which
> added holding a reference when timer is running. I think it fails to
> properly account for concurrently running timer in xfrm_policy_kill().

commit e7d8f6cb2f hold a reference when &pq->hold_timer is armed,
in my case, it's policy->timer, and hold_timer is not armed.
> 
> The time when commit ea2dea9dacc2 was done this was not the case.
> 
> I think it would be preferable if the concurrency issue could be solved
> without additional locking.
> 
>> Call Trace:
>> RIP: 0010:refcount_warn_saturate+0x161/0x210
>> ...
>>  xfrm_policy_timer+0x522/0x600
>>  call_timer_fn+0x1b3/0x5e0
>>  ? __xfrm_decode_session+0x2990/0x2990
>>  ? msleep+0xb0/0xb0
>>  ? _raw_spin_unlock_irq+0x24/0x40
>>  ? __xfrm_decode_session+0x2990/0x2990
>>  ? __xfrm_decode_session+0x2990/0x2990
>>  run_timer_softirq+0x5c5/0x10e0
>>
>> Fix this by use write_lock_bh in xfrm_policy_kill.
>>
>> Fixes: ea2dea9dacc2 ("xfrm: remove policy lock when accessing
>> policy->walk.dead") Signed-off-by: YueHaibing <yuehaibing@xxxxxxxxxx>
> 
> Should be instead:
> Fixes: e7d8f6cb2f ("xfrm: Add refcount handling to queued policies")
> 
>> ---
>> v2:  Fix typo 'write_lock_bh'--> 'write_unlock_bh' while unlocking
>> ---
>>  net/xfrm/xfrm_policy.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
>> index dbda08ec566e..ae0689174bbf 100644
>> --- a/net/xfrm/xfrm_policy.c
>> +++ b/net/xfrm/xfrm_policy.c
>> @@ -434,6 +434,7 @@ EXPORT_SYMBOL(xfrm_policy_destroy);
>>  
>>  static void xfrm_policy_kill(struct xfrm_policy *policy)
>>  {
>> +	write_lock_bh(&policy->lock);
>>  	policy->walk.dead = 1;
>>  
>>  	atomic_inc(&policy->genid);
>> @@ -445,6 +446,7 @@ static void xfrm_policy_kill(struct xfrm_policy
>> *policy) if (del_timer(&policy->timer))
>>  		xfrm_pol_put(policy);
>>  
>> +	write_unlock_bh(&policy->lock);
>>  	xfrm_pol_put(policy);
>>  }
>>  
> 
> 
> .
>