[PATCH] Prevent from potential dead lock for inet_listen_lock

[PATCH] Prevent from potential dead lock for inet_listen_lock

[Gui Jianfeng]

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().

Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
---
 include/net/inet_hashtables.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index 97dc35a..2818c8a 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -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)
-- 
1.5.3

Re: [PATCH] Prevent from potential dead lock for inet_listen_lock

[David Miller]

From: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
Date: Fri, 20 Jun 2008 17:08:20 +0800

> hashinfo->lhash_lock might be acquired by write_lock() in softirq, 

How?

> so using read_lock() here isn't safe, just substitudes by read_lock_bh().
> 
> Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>

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.

Re: [PATCH] Prevent from potential dead lock for inet_listen_lock

[Gui Jianfeng]

David Miller wrote:
> From: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
> Date: Fri, 20 Jun 2008 17:08:20 +0800
> 
>> hashinfo->lhash_lock might be acquired by write_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);

> 
>> so using read_lock() here isn't safe, just substitudes by read_lock_bh().
>>
>> Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
> 
> 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.
> 
> 
> 

-- 
Regards
Gui Jianfeng

Re: [PATCH] Prevent from potential dead lock for inet_listen_lock

[David Miller]

From: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
Date: Mon, 30 Jun 2008 11:16:41 +0800

>   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?

Re: [PATCH] Prevent from potential dead lock for inet_listen_lock

[Gui Jianfeng]

David Miller wrote:
> From: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
> Date: Mon, 30 Jun 2008 11:16:41 +0800
> 
>>   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?

  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.