| Message ID | 20220308002747.122682-12-michael.christie@oracle.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | misc iscsi patches | expand |
On 3/7/22 16:27, Mike Christie wrote: > The conn_send_pdu API is evil in that it returns a pointer to a > iscsi_task, but that task might have been freed already so you can't > touch it. This patch splits the task allocation and transmission, so > functions like iscsi_send_nopout can access the task before its sent and > do whatever book keeping is needed before it's sent. > > Signed-off-by: Mike Christie <michael.christie@oracle.com> > --- > drivers/scsi/libiscsi.c | 85 ++++++++++++++++++++++++++++++----------- > include/scsi/libiscsi.h | 3 -- > 2 files changed, 62 insertions(+), 26 deletions(-) > > diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c > index cde389225059..a165d4d10cea 100644 > --- a/drivers/scsi/libiscsi.c > +++ b/drivers/scsi/libiscsi.c > @@ -695,12 +695,18 @@ static int iscsi_prep_mgmt_task(struct iscsi_conn *conn, > return 0; > } > > +/** > + * iscsi_alloc_mgmt_task - allocate and setup a mgmt task. > + * @conn: iscsi conn that the task will be sent on. > + * @hdr: iscsi pdu that will be sent. > + * @data: buffer for data segment if needed. > + * @data_size: length of data in bytes. > + */ > static struct iscsi_task * > -__iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr, > +iscsi_alloc_mgmt_task(struct iscsi_conn *conn, struct iscsi_hdr *hdr, > char *data, uint32_t data_size) > { > struct iscsi_session *session = conn->session; > - struct iscsi_host *ihost = shost_priv(session->host); > uint8_t opcode = hdr->opcode & ISCSI_OPCODE_MASK; > struct iscsi_task *task; > itt_t itt; > @@ -780,28 +786,57 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr, > task->conn->session->age); > } > > - if (unlikely(READ_ONCE(conn->ping_task) == INVALID_SCSI_TASK)) > - WRITE_ONCE(conn->ping_task, task); > + return task; > + > +free_task: > + iscsi_put_task(task); > + return NULL; > +} > + > +/** > + * iscsi_send_mgmt_task - Send task created with iscsi_alloc_mgmt_task. > + * @task: iscsi task to send. > + * > + * On failure this returns a non-zero error code, and the driver must free > + * the task with iscsi_put_task; > + */ > +static int iscsi_send_mgmt_task(struct iscsi_task *task) > +{ > + struct iscsi_conn *conn = task->conn; > + struct iscsi_session *session = conn->session; > + struct iscsi_host *ihost = shost_priv(conn->session->host); > + int rc = 0; > > if (!ihost->workq) { > - if (iscsi_prep_mgmt_task(conn, task)) > - goto free_task; > + rc = iscsi_prep_mgmt_task(conn, task); > + if (rc) > + return rc; > > - if (session->tt->xmit_task(task)) > - goto free_task; > + rc = session->tt->xmit_task(task); > + if (rc) > + return rc; > } else { > list_add_tail(&task->running, &conn->mgmtqueue); > iscsi_conn_queue_xmit(conn); > } > > - return task; > + return 0; > +} > > -free_task: > - /* regular RX path uses back_lock */ > - spin_lock(&session->back_lock); > - __iscsi_put_task(task); > - spin_unlock(&session->back_lock); > - return NULL; > +static int __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr, > + char *data, uint32_t data_size) > +{ > + struct iscsi_task *task; > + int rc; > + > + task = iscsi_alloc_mgmt_task(conn, hdr, data, data_size); > + if (!task) > + return -ENOMEM; > + > + rc = iscsi_send_mgmt_task(task); > + if (rc) > + iscsi_put_task(task); > + return rc; > } > > int iscsi_conn_send_pdu(struct iscsi_cls_conn *cls_conn, struct iscsi_hdr *hdr, > @@ -812,7 +847,7 @@ int iscsi_conn_send_pdu(struct iscsi_cls_conn *cls_conn, struct iscsi_hdr *hdr, > int err = 0; > > spin_lock_bh(&session->frwd_lock); > - if (!__iscsi_conn_send_pdu(conn, hdr, data, data_size)) > + if (__iscsi_conn_send_pdu(conn, hdr, data, data_size)) > err = -EPERM; > spin_unlock_bh(&session->frwd_lock); > return err; > @@ -985,7 +1020,6 @@ static int iscsi_send_nopout(struct iscsi_conn *conn, struct iscsi_nopin *rhdr) > if (!rhdr) { > if (READ_ONCE(conn->ping_task)) > return -EINVAL; > - WRITE_ONCE(conn->ping_task, INVALID_SCSI_TASK); > } > > memset(&hdr, 0, sizeof(struct iscsi_nopout)); > @@ -999,10 +1033,18 @@ static int iscsi_send_nopout(struct iscsi_conn *conn, struct iscsi_nopin *rhdr) > } else > hdr.ttt = RESERVED_ITT; > > - task = __iscsi_conn_send_pdu(conn, (struct iscsi_hdr *)&hdr, NULL, 0); > - if (!task) { > + task = iscsi_alloc_mgmt_task(conn, (struct iscsi_hdr *)&hdr, NULL, 0); > + if (!task) > + return -ENOMEM; > + > + if (!rhdr) > + WRITE_ONCE(conn->ping_task, task); > + > + if (iscsi_send_mgmt_task(task)) { > if (!rhdr) > WRITE_ONCE(conn->ping_task, NULL); > + iscsi_put_task(task); > + > iscsi_conn_printk(KERN_ERR, conn, "Could not send nopout\n"); > return -EIO; > } else if (!rhdr) { > @@ -1869,11 +1911,8 @@ static int iscsi_exec_task_mgmt_fn(struct iscsi_conn *conn, > __must_hold(&session->frwd_lock) > { > struct iscsi_session *session = conn->session; > - struct iscsi_task *task; > > - task = __iscsi_conn_send_pdu(conn, (struct iscsi_hdr *)hdr, > - NULL, 0); > - if (!task) { > + if (__iscsi_conn_send_pdu(conn, (struct iscsi_hdr *)hdr, NULL, 0)) { > spin_unlock_bh(&session->frwd_lock); > iscsi_conn_printk(KERN_ERR, conn, "Could not send TMF.\n"); > iscsi_conn_failure(conn, ISCSI_ERR_CONN_FAILED); > diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h > index 9032a214104c..412722f44747 100644 > --- a/include/scsi/libiscsi.h > +++ b/include/scsi/libiscsi.h > @@ -134,9 +134,6 @@ struct iscsi_task { > void *dd_data; /* driver/transport data */ > }; > > -/* invalid scsi_task pointer */ > -#define INVALID_SCSI_TASK (struct iscsi_task *)-1l > - > static inline int iscsi_task_has_unsol_data(struct iscsi_task *task) > { > return task->unsol_r2t.data_length > task->unsol_r2t.sent; Reviewed-by: Lee Duncan <lduncan@suse.com>
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c index cde389225059..a165d4d10cea 100644 --- a/drivers/scsi/libiscsi.c +++ b/drivers/scsi/libiscsi.c @@ -695,12 +695,18 @@ static int iscsi_prep_mgmt_task(struct iscsi_conn *conn, return 0; } +/** + * iscsi_alloc_mgmt_task - allocate and setup a mgmt task. + * @conn: iscsi conn that the task will be sent on. + * @hdr: iscsi pdu that will be sent. + * @data: buffer for data segment if needed. + * @data_size: length of data in bytes. + */ static struct iscsi_task * -__iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr, +iscsi_alloc_mgmt_task(struct iscsi_conn *conn, struct iscsi_hdr *hdr, char *data, uint32_t data_size) { struct iscsi_session *session = conn->session; - struct iscsi_host *ihost = shost_priv(session->host); uint8_t opcode = hdr->opcode & ISCSI_OPCODE_MASK; struct iscsi_task *task; itt_t itt; @@ -780,28 +786,57 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr, task->conn->session->age); } - if (unlikely(READ_ONCE(conn->ping_task) == INVALID_SCSI_TASK)) - WRITE_ONCE(conn->ping_task, task); + return task; + +free_task: + iscsi_put_task(task); + return NULL; +} + +/** + * iscsi_send_mgmt_task - Send task created with iscsi_alloc_mgmt_task. + * @task: iscsi task to send. + * + * On failure this returns a non-zero error code, and the driver must free + * the task with iscsi_put_task; + */ +static int iscsi_send_mgmt_task(struct iscsi_task *task) +{ + struct iscsi_conn *conn = task->conn; + struct iscsi_session *session = conn->session; + struct iscsi_host *ihost = shost_priv(conn->session->host); + int rc = 0; if (!ihost->workq) { - if (iscsi_prep_mgmt_task(conn, task)) - goto free_task; + rc = iscsi_prep_mgmt_task(conn, task); + if (rc) + return rc; - if (session->tt->xmit_task(task)) - goto free_task; + rc = session->tt->xmit_task(task); + if (rc) + return rc; } else { list_add_tail(&task->running, &conn->mgmtqueue); iscsi_conn_queue_xmit(conn); } - return task; + return 0; +} -free_task: - /* regular RX path uses back_lock */ - spin_lock(&session->back_lock); - __iscsi_put_task(task); - spin_unlock(&session->back_lock); - return NULL; +static int __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr, + char *data, uint32_t data_size) +{ + struct iscsi_task *task; + int rc; + + task = iscsi_alloc_mgmt_task(conn, hdr, data, data_size); + if (!task) + return -ENOMEM; + + rc = iscsi_send_mgmt_task(task); + if (rc) + iscsi_put_task(task); + return rc; } int iscsi_conn_send_pdu(struct iscsi_cls_conn *cls_conn, struct iscsi_hdr *hdr, @@ -812,7 +847,7 @@ int iscsi_conn_send_pdu(struct iscsi_cls_conn *cls_conn, struct iscsi_hdr *hdr, int err = 0; spin_lock_bh(&session->frwd_lock); - if (!__iscsi_conn_send_pdu(conn, hdr, data, data_size)) + if (__iscsi_conn_send_pdu(conn, hdr, data, data_size)) err = -EPERM; spin_unlock_bh(&session->frwd_lock); return err; @@ -985,7 +1020,6 @@ static int iscsi_send_nopout(struct iscsi_conn *conn, struct iscsi_nopin *rhdr) if (!rhdr) { if (READ_ONCE(conn->ping_task)) return -EINVAL; - WRITE_ONCE(conn->ping_task, INVALID_SCSI_TASK); } memset(&hdr, 0, sizeof(struct iscsi_nopout)); @@ -999,10 +1033,18 @@ static int iscsi_send_nopout(struct iscsi_conn *conn, struct iscsi_nopin *rhdr) } else hdr.ttt = RESERVED_ITT; - task = __iscsi_conn_send_pdu(conn, (struct iscsi_hdr *)&hdr, NULL, 0); - if (!task) { + task = iscsi_alloc_mgmt_task(conn, (struct iscsi_hdr *)&hdr, NULL, 0); + if (!task) + return -ENOMEM; + + if (!rhdr) + WRITE_ONCE(conn->ping_task, task); + + if (iscsi_send_mgmt_task(task)) { if (!rhdr) WRITE_ONCE(conn->ping_task, NULL); + iscsi_put_task(task); + iscsi_conn_printk(KERN_ERR, conn, "Could not send nopout\n"); return -EIO; } else if (!rhdr) { @@ -1869,11 +1911,8 @@ static int iscsi_exec_task_mgmt_fn(struct iscsi_conn *conn, __must_hold(&session->frwd_lock) { struct iscsi_session *session = conn->session; - struct iscsi_task *task; - task = __iscsi_conn_send_pdu(conn, (struct iscsi_hdr *)hdr, - NULL, 0); - if (!task) { + if (__iscsi_conn_send_pdu(conn, (struct iscsi_hdr *)hdr, NULL, 0)) { spin_unlock_bh(&session->frwd_lock); iscsi_conn_printk(KERN_ERR, conn, "Could not send TMF.\n"); iscsi_conn_failure(conn, ISCSI_ERR_CONN_FAILED); diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h index 9032a214104c..412722f44747 100644 --- a/include/scsi/libiscsi.h +++ b/include/scsi/libiscsi.h @@ -134,9 +134,6 @@ struct iscsi_task { void *dd_data; /* driver/transport data */ }; -/* invalid scsi_task pointer */ -#define INVALID_SCSI_TASK (struct iscsi_task *)-1l - static inline int iscsi_task_has_unsol_data(struct iscsi_task *task) { return task->unsol_r2t.data_length > task->unsol_r2t.sent;
The conn_send_pdu API is evil in that it returns a pointer to a iscsi_task, but that task might have been freed already so you can't touch it. This patch splits the task allocation and transmission, so functions like iscsi_send_nopout can access the task before its sent and do whatever book keeping is needed before it's sent. Signed-off-by: Mike Christie <michael.christie@oracle.com> --- drivers/scsi/libiscsi.c | 85 ++++++++++++++++++++++++++++++----------- include/scsi/libiscsi.h | 3 -- 2 files changed, 62 insertions(+), 26 deletions(-)