On 8/2/22 6:23 AM, lijinlin (A) wrote: > So sorry, this patch has problem, please ignore. > Was the issue the fget use? I know I gave the suggestion to do the get, but seeing it now makes me think I was wrong and it's getting too messy. Let's just add a mutex for getting/setting the tcp_sw_conn->sock in the non-io paths (io paths are flushed/locked already). Something like this (patch is only compile tested): diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c index 9fee70d6434a..c1696472965e 100644 --- a/drivers/scsi/iscsi_tcp.c +++ b/drivers/scsi/iscsi_tcp.c @@ -558,6 +558,8 @@ iscsi_sw_tcp_conn_create(struct iscsi_cls_session *cls_session, tcp_conn = conn->dd_data; tcp_sw_conn = tcp_conn->dd_data; + mutex_init(&tcp_sw_conn->sock_lock); + tfm = crypto_alloc_ahash("crc32c", 0, CRYPTO_ALG_ASYNC); if (IS_ERR(tfm)) goto free_conn; @@ -592,11 +594,15 @@ iscsi_sw_tcp_conn_create(struct iscsi_cls_session *cls_session, static void iscsi_sw_tcp_release_conn(struct iscsi_conn *conn) { - struct iscsi_session *session = conn->session; struct iscsi_tcp_conn *tcp_conn = conn->dd_data; struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data; struct socket *sock = tcp_sw_conn->sock; + /* + * The iscsi transport class will make sure we are not called in + * parallel with start, stop, bind and destroys. However, this can be + * called twice if userspace does a stop then a destroy. + */ if (!sock) return; @@ -610,9 +616,9 @@ static void iscsi_sw_tcp_release_conn(struct iscsi_conn *conn) iscsi_sw_tcp_conn_restore_callbacks(conn); sock_put(sock->sk); - spin_lock_bh(&session->frwd_lock); + mutex_lock(&tcp_sw_conn->sock_lock); tcp_sw_conn->sock = NULL; - spin_unlock_bh(&session->frwd_lock); + mutex_unlock(&tcp_sw_conn->sock_lock); sockfd_put(sock); } @@ -664,7 +670,6 @@ iscsi_sw_tcp_conn_bind(struct iscsi_cls_session *cls_session, struct iscsi_cls_conn *cls_conn, uint64_t transport_eph, int is_leading) { - struct iscsi_session *session = cls_session->dd_data; struct iscsi_conn *conn = cls_conn->dd_data; struct iscsi_tcp_conn *tcp_conn = conn->dd_data; struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data; @@ -684,10 +689,10 @@ iscsi_sw_tcp_conn_bind(struct iscsi_cls_session *cls_session, if (err) goto free_socket; - spin_lock_bh(&session->frwd_lock); + mutex_lock(&tcp_sw_conn->sock_lock); /* bind iSCSI connection and socket */ tcp_sw_conn->sock = sock; - spin_unlock_bh(&session->frwd_lock); + mutex_unlock(&tcp_sw_conn->sock_lock); /* setup Socket parameters */ sk = sock->sk; @@ -724,8 +729,15 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn, break; case ISCSI_PARAM_DATADGST_EN: iscsi_set_param(cls_conn, param, buf, buflen); + + mutex_lock(&tcp_sw_conn->sock_lock); + if (!tcp_sw_conn->sock) { + mutex_unlock(&tcp_sw_conn->sock_lock); + return -ENOTCONN; + } tcp_sw_conn->sendpage = conn->datadgst_en ? sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage; + mutex_unlock(&tcp_sw_conn->sock_lock); break; case ISCSI_PARAM_MAX_R2T: return iscsi_tcp_set_max_r2t(conn, buf); @@ -750,14 +762,20 @@ static int iscsi_sw_tcp_conn_get_param(struct iscsi_cls_conn *cls_conn, case ISCSI_PARAM_CONN_PORT: case ISCSI_PARAM_CONN_ADDRESS: case ISCSI_PARAM_LOCAL_PORT: - spin_lock_bh(&conn->session->frwd_lock); - if (!tcp_sw_conn || !tcp_sw_conn->sock) { - spin_unlock_bh(&conn->session->frwd_lock); + /* + * The conn's sysfs interface is exposed to userspace after + * the tcp_sw_conn is setup, and the netlink interface will + * make sure we don't do a get_param while setup is running. + * We do need to make sure a user is not accessing sysfs while + * the netlink interface is releasing the sock via + * iscsi_sw_tcp_release_conn. + */ + mutex_lock(&tcp_sw_conn->sock_lock); + sock = tcp_sw_conn->sock; + if (!sock) { + mutex_unlock(&tcp_sw_conn->sock_lock); return -ENOTCONN; } - sock = tcp_sw_conn->sock; - sock_hold(sock->sk); - spin_unlock_bh(&conn->session->frwd_lock); if (param == ISCSI_PARAM_LOCAL_PORT) rc = kernel_getsockname(sock, @@ -765,7 +783,7 @@ static int iscsi_sw_tcp_conn_get_param(struct iscsi_cls_conn *cls_conn, else rc = kernel_getpeername(sock, (struct sockaddr *)&addr); - sock_put(sock->sk); + mutex_unlock(&tcp_sw_conn->sock_lock); if (rc < 0) return rc; @@ -803,17 +821,25 @@ static int iscsi_sw_tcp_host_get_param(struct Scsi_Host *shost, } tcp_conn = conn->dd_data; tcp_sw_conn = tcp_conn->dd_data; + /* + * If the leadconn is bound then setup has completed and destroy + * has not been run yet. Grab a ref to the conn incase a destroy + * starts to run after we drop the fwrd_lock. + */ + iscsi_get_conn(conn->cls_conn); + spin_unlock_bh(&session->frwd_lock); + + mutex_lock(&tcp_sw_conn->sock_lock); sock = tcp_sw_conn->sock; if (!sock) { - spin_unlock_bh(&session->frwd_lock); + mutex_unlock(&tcp_sw_conn->sock_lock); + iscsi_put_conn(conn->cls_conn); return -ENOTCONN; } - sock_hold(sock->sk); - spin_unlock_bh(&session->frwd_lock); - - rc = kernel_getsockname(sock, - (struct sockaddr *)&addr); - sock_put(sock->sk); + + rc = kernel_getsockname(sock, (struct sockaddr *)&addr); + mutex_unlock(&tcp_sw_conn->sock_lock); + iscsi_put_conn(conn->cls_conn); if (rc < 0) return rc; diff --git a/drivers/scsi/iscsi_tcp.h b/drivers/scsi/iscsi_tcp.h index 791453195099..7c6f90ce6124 100644 --- a/drivers/scsi/iscsi_tcp.h +++ b/drivers/scsi/iscsi_tcp.h @@ -28,6 +28,8 @@ struct iscsi_sw_tcp_send { struct iscsi_sw_tcp_conn { struct socket *sock; + /* Taken when accesing the sock from the netlink/sysfs interface */ + struct mutex sock_lock; struct iscsi_sw_tcp_send out; /* old values for socket callbacks */