diff mbox series

[V4] iscsi: Do Not set param when sock is NULL

Message ID 20210310062651.179792-1-gulam.mohamed@oracle.com
State Superseded
Headers show
Series [V4] iscsi: Do Not set param when sock is NULL | expand

Commit Message

Gulam Mohamed March 10, 2021, 6:26 a.m. UTC 
Description

Comments

Mike Christie March 22, 2021, 7:22 p.m. UTC | #1 
On 3/10/21 12:26 AM, Gulam Mohamed wrote:
> Description

> ===========

> 1. This Kernel panic could be due to a timing issue when there is a

>    race between the sync thread and the initiator was processing of

>    a login response from the target. The session re-open can be invoked

>    from two places:

>     a. Sessions sync thread when the iscsid restart

>     b. From iscsid through iscsi error handler

> 2. The session reopen sequence is as follows in user-space

> 	a. Disconnect the connection

> 	b. Then send the stop connection request to the kernel

> 	   which releases the connection (releases the socket)

> 	c. Queues the reopen for 2 seconds delay

> 	d. Once the delay expires, create the TCP connection again by

>            calling the connect() call

> 	e. Poll for the connection

> 	f. When poll is successful i.e when the TCP connection is

> 	   established, it performs:

> 	       i. Creation of session and connection data structures

> 	      ii. Bind the connection to the session. This is the place

> 		  where we assign the sock to tcp_sw_conn->sock

>              iii. Sets parameters like target name, persistent address

>               iv. Creates the login pdu

> 	       v. Sends the login pdu to kernel

> 	      vi. Returns to the main loop to process further events.

> 	          The kernel then sends the login request over to the

> 	          target node

> 	g. Once login response with success is received, it enters full

> 	   feature phase and sets the negotiable parameters like

> 	   max_recv_data_length,max_transmit_length, data_digest etc.

> 3. While setting the negotiable parameters by calling

>    "iscsi_session_set_neg_params()", kernel panicked as sock was NULL

> 

> What happened here is

> ---------------------

> 1. Before initiator received the login response mentioned in above

>    point 2.f.v, another reopen request was sent from the error

>    handler/sync session for the same session, as the initiator utils

>    was in main loop to process further events (as mentioned in point

>    2.f.vi above).

> 2. While processing this reopen, it stopped the connection which

>    released the socket and queued this connection and at this point

>    of time the login response was received for the earlier on

> 

> Fix

> ---

> 1. Add new connection state ISCSI_CONN_BOUND in "enum iscsi_connection

>    _state"

> 2. Set the connection state value to ISCSI_CONN_DOWN upon

>    iscsi_if_ep_disconnect() and iscsi_if_stop_conn()

> 3. Set the connection state to the newly created value ISCSI_CONN_BOUND

>    after bind connection (transport->bind_conn())

> 4. In iscsi_set_param, return -ENOTCONN if the connection state is not

>    either ISCSI_CONN_BOUND or ISCSI_CONN_UP

> 

> Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>

> ---

>  drivers/scsi/scsi_transport_iscsi.c | 16 +++++++++++++---

>  include/scsi/scsi_transport_iscsi.h |  1 +

>  2 files changed, 14 insertions(+), 3 deletions(-)

> 

> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c

> index 91074fd97f64..19375f1ba4b2 100644

> --- a/drivers/scsi/scsi_transport_iscsi.c

> +++ b/drivers/scsi/scsi_transport_iscsi.c

> @@ -2475,6 +2475,7 @@ static void iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)

>  	 */

>  	mutex_lock(&conn_mutex);

>  	conn->transport->stop_conn(conn, flag);

> +	conn->state = ISCSI_CONN_DOWN;

>  	mutex_unlock(&conn_mutex);

>  

>  }

> @@ -2899,8 +2900,13 @@ iscsi_set_param(struct iscsi_transport *transport, struct iscsi_uevent *ev)

>  			session->recovery_tmo = value;

>  		break;

>  	default:

> -		err = transport->set_param(conn, ev->u.set_param.param,

> -					   data, ev->u.set_param.len);

> +		if ((conn->state == ISCSI_CONN_BOUND) ||

> +		        (conn->state == ISCSI_CONN_UP)) {

> +		        err = transport->set_param(conn, ev->u.set_param.param,

> +						data, ev->u.set_param.len);

> +		}

> +		else


The else should go on the line above

} else

and I'm not 100% sure but some people prefer:

} else {
	return -ENOTCONN;
}

because the first chunk has {}s so this balances it.
Lee Duncan March 23, 2021, 12:11 a.m. UTC | #2 
On 3/22/21 12:22 PM, Mike Christie wrote:
> On 3/10/21 12:26 AM, Gulam Mohamed wrote:

>> Description

>> ===========

>> 1. This Kernel panic could be due to a timing issue when there is a

>>    race between the sync thread and the initiator was processing of

>>    a login response from the target. The session re-open can be invoked

>>    from two places:

>>     a. Sessions sync thread when the iscsid restart

>>     b. From iscsid through iscsi error handler

>> 2. The session reopen sequence is as follows in user-space

>> 	a. Disconnect the connection

>> 	b. Then send the stop connection request to the kernel

>> 	   which releases the connection (releases the socket)

>> 	c. Queues the reopen for 2 seconds delay

>> 	d. Once the delay expires, create the TCP connection again by

>>            calling the connect() call

>> 	e. Poll for the connection

>> 	f. When poll is successful i.e when the TCP connection is

>> 	   established, it performs:

>> 	       i. Creation of session and connection data structures

>> 	      ii. Bind the connection to the session. This is the place

>> 		  where we assign the sock to tcp_sw_conn->sock

>>              iii. Sets parameters like target name, persistent address

>>               iv. Creates the login pdu

>> 	       v. Sends the login pdu to kernel

>> 	      vi. Returns to the main loop to process further events.

>> 	          The kernel then sends the login request over to the

>> 	          target node

>> 	g. Once login response with success is received, it enters full

>> 	   feature phase and sets the negotiable parameters like

>> 	   max_recv_data_length,max_transmit_length, data_digest etc.

>> 3. While setting the negotiable parameters by calling

>>    "iscsi_session_set_neg_params()", kernel panicked as sock was NULL

>>

>> What happened here is

>> ---------------------

>> 1. Before initiator received the login response mentioned in above

>>    point 2.f.v, another reopen request was sent from the error

>>    handler/sync session for the same session, as the initiator utils

>>    was in main loop to process further events (as mentioned in point

>>    2.f.vi above).

>> 2. While processing this reopen, it stopped the connection which

>>    released the socket and queued this connection and at this point

>>    of time the login response was received for the earlier on

>>

>> Fix

>> ---

>> 1. Add new connection state ISCSI_CONN_BOUND in "enum iscsi_connection

>>    _state"

>> 2. Set the connection state value to ISCSI_CONN_DOWN upon

>>    iscsi_if_ep_disconnect() and iscsi_if_stop_conn()

>> 3. Set the connection state to the newly created value ISCSI_CONN_BOUND

>>    after bind connection (transport->bind_conn())

>> 4. In iscsi_set_param, return -ENOTCONN if the connection state is not

>>    either ISCSI_CONN_BOUND or ISCSI_CONN_UP

>>

>> Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>

>> ---

>>  drivers/scsi/scsi_transport_iscsi.c | 16 +++++++++++++---

>>  include/scsi/scsi_transport_iscsi.h |  1 +

>>  2 files changed, 14 insertions(+), 3 deletions(-)

>>

>> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c

>> index 91074fd97f64..19375f1ba4b2 100644

>> --- a/drivers/scsi/scsi_transport_iscsi.c

>> +++ b/drivers/scsi/scsi_transport_iscsi.c

>> @@ -2475,6 +2475,7 @@ static void iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)

>>  	 */

>>  	mutex_lock(&conn_mutex);

>>  	conn->transport->stop_conn(conn, flag);

>> +	conn->state = ISCSI_CONN_DOWN;

>>  	mutex_unlock(&conn_mutex);

>>  

>>  }

>> @@ -2899,8 +2900,13 @@ iscsi_set_param(struct iscsi_transport *transport, struct iscsi_uevent *ev)

>>  			session->recovery_tmo = value;

>>  		break;

>>  	default:

>> -		err = transport->set_param(conn, ev->u.set_param.param,

>> -					   data, ev->u.set_param.len);

>> +		if ((conn->state == ISCSI_CONN_BOUND) ||

>> +		        (conn->state == ISCSI_CONN_UP)) {

>> +		        err = transport->set_param(conn, ev->u.set_param.param,

>> +						data, ev->u.set_param.len);

>> +		}

>> +		else

> 

> The else should go on the line above

> 

> } else

> 

> and I'm not 100% sure but some people prefer:

> 

> } else {

> 	return -ENOTCONN;

> }

> 

> because the first chunk has {}s so this balances it.

> 


I actually prefer brackets all the time :O, so I'm good with that too.
diff mbox series

Patch

===========
1. This Kernel panic could be due to a timing issue when there is a
   race between the sync thread and the initiator was processing of
   a login response from the target. The session re-open can be invoked
   from two places:
    a. Sessions sync thread when the iscsid restart
    b. From iscsid through iscsi error handler
2. The session reopen sequence is as follows in user-space
	a. Disconnect the connection
	b. Then send the stop connection request to the kernel
	   which releases the connection (releases the socket)
	c. Queues the reopen for 2 seconds delay
	d. Once the delay expires, create the TCP connection again by
           calling the connect() call
	e. Poll for the connection
	f. When poll is successful i.e when the TCP connection is
	   established, it performs:
	       i. Creation of session and connection data structures
	      ii. Bind the connection to the session. This is the place
		  where we assign the sock to tcp_sw_conn->sock
             iii. Sets parameters like target name, persistent address
              iv. Creates the login pdu
	       v. Sends the login pdu to kernel
	      vi. Returns to the main loop to process further events.
	          The kernel then sends the login request over to the
	          target node
	g. Once login response with success is received, it enters full
	   feature phase and sets the negotiable parameters like
	   max_recv_data_length,max_transmit_length, data_digest etc.
3. While setting the negotiable parameters by calling
   "iscsi_session_set_neg_params()", kernel panicked as sock was NULL

What happened here is
---------------------
1. Before initiator received the login response mentioned in above
   point 2.f.v, another reopen request was sent from the error
   handler/sync session for the same session, as the initiator utils
   was in main loop to process further events (as mentioned in point
   2.f.vi above).
2. While processing this reopen, it stopped the connection which
   released the socket and queued this connection and at this point
   of time the login response was received for the earlier on

Fix
---
1. Add new connection state ISCSI_CONN_BOUND in "enum iscsi_connection
   _state"
2. Set the connection state value to ISCSI_CONN_DOWN upon
   iscsi_if_ep_disconnect() and iscsi_if_stop_conn()
3. Set the connection state to the newly created value ISCSI_CONN_BOUND
   after bind connection (transport->bind_conn())
4. In iscsi_set_param, return -ENOTCONN if the connection state is not
   either ISCSI_CONN_BOUND or ISCSI_CONN_UP

Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>
---
 drivers/scsi/scsi_transport_iscsi.c | 16 +++++++++++++---
 include/scsi/scsi_transport_iscsi.h |  1 +
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 91074fd97f64..19375f1ba4b2 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2475,6 +2475,7 @@  static void iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)
 	 */
 	mutex_lock(&conn_mutex);
 	conn->transport->stop_conn(conn, flag);
+	conn->state = ISCSI_CONN_DOWN;
 	mutex_unlock(&conn_mutex);
 
 }
@@ -2899,8 +2900,13 @@  iscsi_set_param(struct iscsi_transport *transport, struct iscsi_uevent *ev)
 			session->recovery_tmo = value;
 		break;
 	default:
-		err = transport->set_param(conn, ev->u.set_param.param,
-					   data, ev->u.set_param.len);
+		if ((conn->state == ISCSI_CONN_BOUND) ||
+		        (conn->state == ISCSI_CONN_UP)) {
+		        err = transport->set_param(conn, ev->u.set_param.param,
+						data, ev->u.set_param.len);
+		}
+		else
+		        return -ENOTCONN;
 	}
 
 	return err;
@@ -2960,6 +2966,7 @@  static int iscsi_if_ep_disconnect(struct iscsi_transport *transport,
 		mutex_lock(&conn->ep_mutex);
 		conn->ep = NULL;
 		mutex_unlock(&conn->ep_mutex);
+		conn->state = ISCSI_CONN_DOWN;
 	}
 
 	transport->ep_disconnect(ep);
@@ -3727,6 +3734,8 @@  iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
 		ev->r.retcode =	transport->bind_conn(session, conn,
 						ev->u.b_conn.transport_eph,
 						ev->u.b_conn.is_leading);
+		if (!ev->r.retcode)
+			conn->state = ISCSI_CONN_BOUND;
 		mutex_unlock(&conn_mutex);
 
 		if (ev->r.retcode || !transport->ep_connect)
@@ -3966,7 +3975,8 @@  iscsi_conn_attr(local_ipaddr, ISCSI_PARAM_LOCAL_IPADDR);
 static const char *const connection_state_names[] = {
 	[ISCSI_CONN_UP] = "up",
 	[ISCSI_CONN_DOWN] = "down",
-	[ISCSI_CONN_FAILED] = "failed"
+	[ISCSI_CONN_FAILED] = "failed",
+	[ISCSI_CONN_BOUND] = "bound"
 };
 
 static ssize_t show_conn_state(struct device *dev,
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index 8a26a2ffa952..fc5a39839b4b 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -193,6 +193,7 @@  enum iscsi_connection_state {
 	ISCSI_CONN_UP = 0,
 	ISCSI_CONN_DOWN,
 	ISCSI_CONN_FAILED,
+	ISCSI_CONN_BOUND,
 };
 
 struct iscsi_cls_conn {