* [PATCH] tipc: fix an interrupt unsafe locking scenario
@ 2018-08-07 7:52 Ying Xue
2018-08-07 16:10 ` Jon Maloy
2018-08-07 20:18 ` David Miller
0 siblings, 2 replies; 3+ messages in thread
From: Ying Xue @ 2018-08-07 7:52 UTC (permalink / raw)
To: netdev, jon.maloy; +Cc: tipc-discussion, davem
Commit 9faa89d4ed9d ("tipc: make function tipc_net_finalize() thread
safe") tries to make it thread safe to set node address, so it uses
node_list_lock lock to serialize the whole process of setting node
address in tipc_net_finalize(). But it causes the following interrupt
unsafe locking scenario:
CPU0 CPU1
---- ----
rht_deferred_worker()
rhashtable_rehash_table()
lock(&(&ht->lock)->rlock)
tipc_nl_compat_doit()
tipc_net_finalize()
local_irq_disable();
lock(&(&tn->node_list_lock)->rlock);
tipc_sk_reinit()
rhashtable_walk_enter()
lock(&(&ht->lock)->rlock);
<Interrupt>
tipc_disc_rcv()
tipc_node_check_dest()
tipc_node_create()
lock(&(&tn->node_list_lock)->rlock);
*** DEADLOCK ***
When rhashtable_rehash_table() holds ht->lock on CPU0, it doesn't
disable BH. So if an interrupt happens after the lock, it can create
an inverse lock ordering between ht->lock and tn->node_list_lock. As
a consequence, deadlock might happen.
The reason causing the inverse lock ordering scenario above is because
the initial purpose of node_list_lock is not designed to do the
serialization of node address setting.
As cmpxchg() can guarantee CAS (compare-and-swap) process is atomic,
we use it to replace node_list_lock to ensure setting node address can
be atomically finished. It turns out the potential deadlock can be
avoided as well.
Fixes: 9faa89d4ed9d ("tipc: make function tipc_net_finalize() thread safe")
Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
net/tipc/net.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/net/tipc/net.c b/net/tipc/net.c
index a7f6964..62199cf 100644
--- a/net/tipc/net.c
+++ b/net/tipc/net.c
@@ -123,15 +123,13 @@ void tipc_net_finalize(struct net *net, u32 addr)
{
struct tipc_net *tn = tipc_net(net);
- spin_lock_bh(&tn->node_list_lock);
- if (!tipc_own_addr(net)) {
+ if (!cmpxchg(&tn->node_addr, 0, addr)) {
tipc_set_node_addr(net, addr);
tipc_named_reinit(net);
tipc_sk_reinit(net);
tipc_nametbl_publish(net, TIPC_CFG_SRV, addr, addr,
TIPC_CLUSTER_SCOPE, 0, addr);
}
- spin_unlock_bh(&tn->node_list_lock);
}
void tipc_net_stop(struct net *net)
--
2.7.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply related [flat|nested] 3+ messages in thread
* RE: [PATCH] tipc: fix an interrupt unsafe locking scenario
2018-08-07 7:52 [PATCH] tipc: fix an interrupt unsafe locking scenario Ying Xue
@ 2018-08-07 16:10 ` Jon Maloy
2018-08-07 20:18 ` David Miller
1 sibling, 0 replies; 3+ messages in thread
From: Jon Maloy @ 2018-08-07 16:10 UTC (permalink / raw)
To: Ying Xue, netdev; +Cc: davem, tipc-discussion
Nice fix. I was a little in doubt about using that lock, but deemed it was overkill to introduce an extra lock just for this case.
Acked-by: Jon Maloy <maloy@donjonn.com>
> -----Original Message-----
> From: Ying Xue <ying.xue@windriver.com>
> Sent: Tuesday, 07 August, 2018 02:53
> To: netdev@vger.kernel.org; Jon Maloy <jon.maloy@ericsson.com>
> Cc: davem@davemloft.net; tipc-discussion@lists.sourceforge.net
> Subject: [PATCH] tipc: fix an interrupt unsafe locking scenario
>
> Commit 9faa89d4ed9d ("tipc: make function tipc_net_finalize() thread
> safe") tries to make it thread safe to set node address, so it uses
> node_list_lock lock to serialize the whole process of setting node
> address in tipc_net_finalize(). But it causes the following interrupt
> unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> rht_deferred_worker()
> rhashtable_rehash_table()
> lock(&(&ht->lock)->rlock)
> tipc_nl_compat_doit()
> tipc_net_finalize()
> local_irq_disable();
> lock(&(&tn->node_list_lock)->rlock);
> tipc_sk_reinit()
> rhashtable_walk_enter()
> lock(&(&ht->lock)->rlock);
> <Interrupt>
> tipc_disc_rcv()
> tipc_node_check_dest()
> tipc_node_create()
> lock(&(&tn->node_list_lock)->rlock);
>
> *** DEADLOCK ***
>
> When rhashtable_rehash_table() holds ht->lock on CPU0, it doesn't
> disable BH. So if an interrupt happens after the lock, it can create
> an inverse lock ordering between ht->lock and tn->node_list_lock. As
> a consequence, deadlock might happen.
>
> The reason causing the inverse lock ordering scenario above is because
> the initial purpose of node_list_lock is not designed to do the
> serialization of node address setting.
>
> As cmpxchg() can guarantee CAS (compare-and-swap) process is atomic,
> we use it to replace node_list_lock to ensure setting node address can
> be atomically finished. It turns out the potential deadlock can be
> avoided as well.
>
> Fixes: 9faa89d4ed9d ("tipc: make function tipc_net_finalize() thread safe")
> Signed-off-by: Ying Xue <ying.xue@windriver.com>
> ---
> net/tipc/net.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/net/tipc/net.c b/net/tipc/net.c
> index a7f6964..62199cf 100644
> --- a/net/tipc/net.c
> +++ b/net/tipc/net.c
> @@ -123,15 +123,13 @@ void tipc_net_finalize(struct net *net, u32 addr)
> {
> struct tipc_net *tn = tipc_net(net);
>
> - spin_lock_bh(&tn->node_list_lock);
> - if (!tipc_own_addr(net)) {
> + if (!cmpxchg(&tn->node_addr, 0, addr)) {
> tipc_set_node_addr(net, addr);
> tipc_named_reinit(net);
> tipc_sk_reinit(net);
> tipc_nametbl_publish(net, TIPC_CFG_SRV, addr, addr,
> TIPC_CLUSTER_SCOPE, 0, addr);
> }
> - spin_unlock_bh(&tn->node_list_lock);
> }
>
> void tipc_net_stop(struct net *net)
> --
> 2.7.4
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] tipc: fix an interrupt unsafe locking scenario
2018-08-07 7:52 [PATCH] tipc: fix an interrupt unsafe locking scenario Ying Xue
2018-08-07 16:10 ` Jon Maloy
@ 2018-08-07 20:18 ` David Miller
1 sibling, 0 replies; 3+ messages in thread
From: David Miller @ 2018-08-07 20:18 UTC (permalink / raw)
To: ying.xue; +Cc: netdev, jon.maloy, tipc-discussion
From: Ying Xue <ying.xue@windriver.com>
Date: Tue, 7 Aug 2018 15:52:32 +0800
> Commit 9faa89d4ed9d ("tipc: make function tipc_net_finalize() thread
> safe") tries to make it thread safe to set node address, so it uses
> node_list_lock lock to serialize the whole process of setting node
> address in tipc_net_finalize(). But it causes the following interrupt
> unsafe locking scenario:
...
> When rhashtable_rehash_table() holds ht->lock on CPU0, it doesn't
> disable BH. So if an interrupt happens after the lock, it can create
> an inverse lock ordering between ht->lock and tn->node_list_lock. As
> a consequence, deadlock might happen.
>
> The reason causing the inverse lock ordering scenario above is because
> the initial purpose of node_list_lock is not designed to do the
> serialization of node address setting.
>
> As cmpxchg() can guarantee CAS (compare-and-swap) process is atomic,
> we use it to replace node_list_lock to ensure setting node address can
> be atomically finished. It turns out the potential deadlock can be
> avoided as well.
>
> Fixes: 9faa89d4ed9d ("tipc: make function tipc_net_finalize() thread safe")
> Signed-off-by: Ying Xue <ying.xue@windriver.com>
Applied, thanks.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-08-07 22:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-07 7:52 [PATCH] tipc: fix an interrupt unsafe locking scenario Ying Xue
2018-08-07 16:10 ` Jon Maloy
2018-08-07 20:18 ` David Miller
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.