* [PATCH] xfrm: policy: Fix doulbe free in xfrm_policy_timer
@ 2020-03-18 3:48 YueHaibing
2020-03-20 12:37 ` Dan Carpenter
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: YueHaibing @ 2020-03-18 3:48 UTC (permalink / raw)
To: steffen.klassert, herbert, davem, kuba, timo.teras
Cc: netdev, linux-kernel, YueHaibing
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 <yuehaibing@huawei.com>
---
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] xfrm: policy: Fix doulbe free in xfrm_policy_timer
2020-03-18 3:48 [PATCH] xfrm: policy: Fix doulbe free in xfrm_policy_timer YueHaibing
@ 2020-03-20 12:37 ` Dan Carpenter
2020-03-23 1:41 ` [PATCH v2] " YueHaibing
2020-03-23 7:32 ` [PATCH v3] " YueHaibing
2 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2020-03-20 12:37 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]
Hi YueHaibing,
Thank you for the patch! Perhaps something to improve:
url: https://github.com/0day-ci/linux/commits/YueHaibing/xfrm-policy-Fix-doulbe-free-in-xfrm_policy_timer/20200318-130228
base: https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec-next.git master
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
net/xfrm/xfrm_policy.c:449 xfrm_policy_kill() error: double locked 'policy->lock' (orig line 437)
# https://github.com/0day-ci/linux/commit/c5e76419bf56de53360e5d79e2708fd0eed0cf38
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout c5e76419bf56de53360e5d79e2708fd0eed0cf38
vim +449 net/xfrm/xfrm_policy.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 435 static void xfrm_policy_kill(struct xfrm_policy *policy)
^1da177e4c3f41 Linus Torvalds 2005-04-16 436 {
c5e76419bf56de YueHaibing 2020-03-18 @437 write_lock_bh(&policy->lock);
12a169e7d8f4b1 Herbert Xu 2008-10-01 438 policy->walk.dead = 1;
^1da177e4c3f41 Linus Torvalds 2005-04-16 439
285ead175c5dd5 Timo Teräs 2010-04-07 440 atomic_inc(&policy->genid);
^1da177e4c3f41 Linus Torvalds 2005-04-16 441
e7d8f6cb2f8735 Steffen Klassert 2013-10-08 442 if (del_timer(&policy->polq.hold_timer))
e7d8f6cb2f8735 Steffen Klassert 2013-10-08 443 xfrm_pol_put(policy);
1ee5e6676bccbf Li RongQing 2015-04-22 444 skb_queue_purge(&policy->polq.hold_queue);
a0073fe18e718a Steffen Klassert 2013-02-05 445
285ead175c5dd5 Timo Teräs 2010-04-07 446 if (del_timer(&policy->timer))
285ead175c5dd5 Timo Teräs 2010-04-07 447 xfrm_pol_put(policy);
285ead175c5dd5 Timo Teräs 2010-04-07 448
c5e76419bf56de YueHaibing 2020-03-18 @449 write_lock_bh(&policy->lock);
s/lock/unlock/
285ead175c5dd5 Timo Teräs 2010-04-07 450 xfrm_pol_put(policy);
^1da177e4c3f41 Linus Torvalds 2005-04-16 451 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xfrm: policy: Fix doulbe free in xfrm_policy_timer
@ 2020-03-20 12:37 ` Dan Carpenter
0 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2020-03-20 12:37 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]
Hi YueHaibing,
Thank you for the patch! Perhaps something to improve:
url: https://github.com/0day-ci/linux/commits/YueHaibing/xfrm-policy-Fix-doulbe-free-in-xfrm_policy_timer/20200318-130228
base: https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec-next.git master
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
net/xfrm/xfrm_policy.c:449 xfrm_policy_kill() error: double locked 'policy->lock' (orig line 437)
# https://github.com/0day-ci/linux/commit/c5e76419bf56de53360e5d79e2708fd0eed0cf38
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout c5e76419bf56de53360e5d79e2708fd0eed0cf38
vim +449 net/xfrm/xfrm_policy.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 435 static void xfrm_policy_kill(struct xfrm_policy *policy)
^1da177e4c3f41 Linus Torvalds 2005-04-16 436 {
c5e76419bf56de YueHaibing 2020-03-18 @437 write_lock_bh(&policy->lock);
12a169e7d8f4b1 Herbert Xu 2008-10-01 438 policy->walk.dead = 1;
^1da177e4c3f41 Linus Torvalds 2005-04-16 439
285ead175c5dd5 Timo Teräs 2010-04-07 440 atomic_inc(&policy->genid);
^1da177e4c3f41 Linus Torvalds 2005-04-16 441
e7d8f6cb2f8735 Steffen Klassert 2013-10-08 442 if (del_timer(&policy->polq.hold_timer))
e7d8f6cb2f8735 Steffen Klassert 2013-10-08 443 xfrm_pol_put(policy);
1ee5e6676bccbf Li RongQing 2015-04-22 444 skb_queue_purge(&policy->polq.hold_queue);
a0073fe18e718a Steffen Klassert 2013-02-05 445
285ead175c5dd5 Timo Teräs 2010-04-07 446 if (del_timer(&policy->timer))
285ead175c5dd5 Timo Teräs 2010-04-07 447 xfrm_pol_put(policy);
285ead175c5dd5 Timo Teräs 2010-04-07 448
c5e76419bf56de YueHaibing 2020-03-18 @449 write_lock_bh(&policy->lock);
s/lock/unlock/
285ead175c5dd5 Timo Teräs 2010-04-07 450 xfrm_pol_put(policy);
^1da177e4c3f41 Linus Torvalds 2005-04-16 451 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] xfrm: policy: Fix doulbe free in xfrm_policy_timer
2020-03-18 3:48 [PATCH] xfrm: policy: Fix doulbe free in xfrm_policy_timer YueHaibing
2020-03-20 12:37 ` Dan Carpenter
@ 2020-03-23 1:41 ` YueHaibing
2020-03-23 6:29 ` Herbert Xu
2020-03-23 6:53 ` Timo Teras
2020-03-23 7:32 ` [PATCH v3] " YueHaibing
2 siblings, 2 replies; 13+ messages in thread
From: YueHaibing @ 2020-03-23 1:41 UTC (permalink / raw)
To: steffen.klassert, herbert, davem, kuba, timo.teras
Cc: netdev, linux-kernel, YueHaibing
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 <yuehaibing@huawei.com>
---
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] xfrm: policy: Fix doulbe free in xfrm_policy_timer
2020-03-23 1:41 ` [PATCH v2] " YueHaibing
@ 2020-03-23 6:29 ` Herbert Xu
2020-03-23 7:04 ` Yuehaibing
2020-03-23 6:53 ` Timo Teras
1 sibling, 1 reply; 13+ messages in thread
From: Herbert Xu @ 2020-03-23 6:29 UTC (permalink / raw)
To: YueHaibing
Cc: steffen.klassert, davem, kuba, timo.teras, netdev, linux-kernel
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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] xfrm: policy: Fix doulbe free in xfrm_policy_timer
2020-03-23 1:41 ` [PATCH v2] " YueHaibing
2020-03-23 6:29 ` Herbert Xu
@ 2020-03-23 6:53 ` Timo Teras
2020-03-23 7:21 ` Yuehaibing
1 sibling, 1 reply; 13+ messages in thread
From: Timo Teras @ 2020-03-23 6:53 UTC (permalink / raw)
To: YueHaibing; +Cc: steffen.klassert, herbert, davem, kuba, netdev, linux-kernel
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);
> }
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] xfrm: policy: Fix doulbe free in xfrm_policy_timer
2020-03-23 6:29 ` Herbert Xu
@ 2020-03-23 7:04 ` Yuehaibing
0 siblings, 0 replies; 13+ messages in thread
From: Yuehaibing @ 2020-03-23 7:04 UTC (permalink / raw)
To: Herbert Xu
Cc: steffen.klassert, davem, kuba, timo.teras, netdev, linux-kernel
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,
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] xfrm: policy: Fix doulbe free in xfrm_policy_timer
2020-03-23 6:53 ` Timo Teras
@ 2020-03-23 7:21 ` Yuehaibing
2020-03-23 7:56 ` Timo Teras
0 siblings, 1 reply; 13+ messages in thread
From: Yuehaibing @ 2020-03-23 7:21 UTC (permalink / raw)
To: Timo Teras; +Cc: steffen.klassert, herbert, davem, kuba, netdev, linux-kernel
On 2020/3/23 14:53, Timo Teras wrote:
> 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().
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@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);
>> }
>>
>
>
> .
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] xfrm: policy: Fix doulbe free in xfrm_policy_timer
2020-03-18 3:48 [PATCH] xfrm: policy: Fix doulbe free in xfrm_policy_timer YueHaibing
2020-03-20 12:37 ` Dan Carpenter
2020-03-23 1:41 ` [PATCH v2] " YueHaibing
@ 2020-03-23 7:32 ` YueHaibing
2020-03-23 8:00 ` Timo Teras
` (2 more replies)
2 siblings, 3 replies; 13+ messages in thread
From: YueHaibing @ 2020-03-23 7:32 UTC (permalink / raw)
To: steffen.klassert, herbert, davem, kuba, timo.teras
Cc: netdev, linux-kernel, YueHaibing
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 <yuehaibing@huawei.com>
---
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] xfrm: policy: Fix doulbe free in xfrm_policy_timer
2020-03-23 7:21 ` Yuehaibing
@ 2020-03-23 7:56 ` Timo Teras
0 siblings, 0 replies; 13+ messages in thread
From: Timo Teras @ 2020-03-23 7:56 UTC (permalink / raw)
To: Yuehaibing; +Cc: steffen.klassert, herbert, davem, kuba, netdev, linux-kernel
On Mon, 23 Mar 2020 15:21:45 +0800
Yuehaibing <yuehaibing@huawei.com> wrote:
> On 2020/3/23 14:53, Timo Teras wrote:
> > 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().
>
> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] xfrm: policy: Fix doulbe free in xfrm_policy_timer
2020-03-23 7:32 ` [PATCH v3] " YueHaibing
@ 2020-03-23 8:00 ` Timo Teras
2020-03-23 21:44 ` Herbert Xu
2020-03-25 12:53 ` Steffen Klassert
2 siblings, 0 replies; 13+ messages in thread
From: Timo Teras @ 2020-03-23 8:00 UTC (permalink / raw)
To: YueHaibing; +Cc: steffen.klassert, herbert, davem, kuba, netdev, linux-kernel
On Mon, 23 Mar 2020 15:32:39 +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.
>
> 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>
Acked-by: Timo Teräs <timo.teras@iki.fi>
> ---
> 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
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] xfrm: policy: Fix doulbe free in xfrm_policy_timer
2020-03-23 7:32 ` [PATCH v3] " YueHaibing
2020-03-23 8:00 ` Timo Teras
@ 2020-03-23 21:44 ` Herbert Xu
2020-03-25 12:53 ` Steffen Klassert
2 siblings, 0 replies; 13+ messages in thread
From: Herbert Xu @ 2020-03-23 21:44 UTC (permalink / raw)
To: YueHaibing
Cc: steffen.klassert, davem, kuba, timo.teras, netdev, linux-kernel
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 <yuehaibing@huawei.com>
> ---
> 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 <herbert@gondor.apana.org.au>
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] xfrm: policy: Fix doulbe free in xfrm_policy_timer
2020-03-23 7:32 ` [PATCH v3] " YueHaibing
2020-03-23 8:00 ` Timo Teras
2020-03-23 21:44 ` Herbert Xu
@ 2020-03-25 12:53 ` Steffen Klassert
2 siblings, 0 replies; 13+ messages in thread
From: Steffen Klassert @ 2020-03-25 12:53 UTC (permalink / raw)
To: YueHaibing; +Cc: herbert, davem, kuba, timo.teras, netdev, linux-kernel
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 <yuehaibing@huawei.com>
> ---
> v3: Only lock 'policy->walk.dead'
> v2: Fix typo 'write_lock_bh'--> 'write_unlock_bh' while unlocking
> ---
Applied, thanks everyone!
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-03-25 12:54 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-18 3:48 [PATCH] xfrm: policy: Fix doulbe free in xfrm_policy_timer YueHaibing
2020-03-20 12:37 ` Dan Carpenter
2020-03-20 12:37 ` Dan Carpenter
2020-03-23 1:41 ` [PATCH v2] " YueHaibing
2020-03-23 6:29 ` Herbert Xu
2020-03-23 7:04 ` Yuehaibing
2020-03-23 6:53 ` Timo Teras
2020-03-23 7:21 ` Yuehaibing
2020-03-23 7:56 ` Timo Teras
2020-03-23 7:32 ` [PATCH v3] " YueHaibing
2020-03-23 8:00 ` Timo Teras
2020-03-23 21:44 ` Herbert Xu
2020-03-25 12:53 ` Steffen Klassert
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.