Messages in this thread | | | Date | Mon, 23 Mar 2020 08:53:11 +0200 | From | Timo Teras <> | Subject | Re: [PATCH v2] xfrm: policy: Fix doulbe free in xfrm_policy_timer |
| |
Hi
On Mon, 23 Mar 2020 09:41:55 +0800 YueHaibing <yuehaibing@huawei.com> 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().
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@huawei.com>
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); > } >
| |