| Message ID | 20201022100126.19565-1-zhuoliang.zhang@mediatek.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v2] net: xfrm: fix a race condition during allocing spi | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Guessed tree name to be net-next |
| jkicinski/subject_prefix | warning | Target tree name not specified in the subject |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | success | Errors and warnings before: 102 this patch: 102 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 32 lines checked |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 102 this patch: 102 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
On Thu, Oct 22, 2020 at 06:01:27PM +0800, Zhuoliang Zhang wrote: > From: zhuoliang zhang <zhuoliang.zhang@mediatek.com> > > we found that the following race condition exists in > xfrm_alloc_userspi flow: > > user thread state_hash_work thread > ---- ---- > xfrm_alloc_userspi() > __find_acq_core() > /*alloc new xfrm_state:x*/ > xfrm_state_alloc() > /*schedule state_hash_work thread*/ > xfrm_hash_grow_check() xfrm_hash_resize() > xfrm_alloc_spi /*hold lock*/ > x->id.spi = htonl(spi) spin_lock_bh(&net->xfrm.xfrm_state_lock) > /*waiting lock release*/ xfrm_hash_transfer() > spin_lock_bh(&net->xfrm.xfrm_state_lock) /*add x into hlist:net->xfrm.state_byspi*/ > hlist_add_head_rcu(&x->byspi) > spin_unlock_bh(&net->xfrm.xfrm_state_lock) > > /*add x into hlist:net->xfrm.state_byspi 2 times*/ > hlist_add_head_rcu(&x->byspi) > > 1. a new state x is alloced in xfrm_state_alloc() and added into the bydst hlist > in __find_acq_core() on the LHS; > 2. on the RHS, state_hash_work thread travels the old bydst and tranfers every xfrm_state > (include x) into the new bydst hlist and new byspi hlist; > 3. user thread on the LHS gets the lock and adds x into the new byspi hlist again. > > So the same xfrm_state (x) is added into the same list_hash > (net->xfrm.state_byspi) 2 times that makes the list_hash become > an inifite loop. > > To fix the race, x->id.spi = htonl(spi) in the xfrm_alloc_spi() is moved > to the back of spin_lock_bh, sothat state_hash_work thread no longer add x > which id.spi is zero into the hash_list. > > Fixes: f034b5d4efdf ("[XFRM]: Dynamic xfrm_state hash table sizing.") > Signed-off-by: zhuoliang zhang <zhuoliang.zhang@mediatek.com> > --- > net/xfrm/xfrm_state.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
On Thu, Oct 22, 2020 at 06:01:27PM +0800, Zhuoliang Zhang wrote: > From: zhuoliang zhang <zhuoliang.zhang@mediatek.com> > > we found that the following race condition exists in > xfrm_alloc_userspi flow: > > user thread state_hash_work thread > ---- ---- > xfrm_alloc_userspi() > __find_acq_core() > /*alloc new xfrm_state:x*/ > xfrm_state_alloc() > /*schedule state_hash_work thread*/ > xfrm_hash_grow_check() xfrm_hash_resize() > xfrm_alloc_spi /*hold lock*/ > x->id.spi = htonl(spi) spin_lock_bh(&net->xfrm.xfrm_state_lock) > /*waiting lock release*/ xfrm_hash_transfer() > spin_lock_bh(&net->xfrm.xfrm_state_lock) /*add x into hlist:net->xfrm.state_byspi*/ > hlist_add_head_rcu(&x->byspi) > spin_unlock_bh(&net->xfrm.xfrm_state_lock) > > /*add x into hlist:net->xfrm.state_byspi 2 times*/ > hlist_add_head_rcu(&x->byspi) > > 1. a new state x is alloced in xfrm_state_alloc() and added into the bydst hlist > in __find_acq_core() on the LHS; > 2. on the RHS, state_hash_work thread travels the old bydst and tranfers every xfrm_state > (include x) into the new bydst hlist and new byspi hlist; > 3. user thread on the LHS gets the lock and adds x into the new byspi hlist again. > > So the same xfrm_state (x) is added into the same list_hash > (net->xfrm.state_byspi) 2 times that makes the list_hash become > an inifite loop. > > To fix the race, x->id.spi = htonl(spi) in the xfrm_alloc_spi() is moved > to the back of spin_lock_bh, sothat state_hash_work thread no longer add x > which id.spi is zero into the hash_list. > > Fixes: f034b5d4efdf ("[XFRM]: Dynamic xfrm_state hash table sizing.") > Signed-off-by: zhuoliang zhang <zhuoliang.zhang@mediatek.com> Applied, thanks a lot! One remark, please don't use base64 encoding when you send patches. I had to hand edit your patch to get it applied.
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c index bbd4643d7e82..a77da7aae6fe 100644 --- a/net/xfrm/xfrm_state.c +++ b/net/xfrm/xfrm_state.c @@ -2004,6 +2004,7 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high) int err = -ENOENT; __be32 minspi = htonl(low); __be32 maxspi = htonl(high); + __be32 newspi = 0; u32 mark = x->mark.v & x->mark.m; spin_lock_bh(&x->lock); @@ -2022,21 +2023,22 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high) xfrm_state_put(x0); goto unlock; } - x->id.spi = minspi; + newspi = minspi; } else { u32 spi = 0; for (h = 0; h < high-low+1; h++) { spi = low + prandom_u32()%(high-low+1); x0 = xfrm_state_lookup(net, mark, &x->id.daddr, htonl(spi), x->id.proto, x->props.family); if (x0 == NULL) { - x->id.spi = htonl(spi); + newspi = htonl(spi); break; } xfrm_state_put(x0); } } - if (x->id.spi) { + if (newspi) { spin_lock_bh(&net->xfrm.xfrm_state_lock); + x->id.spi = newspi; h = xfrm_spi_hash(net, &x->id.daddr, x->id.spi, x->id.proto, x->props.family); hlist_add_head_rcu(&x->byspi, net->xfrm.state_byspi + h); spin_unlock_bh(&net->xfrm.xfrm_state_lock);