Prevent from potential dead lock for inet_listen_lock
Subsystems (2)
PATCH 1/1 Prevent from potential dead lock for inet_listen_lock
hashinfo->lhash_lock might be acquired by write_lock() in softirq, so using read_lock() here isn't safe, just substitudes by read_lock_bh().
--- include/net/inet_hashtables.h | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
include/net/inet_hashtables.h +2 -2
@@ -254,9 +254,9 @@ extern void inet_listen_wlock(struct inet_hashinfo *hashinfo);static inline void inet_listen_lock (struct inet_hashinfo *hashinfo) {/* read_lock synchronizes to candidates to writers */ -read_lock (&hashinfo->lhash_lock); +read_lock_bh (&hashinfo->lhash_lock);atomic_inc (&hashinfo->lhash_users); -read_unlock (&hashinfo->lhash_lock); +read_unlock_bh (&hashinfo->lhash_lock); }static inline void inet_listen_unlock (struct inet_hashinfo *hashinfo)
Signature · 1 line
1.5.3
David Miller
maintainerFrom: Gui Jianfeng <[email protected]>
Date: Fri, 20 Jun 2008 17:08:20 +0800
Quoted 1 line
hashinfo->lhash_lockmight be acquired bywrite_lock()in softirq,
How?
Quoted 3 lines
so using
read_lock()here isn't safe, just substitudes byread_lock_bh().Signed-off-by Gui Jianfeng <[email protected]>
I don't think this is necessary.
The only place the write lock is obtained, is via the ->hash() and ->unhash() sk_prot operation methods.
And for listening sockets that only occurs in normal base context. Never from softirqs.
If there is a patch from softirqs where this can occur, that is a bug and must be fixed.
Quoted 6 lines · David Miller
From: Gui Jianfeng <[email protected]>
Date: Fri, 20 Jun 2008 17:08:20 +0800Quoted 1 line
hashinfo->lhash_lockmight be acquired bywrite_lock()in softirq,How?
How about the following call trace.dccp_v4_rcv
-> sk_receive_skb(sk, skb, 1);
-> sk->sk_backlog_rcv(sk, skb);(dccp_v4_do_rcv)
-> dccp_rcv_state_process()
-> dccp_rcv_request_sent_state_process(sk, skb, dh, len);
-> icsk->icsk_af_ops->rebuild_header(sk); (inet_sk_rebuild_header)
-> inet_sk_reselect_saddr(sk))
-> __sk_prot_rehash(sk);
-> sk->sk_prot->hash(sk);
-> inet_hash(struct sock *sk)
-> __inet_hash(struct sock *sk)
-> inet_listen_wlock(hashinfo);
-> write_lock(&hashinfo->lhash_lock);Quoted 18 lines
Quoted 4 lines
so using
read_lock()here isn't safe, just substitudes byread_lock_bh().Signed-off-by Gui Jianfeng <[email protected]>I don't think this is necessary.
The only place the write lock is obtained, is via the ->hash() and ->unhash()
sk_protoperation methods.And for listening sockets that only occurs in normal base context. Never from softirqs.
If there is a patch from softirqs where this can occur, that is a bug and must be fixed.
Signature · 2 lines
Regards Gui Jianfeng
David Miller
maintainerFrom: Gui Jianfeng <[email protected]>
Date: Mon, 30 Jun 2008 11:16:41 +0800
Quoted 14 lines
How about the following call trace.
dccp_v4_rcv-> sk_receive_skb(sk, skb, 1); -> sk->sk_backlog_rcv(sk, skb);(dccp_v4_do_rcv) -> dccp_rcv_state_process() -> dccp_rcv_request_sent_state_process(sk, skb, dh, len); -> icsk->icsk_af_ops->rebuild_header(sk); (inet_sk_rebuild_header) -> inet_sk_reselect_saddr(sk)) -> __sk_prot_rehash(sk); -> sk->sk_prot->hash(sk); -> inet_hash(struct sock *sk) -> __inet_hash(struct sock *sk) -> inet_listen_wlock(hashinfo); -> write_lock(&hashinfo->lhash_lock);
You're not answering my question.
I'll ask my question one more time.
How can this happen for a LISTENING SOCKET? Ie. with sk_state == TCP_LISTEN.
Only listening sockets go into inet_listen_wlock().
This DCCP call trace you're showing sets the sk_state to DCCP_PARTOPEN right before that ->rebuild_header() call. (DCCP_PARTOPEN is defined to be equal to TCP_MAX_STATES in include/linux/dccp.h)
So this call chain is absolutely impossible.
We specifically forbid listening sockets from calling hash or unhash in BH context. And this is exactly what makes the locking legal.
You had to have a reason for writing this patch. You saw something, either a deadlock or a lockdep trace. My theory is that you saw lockdep triggered erroneously because it can't see what prevents BH contexts from invoking inet_listen_wlock().
Or did you just write this patch in response to pure code reading?
Quoted 36 lines · David Miller
From: Gui Jianfeng <[email protected]>
Date: Mon, 30 Jun 2008 11:16:41 +0800Quoted 14 lines
How about the following call trace.
dccp_v4_rcv-> sk_receive_skb(sk, skb, 1); -> sk->sk_backlog_rcv(sk, skb);(dccp_v4_do_rcv) -> dccp_rcv_state_process() -> dccp_rcv_request_sent_state_process(sk, skb, dh, len); -> icsk->icsk_af_ops->rebuild_header(sk); (inet_sk_rebuild_header) -> inet_sk_reselect_saddr(sk)) -> __sk_prot_rehash(sk); -> sk->sk_prot->hash(sk); -> inet_hash(struct sock *sk) -> __inet_hash(struct sock *sk) -> inet_listen_wlock(hashinfo); -> write_lock(&hashinfo->lhash_lock);You're not answering my question.
I'll ask my question one more time.
How can this happen for a LISTENING SOCKET? Ie. with
sk_state==TCP_LISTEN.Only listening sockets go into
inet_listen_wlock().This DCCP call trace you're showing sets the
sk_statetoDCCP_PARTOPENright before that ->rebuild_header() call. (DCCP_PARTOPENis defined to be equal toTCP_MAX_STATESininclude/linux/dccp.h)So this call chain is absolutely impossible.
We specifically forbid listening sockets from calling hash or unhash in BH context. And this is exactly what makes the locking legal.
Quoted 6 lines
You had to have a reason for writing this patch. You saw something, either a deadlock or a lockdep trace. My theory is that you saw lockdep triggered erroneously because it can't see what prevents BH contexts from invoking
inet_listen_wlock().Or did you just write this patch in response to pure code reading?
I think you are right. I read the code, and thought it might have deadlock problem. I'm very sorry for my mistake. Please ignore this patch.