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.
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 add write_lock_bh in xfrm_policy_kill.
Fixes: ea2dea9dacc2 ("xfrm: remove policy lock when accessing policy->walk.dead")
Signed-off-by: YueHaibing <[email protected]>
---
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_lock_bh(&policy->lock);
xfrm_pol_put(policy);
}
--
2.17.1
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.
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 <[email protected]>
---
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);
}
--
2.17.1
On Mon, Mar 23, 2020 at 09:41:55AM +0800, YueHaibing wrote:
>
> 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);
Why did you expand the critical section? Can't you just undo the
patch in xfrm_policy_kill?
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Hi
On Mon, 23 Mar 2020 09:41:55 +0800
YueHaibing <[email protected]> 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 <[email protected]>
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);
> }
>
On 2020/3/23 14:29, Herbert Xu wrote:
> On Mon, Mar 23, 2020 at 09:41:55AM +0800, YueHaibing wrote:
>>
>> 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);
>
> Why did you expand the critical section? Can't you just undo the
> patch in xfrm_policy_kill?
Indeed, the critical section should not be expanded, thanks!
>
> Cheers,
>
On 2020/3/23 14:53, Timo Teras wrote:
> Hi
>
> On Mon, 23 Mar 2020 09:41:55 +0800
> YueHaibing <[email protected]> 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 <[email protected]>
>
> 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);
>> }
>>
>
>
> .
>
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.
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 <[email protected]>
---
v3: Only lock 'policy->walk.dead'
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..8a4af86a285e 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -434,7 +434,9 @@ EXPORT_SYMBOL(xfrm_policy_destroy);
static void xfrm_policy_kill(struct xfrm_policy *policy)
{
+ write_lock_bh(&policy->lock);
policy->walk.dead = 1;
+ write_unlock_bh(&policy->lock);
atomic_inc(&policy->genid);
--
2.17.1
On Mon, 23 Mar 2020 15:21:45 +0800
Yuehaibing <[email protected]> wrote:
> On 2020/3/23 14:53, Timo Teras wrote:
> > Hi
> >
> > On Mon, 23 Mar 2020 09:41:55 +0800
> > YueHaibing <[email protected]> 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.
Ah, misread. Should have waited until first cup of coffee of the
morning..
I must have not understood del_timer() return value fully back then.
I first thought a more robust fix would be to take an extra reference
in the beginning of the timer function (and instead of using mod_timer()
return to see if a new reference is needed, it could be used in the
prologue to "keep" the reference). This would guarantee always proper
reference count inside the timer function.
But I suppose because of the above xfrm_policy_kill() is the only place
supposed to delete the timer, and that's why it had the locking in the
first place. And the above "fix" might still end up having timer armed
after kill_policy called del_timer() which is wrong.
So perhaps it's more straightforward to just have the lock as it was
originally around policy->walk.dead only. Perhaps adding a comment that
it's synchronizing with the timer function.
Since xfrm_policy_timer() ends with policy unref already now, the above
reference keeping tricking might be good to do even for the current
code as separate patch to avoid atomic ops if possible.
Thanks,
Timo
On Mon, 23 Mar 2020 15:32:39 +0800
YueHaibing <[email protected]> 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.
>
> 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 <[email protected]>
Acked-by: Timo Teräs <[email protected]>
> ---
> v3: Only lock 'policy->walk.dead'
> 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..8a4af86a285e 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -434,7 +434,9 @@ EXPORT_SYMBOL(xfrm_policy_destroy);
>
> static void xfrm_policy_kill(struct xfrm_policy *policy)
> {
> + write_lock_bh(&policy->lock);
> policy->walk.dead = 1;
> + write_unlock_bh(&policy->lock);
>
> atomic_inc(&policy->genid);
>
> --
> 2.17.1
>
>
On Mon, Mar 23, 2020 at 03:32:39PM +0800, YueHaibing 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.
>
> 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 <[email protected]>
> ---
> v3: Only lock 'policy->walk.dead'
> v2: Fix typo 'write_lock_bh'--> 'write_unlock_bh' while unlocking
> ---
> net/xfrm/xfrm_policy.c | 2 ++
> 1 file changed, 2 insertions(+)
Acked-by: Herbert Xu <[email protected]>
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Mon, Mar 23, 2020 at 03:32:39PM +0800, YueHaibing 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.
>
> 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 <[email protected]>
> ---
> v3: Only lock 'policy->walk.dead'
> v2: Fix typo 'write_lock_bh'--> 'write_unlock_bh' while unlocking
> ---
Applied, thanks everyone!