[PATCH net-next 0/4] net: block BH in TCP callbacks

[PATCH net-next 0/4] net: block BH in TCP callbacks

[Eric Dumazet]

Four layers using TCP stack were assuming sk_callback_lock could
be locked using read_lock() in their handlers because TCP stack
was running with BH disabled.

This is no longer the case. Since presumably the rest could
also depend on BH being disabled, just use read_lock_bh().

Then each layer might consider switching to RCU protection
and no longer depend on BH.

Eric Dumazet (4):
  scsi_tcp: block BH in TCP callbacks
  ocfs2/cluster: block BH in TCP callbacks
  rds: tcp: block BH in TCP callbacks
  tipc: block BH in TCP callbacks

 drivers/scsi/iscsi_tcp.c | 12 ++++++------
 fs/ocfs2/cluster/tcp.c   | 17 +++++++++--------
 net/rds/tcp_connect.c    |  4 ++--
 net/rds/tcp_listen.c     |  4 ++--
 net/rds/tcp_recv.c       |  4 ++--
 net/rds/tcp_send.c       |  4 ++--
 net/tipc/server.c        |  8 ++++----
 7 files changed, 27 insertions(+), 26 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH net-next 1/4] scsi_tcp: block BH in TCP callbacks

[Eric Dumazet]

iscsi_sw_tcp_data_ready() and iscsi_sw_tcp_state_change() were
using read_lock(&sk->sk_callback_lock) which is fine if caller
disabled BH.

TCP stack no longer has this requirement and can run from
process context.

Use read_lock_bh() variant to restore previous assumption.

Ideally this code could use RCU instead...

Fixes: 5413d1babe8f ("net: do not block BH while processing socket backlog")
Fixes: d41a69f1d390 ("tcp: make tcp_sendmsg() aware of socket backlog")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Venkatesh Srinivas <venkateshs@google.com>
---
 drivers/scsi/iscsi_tcp.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 2e4c82f8329c..ace4f1f41b8e 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -131,10 +131,10 @@ static void iscsi_sw_tcp_data_ready(struct sock *sk)
 	struct iscsi_tcp_conn *tcp_conn;
 	read_descriptor_t rd_desc;
 
-	read_lock(&sk->sk_callback_lock);
+	read_lock_bh(&sk->sk_callback_lock);
 	conn = sk->sk_user_data;
 	if (!conn) {
-		read_unlock(&sk->sk_callback_lock);
+		read_unlock_bh(&sk->sk_callback_lock);
 		return;
 	}
 	tcp_conn = conn->dd_data;
@@ -154,7 +154,7 @@ static void iscsi_sw_tcp_data_ready(struct sock *sk)
 	/* If we had to (atomically) map a highmem page,
 	 * unmap it now. */
 	iscsi_tcp_segment_unmap(&tcp_conn->in.segment);
-	read_unlock(&sk->sk_callback_lock);
+	read_unlock_bh(&sk->sk_callback_lock);
 }
 
 static void iscsi_sw_tcp_state_change(struct sock *sk)
@@ -165,10 +165,10 @@ static void iscsi_sw_tcp_state_change(struct sock *sk)
 	struct iscsi_session *session;
 	void (*old_state_change)(struct sock *);
 
-	read_lock(&sk->sk_callback_lock);
+	read_lock_bh(&sk->sk_callback_lock);
 	conn = sk->sk_user_data;
 	if (!conn) {
-		read_unlock(&sk->sk_callback_lock);
+		read_unlock_bh(&sk->sk_callback_lock);
 		return;
 	}
 	session = conn->session;
@@ -179,7 +179,7 @@ static void iscsi_sw_tcp_state_change(struct sock *sk)
 	tcp_sw_conn = tcp_conn->dd_data;
 	old_state_change = tcp_sw_conn->old_state_change;
 
-	read_unlock(&sk->sk_callback_lock);
+	read_unlock_bh(&sk->sk_callback_lock);
 
 	old_state_change(sk);
 }
-- 
2.8.0.rc3.226.g39d4020

[PATCH net-next 2/4] ocfs2/cluster: block BH in TCP callbacks

[Eric Dumazet]

TCP stack can now run from process context.

Use read_lock_bh() variant to restore previous assumption.

Fixes: 5413d1babe8f ("net: do not block BH while processing socket backlog")
Fixes: d41a69f1d390 ("tcp: make tcp_sendmsg() aware of socket backlog")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 fs/ocfs2/cluster/tcp.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/ocfs2/cluster/tcp.c b/fs/ocfs2/cluster/tcp.c
index 2d0acd6678fe..4238eb28889f 100644
--- a/fs/ocfs2/cluster/tcp.c
+++ b/fs/ocfs2/cluster/tcp.c
@@ -600,10 +600,11 @@ static void o2net_set_nn_state(struct o2net_node *nn,
 static void o2net_data_ready(struct sock *sk)
 {
 	void (*ready)(struct sock *sk);
+	struct o2net_sock_container *sc;
 
-	read_lock(&sk->sk_callback_lock);
-	if (sk->sk_user_data) {
-		struct o2net_sock_container *sc = sk->sk_user_data;
+	read_lock_bh(&sk->sk_callback_lock);
+	sc = sk->sk_user_data;
+	if (sc) {
 		sclog(sc, "data_ready hit\n");
 		o2net_set_data_ready_time(sc);
 		o2net_sc_queue_work(sc, &sc->sc_rx_work);
@@ -611,7 +612,7 @@ static void o2net_data_ready(struct sock *sk)
 	} else {
 		ready = sk->sk_data_ready;
 	}
-	read_unlock(&sk->sk_callback_lock);
+	read_unlock_bh(&sk->sk_callback_lock);
 
 	ready(sk);
 }
@@ -622,7 +623,7 @@ static void o2net_state_change(struct sock *sk)
 	void (*state_change)(struct sock *sk);
 	struct o2net_sock_container *sc;
 
-	read_lock(&sk->sk_callback_lock);
+	read_lock_bh(&sk->sk_callback_lock);
 	sc = sk->sk_user_data;
 	if (sc == NULL) {
 		state_change = sk->sk_state_change;
@@ -649,7 +650,7 @@ static void o2net_state_change(struct sock *sk)
 		break;
 	}
 out:
-	read_unlock(&sk->sk_callback_lock);
+	read_unlock_bh(&sk->sk_callback_lock);
 	state_change(sk);
 }
 
@@ -2012,7 +2013,7 @@ static void o2net_listen_data_ready(struct sock *sk)
 {
 	void (*ready)(struct sock *sk);
 
-	read_lock(&sk->sk_callback_lock);
+	read_lock_bh(&sk->sk_callback_lock);
 	ready = sk->sk_user_data;
 	if (ready == NULL) { /* check for teardown race */
 		ready = sk->sk_data_ready;
@@ -2039,7 +2040,7 @@ static void o2net_listen_data_ready(struct sock *sk)
 	}
 
 out:
-	read_unlock(&sk->sk_callback_lock);
+	read_unlock_bh(&sk->sk_callback_lock);
 	if (ready != NULL)
 		ready(sk);
 }
-- 
2.8.0.rc3.226.g39d4020

[PATCH net-next 3/4] rds: tcp: block BH in TCP callbacks

[Eric Dumazet]

TCP stack can now run from process context.

Use read_lock_bh(&sk->sk_callback_lock) variant to restore previous
assumption.

Fixes: 5413d1babe8f ("net: do not block BH while processing socket backlog")
Fixes: d41a69f1d390 ("tcp: make tcp_sendmsg() aware of socket backlog")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/rds/tcp_connect.c | 4 ++--
 net/rds/tcp_listen.c  | 4 ++--
 net/rds/tcp_recv.c    | 4 ++--
 net/rds/tcp_send.c    | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/rds/tcp_connect.c b/net/rds/tcp_connect.c
index 49a3fcfed360..fb82e0a0bf89 100644
--- a/net/rds/tcp_connect.c
+++ b/net/rds/tcp_connect.c
@@ -43,7 +43,7 @@ void rds_tcp_state_change(struct sock *sk)
 	struct rds_connection *conn;
 	struct rds_tcp_connection *tc;
 
-	read_lock(&sk->sk_callback_lock);
+	read_lock_bh(&sk->sk_callback_lock);
 	conn = sk->sk_user_data;
 	if (!conn) {
 		state_change = sk->sk_state_change;
@@ -69,7 +69,7 @@ void rds_tcp_state_change(struct sock *sk)
 			break;
 	}
 out:
-	read_unlock(&sk->sk_callback_lock);
+	read_unlock_bh(&sk->sk_callback_lock);
 	state_change(sk);
 }
 
diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
index be263cdf268b..3fa367945105 100644
--- a/net/rds/tcp_listen.c
+++ b/net/rds/tcp_listen.c
@@ -166,7 +166,7 @@ void rds_tcp_listen_data_ready(struct sock *sk)
 
 	rdsdebug("listen data ready sk %p\n", sk);
 
-	read_lock(&sk->sk_callback_lock);
+	read_lock_bh(&sk->sk_callback_lock);
 	ready = sk->sk_user_data;
 	if (!ready) { /* check for teardown race */
 		ready = sk->sk_data_ready;
@@ -183,7 +183,7 @@ void rds_tcp_listen_data_ready(struct sock *sk)
 		rds_tcp_accept_work(sk);
 
 out:
-	read_unlock(&sk->sk_callback_lock);
+	read_unlock_bh(&sk->sk_callback_lock);
 	ready(sk);
 }
 
diff --git a/net/rds/tcp_recv.c b/net/rds/tcp_recv.c
index d75d8b56a9e3..c3196f9d070a 100644
--- a/net/rds/tcp_recv.c
+++ b/net/rds/tcp_recv.c
@@ -301,7 +301,7 @@ void rds_tcp_data_ready(struct sock *sk)
 
 	rdsdebug("data ready sk %p\n", sk);
 
-	read_lock(&sk->sk_callback_lock);
+	read_lock_bh(&sk->sk_callback_lock);
 	conn = sk->sk_user_data;
 	if (!conn) { /* check for teardown race */
 		ready = sk->sk_data_ready;
@@ -315,7 +315,7 @@ void rds_tcp_data_ready(struct sock *sk)
 	if (rds_tcp_read_sock(conn, GFP_ATOMIC) == -ENOMEM)
 		queue_delayed_work(rds_wq, &conn->c_recv_w, 0);
 out:
-	read_unlock(&sk->sk_callback_lock);
+	read_unlock_bh(&sk->sk_callback_lock);
 	ready(sk);
 }
 
diff --git a/net/rds/tcp_send.c b/net/rds/tcp_send.c
index 2894e6095e3b..22d0f2020a79 100644
--- a/net/rds/tcp_send.c
+++ b/net/rds/tcp_send.c
@@ -180,7 +180,7 @@ void rds_tcp_write_space(struct sock *sk)
 	struct rds_connection *conn;
 	struct rds_tcp_connection *tc;
 
-	read_lock(&sk->sk_callback_lock);
+	read_lock_bh(&sk->sk_callback_lock);
 	conn = sk->sk_user_data;
 	if (!conn) {
 		write_space = sk->sk_write_space;
@@ -200,7 +200,7 @@ void rds_tcp_write_space(struct sock *sk)
 		queue_delayed_work(rds_wq, &conn->c_send_w, 0);
 
 out:
-	read_unlock(&sk->sk_callback_lock);
+	read_unlock_bh(&sk->sk_callback_lock);
 
 	/*
 	 * write_space is only called when data leaves tcp's send queue if
-- 
2.8.0.rc3.226.g39d4020

[PATCH net-next 4/4] tipc: block BH in TCP callbacks

[Eric Dumazet]

TCP stack can now run from process context.

Use read_lock_bh(&sk->sk_callback_lock) variant to restore previous
assumption.

Fixes: 5413d1babe8f ("net: do not block BH while processing socket backlog")
Fixes: d41a69f1d390 ("tcp: make tcp_sendmsg() aware of socket backlog")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jon Maloy <jon.maloy@ericsson.com>
Cc: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/server.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/tipc/server.c b/net/tipc/server.c
index 7a0af2dc0406..272d20a795d5 100644
--- a/net/tipc/server.c
+++ b/net/tipc/server.c
@@ -138,28 +138,28 @@ static void sock_data_ready(struct sock *sk)
 {
 	struct tipc_conn *con;
 
-	read_lock(&sk->sk_callback_lock);
+	read_lock_bh(&sk->sk_callback_lock);
 	con = sock2con(sk);
 	if (con && test_bit(CF_CONNECTED, &con->flags)) {
 		conn_get(con);
 		if (!queue_work(con->server->rcv_wq, &con->rwork))
 			conn_put(con);
 	}
-	read_unlock(&sk->sk_callback_lock);
+	read_unlock_bh(&sk->sk_callback_lock);
 }
 
 static void sock_write_space(struct sock *sk)
 {
 	struct tipc_conn *con;
 
-	read_lock(&sk->sk_callback_lock);
+	read_lock_bh(&sk->sk_callback_lock);
 	con = sock2con(sk);
 	if (con && test_bit(CF_CONNECTED, &con->flags)) {
 		conn_get(con);
 		if (!queue_work(con->server->send_wq, &con->swork))
 			conn_put(con);
 	}
-	read_unlock(&sk->sk_callback_lock);
+	read_unlock_bh(&sk->sk_callback_lock);
 }
 
 static void tipc_register_callbacks(struct socket *sock, struct tipc_conn *con)
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH net-next 1/4] scsi_tcp: block BH in TCP callbacks

[Mike Christie]

On 05/17/2016 07:44 PM, Eric Dumazet wrote:
> iscsi_sw_tcp_data_ready() and iscsi_sw_tcp_state_change() were
> using read_lock(&sk->sk_callback_lock) which is fine if caller
> disabled BH.
> 
> TCP stack no longer has this requirement and can run from
> process context.
> 
> Use read_lock_bh() variant to restore previous assumption.
> 
> Ideally this code could use RCU instead...
> 
> Fixes: 5413d1babe8f ("net: do not block BH while processing socket backlog")
> Fixes: d41a69f1d390 ("tcp: make tcp_sendmsg() aware of socket backlog")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Mike Christie <michaelc@cs.wisc.edu>
> Cc: Venkatesh Srinivas <venkateshs@google.com>
> ---
>  drivers/scsi/iscsi_tcp.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 2e4c82f8329c..ace4f1f41b8e 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -131,10 +131,10 @@ static void iscsi_sw_tcp_data_ready(struct sock *sk)
>  	struct iscsi_tcp_conn *tcp_conn;
>  	read_descriptor_t rd_desc;
>  
> -	read_lock(&sk->sk_callback_lock);
> +	read_lock_bh(&sk->sk_callback_lock);
>  	conn = sk->sk_user_data;
>  	if (!conn) {
> -		read_unlock(&sk->sk_callback_lock);
> +		read_unlock_bh(&sk->sk_callback_lock);
>  		return;
>  	}
>  	tcp_conn = conn->dd_data;
> @@ -154,7 +154,7 @@ static void iscsi_sw_tcp_data_ready(struct sock *sk)
>  	/* If we had to (atomically) map a highmem page,
>  	 * unmap it now. */
>  	iscsi_tcp_segment_unmap(&tcp_conn->in.segment);
> -	read_unlock(&sk->sk_callback_lock);
> +	read_unlock_bh(&sk->sk_callback_lock);
>  }
>  
>  static void iscsi_sw_tcp_state_change(struct sock *sk)
> @@ -165,10 +165,10 @@ static void iscsi_sw_tcp_state_change(struct sock *sk)
>  	struct iscsi_session *session;
>  	void (*old_state_change)(struct sock *);
>  
> -	read_lock(&sk->sk_callback_lock);
> +	read_lock_bh(&sk->sk_callback_lock);
>  	conn = sk->sk_user_data;
>  	if (!conn) {
> -		read_unlock(&sk->sk_callback_lock);
> +		read_unlock_bh(&sk->sk_callback_lock);
>  		return;
>  	}
>  	session = conn->session;
> @@ -179,7 +179,7 @@ static void iscsi_sw_tcp_state_change(struct sock *sk)
>  	tcp_sw_conn = tcp_conn->dd_data;
>  	old_state_change = tcp_sw_conn->old_state_change;
>  
> -	read_unlock(&sk->sk_callback_lock);
> +	read_unlock_bh(&sk->sk_callback_lock);
>  
>  	old_state_change(sk);
>  }

Can I just confirm that nested bh lock calls like:

spin_lock_bh(lock1);
spin_lock_bh(lock2);

do something

spin_unlock_bh(lock2);
spin_unlock_bh(lock1);

is ok? It seems smatch sometimes warns about this.

I found this thread

https://lkml.org/lkml/2011/1/25/232

which says it is ok.

Re: [PATCH net-next 1/4] scsi_tcp: block BH in TCP callbacks

[Eric Dumazet]

On Wed, 2016-05-18 at 12:21 -0500, Mike Christie wrote:

> Can I just confirm that nested bh lock calls like:
> 
> spin_lock_bh(lock1);
> spin_lock_bh(lock2);
> 
> do something
> 
> spin_unlock_bh(lock2);
> spin_unlock_bh(lock1);
> 
> is ok? It seems smatch sometimes warns about this.

It is ok.

More generally

local_bh_disable();
local_bh_disable();

..

local_bh_enable();
local_bh_enable();

is ok, we already have a lot of them.

Re: [PATCH net-next 1/4] scsi_tcp: block BH in TCP callbacks

[Mike Christie]

On 05/17/2016 07:44 PM, Eric Dumazet wrote:
> iscsi_sw_tcp_data_ready() and iscsi_sw_tcp_state_change() were
> using read_lock(&sk->sk_callback_lock) which is fine if caller
> disabled BH.
> 
> TCP stack no longer has this requirement and can run from
> process context.
> 
> Use read_lock_bh() variant to restore previous assumption.
> 
> Ideally this code could use RCU instead...
> 
> Fixes: 5413d1babe8f ("net: do not block BH while processing socket backlog")
> Fixes: d41a69f1d390 ("tcp: make tcp_sendmsg() aware of socket backlog")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Mike Christie <michaelc@cs.wisc.edu>
> Cc: Venkatesh Srinivas <venkateshs@google.com>
> ---
>  drivers/scsi/iscsi_tcp.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 2e4c82f8329c..ace4f1f41b8e 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -131,10 +131,10 @@ static void iscsi_sw_tcp_data_ready(struct sock *sk)
>  	struct iscsi_tcp_conn *tcp_conn;
>  	read_descriptor_t rd_desc;
>  
> -	read_lock(&sk->sk_callback_lock);
> +	read_lock_bh(&sk->sk_callback_lock);
>  	conn = sk->sk_user_data;
>  	if (!conn) {
> -		read_unlock(&sk->sk_callback_lock);
> +		read_unlock_bh(&sk->sk_callback_lock);
>  		return;
>  	}
>  	tcp_conn = conn->dd_data;
> @@ -154,7 +154,7 @@ static void iscsi_sw_tcp_data_ready(struct sock *sk)
>  	/* If we had to (atomically) map a highmem page,
>  	 * unmap it now. */
>  	iscsi_tcp_segment_unmap(&tcp_conn->in.segment);
> -	read_unlock(&sk->sk_callback_lock);
> +	read_unlock_bh(&sk->sk_callback_lock);
>  }
>  
>  static void iscsi_sw_tcp_state_change(struct sock *sk)
> @@ -165,10 +165,10 @@ static void iscsi_sw_tcp_state_change(struct sock *sk)
>  	struct iscsi_session *session;
>  	void (*old_state_change)(struct sock *);
>  
> -	read_lock(&sk->sk_callback_lock);
> +	read_lock_bh(&sk->sk_callback_lock);
>  	conn = sk->sk_user_data;
>  	if (!conn) {
> -		read_unlock(&sk->sk_callback_lock);
> +		read_unlock_bh(&sk->sk_callback_lock);
>  		return;
>  	}
>  	session = conn->session;
> @@ -179,7 +179,7 @@ static void iscsi_sw_tcp_state_change(struct sock *sk)
>  	tcp_sw_conn = tcp_conn->dd_data;
>  	old_state_change = tcp_sw_conn->old_state_change;
>  
> -	read_unlock(&sk->sk_callback_lock);
> +	read_unlock_bh(&sk->sk_callback_lock);
>  
>  	old_state_change(sk);
>  }
> 

Reviewed and tested. Thanks

Acked-by: Mike Christie <michaelc@cs.wisc.edu>

Re: [PATCH net-next 1/4] scsi_tcp: block BH in TCP callbacks

[Eric Dumazet]

On Wed, 2016-05-18 at 15:48 -0500, Mike Christie wrote:

> Reviewed and tested. Thanks
> 
> Acked-by: Mike Christie <michaelc@cs.wisc.edu>

Excellent, thanks Mike !

Re: [PATCH net-next 0/4] net: block BH in TCP callbacks

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Tue, 17 May 2016 17:44:05 -0700

> Four layers using TCP stack were assuming sk_callback_lock could
> be locked using read_lock() in their handlers because TCP stack
> was running with BH disabled.
> 
> This is no longer the case. Since presumably the rest could
> also depend on BH being disabled, just use read_lock_bh().
> 
> Then each layer might consider switching to RCU protection
> and no longer depend on BH.

Series applied, thanks Eric.