| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <EB28B29A-A58C-4A5C-A4F9-DD30E82B7449@gmail.com>
Date: Thu, 5 Dec 2013 16:13:21 +0800
From: Jason HU <huzhijiang@...il.com>
To: Ying Xue <ying.xue@...driver.com>
Cc: wangweidong <wangweidong1@...wei.com>,
"<jon.maloy@...csson.com>" <jon.maloy@...csson.com>,
"<allan.stephens@...driver.com>" <allan.stephens@...driver.com>,
"<davem@...emloft.net>" <davem@...emloft.net>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"tipc-discussion@...ts.sourceforge.net"
<tipc-discussion@...ts.sourceforge.net>,
"dingtianhong@...wei.com" <dingtianhong@...wei.com>
Subject: Re: [tipc-discussion] [PATCH] tipc: fix a lockdep warning
Yours,
Jason
> On Nov 22, 2013, at 17:08, Ying Xue <ying.xue@...driver.com> wrote:
>
> Hi Weidong,
>
> Please ignore my last mail because you are to fix the issue which is
> different with my mentioned one, and please see my below comments about
> your patch;
>
>> On 11/22/2013 04:18 PM, wangweidong wrote:
>> PC1:tipc-config -netid=1234 -a=1.1.2 -be=eth:eth0/1.1.0
>> PC2:tipc-config -netid=1234 -a=1.1.3 -be=eth:eth0/1.1.0
>>
>> I used a server code Like this:
>> ----------------
>> sk=socket(AF_TIPC,SOCK_RDM,0);
>> bind(sk, &addr, len);
>> while(1) {
>> recvfrom(sk,...);
>> ...
>> sendto(sk,...);
>> }
>> ----------------
>>
>> when I did ./server in PC1, I got a lockdep as bellow:
>>
>> ======================================================
>> [ INFO: possible circular locking dependency detected ]
>> 3.12.0-lockdep+ #4 Not tainted
>> -------------------------------------------------------
>> server/3772 is trying to acquire lock:
>> (tipc_net_lock){++.-..}, at: [<ffffffffa02e324f>] tipc_link_send+0x2f/0xc0 [tipc]
>>
>> but task is already holding lock:
>> (tipc_nametbl_lock){++--..}, at: [<ffffffffa02e83e6>] tipc_nametbl_publish+0x46/0xc0 [tipc]
>> which lock already depends on the new lock.
>>
>> the existing dependency chain (in reverse order) is:
>> -> #1 (tipc_nametbl_lock){++--..}:
>> [<ffffffff810a2547>] validate_chain+0x6a7/0x7d0
>> [<ffffffff810a29d1>] __lock_acquire+0x361/0x610
>> [<ffffffff810a2d62>] lock_acquire+0xe2/0x110
>> [<ffffffff8151e061>] _raw_write_lock_bh+0x31/0x40
>> [<ffffffffa02e5d40>] tipc_named_reinit+0x10/0x70 [tipc]
>> [<ffffffffa02e8512>] tipc_net_start+0x22/0x80 [tipc]
>> [<ffffffffa02dff0e>] tipc_core_start_net+0xe/0x40 [tipc]
>> [<ffffffffa02df625>] cfg_set_own_addr+0x75/0xc0 [tipc]
>> [<ffffffffa02df8f5>] tipc_cfg_do_cmd+0x135/0x550 [tipc]
>> [<ffffffffa02e87f9>] handle_cmd+0x49/0xe0 [tipc]
>> [<ffffffff814764fd>] genl_family_rcv_msg+0x22d/0x3c0
>> [<ffffffff81476700>] genl_rcv_msg+0x70/0xd0
>> [<ffffffff81474dc9>] netlink_rcv_skb+0x89/0xb0
>> [<ffffffff81475f87>] genl_rcv+0x27/0x40
>> [<ffffffff81474b1e>] netlink_unicast+0x14e/0x1a0
>> [<ffffffff81475735>] netlink_sendmsg+0x245/0x420
>> [<ffffffff814294f6>] __sock_sendmsg+0x66/0x80
>> [<ffffffff814295c2>] sock_aio_write+0xb2/0xc0
>> [<ffffffff811968f0>] do_sync_write+0x60/0x90
>> [<ffffffff81198891>] vfs_write+0x1d1/0x1e0
>> [<ffffffff811989bd>] SyS_write+0x5d/0xa0
>> [<ffffffff81527522>] system_call_fastpath+0x16/0x1b
>>
>> -> #0 (tipc_net_lock){++.-..}:
>> [<ffffffff810a1e2e>] check_prev_add+0x41e/0x490
>> [<ffffffff810a2547>] validate_chain+0x6a7/0x7d0
>> [<ffffffff810a29d1>] __lock_acquire+0x361/0x610
>> [<ffffffff810a2d62>] lock_acquire+0xe2/0x110
>> [<ffffffff8151e2f4>] _raw_read_lock_bh+0x34/0x50
>> [<ffffffffa02e324f>] tipc_link_send+0x2f/0xc0 [tipc]
>> [<ffffffffa02e617b>] named_cluster_distribute+0x6b/0x80 [tipc]
>> [<ffffffffa02e62ab>] tipc_named_publish+0x7b/0x90 [tipc]
>> [<ffffffffa02e841b>] tipc_nametbl_publish+0x7b/0xc0 [tipc]
>> [<ffffffffa02e9958>] tipc_publish+0x98/0xf0 [tipc]
>> [<ffffffffa02ebf58>] bind+0x78/0xb0 [tipc]
>> [<ffffffff81428dc0>] SyS_bind+0xb0/0xd0
>> [<ffffffff81527522>] system_call_fastpath+0x16/0x1b
>>
>> other info that might help us debug this:
>>
>> Possible unsafe locking scenario:
>>
>> CPU0 CPU1
>> ---- ----
>> lock(tipc_nametbl_lock);
>> lock(tipc_net_lock);
>> lock(tipc_nametbl_lock);
>> lock(tipc_net_lock);
>>
>> *** DEADLOCK ***
>> ----------------------------------------------------------
>>
>> problem is that tipc_nametbl_publish which will hold tipc_nametbl_lock
>> and acquire tipc_net_lock, while the tipc_net_start which hold
>> tipc_net_lock and acquir tipc_nametbl_lock, so the dead lock occurs.
>>
>> tipc_link_send protected by tipc_net_lock, we can unlock the
>> tipc_nametbl_lock, and it no need the tipc_nametbl_lock to protect it.
>> so I just unlock the tbl_lock before it, and lock the tbl_lock after it.
>>
>> Signed-off-by: Wang Weidong <wangweidong1@...wei.com>
>> ---
>> net/tipc/name_distr.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
>> index e0d0805..ab8f96c 100644
>> --- a/net/tipc/name_distr.c
>> +++ b/net/tipc/name_distr.c
>> @@ -138,7 +138,9 @@ static void named_cluster_distribute(struct sk_buff *buf)
>> if (!buf_copy)
>> break;
>> msg_set_destnode(buf_msg(buf_copy), n_ptr->addr);
>> + write_unlock_bh(&tipc_nametbl_lock);
>> tipc_link_send(buf_copy, n_ptr->addr, n_ptr->addr);
>> + write_lock_bh(&tipc_nametbl_lock);
>
> We cannot temporarily release/hold tipc_nametbl_lock here, please see
> below call path:
>
> tipc_nametbl_withdraw()
> tipc_named_withdraw()
> named_cluster_distribute()
> tipc_link_send()
>
> Especially in tipc_nametbl_withdraw(), we must hold tipc_nametbl_lock to
> protect name table before tipc_named_withdraw() is called. If we
> temporarily release tipc_nametbl_lock in named_cluster_distribute(), I
> am afraid that name table might be changed by another thread at the
> moment, having name table inconsistent possibly.
If the inconsistent possibility that you worried about is the matter of mutually exclusive between user threads which call bind(), then I think it will not occur, because we still got port lock hold when calling tipc_nametbl_publish(). So even we do not have to use tipc_nametbl_lock to warp up both tipc_nametbl_insert_publ() and tipc_name_publish() . Just tipc_nametbl_insert_publ() is enough. May be I missed something, please correct me.
>
> Actually we are implementing another patchset purging the tipc_net_lock
> from TIPC stack. If the patchset is involved, I guess the issue would
> disappear.
>
> If you have an interesting to see how to purge to tipc_net_lock, please
> monitor tipc-discussion mail list.
>
> Regards,
> Ying
>
>
>> }
>> }
>
>
> ------------------------------------------------------------------------------
> Shape the Mobile Experience: Free Subscription
> Software experts and developers: Be at the forefront of tech innovation.
> Intel(R) Software Adrenaline delivers strategic insight and game-changing
> conversations that shape the rapidly evolving mobile landscape. Sign up now.
> http://pubads.g.doubleclick.net/gampad/clk?id=63431311&iu=/4140/ostg.clktrk
> _______________________________________________
> tipc-discussion mailing list
> tipc-discussion@...ts.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists