* Re: [PATCH 2/9] libiscsi: drop taskqueuelock
@ 2021-02-03 7:39 kernel test robot
0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2021-02-03 7:39 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 11581 bytes --]
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210203013356.11177-3-michael.christie@oracle.com>
References: <20210203013356.11177-3-michael.christie@oracle.com>
TO: Mike Christie <michael.christie@oracle.com>
TO: lduncan(a)suse.com
TO: cleech(a)redhat.com
TO: martin.petersen(a)oracle.com
TO: linux-scsi(a)vger.kernel.org
TO: james.bottomley(a)hansenpartnership.com
CC: lutianxiong(a)huawei.com
CC: linfeilong(a)huawei.com
CC: liuzhiqiang26(a)huawei.com
CC: haowenchao(a)huawei.com
CC: Mike Christie <michael.christie@oracle.com>
Hi Mike,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on scsi/for-next]
[also build test WARNING on mkp-scsi/for-next v5.11-rc6 next-20210125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Mike-Christie/iscsi-fixes-and-cleanups/20210203-122757
base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
:::::: branch date: 3 hours ago
:::::: commit date: 3 hours ago
config: i386-randconfig-m021-20210202 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/scsi/libiscsi_tcp.c:586 iscsi_tcp_r2t_rsp() warn: variable dereferenced before check 'task->sc' (see line 547)
vim +586 drivers/scsi/libiscsi_tcp.c
a081c13e39b5c1 Mike Christie 2008-12-02 523
a081c13e39b5c1 Mike Christie 2008-12-02 524 /**
a081c13e39b5c1 Mike Christie 2008-12-02 525 * iscsi_tcp_r2t_rsp - iSCSI R2T Response processing
a081c13e39b5c1 Mike Christie 2008-12-02 526 * @conn: iscsi connection
f7dbf0662a0167 Mike Christie 2021-02-02 527 * @hdr: PDU header
a081c13e39b5c1 Mike Christie 2008-12-02 528 */
f7dbf0662a0167 Mike Christie 2021-02-02 529 static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
a081c13e39b5c1 Mike Christie 2008-12-02 530 {
a081c13e39b5c1 Mike Christie 2008-12-02 531 struct iscsi_session *session = conn->session;
f7dbf0662a0167 Mike Christie 2021-02-02 532 struct iscsi_tcp_task *tcp_task;
f7dbf0662a0167 Mike Christie 2021-02-02 533 struct iscsi_tcp_conn *tcp_conn;
f7dbf0662a0167 Mike Christie 2021-02-02 534 struct iscsi_r2t_rsp *rhdr;
a081c13e39b5c1 Mike Christie 2008-12-02 535 struct iscsi_r2t_info *r2t;
f7dbf0662a0167 Mike Christie 2021-02-02 536 struct iscsi_task *task;
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 537 u32 data_length;
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 538 u32 data_offset;
f7dbf0662a0167 Mike Christie 2021-02-02 539 int r2tsn;
a081c13e39b5c1 Mike Christie 2008-12-02 540 int rc;
a081c13e39b5c1 Mike Christie 2008-12-02 541
f7dbf0662a0167 Mike Christie 2021-02-02 542 spin_lock(&session->back_lock);
f7dbf0662a0167 Mike Christie 2021-02-02 543 task = iscsi_itt_to_ctask(conn, hdr->itt);
f7dbf0662a0167 Mike Christie 2021-02-02 544 if (!task) {
f7dbf0662a0167 Mike Christie 2021-02-02 545 spin_unlock(&session->back_lock);
f7dbf0662a0167 Mike Christie 2021-02-02 546 return ISCSI_ERR_BAD_ITT;
f7dbf0662a0167 Mike Christie 2021-02-02 @547 } else if (task->sc->sc_data_direction != DMA_TO_DEVICE) {
f7dbf0662a0167 Mike Christie 2021-02-02 548 spin_unlock(&session->back_lock);
f7dbf0662a0167 Mike Christie 2021-02-02 549 return ISCSI_ERR_PROTO;
f7dbf0662a0167 Mike Christie 2021-02-02 550 }
f7dbf0662a0167 Mike Christie 2021-02-02 551 /*
f7dbf0662a0167 Mike Christie 2021-02-02 552 * A bad target might complete the cmd before we have handled R2Ts
f7dbf0662a0167 Mike Christie 2021-02-02 553 * so get a ref to the task that will be dropped in the xmit path.
f7dbf0662a0167 Mike Christie 2021-02-02 554 */
f7dbf0662a0167 Mike Christie 2021-02-02 555 if (task->state != ISCSI_TASK_RUNNING) {
f7dbf0662a0167 Mike Christie 2021-02-02 556 spin_unlock(&session->back_lock);
f7dbf0662a0167 Mike Christie 2021-02-02 557 /* Let the path that got the early rsp complete it */
f7dbf0662a0167 Mike Christie 2021-02-02 558 return 0;
f7dbf0662a0167 Mike Christie 2021-02-02 559 }
f7dbf0662a0167 Mike Christie 2021-02-02 560 task->last_xfer = jiffies;
f7dbf0662a0167 Mike Christie 2021-02-02 561 __iscsi_get_task(task);
f7dbf0662a0167 Mike Christie 2021-02-02 562
f7dbf0662a0167 Mike Christie 2021-02-02 563 tcp_conn = conn->dd_data;
f7dbf0662a0167 Mike Christie 2021-02-02 564 rhdr = (struct iscsi_r2t_rsp *)tcp_conn->in.hdr;
f7dbf0662a0167 Mike Christie 2021-02-02 565 /* fill-in new R2T associated with the task */
f7dbf0662a0167 Mike Christie 2021-02-02 566 iscsi_update_cmdsn(session, (struct iscsi_nopin *)rhdr);
f7dbf0662a0167 Mike Christie 2021-02-02 567 spin_unlock(&session->back_lock);
f7dbf0662a0167 Mike Christie 2021-02-02 568
a081c13e39b5c1 Mike Christie 2008-12-02 569 if (tcp_conn->in.datalen) {
a081c13e39b5c1 Mike Christie 2008-12-02 570 iscsi_conn_printk(KERN_ERR, conn,
a081c13e39b5c1 Mike Christie 2008-12-02 571 "invalid R2t with datalen %d\n",
a081c13e39b5c1 Mike Christie 2008-12-02 572 tcp_conn->in.datalen);
f7dbf0662a0167 Mike Christie 2021-02-02 573 rc = ISCSI_ERR_DATALEN;
f7dbf0662a0167 Mike Christie 2021-02-02 574 goto put_task;
a081c13e39b5c1 Mike Christie 2008-12-02 575 }
a081c13e39b5c1 Mike Christie 2008-12-02 576
f7dbf0662a0167 Mike Christie 2021-02-02 577 tcp_task = task->dd_data;
f7dbf0662a0167 Mike Christie 2021-02-02 578 r2tsn = be32_to_cpu(rhdr->r2tsn);
a081c13e39b5c1 Mike Christie 2008-12-02 579 if (tcp_task->exp_datasn != r2tsn){
0ab1c2529e6a70 Mike Christie 2009-03-05 580 ISCSI_DBG_TCP(conn, "task->exp_datasn(%d) != rhdr->r2tsn(%d)\n",
0ab1c2529e6a70 Mike Christie 2009-03-05 581 tcp_task->exp_datasn, r2tsn);
f7dbf0662a0167 Mike Christie 2021-02-02 582 rc = ISCSI_ERR_R2TSN;
f7dbf0662a0167 Mike Christie 2021-02-02 583 goto put_task;
a081c13e39b5c1 Mike Christie 2008-12-02 584 }
a081c13e39b5c1 Mike Christie 2008-12-02 585
a081c13e39b5c1 Mike Christie 2008-12-02 @586 if (!task->sc || session->state != ISCSI_STATE_LOGGED_IN) {
a081c13e39b5c1 Mike Christie 2008-12-02 587 iscsi_conn_printk(KERN_INFO, conn,
a081c13e39b5c1 Mike Christie 2008-12-02 588 "dropping R2T itt %d in recovery.\n",
a081c13e39b5c1 Mike Christie 2008-12-02 589 task->itt);
f7dbf0662a0167 Mike Christie 2021-02-02 590 rc = 0;
f7dbf0662a0167 Mike Christie 2021-02-02 591 goto put_task;
a081c13e39b5c1 Mike Christie 2008-12-02 592 }
a081c13e39b5c1 Mike Christie 2008-12-02 593
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 594 data_length = be32_to_cpu(rhdr->data_length);
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 595 if (data_length == 0) {
a081c13e39b5c1 Mike Christie 2008-12-02 596 iscsi_conn_printk(KERN_ERR, conn,
a081c13e39b5c1 Mike Christie 2008-12-02 597 "invalid R2T with zero data len\n");
f7dbf0662a0167 Mike Christie 2021-02-02 598 rc = ISCSI_ERR_DATALEN;
f7dbf0662a0167 Mike Christie 2021-02-02 599 goto put_task;
a081c13e39b5c1 Mike Christie 2008-12-02 600 }
a081c13e39b5c1 Mike Christie 2008-12-02 601
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 602 if (data_length > session->max_burst)
0ab1c2529e6a70 Mike Christie 2009-03-05 603 ISCSI_DBG_TCP(conn, "invalid R2T with data len %u and max "
0ab1c2529e6a70 Mike Christie 2009-03-05 604 "burst %u. Attempting to execute request.\n",
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 605 data_length, session->max_burst);
a081c13e39b5c1 Mike Christie 2008-12-02 606
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 607 data_offset = be32_to_cpu(rhdr->data_offset);
ae3d56d81507c3 Christoph Hellwig 2019-01-29 608 if (data_offset + data_length > task->sc->sdb.length) {
a081c13e39b5c1 Mike Christie 2008-12-02 609 iscsi_conn_printk(KERN_ERR, conn,
a081c13e39b5c1 Mike Christie 2008-12-02 610 "invalid R2T with data len %u at offset %u "
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 611 "and total length %d\n", data_length,
ae3d56d81507c3 Christoph Hellwig 2019-01-29 612 data_offset, task->sc->sdb.length);
f7dbf0662a0167 Mike Christie 2021-02-02 613 rc = ISCSI_ERR_DATALEN;
f7dbf0662a0167 Mike Christie 2021-02-02 614 goto put_task;
a081c13e39b5c1 Mike Christie 2008-12-02 615 }
a081c13e39b5c1 Mike Christie 2008-12-02 616
659743b02c4110 Shlomo Pongratz 2014-02-07 617 spin_lock(&tcp_task->pool2queue);
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 618 rc = kfifo_out(&tcp_task->r2tpool.queue, (void *)&r2t, sizeof(void *));
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 619 if (!rc) {
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 620 iscsi_conn_printk(KERN_ERR, conn, "Could not allocate R2T. "
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 621 "Target has sent more R2Ts than it "
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 622 "negotiated for or driver has leaked.\n");
659743b02c4110 Shlomo Pongratz 2014-02-07 623 spin_unlock(&tcp_task->pool2queue);
f7dbf0662a0167 Mike Christie 2021-02-02 624 rc = ISCSI_ERR_PROTO;
f7dbf0662a0167 Mike Christie 2021-02-02 625 goto put_task;
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 626 }
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 627
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 628 r2t->exp_statsn = rhdr->statsn;
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 629 r2t->data_length = data_length;
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 630 r2t->data_offset = data_offset;
5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 631
a081c13e39b5c1 Mike Christie 2008-12-02 632 r2t->ttt = rhdr->ttt; /* no flip */
a081c13e39b5c1 Mike Christie 2008-12-02 633 r2t->datasn = 0;
a081c13e39b5c1 Mike Christie 2008-12-02 634 r2t->sent = 0;
a081c13e39b5c1 Mike Christie 2008-12-02 635
a081c13e39b5c1 Mike Christie 2008-12-02 636 tcp_task->exp_datasn = r2tsn + 1;
7acd72eb85f1c7 Stefani Seibold 2009-12-21 637 kfifo_in(&tcp_task->r2tqueue, (void*)&r2t, sizeof(void*));
a081c13e39b5c1 Mike Christie 2008-12-02 638 conn->r2t_pdus_cnt++;
659743b02c4110 Shlomo Pongratz 2014-02-07 639 spin_unlock(&tcp_task->pool2queue);
a081c13e39b5c1 Mike Christie 2008-12-02 640
a081c13e39b5c1 Mike Christie 2008-12-02 641 iscsi_requeue_task(task);
a081c13e39b5c1 Mike Christie 2008-12-02 642 return 0;
f7dbf0662a0167 Mike Christie 2021-02-02 643
f7dbf0662a0167 Mike Christie 2021-02-02 644 put_task:
f7dbf0662a0167 Mike Christie 2021-02-02 645 iscsi_put_task(task);
f7dbf0662a0167 Mike Christie 2021-02-02 646 return rc;
a081c13e39b5c1 Mike Christie 2008-12-02 647 }
a081c13e39b5c1 Mike Christie 2008-12-02 648
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38567 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 0/9 V6] iscsi fixes and cleanups
@ 2021-02-07 4:45 Mike Christie
2021-02-07 4:46 ` [PATCH 2/9] libiscsi: drop taskqueuelock Mike Christie
0 siblings, 1 reply; 8+ messages in thread
From: Mike Christie @ 2021-02-07 4:45 UTC (permalink / raw)
To: lduncan, cleech, martin.petersen, linux-scsi, james.bottomley
Cc: lutianxiong, linfeilong, liuzhiqiang26, haowenchao
The following patches made over Martin's 5.12 branches contain
fixes for a cmd lifetime bug, software iscsi can_queue setup,
and a couple of the lock cleanup patches Lee has already ackd.
V6:
- Remove task->sc check that is no longer needed because the
helper function we use does the check for us.
V5:
- Fix up KERN_ERR/INFO use when detecting invalid max_cmds values
from the user.
- Set iscsi_tcp can queue to max value it can support not including
mgmt cmds since the driver itself is not limited and that is a libiscsi
layer limit.
- Added the iscsi session class lock cleanup from the lock cleanup
patchset since it was reviewed already and this is now a patchset
for the next feature window.
V4:
- Add patch:
[PATCH 4/7] libiscsi: fix iscsi host workq destruction
to fix an issue where the user might only call iscsi_host_alloc then
iscsi_host_free and that was leaving the iscsi workqueue running.
- Add check for if a driver were to set can_queue to ISCSI_MGMT_CMDS_MAX
or less.
V3:
- Add some patches for issues found while testing.
- session queue depth was stuck at 128
- cmd window could not be detected when session was relogged in.
- Patch "libiscsi: drop taskqueuelock" had a bug where we did not
disable bhs and during xmit thread suspension leaked the current task.
V2:
- Take back_lock when looping over running cmds in iscsi_eh_cmd_timed_out
in case those complete while we are accessing them.
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 2/9] libiscsi: drop taskqueuelock 2021-02-07 4:45 [PATCH 0/9 V6] iscsi fixes and cleanups Mike Christie @ 2021-02-07 4:46 ` Mike Christie 0 siblings, 0 replies; 8+ messages in thread From: Mike Christie @ 2021-02-07 4:46 UTC (permalink / raw) To: lduncan, cleech, martin.petersen, linux-scsi, james.bottomley Cc: lutianxiong, linfeilong, liuzhiqiang26, haowenchao, Mike Christie The purpose of the taskqueuelock was to handle the issue where a bad target decides to send a R2T and before it's data has been sent decides to send a cmd response to complete the cmd. The following patches fix up the frwd/back locks so they are taken from the queue/xmit (frwd) and completion (back) paths again. To get there this patch removes the taskqueuelock which for iscsi xmit wq based drivers was taken in the queue, xmit and completion paths. Instead of the lock, we just make sure we have a ref to the task when we queue a R2T, and then we always remove the task from the requeue list in the xmit path or the forced cleanup paths. Signed-off-by: Mike Christie <michael.christie@oracle.com> Reviewed-by: Lee Duncan <lduncan@suse.com> --- drivers/scsi/libiscsi.c | 178 +++++++++++++++++++++++------------- drivers/scsi/libiscsi_tcp.c | 86 +++++++++++------ include/scsi/libiscsi.h | 4 +- 3 files changed, 172 insertions(+), 96 deletions(-) diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c index cee1dbaa1b93..3d74fdd9f31a 100644 --- a/drivers/scsi/libiscsi.c +++ b/drivers/scsi/libiscsi.c @@ -523,16 +523,6 @@ static void iscsi_complete_task(struct iscsi_task *task, int state) WARN_ON_ONCE(task->state == ISCSI_TASK_FREE); task->state = state; - spin_lock_bh(&conn->taskqueuelock); - if (!list_empty(&task->running)) { - pr_debug_once("%s while task on list", __func__); - list_del_init(&task->running); - } - spin_unlock_bh(&conn->taskqueuelock); - - if (conn->task == task) - conn->task = NULL; - if (READ_ONCE(conn->ping_task) == task) WRITE_ONCE(conn->ping_task, NULL); @@ -564,9 +554,40 @@ void iscsi_complete_scsi_task(struct iscsi_task *task, } EXPORT_SYMBOL_GPL(iscsi_complete_scsi_task); +/* + * Must be called with back and frwd lock + */ +static bool cleanup_queued_task(struct iscsi_task *task) +{ + struct iscsi_conn *conn = task->conn; + bool early_complete = false; + + /* Bad target might have completed task while it was still running */ + if (task->state == ISCSI_TASK_COMPLETED) + early_complete = true; + + if (!list_empty(&task->running)) { + list_del_init(&task->running); + /* + * If it's on a list but still running, this could be from + * a bad target sending a rsp early, cleanup from a TMF, or + * session recovery. + */ + if (task->state == ISCSI_TASK_RUNNING || + task->state == ISCSI_TASK_COMPLETED) + __iscsi_put_task(task); + } + + if (conn->task == task) { + conn->task = NULL; + __iscsi_put_task(task); + } + + return early_complete; +} /* - * session back_lock must be held and if not called for a task that is + * session frwd_lock must be held and if not called for a task that is * still pending or from the xmit thread, then xmit thread must * be suspended. */ @@ -585,6 +606,13 @@ static void fail_scsi_task(struct iscsi_task *task, int err) if (!sc) return; + /* regular RX path uses back_lock */ + spin_lock_bh(&conn->session->back_lock); + if (cleanup_queued_task(task)) { + spin_unlock_bh(&conn->session->back_lock); + return; + } + if (task->state == ISCSI_TASK_PENDING) { /* * cmd never made it to the xmit thread, so we should not count @@ -600,9 +628,6 @@ static void fail_scsi_task(struct iscsi_task *task, int err) sc->result = err << 16; scsi_set_resid(sc, scsi_bufflen(sc)); - - /* regular RX path uses back_lock */ - spin_lock_bh(&conn->session->back_lock); iscsi_complete_task(task, state); spin_unlock_bh(&conn->session->back_lock); } @@ -748,9 +773,7 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr, if (session->tt->xmit_task(task)) goto free_task; } else { - spin_lock_bh(&conn->taskqueuelock); list_add_tail(&task->running, &conn->mgmtqueue); - spin_unlock_bh(&conn->taskqueuelock); iscsi_conn_queue_work(conn); } @@ -1411,31 +1434,61 @@ static int iscsi_check_cmdsn_window_closed(struct iscsi_conn *conn) return 0; } -static int iscsi_xmit_task(struct iscsi_conn *conn) +static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task, + bool was_requeue) { - struct iscsi_task *task = conn->task; int rc; - if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx)) - return -ENODATA; - spin_lock_bh(&conn->session->back_lock); - if (conn->task == NULL) { + + if (!conn->task) { + /* Take a ref so we can access it after xmit_task() */ + __iscsi_get_task(task); + } else { + /* Already have a ref from when we failed to send it last call */ + conn->task = NULL; + } + + /* + * If this was a requeue for a R2T we have an extra ref on the task in + * case a bad target sends a cmd rsp before we have handled the task. + */ + if (was_requeue) + __iscsi_put_task(task); + + /* + * Do this after dropping the extra ref because if this was a requeue + * it's removed from that list and cleanup_queued_task would miss it. + */ + if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx)) { + /* + * Save the task and ref in case we weren't cleaning up this + * task and get woken up again. + */ + conn->task = task; spin_unlock_bh(&conn->session->back_lock); return -ENODATA; } - __iscsi_get_task(task); spin_unlock_bh(&conn->session->back_lock); + spin_unlock_bh(&conn->session->frwd_lock); rc = conn->session->tt->xmit_task(task); spin_lock_bh(&conn->session->frwd_lock); if (!rc) { /* done with this task */ task->last_xfer = jiffies; - conn->task = NULL; } /* regular RX path uses back_lock */ spin_lock(&conn->session->back_lock); + if (rc && task->state == ISCSI_TASK_RUNNING) { + /* + * get an extra ref that is released next time we access it + * as conn->task above. + */ + __iscsi_get_task(task); + conn->task = task; + } + __iscsi_put_task(task); spin_unlock(&conn->session->back_lock); return rc; @@ -1445,9 +1498,7 @@ static int iscsi_xmit_task(struct iscsi_conn *conn) * iscsi_requeue_task - requeue task to run from session workqueue * @task: task to requeue * - * LLDs that need to run a task from the session workqueue should call - * this. The session frwd_lock must be held. This should only be called - * by software drivers. + * Callers must have taken a ref to the task that is going to be requeued. */ void iscsi_requeue_task(struct iscsi_task *task) { @@ -1457,11 +1508,18 @@ void iscsi_requeue_task(struct iscsi_task *task) * this may be on the requeue list already if the xmit_task callout * is handling the r2ts while we are adding new ones */ - spin_lock_bh(&conn->taskqueuelock); - if (list_empty(&task->running)) + spin_lock_bh(&conn->session->frwd_lock); + if (list_empty(&task->running)) { list_add_tail(&task->running, &conn->requeue); - spin_unlock_bh(&conn->taskqueuelock); + } else { + /* + * Don't need the extra ref since it's already requeued and + * has a ref. + */ + iscsi_put_task(task); + } iscsi_conn_queue_work(conn); + spin_unlock_bh(&conn->session->frwd_lock); } EXPORT_SYMBOL_GPL(iscsi_requeue_task); @@ -1487,7 +1545,7 @@ static int iscsi_data_xmit(struct iscsi_conn *conn) } if (conn->task) { - rc = iscsi_xmit_task(conn); + rc = iscsi_xmit_task(conn, conn->task, false); if (rc) goto done; } @@ -1497,49 +1555,41 @@ static int iscsi_data_xmit(struct iscsi_conn *conn) * only have one nop-out as a ping from us and targets should not * overflow us with nop-ins */ - spin_lock_bh(&conn->taskqueuelock); check_mgmt: while (!list_empty(&conn->mgmtqueue)) { - conn->task = list_entry(conn->mgmtqueue.next, - struct iscsi_task, running); - list_del_init(&conn->task->running); - spin_unlock_bh(&conn->taskqueuelock); - if (iscsi_prep_mgmt_task(conn, conn->task)) { + task = list_entry(conn->mgmtqueue.next, struct iscsi_task, + running); + list_del_init(&task->running); + if (iscsi_prep_mgmt_task(conn, task)) { /* regular RX path uses back_lock */ spin_lock_bh(&conn->session->back_lock); - __iscsi_put_task(conn->task); + __iscsi_put_task(task); spin_unlock_bh(&conn->session->back_lock); - conn->task = NULL; - spin_lock_bh(&conn->taskqueuelock); continue; } - rc = iscsi_xmit_task(conn); + rc = iscsi_xmit_task(conn, task, false); if (rc) goto done; - spin_lock_bh(&conn->taskqueuelock); } /* process pending command queue */ while (!list_empty(&conn->cmdqueue)) { - conn->task = list_entry(conn->cmdqueue.next, struct iscsi_task, - running); - list_del_init(&conn->task->running); - spin_unlock_bh(&conn->taskqueuelock); + task = list_entry(conn->cmdqueue.next, struct iscsi_task, + running); + list_del_init(&task->running); if (conn->session->state == ISCSI_STATE_LOGGING_OUT) { - fail_scsi_task(conn->task, DID_IMM_RETRY); - spin_lock_bh(&conn->taskqueuelock); + fail_scsi_task(task, DID_IMM_RETRY); continue; } - rc = iscsi_prep_scsi_cmd_pdu(conn->task); + rc = iscsi_prep_scsi_cmd_pdu(task); if (rc) { if (rc == -ENOMEM || rc == -EACCES) - fail_scsi_task(conn->task, DID_IMM_RETRY); + fail_scsi_task(task, DID_IMM_RETRY); else - fail_scsi_task(conn->task, DID_ABORT); - spin_lock_bh(&conn->taskqueuelock); + fail_scsi_task(task, DID_ABORT); continue; } - rc = iscsi_xmit_task(conn); + rc = iscsi_xmit_task(conn, task, false); if (rc) goto done; /* @@ -1547,7 +1597,6 @@ static int iscsi_data_xmit(struct iscsi_conn *conn) * we need to check the mgmt queue for nops that need to * be sent to aviod starvation */ - spin_lock_bh(&conn->taskqueuelock); if (!list_empty(&conn->mgmtqueue)) goto check_mgmt; } @@ -1561,21 +1610,17 @@ static int iscsi_data_xmit(struct iscsi_conn *conn) task = list_entry(conn->requeue.next, struct iscsi_task, running); + if (iscsi_check_tmf_restrictions(task, ISCSI_OP_SCSI_DATA_OUT)) break; - conn->task = task; - list_del_init(&conn->task->running); - conn->task->state = ISCSI_TASK_RUNNING; - spin_unlock_bh(&conn->taskqueuelock); - rc = iscsi_xmit_task(conn); + list_del_init(&task->running); + rc = iscsi_xmit_task(conn, task, true); if (rc) goto done; - spin_lock_bh(&conn->taskqueuelock); if (!list_empty(&conn->mgmtqueue)) goto check_mgmt; } - spin_unlock_bh(&conn->taskqueuelock); spin_unlock_bh(&conn->session->frwd_lock); return -ENODATA; @@ -1741,9 +1786,7 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *sc) goto prepd_reject; } } else { - spin_lock_bh(&conn->taskqueuelock); list_add_tail(&task->running, &conn->cmdqueue); - spin_unlock_bh(&conn->taskqueuelock); iscsi_conn_queue_work(conn); } @@ -2914,7 +2957,6 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, int dd_size, INIT_LIST_HEAD(&conn->mgmtqueue); INIT_LIST_HEAD(&conn->cmdqueue); INIT_LIST_HEAD(&conn->requeue); - spin_lock_init(&conn->taskqueuelock); INIT_WORK(&conn->xmitwork, iscsi_xmitworker); /* allocate login_task used for the login/text sequences */ @@ -3080,10 +3122,16 @@ fail_mgmt_tasks(struct iscsi_session *session, struct iscsi_conn *conn) ISCSI_DBG_SESSION(conn->session, "failing mgmt itt 0x%x state %d\n", task->itt, task->state); + + spin_lock_bh(&session->back_lock); + if (cleanup_queued_task(task)) { + spin_unlock_bh(&session->back_lock); + continue; + } + state = ISCSI_TASK_ABRT_SESS_RECOV; if (task->state == ISCSI_TASK_PENDING) state = ISCSI_TASK_COMPLETED; - spin_lock_bh(&session->back_lock); iscsi_complete_task(task, state); spin_unlock_bh(&session->back_lock); } diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c index 83f14b2c8804..2e9ffe3d1a55 100644 --- a/drivers/scsi/libiscsi_tcp.c +++ b/drivers/scsi/libiscsi_tcp.c @@ -524,48 +524,79 @@ static int iscsi_tcp_data_in(struct iscsi_conn *conn, struct iscsi_task *task) /** * iscsi_tcp_r2t_rsp - iSCSI R2T Response processing * @conn: iscsi connection - * @task: scsi command task + * @hdr: PDU header */ -static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_task *task) +static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr) { struct iscsi_session *session = conn->session; - struct iscsi_tcp_task *tcp_task = task->dd_data; - struct iscsi_tcp_conn *tcp_conn = conn->dd_data; - struct iscsi_r2t_rsp *rhdr = (struct iscsi_r2t_rsp *)tcp_conn->in.hdr; + struct iscsi_tcp_task *tcp_task; + struct iscsi_tcp_conn *tcp_conn; + struct iscsi_r2t_rsp *rhdr; struct iscsi_r2t_info *r2t; - int r2tsn = be32_to_cpu(rhdr->r2tsn); + struct iscsi_task *task; u32 data_length; u32 data_offset; + int r2tsn; int rc; + spin_lock(&session->back_lock); + task = iscsi_itt_to_ctask(conn, hdr->itt); + if (!task) { + spin_unlock(&session->back_lock); + return ISCSI_ERR_BAD_ITT; + } else if (task->sc->sc_data_direction != DMA_TO_DEVICE) { + spin_unlock(&session->back_lock); + return ISCSI_ERR_PROTO; + } + /* + * A bad target might complete the cmd before we have handled R2Ts + * so get a ref to the task that will be dropped in the xmit path. + */ + if (task->state != ISCSI_TASK_RUNNING) { + spin_unlock(&session->back_lock); + /* Let the path that got the early rsp complete it */ + return 0; + } + task->last_xfer = jiffies; + __iscsi_get_task(task); + + tcp_conn = conn->dd_data; + rhdr = (struct iscsi_r2t_rsp *)tcp_conn->in.hdr; + /* fill-in new R2T associated with the task */ + iscsi_update_cmdsn(session, (struct iscsi_nopin *)rhdr); + spin_unlock(&session->back_lock); + if (tcp_conn->in.datalen) { iscsi_conn_printk(KERN_ERR, conn, "invalid R2t with datalen %d\n", tcp_conn->in.datalen); - return ISCSI_ERR_DATALEN; + rc = ISCSI_ERR_DATALEN; + goto put_task; } + tcp_task = task->dd_data; + r2tsn = be32_to_cpu(rhdr->r2tsn); if (tcp_task->exp_datasn != r2tsn){ ISCSI_DBG_TCP(conn, "task->exp_datasn(%d) != rhdr->r2tsn(%d)\n", tcp_task->exp_datasn, r2tsn); - return ISCSI_ERR_R2TSN; + rc = ISCSI_ERR_R2TSN; + goto put_task; } - /* fill-in new R2T associated with the task */ - iscsi_update_cmdsn(session, (struct iscsi_nopin*)rhdr); - - if (!task->sc || session->state != ISCSI_STATE_LOGGED_IN) { + if (session->state != ISCSI_STATE_LOGGED_IN) { iscsi_conn_printk(KERN_INFO, conn, "dropping R2T itt %d in recovery.\n", task->itt); - return 0; + rc = 0; + goto put_task; } data_length = be32_to_cpu(rhdr->data_length); if (data_length == 0) { iscsi_conn_printk(KERN_ERR, conn, "invalid R2T with zero data len\n"); - return ISCSI_ERR_DATALEN; + rc = ISCSI_ERR_DATALEN; + goto put_task; } if (data_length > session->max_burst) @@ -579,7 +610,8 @@ static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_task *task) "invalid R2T with data len %u at offset %u " "and total length %d\n", data_length, data_offset, task->sc->sdb.length); - return ISCSI_ERR_DATALEN; + rc = ISCSI_ERR_DATALEN; + goto put_task; } spin_lock(&tcp_task->pool2queue); @@ -589,7 +621,8 @@ static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_task *task) "Target has sent more R2Ts than it " "negotiated for or driver has leaked.\n"); spin_unlock(&tcp_task->pool2queue); - return ISCSI_ERR_PROTO; + rc = ISCSI_ERR_PROTO; + goto put_task; } r2t->exp_statsn = rhdr->statsn; @@ -607,6 +640,10 @@ static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_task *task) iscsi_requeue_task(task); return 0; + +put_task: + iscsi_put_task(task); + return rc; } /* @@ -730,20 +767,11 @@ iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr) rc = iscsi_complete_pdu(conn, hdr, NULL, 0); break; case ISCSI_OP_R2T: - spin_lock(&conn->session->back_lock); - task = iscsi_itt_to_ctask(conn, hdr->itt); - spin_unlock(&conn->session->back_lock); - if (!task) - rc = ISCSI_ERR_BAD_ITT; - else if (ahslen) + if (ahslen) { rc = ISCSI_ERR_AHSLEN; - else if (task->sc->sc_data_direction == DMA_TO_DEVICE) { - task->last_xfer = jiffies; - spin_lock(&conn->session->frwd_lock); - rc = iscsi_tcp_r2t_rsp(conn, task); - spin_unlock(&conn->session->frwd_lock); - } else - rc = ISCSI_ERR_PROTO; + break; + } + rc = iscsi_tcp_r2t_rsp(conn, hdr); break; case ISCSI_OP_LOGIN_RSP: case ISCSI_OP_TEXT_RSP: diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h index b3bbd10eb3f0..44a9554aea62 100644 --- a/include/scsi/libiscsi.h +++ b/include/scsi/libiscsi.h @@ -187,7 +187,7 @@ struct iscsi_conn { struct iscsi_task *task; /* xmit task in progress */ /* xmit */ - spinlock_t taskqueuelock; /* protects the next three lists */ + /* items must be added/deleted under frwd lock */ struct list_head mgmtqueue; /* mgmt (control) xmit queue */ struct list_head cmdqueue; /* data-path cmd queue */ struct list_head requeue; /* tasks needing another run */ @@ -332,7 +332,7 @@ struct iscsi_session { * cmdsn, queued_cmdsn * * session resources: * * - cmdpool kfifo_out , * - * - mgmtpool, */ + * - mgmtpool, queues */ spinlock_t back_lock; /* protects cmdsn_exp * * cmdsn_max, * * cmdpool kfifo_in */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 0/9 V5] iscsi fixes and cleanups @ 2021-02-03 1:33 Mike Christie 2021-02-03 1:33 ` [PATCH 2/9] libiscsi: drop taskqueuelock Mike Christie 0 siblings, 1 reply; 8+ messages in thread From: Mike Christie @ 2021-02-03 1:33 UTC (permalink / raw) To: lduncan, cleech, martin.petersen, linux-scsi, james.bottomley Cc: lutianxiong, linfeilong, liuzhiqiang26, haowenchao The following patches made over Martin's 5.12 branches contain fixes for a cmd lifetime bug, software iscsi can_queue setup, and a couple of the lock cleanup patches Lee has already ackd. V5: - Fix up KERN_ERR/INFO use when detecting invalid max_cmds values from the user. - Set iscsi_tcp can queue to max value it can support not including mgmt cmds since the driver itself is not limited and that is a libiscsi layer limit. - Added the iscsi session class lock cleanup from the lock cleanup patchset since it was reviewed already and this is now a patchset for the next feature window. V4: - Add patch: [PATCH 4/7] libiscsi: fix iscsi host workq destruction to fix an issue where the user might only call iscsi_host_alloc then iscsi_host_free and that was leaving the iscsi workqueue running. - Add check for if a driver were to set can_queue to ISCSI_MGMT_CMDS_MAX or less. V3: - Add some patches for issues found while testing. - session queue depth was stuck at 128 - cmd window could not be detected when session was relogged in. - Patch "libiscsi: drop taskqueuelock" had a bug where we did not disable bhs and during xmit thread suspension leaked the current task. V2: - Take back_lock when looping over running cmds in iscsi_eh_cmd_timed_out in case those complete while we are accessing them. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/9] libiscsi: drop taskqueuelock 2021-02-03 1:33 [PATCH 0/9 V5] iscsi fixes and cleanups Mike Christie @ 2021-02-03 1:33 ` Mike Christie 2021-02-03 10:19 ` Dan Carpenter 0 siblings, 1 reply; 8+ messages in thread From: Mike Christie @ 2021-02-03 1:33 UTC (permalink / raw) To: lduncan, cleech, martin.petersen, linux-scsi, james.bottomley Cc: lutianxiong, linfeilong, liuzhiqiang26, haowenchao, Mike Christie The purpose of the taskqueuelock was to handle the issue where a bad target decides to send a R2T and before it's data has been sent decides to send a cmd response to complete the cmd. The following patches fix up the frwd/back locks so they are taken from the queue/xmit (frwd) and completion (back) paths again. To get there this patch removes the taskqueuelock which for iscsi xmit wq based drivers was taken in the queue, xmit and completion paths. Instead of the lock, we just make sure we have a ref to the task when we queue a R2T, and then we always remove the task from the requeue list in the xmit path or the forced cleanup paths. Signed-off-by: Mike Christie <michael.christie@oracle.com> Reviewed-by: Lee Duncan <lduncan@suse.com> --- drivers/scsi/libiscsi.c | 178 +++++++++++++++++++++++------------- drivers/scsi/libiscsi_tcp.c | 84 +++++++++++------ include/scsi/libiscsi.h | 4 +- 3 files changed, 171 insertions(+), 95 deletions(-) diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c index cee1dbaa1b93..3d74fdd9f31a 100644 --- a/drivers/scsi/libiscsi.c +++ b/drivers/scsi/libiscsi.c @@ -523,16 +523,6 @@ static void iscsi_complete_task(struct iscsi_task *task, int state) WARN_ON_ONCE(task->state == ISCSI_TASK_FREE); task->state = state; - spin_lock_bh(&conn->taskqueuelock); - if (!list_empty(&task->running)) { - pr_debug_once("%s while task on list", __func__); - list_del_init(&task->running); - } - spin_unlock_bh(&conn->taskqueuelock); - - if (conn->task == task) - conn->task = NULL; - if (READ_ONCE(conn->ping_task) == task) WRITE_ONCE(conn->ping_task, NULL); @@ -564,9 +554,40 @@ void iscsi_complete_scsi_task(struct iscsi_task *task, } EXPORT_SYMBOL_GPL(iscsi_complete_scsi_task); +/* + * Must be called with back and frwd lock + */ +static bool cleanup_queued_task(struct iscsi_task *task) +{ + struct iscsi_conn *conn = task->conn; + bool early_complete = false; + + /* Bad target might have completed task while it was still running */ + if (task->state == ISCSI_TASK_COMPLETED) + early_complete = true; + + if (!list_empty(&task->running)) { + list_del_init(&task->running); + /* + * If it's on a list but still running, this could be from + * a bad target sending a rsp early, cleanup from a TMF, or + * session recovery. + */ + if (task->state == ISCSI_TASK_RUNNING || + task->state == ISCSI_TASK_COMPLETED) + __iscsi_put_task(task); + } + + if (conn->task == task) { + conn->task = NULL; + __iscsi_put_task(task); + } + + return early_complete; +} /* - * session back_lock must be held and if not called for a task that is + * session frwd_lock must be held and if not called for a task that is * still pending or from the xmit thread, then xmit thread must * be suspended. */ @@ -585,6 +606,13 @@ static void fail_scsi_task(struct iscsi_task *task, int err) if (!sc) return; + /* regular RX path uses back_lock */ + spin_lock_bh(&conn->session->back_lock); + if (cleanup_queued_task(task)) { + spin_unlock_bh(&conn->session->back_lock); + return; + } + if (task->state == ISCSI_TASK_PENDING) { /* * cmd never made it to the xmit thread, so we should not count @@ -600,9 +628,6 @@ static void fail_scsi_task(struct iscsi_task *task, int err) sc->result = err << 16; scsi_set_resid(sc, scsi_bufflen(sc)); - - /* regular RX path uses back_lock */ - spin_lock_bh(&conn->session->back_lock); iscsi_complete_task(task, state); spin_unlock_bh(&conn->session->back_lock); } @@ -748,9 +773,7 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr, if (session->tt->xmit_task(task)) goto free_task; } else { - spin_lock_bh(&conn->taskqueuelock); list_add_tail(&task->running, &conn->mgmtqueue); - spin_unlock_bh(&conn->taskqueuelock); iscsi_conn_queue_work(conn); } @@ -1411,31 +1434,61 @@ static int iscsi_check_cmdsn_window_closed(struct iscsi_conn *conn) return 0; } -static int iscsi_xmit_task(struct iscsi_conn *conn) +static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task, + bool was_requeue) { - struct iscsi_task *task = conn->task; int rc; - if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx)) - return -ENODATA; - spin_lock_bh(&conn->session->back_lock); - if (conn->task == NULL) { + + if (!conn->task) { + /* Take a ref so we can access it after xmit_task() */ + __iscsi_get_task(task); + } else { + /* Already have a ref from when we failed to send it last call */ + conn->task = NULL; + } + + /* + * If this was a requeue for a R2T we have an extra ref on the task in + * case a bad target sends a cmd rsp before we have handled the task. + */ + if (was_requeue) + __iscsi_put_task(task); + + /* + * Do this after dropping the extra ref because if this was a requeue + * it's removed from that list and cleanup_queued_task would miss it. + */ + if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx)) { + /* + * Save the task and ref in case we weren't cleaning up this + * task and get woken up again. + */ + conn->task = task; spin_unlock_bh(&conn->session->back_lock); return -ENODATA; } - __iscsi_get_task(task); spin_unlock_bh(&conn->session->back_lock); + spin_unlock_bh(&conn->session->frwd_lock); rc = conn->session->tt->xmit_task(task); spin_lock_bh(&conn->session->frwd_lock); if (!rc) { /* done with this task */ task->last_xfer = jiffies; - conn->task = NULL; } /* regular RX path uses back_lock */ spin_lock(&conn->session->back_lock); + if (rc && task->state == ISCSI_TASK_RUNNING) { + /* + * get an extra ref that is released next time we access it + * as conn->task above. + */ + __iscsi_get_task(task); + conn->task = task; + } + __iscsi_put_task(task); spin_unlock(&conn->session->back_lock); return rc; @@ -1445,9 +1498,7 @@ static int iscsi_xmit_task(struct iscsi_conn *conn) * iscsi_requeue_task - requeue task to run from session workqueue * @task: task to requeue * - * LLDs that need to run a task from the session workqueue should call - * this. The session frwd_lock must be held. This should only be called - * by software drivers. + * Callers must have taken a ref to the task that is going to be requeued. */ void iscsi_requeue_task(struct iscsi_task *task) { @@ -1457,11 +1508,18 @@ void iscsi_requeue_task(struct iscsi_task *task) * this may be on the requeue list already if the xmit_task callout * is handling the r2ts while we are adding new ones */ - spin_lock_bh(&conn->taskqueuelock); - if (list_empty(&task->running)) + spin_lock_bh(&conn->session->frwd_lock); + if (list_empty(&task->running)) { list_add_tail(&task->running, &conn->requeue); - spin_unlock_bh(&conn->taskqueuelock); + } else { + /* + * Don't need the extra ref since it's already requeued and + * has a ref. + */ + iscsi_put_task(task); + } iscsi_conn_queue_work(conn); + spin_unlock_bh(&conn->session->frwd_lock); } EXPORT_SYMBOL_GPL(iscsi_requeue_task); @@ -1487,7 +1545,7 @@ static int iscsi_data_xmit(struct iscsi_conn *conn) } if (conn->task) { - rc = iscsi_xmit_task(conn); + rc = iscsi_xmit_task(conn, conn->task, false); if (rc) goto done; } @@ -1497,49 +1555,41 @@ static int iscsi_data_xmit(struct iscsi_conn *conn) * only have one nop-out as a ping from us and targets should not * overflow us with nop-ins */ - spin_lock_bh(&conn->taskqueuelock); check_mgmt: while (!list_empty(&conn->mgmtqueue)) { - conn->task = list_entry(conn->mgmtqueue.next, - struct iscsi_task, running); - list_del_init(&conn->task->running); - spin_unlock_bh(&conn->taskqueuelock); - if (iscsi_prep_mgmt_task(conn, conn->task)) { + task = list_entry(conn->mgmtqueue.next, struct iscsi_task, + running); + list_del_init(&task->running); + if (iscsi_prep_mgmt_task(conn, task)) { /* regular RX path uses back_lock */ spin_lock_bh(&conn->session->back_lock); - __iscsi_put_task(conn->task); + __iscsi_put_task(task); spin_unlock_bh(&conn->session->back_lock); - conn->task = NULL; - spin_lock_bh(&conn->taskqueuelock); continue; } - rc = iscsi_xmit_task(conn); + rc = iscsi_xmit_task(conn, task, false); if (rc) goto done; - spin_lock_bh(&conn->taskqueuelock); } /* process pending command queue */ while (!list_empty(&conn->cmdqueue)) { - conn->task = list_entry(conn->cmdqueue.next, struct iscsi_task, - running); - list_del_init(&conn->task->running); - spin_unlock_bh(&conn->taskqueuelock); + task = list_entry(conn->cmdqueue.next, struct iscsi_task, + running); + list_del_init(&task->running); if (conn->session->state == ISCSI_STATE_LOGGING_OUT) { - fail_scsi_task(conn->task, DID_IMM_RETRY); - spin_lock_bh(&conn->taskqueuelock); + fail_scsi_task(task, DID_IMM_RETRY); continue; } - rc = iscsi_prep_scsi_cmd_pdu(conn->task); + rc = iscsi_prep_scsi_cmd_pdu(task); if (rc) { if (rc == -ENOMEM || rc == -EACCES) - fail_scsi_task(conn->task, DID_IMM_RETRY); + fail_scsi_task(task, DID_IMM_RETRY); else - fail_scsi_task(conn->task, DID_ABORT); - spin_lock_bh(&conn->taskqueuelock); + fail_scsi_task(task, DID_ABORT); continue; } - rc = iscsi_xmit_task(conn); + rc = iscsi_xmit_task(conn, task, false); if (rc) goto done; /* @@ -1547,7 +1597,6 @@ static int iscsi_data_xmit(struct iscsi_conn *conn) * we need to check the mgmt queue for nops that need to * be sent to aviod starvation */ - spin_lock_bh(&conn->taskqueuelock); if (!list_empty(&conn->mgmtqueue)) goto check_mgmt; } @@ -1561,21 +1610,17 @@ static int iscsi_data_xmit(struct iscsi_conn *conn) task = list_entry(conn->requeue.next, struct iscsi_task, running); + if (iscsi_check_tmf_restrictions(task, ISCSI_OP_SCSI_DATA_OUT)) break; - conn->task = task; - list_del_init(&conn->task->running); - conn->task->state = ISCSI_TASK_RUNNING; - spin_unlock_bh(&conn->taskqueuelock); - rc = iscsi_xmit_task(conn); + list_del_init(&task->running); + rc = iscsi_xmit_task(conn, task, true); if (rc) goto done; - spin_lock_bh(&conn->taskqueuelock); if (!list_empty(&conn->mgmtqueue)) goto check_mgmt; } - spin_unlock_bh(&conn->taskqueuelock); spin_unlock_bh(&conn->session->frwd_lock); return -ENODATA; @@ -1741,9 +1786,7 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *sc) goto prepd_reject; } } else { - spin_lock_bh(&conn->taskqueuelock); list_add_tail(&task->running, &conn->cmdqueue); - spin_unlock_bh(&conn->taskqueuelock); iscsi_conn_queue_work(conn); } @@ -2914,7 +2957,6 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, int dd_size, INIT_LIST_HEAD(&conn->mgmtqueue); INIT_LIST_HEAD(&conn->cmdqueue); INIT_LIST_HEAD(&conn->requeue); - spin_lock_init(&conn->taskqueuelock); INIT_WORK(&conn->xmitwork, iscsi_xmitworker); /* allocate login_task used for the login/text sequences */ @@ -3080,10 +3122,16 @@ fail_mgmt_tasks(struct iscsi_session *session, struct iscsi_conn *conn) ISCSI_DBG_SESSION(conn->session, "failing mgmt itt 0x%x state %d\n", task->itt, task->state); + + spin_lock_bh(&session->back_lock); + if (cleanup_queued_task(task)) { + spin_unlock_bh(&session->back_lock); + continue; + } + state = ISCSI_TASK_ABRT_SESS_RECOV; if (task->state == ISCSI_TASK_PENDING) state = ISCSI_TASK_COMPLETED; - spin_lock_bh(&session->back_lock); iscsi_complete_task(task, state); spin_unlock_bh(&session->back_lock); } diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c index 83f14b2c8804..14c9169f13ec 100644 --- a/drivers/scsi/libiscsi_tcp.c +++ b/drivers/scsi/libiscsi_tcp.c @@ -524,48 +524,79 @@ static int iscsi_tcp_data_in(struct iscsi_conn *conn, struct iscsi_task *task) /** * iscsi_tcp_r2t_rsp - iSCSI R2T Response processing * @conn: iscsi connection - * @task: scsi command task + * @hdr: PDU header */ -static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_task *task) +static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr) { struct iscsi_session *session = conn->session; - struct iscsi_tcp_task *tcp_task = task->dd_data; - struct iscsi_tcp_conn *tcp_conn = conn->dd_data; - struct iscsi_r2t_rsp *rhdr = (struct iscsi_r2t_rsp *)tcp_conn->in.hdr; + struct iscsi_tcp_task *tcp_task; + struct iscsi_tcp_conn *tcp_conn; + struct iscsi_r2t_rsp *rhdr; struct iscsi_r2t_info *r2t; - int r2tsn = be32_to_cpu(rhdr->r2tsn); + struct iscsi_task *task; u32 data_length; u32 data_offset; + int r2tsn; int rc; + spin_lock(&session->back_lock); + task = iscsi_itt_to_ctask(conn, hdr->itt); + if (!task) { + spin_unlock(&session->back_lock); + return ISCSI_ERR_BAD_ITT; + } else if (task->sc->sc_data_direction != DMA_TO_DEVICE) { + spin_unlock(&session->back_lock); + return ISCSI_ERR_PROTO; + } + /* + * A bad target might complete the cmd before we have handled R2Ts + * so get a ref to the task that will be dropped in the xmit path. + */ + if (task->state != ISCSI_TASK_RUNNING) { + spin_unlock(&session->back_lock); + /* Let the path that got the early rsp complete it */ + return 0; + } + task->last_xfer = jiffies; + __iscsi_get_task(task); + + tcp_conn = conn->dd_data; + rhdr = (struct iscsi_r2t_rsp *)tcp_conn->in.hdr; + /* fill-in new R2T associated with the task */ + iscsi_update_cmdsn(session, (struct iscsi_nopin *)rhdr); + spin_unlock(&session->back_lock); + if (tcp_conn->in.datalen) { iscsi_conn_printk(KERN_ERR, conn, "invalid R2t with datalen %d\n", tcp_conn->in.datalen); - return ISCSI_ERR_DATALEN; + rc = ISCSI_ERR_DATALEN; + goto put_task; } + tcp_task = task->dd_data; + r2tsn = be32_to_cpu(rhdr->r2tsn); if (tcp_task->exp_datasn != r2tsn){ ISCSI_DBG_TCP(conn, "task->exp_datasn(%d) != rhdr->r2tsn(%d)\n", tcp_task->exp_datasn, r2tsn); - return ISCSI_ERR_R2TSN; + rc = ISCSI_ERR_R2TSN; + goto put_task; } - /* fill-in new R2T associated with the task */ - iscsi_update_cmdsn(session, (struct iscsi_nopin*)rhdr); - if (!task->sc || session->state != ISCSI_STATE_LOGGED_IN) { iscsi_conn_printk(KERN_INFO, conn, "dropping R2T itt %d in recovery.\n", task->itt); - return 0; + rc = 0; + goto put_task; } data_length = be32_to_cpu(rhdr->data_length); if (data_length == 0) { iscsi_conn_printk(KERN_ERR, conn, "invalid R2T with zero data len\n"); - return ISCSI_ERR_DATALEN; + rc = ISCSI_ERR_DATALEN; + goto put_task; } if (data_length > session->max_burst) @@ -579,7 +610,8 @@ static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_task *task) "invalid R2T with data len %u at offset %u " "and total length %d\n", data_length, data_offset, task->sc->sdb.length); - return ISCSI_ERR_DATALEN; + rc = ISCSI_ERR_DATALEN; + goto put_task; } spin_lock(&tcp_task->pool2queue); @@ -589,7 +621,8 @@ static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_task *task) "Target has sent more R2Ts than it " "negotiated for or driver has leaked.\n"); spin_unlock(&tcp_task->pool2queue); - return ISCSI_ERR_PROTO; + rc = ISCSI_ERR_PROTO; + goto put_task; } r2t->exp_statsn = rhdr->statsn; @@ -607,6 +640,10 @@ static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_task *task) iscsi_requeue_task(task); return 0; + +put_task: + iscsi_put_task(task); + return rc; } /* @@ -730,20 +767,11 @@ iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr) rc = iscsi_complete_pdu(conn, hdr, NULL, 0); break; case ISCSI_OP_R2T: - spin_lock(&conn->session->back_lock); - task = iscsi_itt_to_ctask(conn, hdr->itt); - spin_unlock(&conn->session->back_lock); - if (!task) - rc = ISCSI_ERR_BAD_ITT; - else if (ahslen) + if (ahslen) { rc = ISCSI_ERR_AHSLEN; - else if (task->sc->sc_data_direction == DMA_TO_DEVICE) { - task->last_xfer = jiffies; - spin_lock(&conn->session->frwd_lock); - rc = iscsi_tcp_r2t_rsp(conn, task); - spin_unlock(&conn->session->frwd_lock); - } else - rc = ISCSI_ERR_PROTO; + break; + } + rc = iscsi_tcp_r2t_rsp(conn, hdr); break; case ISCSI_OP_LOGIN_RSP: case ISCSI_OP_TEXT_RSP: diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h index b3bbd10eb3f0..44a9554aea62 100644 --- a/include/scsi/libiscsi.h +++ b/include/scsi/libiscsi.h @@ -187,7 +187,7 @@ struct iscsi_conn { struct iscsi_task *task; /* xmit task in progress */ /* xmit */ - spinlock_t taskqueuelock; /* protects the next three lists */ + /* items must be added/deleted under frwd lock */ struct list_head mgmtqueue; /* mgmt (control) xmit queue */ struct list_head cmdqueue; /* data-path cmd queue */ struct list_head requeue; /* tasks needing another run */ @@ -332,7 +332,7 @@ struct iscsi_session { * cmdsn, queued_cmdsn * * session resources: * * - cmdpool kfifo_out , * - * - mgmtpool, */ + * - mgmtpool, queues */ spinlock_t back_lock; /* protects cmdsn_exp * * cmdsn_max, * * cmdpool kfifo_in */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/9] libiscsi: drop taskqueuelock 2021-02-03 1:33 ` [PATCH 2/9] libiscsi: drop taskqueuelock Mike Christie 2021-02-03 10:19 ` Dan Carpenter @ 2021-02-03 10:19 ` Dan Carpenter 0 siblings, 0 replies; 8+ messages in thread From: Dan Carpenter @ 2021-02-03 10:19 UTC (permalink / raw) To: kbuild, Mike Christie, lduncan, cleech, martin.petersen, linux-scsi, james.bottomley Cc: lkp, kbuild-all, lutianxiong, linfeilong, liuzhiqiang26, haowenchao, Mike Christie [-- Attachment #1: Type: text/plain, Size: 10189 bytes --] Hi Mike, url: https://github.com/0day-ci/linux/commits/Mike-Christie/iscsi-fixes-and-cleanups/20210203-122757 base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next config: i386-randconfig-m021-20210202 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> smatch warnings: drivers/scsi/libiscsi_tcp.c:586 iscsi_tcp_r2t_rsp() warn: variable dereferenced before check 'task->sc' (see line 547) vim +586 drivers/scsi/libiscsi_tcp.c f7dbf0662a0167 Mike Christie 2021-02-02 529 static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr) a081c13e39b5c1 Mike Christie 2008-12-02 530 { a081c13e39b5c1 Mike Christie 2008-12-02 531 struct iscsi_session *session = conn->session; f7dbf0662a0167 Mike Christie 2021-02-02 532 struct iscsi_tcp_task *tcp_task; f7dbf0662a0167 Mike Christie 2021-02-02 533 struct iscsi_tcp_conn *tcp_conn; f7dbf0662a0167 Mike Christie 2021-02-02 534 struct iscsi_r2t_rsp *rhdr; a081c13e39b5c1 Mike Christie 2008-12-02 535 struct iscsi_r2t_info *r2t; f7dbf0662a0167 Mike Christie 2021-02-02 536 struct iscsi_task *task; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 537 u32 data_length; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 538 u32 data_offset; f7dbf0662a0167 Mike Christie 2021-02-02 539 int r2tsn; a081c13e39b5c1 Mike Christie 2008-12-02 540 int rc; a081c13e39b5c1 Mike Christie 2008-12-02 541 f7dbf0662a0167 Mike Christie 2021-02-02 542 spin_lock(&session->back_lock); f7dbf0662a0167 Mike Christie 2021-02-02 543 task = iscsi_itt_to_ctask(conn, hdr->itt); f7dbf0662a0167 Mike Christie 2021-02-02 544 if (!task) { f7dbf0662a0167 Mike Christie 2021-02-02 545 spin_unlock(&session->back_lock); f7dbf0662a0167 Mike Christie 2021-02-02 546 return ISCSI_ERR_BAD_ITT; f7dbf0662a0167 Mike Christie 2021-02-02 @547 } else if (task->sc->sc_data_direction != DMA_TO_DEVICE) { ^^^^^^^^ New unchecked dereference. f7dbf0662a0167 Mike Christie 2021-02-02 548 spin_unlock(&session->back_lock); f7dbf0662a0167 Mike Christie 2021-02-02 549 return ISCSI_ERR_PROTO; f7dbf0662a0167 Mike Christie 2021-02-02 550 } f7dbf0662a0167 Mike Christie 2021-02-02 551 /* f7dbf0662a0167 Mike Christie 2021-02-02 552 * A bad target might complete the cmd before we have handled R2Ts f7dbf0662a0167 Mike Christie 2021-02-02 553 * so get a ref to the task that will be dropped in the xmit path. f7dbf0662a0167 Mike Christie 2021-02-02 554 */ f7dbf0662a0167 Mike Christie 2021-02-02 555 if (task->state != ISCSI_TASK_RUNNING) { f7dbf0662a0167 Mike Christie 2021-02-02 556 spin_unlock(&session->back_lock); f7dbf0662a0167 Mike Christie 2021-02-02 557 /* Let the path that got the early rsp complete it */ f7dbf0662a0167 Mike Christie 2021-02-02 558 return 0; f7dbf0662a0167 Mike Christie 2021-02-02 559 } f7dbf0662a0167 Mike Christie 2021-02-02 560 task->last_xfer = jiffies; f7dbf0662a0167 Mike Christie 2021-02-02 561 __iscsi_get_task(task); f7dbf0662a0167 Mike Christie 2021-02-02 562 f7dbf0662a0167 Mike Christie 2021-02-02 563 tcp_conn = conn->dd_data; f7dbf0662a0167 Mike Christie 2021-02-02 564 rhdr = (struct iscsi_r2t_rsp *)tcp_conn->in.hdr; f7dbf0662a0167 Mike Christie 2021-02-02 565 /* fill-in new R2T associated with the task */ f7dbf0662a0167 Mike Christie 2021-02-02 566 iscsi_update_cmdsn(session, (struct iscsi_nopin *)rhdr); f7dbf0662a0167 Mike Christie 2021-02-02 567 spin_unlock(&session->back_lock); f7dbf0662a0167 Mike Christie 2021-02-02 568 a081c13e39b5c1 Mike Christie 2008-12-02 569 if (tcp_conn->in.datalen) { a081c13e39b5c1 Mike Christie 2008-12-02 570 iscsi_conn_printk(KERN_ERR, conn, a081c13e39b5c1 Mike Christie 2008-12-02 571 "invalid R2t with datalen %d\n", a081c13e39b5c1 Mike Christie 2008-12-02 572 tcp_conn->in.datalen); f7dbf0662a0167 Mike Christie 2021-02-02 573 rc = ISCSI_ERR_DATALEN; f7dbf0662a0167 Mike Christie 2021-02-02 574 goto put_task; a081c13e39b5c1 Mike Christie 2008-12-02 575 } a081c13e39b5c1 Mike Christie 2008-12-02 576 f7dbf0662a0167 Mike Christie 2021-02-02 577 tcp_task = task->dd_data; f7dbf0662a0167 Mike Christie 2021-02-02 578 r2tsn = be32_to_cpu(rhdr->r2tsn); a081c13e39b5c1 Mike Christie 2008-12-02 579 if (tcp_task->exp_datasn != r2tsn){ 0ab1c2529e6a70 Mike Christie 2009-03-05 580 ISCSI_DBG_TCP(conn, "task->exp_datasn(%d) != rhdr->r2tsn(%d)\n", 0ab1c2529e6a70 Mike Christie 2009-03-05 581 tcp_task->exp_datasn, r2tsn); f7dbf0662a0167 Mike Christie 2021-02-02 582 rc = ISCSI_ERR_R2TSN; f7dbf0662a0167 Mike Christie 2021-02-02 583 goto put_task; a081c13e39b5c1 Mike Christie 2008-12-02 584 } a081c13e39b5c1 Mike Christie 2008-12-02 585 a081c13e39b5c1 Mike Christie 2008-12-02 @586 if (!task->sc || session->state != ISCSI_STATE_LOGGED_IN) { ^^^^^^^^ Checked too late. a081c13e39b5c1 Mike Christie 2008-12-02 587 iscsi_conn_printk(KERN_INFO, conn, a081c13e39b5c1 Mike Christie 2008-12-02 588 "dropping R2T itt %d in recovery.\n", a081c13e39b5c1 Mike Christie 2008-12-02 589 task->itt); f7dbf0662a0167 Mike Christie 2021-02-02 590 rc = 0; f7dbf0662a0167 Mike Christie 2021-02-02 591 goto put_task; a081c13e39b5c1 Mike Christie 2008-12-02 592 } a081c13e39b5c1 Mike Christie 2008-12-02 593 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 594 data_length = be32_to_cpu(rhdr->data_length); 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 595 if (data_length == 0) { a081c13e39b5c1 Mike Christie 2008-12-02 596 iscsi_conn_printk(KERN_ERR, conn, a081c13e39b5c1 Mike Christie 2008-12-02 597 "invalid R2T with zero data len\n"); f7dbf0662a0167 Mike Christie 2021-02-02 598 rc = ISCSI_ERR_DATALEN; f7dbf0662a0167 Mike Christie 2021-02-02 599 goto put_task; a081c13e39b5c1 Mike Christie 2008-12-02 600 } a081c13e39b5c1 Mike Christie 2008-12-02 601 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 602 if (data_length > session->max_burst) 0ab1c2529e6a70 Mike Christie 2009-03-05 603 ISCSI_DBG_TCP(conn, "invalid R2T with data len %u and max " 0ab1c2529e6a70 Mike Christie 2009-03-05 604 "burst %u. Attempting to execute request.\n", 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 605 data_length, session->max_burst); a081c13e39b5c1 Mike Christie 2008-12-02 606 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 607 data_offset = be32_to_cpu(rhdr->data_offset); ae3d56d81507c3 Christoph Hellwig 2019-01-29 608 if (data_offset + data_length > task->sc->sdb.length) { a081c13e39b5c1 Mike Christie 2008-12-02 609 iscsi_conn_printk(KERN_ERR, conn, a081c13e39b5c1 Mike Christie 2008-12-02 610 "invalid R2T with data len %u at offset %u " 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 611 "and total length %d\n", data_length, ae3d56d81507c3 Christoph Hellwig 2019-01-29 612 data_offset, task->sc->sdb.length); f7dbf0662a0167 Mike Christie 2021-02-02 613 rc = ISCSI_ERR_DATALEN; f7dbf0662a0167 Mike Christie 2021-02-02 614 goto put_task; a081c13e39b5c1 Mike Christie 2008-12-02 615 } a081c13e39b5c1 Mike Christie 2008-12-02 616 659743b02c4110 Shlomo Pongratz 2014-02-07 617 spin_lock(&tcp_task->pool2queue); 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 618 rc = kfifo_out(&tcp_task->r2tpool.queue, (void *)&r2t, sizeof(void *)); 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 619 if (!rc) { 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 620 iscsi_conn_printk(KERN_ERR, conn, "Could not allocate R2T. " 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 621 "Target has sent more R2Ts than it " 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 622 "negotiated for or driver has leaked.\n"); 659743b02c4110 Shlomo Pongratz 2014-02-07 623 spin_unlock(&tcp_task->pool2queue); f7dbf0662a0167 Mike Christie 2021-02-02 624 rc = ISCSI_ERR_PROTO; f7dbf0662a0167 Mike Christie 2021-02-02 625 goto put_task; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 626 } 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 627 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 628 r2t->exp_statsn = rhdr->statsn; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 629 r2t->data_length = data_length; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 630 r2t->data_offset = data_offset; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 631 a081c13e39b5c1 Mike Christie 2008-12-02 632 r2t->ttt = rhdr->ttt; /* no flip */ a081c13e39b5c1 Mike Christie 2008-12-02 633 r2t->datasn = 0; a081c13e39b5c1 Mike Christie 2008-12-02 634 r2t->sent = 0; a081c13e39b5c1 Mike Christie 2008-12-02 635 a081c13e39b5c1 Mike Christie 2008-12-02 636 tcp_task->exp_datasn = r2tsn + 1; 7acd72eb85f1c7 Stefani Seibold 2009-12-21 637 kfifo_in(&tcp_task->r2tqueue, (void*)&r2t, sizeof(void*)); a081c13e39b5c1 Mike Christie 2008-12-02 638 conn->r2t_pdus_cnt++; 659743b02c4110 Shlomo Pongratz 2014-02-07 639 spin_unlock(&tcp_task->pool2queue); a081c13e39b5c1 Mike Christie 2008-12-02 640 a081c13e39b5c1 Mike Christie 2008-12-02 641 iscsi_requeue_task(task); a081c13e39b5c1 Mike Christie 2008-12-02 642 return 0; f7dbf0662a0167 Mike Christie 2021-02-02 643 f7dbf0662a0167 Mike Christie 2021-02-02 644 put_task: f7dbf0662a0167 Mike Christie 2021-02-02 645 iscsi_put_task(task); f7dbf0662a0167 Mike Christie 2021-02-02 646 return rc; a081c13e39b5c1 Mike Christie 2008-12-02 647 } --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 38567 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/9] libiscsi: drop taskqueuelock @ 2021-02-03 10:19 ` Dan Carpenter 0 siblings, 0 replies; 8+ messages in thread From: Dan Carpenter @ 2021-02-03 10:19 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 10336 bytes --] Hi Mike, url: https://github.com/0day-ci/linux/commits/Mike-Christie/iscsi-fixes-and-cleanups/20210203-122757 base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next config: i386-randconfig-m021-20210202 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> smatch warnings: drivers/scsi/libiscsi_tcp.c:586 iscsi_tcp_r2t_rsp() warn: variable dereferenced before check 'task->sc' (see line 547) vim +586 drivers/scsi/libiscsi_tcp.c f7dbf0662a0167 Mike Christie 2021-02-02 529 static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr) a081c13e39b5c1 Mike Christie 2008-12-02 530 { a081c13e39b5c1 Mike Christie 2008-12-02 531 struct iscsi_session *session = conn->session; f7dbf0662a0167 Mike Christie 2021-02-02 532 struct iscsi_tcp_task *tcp_task; f7dbf0662a0167 Mike Christie 2021-02-02 533 struct iscsi_tcp_conn *tcp_conn; f7dbf0662a0167 Mike Christie 2021-02-02 534 struct iscsi_r2t_rsp *rhdr; a081c13e39b5c1 Mike Christie 2008-12-02 535 struct iscsi_r2t_info *r2t; f7dbf0662a0167 Mike Christie 2021-02-02 536 struct iscsi_task *task; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 537 u32 data_length; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 538 u32 data_offset; f7dbf0662a0167 Mike Christie 2021-02-02 539 int r2tsn; a081c13e39b5c1 Mike Christie 2008-12-02 540 int rc; a081c13e39b5c1 Mike Christie 2008-12-02 541 f7dbf0662a0167 Mike Christie 2021-02-02 542 spin_lock(&session->back_lock); f7dbf0662a0167 Mike Christie 2021-02-02 543 task = iscsi_itt_to_ctask(conn, hdr->itt); f7dbf0662a0167 Mike Christie 2021-02-02 544 if (!task) { f7dbf0662a0167 Mike Christie 2021-02-02 545 spin_unlock(&session->back_lock); f7dbf0662a0167 Mike Christie 2021-02-02 546 return ISCSI_ERR_BAD_ITT; f7dbf0662a0167 Mike Christie 2021-02-02 @547 } else if (task->sc->sc_data_direction != DMA_TO_DEVICE) { ^^^^^^^^ New unchecked dereference. f7dbf0662a0167 Mike Christie 2021-02-02 548 spin_unlock(&session->back_lock); f7dbf0662a0167 Mike Christie 2021-02-02 549 return ISCSI_ERR_PROTO; f7dbf0662a0167 Mike Christie 2021-02-02 550 } f7dbf0662a0167 Mike Christie 2021-02-02 551 /* f7dbf0662a0167 Mike Christie 2021-02-02 552 * A bad target might complete the cmd before we have handled R2Ts f7dbf0662a0167 Mike Christie 2021-02-02 553 * so get a ref to the task that will be dropped in the xmit path. f7dbf0662a0167 Mike Christie 2021-02-02 554 */ f7dbf0662a0167 Mike Christie 2021-02-02 555 if (task->state != ISCSI_TASK_RUNNING) { f7dbf0662a0167 Mike Christie 2021-02-02 556 spin_unlock(&session->back_lock); f7dbf0662a0167 Mike Christie 2021-02-02 557 /* Let the path that got the early rsp complete it */ f7dbf0662a0167 Mike Christie 2021-02-02 558 return 0; f7dbf0662a0167 Mike Christie 2021-02-02 559 } f7dbf0662a0167 Mike Christie 2021-02-02 560 task->last_xfer = jiffies; f7dbf0662a0167 Mike Christie 2021-02-02 561 __iscsi_get_task(task); f7dbf0662a0167 Mike Christie 2021-02-02 562 f7dbf0662a0167 Mike Christie 2021-02-02 563 tcp_conn = conn->dd_data; f7dbf0662a0167 Mike Christie 2021-02-02 564 rhdr = (struct iscsi_r2t_rsp *)tcp_conn->in.hdr; f7dbf0662a0167 Mike Christie 2021-02-02 565 /* fill-in new R2T associated with the task */ f7dbf0662a0167 Mike Christie 2021-02-02 566 iscsi_update_cmdsn(session, (struct iscsi_nopin *)rhdr); f7dbf0662a0167 Mike Christie 2021-02-02 567 spin_unlock(&session->back_lock); f7dbf0662a0167 Mike Christie 2021-02-02 568 a081c13e39b5c1 Mike Christie 2008-12-02 569 if (tcp_conn->in.datalen) { a081c13e39b5c1 Mike Christie 2008-12-02 570 iscsi_conn_printk(KERN_ERR, conn, a081c13e39b5c1 Mike Christie 2008-12-02 571 "invalid R2t with datalen %d\n", a081c13e39b5c1 Mike Christie 2008-12-02 572 tcp_conn->in.datalen); f7dbf0662a0167 Mike Christie 2021-02-02 573 rc = ISCSI_ERR_DATALEN; f7dbf0662a0167 Mike Christie 2021-02-02 574 goto put_task; a081c13e39b5c1 Mike Christie 2008-12-02 575 } a081c13e39b5c1 Mike Christie 2008-12-02 576 f7dbf0662a0167 Mike Christie 2021-02-02 577 tcp_task = task->dd_data; f7dbf0662a0167 Mike Christie 2021-02-02 578 r2tsn = be32_to_cpu(rhdr->r2tsn); a081c13e39b5c1 Mike Christie 2008-12-02 579 if (tcp_task->exp_datasn != r2tsn){ 0ab1c2529e6a70 Mike Christie 2009-03-05 580 ISCSI_DBG_TCP(conn, "task->exp_datasn(%d) != rhdr->r2tsn(%d)\n", 0ab1c2529e6a70 Mike Christie 2009-03-05 581 tcp_task->exp_datasn, r2tsn); f7dbf0662a0167 Mike Christie 2021-02-02 582 rc = ISCSI_ERR_R2TSN; f7dbf0662a0167 Mike Christie 2021-02-02 583 goto put_task; a081c13e39b5c1 Mike Christie 2008-12-02 584 } a081c13e39b5c1 Mike Christie 2008-12-02 585 a081c13e39b5c1 Mike Christie 2008-12-02 @586 if (!task->sc || session->state != ISCSI_STATE_LOGGED_IN) { ^^^^^^^^ Checked too late. a081c13e39b5c1 Mike Christie 2008-12-02 587 iscsi_conn_printk(KERN_INFO, conn, a081c13e39b5c1 Mike Christie 2008-12-02 588 "dropping R2T itt %d in recovery.\n", a081c13e39b5c1 Mike Christie 2008-12-02 589 task->itt); f7dbf0662a0167 Mike Christie 2021-02-02 590 rc = 0; f7dbf0662a0167 Mike Christie 2021-02-02 591 goto put_task; a081c13e39b5c1 Mike Christie 2008-12-02 592 } a081c13e39b5c1 Mike Christie 2008-12-02 593 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 594 data_length = be32_to_cpu(rhdr->data_length); 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 595 if (data_length == 0) { a081c13e39b5c1 Mike Christie 2008-12-02 596 iscsi_conn_printk(KERN_ERR, conn, a081c13e39b5c1 Mike Christie 2008-12-02 597 "invalid R2T with zero data len\n"); f7dbf0662a0167 Mike Christie 2021-02-02 598 rc = ISCSI_ERR_DATALEN; f7dbf0662a0167 Mike Christie 2021-02-02 599 goto put_task; a081c13e39b5c1 Mike Christie 2008-12-02 600 } a081c13e39b5c1 Mike Christie 2008-12-02 601 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 602 if (data_length > session->max_burst) 0ab1c2529e6a70 Mike Christie 2009-03-05 603 ISCSI_DBG_TCP(conn, "invalid R2T with data len %u and max " 0ab1c2529e6a70 Mike Christie 2009-03-05 604 "burst %u. Attempting to execute request.\n", 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 605 data_length, session->max_burst); a081c13e39b5c1 Mike Christie 2008-12-02 606 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 607 data_offset = be32_to_cpu(rhdr->data_offset); ae3d56d81507c3 Christoph Hellwig 2019-01-29 608 if (data_offset + data_length > task->sc->sdb.length) { a081c13e39b5c1 Mike Christie 2008-12-02 609 iscsi_conn_printk(KERN_ERR, conn, a081c13e39b5c1 Mike Christie 2008-12-02 610 "invalid R2T with data len %u at offset %u " 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 611 "and total length %d\n", data_length, ae3d56d81507c3 Christoph Hellwig 2019-01-29 612 data_offset, task->sc->sdb.length); f7dbf0662a0167 Mike Christie 2021-02-02 613 rc = ISCSI_ERR_DATALEN; f7dbf0662a0167 Mike Christie 2021-02-02 614 goto put_task; a081c13e39b5c1 Mike Christie 2008-12-02 615 } a081c13e39b5c1 Mike Christie 2008-12-02 616 659743b02c4110 Shlomo Pongratz 2014-02-07 617 spin_lock(&tcp_task->pool2queue); 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 618 rc = kfifo_out(&tcp_task->r2tpool.queue, (void *)&r2t, sizeof(void *)); 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 619 if (!rc) { 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 620 iscsi_conn_printk(KERN_ERR, conn, "Could not allocate R2T. " 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 621 "Target has sent more R2Ts than it " 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 622 "negotiated for or driver has leaked.\n"); 659743b02c4110 Shlomo Pongratz 2014-02-07 623 spin_unlock(&tcp_task->pool2queue); f7dbf0662a0167 Mike Christie 2021-02-02 624 rc = ISCSI_ERR_PROTO; f7dbf0662a0167 Mike Christie 2021-02-02 625 goto put_task; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 626 } 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 627 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 628 r2t->exp_statsn = rhdr->statsn; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 629 r2t->data_length = data_length; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 630 r2t->data_offset = data_offset; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 631 a081c13e39b5c1 Mike Christie 2008-12-02 632 r2t->ttt = rhdr->ttt; /* no flip */ a081c13e39b5c1 Mike Christie 2008-12-02 633 r2t->datasn = 0; a081c13e39b5c1 Mike Christie 2008-12-02 634 r2t->sent = 0; a081c13e39b5c1 Mike Christie 2008-12-02 635 a081c13e39b5c1 Mike Christie 2008-12-02 636 tcp_task->exp_datasn = r2tsn + 1; 7acd72eb85f1c7 Stefani Seibold 2009-12-21 637 kfifo_in(&tcp_task->r2tqueue, (void*)&r2t, sizeof(void*)); a081c13e39b5c1 Mike Christie 2008-12-02 638 conn->r2t_pdus_cnt++; 659743b02c4110 Shlomo Pongratz 2014-02-07 639 spin_unlock(&tcp_task->pool2queue); a081c13e39b5c1 Mike Christie 2008-12-02 640 a081c13e39b5c1 Mike Christie 2008-12-02 641 iscsi_requeue_task(task); a081c13e39b5c1 Mike Christie 2008-12-02 642 return 0; f7dbf0662a0167 Mike Christie 2021-02-02 643 f7dbf0662a0167 Mike Christie 2021-02-02 644 put_task: f7dbf0662a0167 Mike Christie 2021-02-02 645 iscsi_put_task(task); f7dbf0662a0167 Mike Christie 2021-02-02 646 return rc; a081c13e39b5c1 Mike Christie 2008-12-02 647 } --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 38567 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/9] libiscsi: drop taskqueuelock @ 2021-02-03 10:19 ` Dan Carpenter 0 siblings, 0 replies; 8+ messages in thread From: Dan Carpenter @ 2021-02-03 10:19 UTC (permalink / raw) To: kbuild [-- Attachment #1: Type: text/plain, Size: 10336 bytes --] Hi Mike, url: https://github.com/0day-ci/linux/commits/Mike-Christie/iscsi-fixes-and-cleanups/20210203-122757 base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next config: i386-randconfig-m021-20210202 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> smatch warnings: drivers/scsi/libiscsi_tcp.c:586 iscsi_tcp_r2t_rsp() warn: variable dereferenced before check 'task->sc' (see line 547) vim +586 drivers/scsi/libiscsi_tcp.c f7dbf0662a0167 Mike Christie 2021-02-02 529 static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr) a081c13e39b5c1 Mike Christie 2008-12-02 530 { a081c13e39b5c1 Mike Christie 2008-12-02 531 struct iscsi_session *session = conn->session; f7dbf0662a0167 Mike Christie 2021-02-02 532 struct iscsi_tcp_task *tcp_task; f7dbf0662a0167 Mike Christie 2021-02-02 533 struct iscsi_tcp_conn *tcp_conn; f7dbf0662a0167 Mike Christie 2021-02-02 534 struct iscsi_r2t_rsp *rhdr; a081c13e39b5c1 Mike Christie 2008-12-02 535 struct iscsi_r2t_info *r2t; f7dbf0662a0167 Mike Christie 2021-02-02 536 struct iscsi_task *task; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 537 u32 data_length; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 538 u32 data_offset; f7dbf0662a0167 Mike Christie 2021-02-02 539 int r2tsn; a081c13e39b5c1 Mike Christie 2008-12-02 540 int rc; a081c13e39b5c1 Mike Christie 2008-12-02 541 f7dbf0662a0167 Mike Christie 2021-02-02 542 spin_lock(&session->back_lock); f7dbf0662a0167 Mike Christie 2021-02-02 543 task = iscsi_itt_to_ctask(conn, hdr->itt); f7dbf0662a0167 Mike Christie 2021-02-02 544 if (!task) { f7dbf0662a0167 Mike Christie 2021-02-02 545 spin_unlock(&session->back_lock); f7dbf0662a0167 Mike Christie 2021-02-02 546 return ISCSI_ERR_BAD_ITT; f7dbf0662a0167 Mike Christie 2021-02-02 @547 } else if (task->sc->sc_data_direction != DMA_TO_DEVICE) { ^^^^^^^^ New unchecked dereference. f7dbf0662a0167 Mike Christie 2021-02-02 548 spin_unlock(&session->back_lock); f7dbf0662a0167 Mike Christie 2021-02-02 549 return ISCSI_ERR_PROTO; f7dbf0662a0167 Mike Christie 2021-02-02 550 } f7dbf0662a0167 Mike Christie 2021-02-02 551 /* f7dbf0662a0167 Mike Christie 2021-02-02 552 * A bad target might complete the cmd before we have handled R2Ts f7dbf0662a0167 Mike Christie 2021-02-02 553 * so get a ref to the task that will be dropped in the xmit path. f7dbf0662a0167 Mike Christie 2021-02-02 554 */ f7dbf0662a0167 Mike Christie 2021-02-02 555 if (task->state != ISCSI_TASK_RUNNING) { f7dbf0662a0167 Mike Christie 2021-02-02 556 spin_unlock(&session->back_lock); f7dbf0662a0167 Mike Christie 2021-02-02 557 /* Let the path that got the early rsp complete it */ f7dbf0662a0167 Mike Christie 2021-02-02 558 return 0; f7dbf0662a0167 Mike Christie 2021-02-02 559 } f7dbf0662a0167 Mike Christie 2021-02-02 560 task->last_xfer = jiffies; f7dbf0662a0167 Mike Christie 2021-02-02 561 __iscsi_get_task(task); f7dbf0662a0167 Mike Christie 2021-02-02 562 f7dbf0662a0167 Mike Christie 2021-02-02 563 tcp_conn = conn->dd_data; f7dbf0662a0167 Mike Christie 2021-02-02 564 rhdr = (struct iscsi_r2t_rsp *)tcp_conn->in.hdr; f7dbf0662a0167 Mike Christie 2021-02-02 565 /* fill-in new R2T associated with the task */ f7dbf0662a0167 Mike Christie 2021-02-02 566 iscsi_update_cmdsn(session, (struct iscsi_nopin *)rhdr); f7dbf0662a0167 Mike Christie 2021-02-02 567 spin_unlock(&session->back_lock); f7dbf0662a0167 Mike Christie 2021-02-02 568 a081c13e39b5c1 Mike Christie 2008-12-02 569 if (tcp_conn->in.datalen) { a081c13e39b5c1 Mike Christie 2008-12-02 570 iscsi_conn_printk(KERN_ERR, conn, a081c13e39b5c1 Mike Christie 2008-12-02 571 "invalid R2t with datalen %d\n", a081c13e39b5c1 Mike Christie 2008-12-02 572 tcp_conn->in.datalen); f7dbf0662a0167 Mike Christie 2021-02-02 573 rc = ISCSI_ERR_DATALEN; f7dbf0662a0167 Mike Christie 2021-02-02 574 goto put_task; a081c13e39b5c1 Mike Christie 2008-12-02 575 } a081c13e39b5c1 Mike Christie 2008-12-02 576 f7dbf0662a0167 Mike Christie 2021-02-02 577 tcp_task = task->dd_data; f7dbf0662a0167 Mike Christie 2021-02-02 578 r2tsn = be32_to_cpu(rhdr->r2tsn); a081c13e39b5c1 Mike Christie 2008-12-02 579 if (tcp_task->exp_datasn != r2tsn){ 0ab1c2529e6a70 Mike Christie 2009-03-05 580 ISCSI_DBG_TCP(conn, "task->exp_datasn(%d) != rhdr->r2tsn(%d)\n", 0ab1c2529e6a70 Mike Christie 2009-03-05 581 tcp_task->exp_datasn, r2tsn); f7dbf0662a0167 Mike Christie 2021-02-02 582 rc = ISCSI_ERR_R2TSN; f7dbf0662a0167 Mike Christie 2021-02-02 583 goto put_task; a081c13e39b5c1 Mike Christie 2008-12-02 584 } a081c13e39b5c1 Mike Christie 2008-12-02 585 a081c13e39b5c1 Mike Christie 2008-12-02 @586 if (!task->sc || session->state != ISCSI_STATE_LOGGED_IN) { ^^^^^^^^ Checked too late. a081c13e39b5c1 Mike Christie 2008-12-02 587 iscsi_conn_printk(KERN_INFO, conn, a081c13e39b5c1 Mike Christie 2008-12-02 588 "dropping R2T itt %d in recovery.\n", a081c13e39b5c1 Mike Christie 2008-12-02 589 task->itt); f7dbf0662a0167 Mike Christie 2021-02-02 590 rc = 0; f7dbf0662a0167 Mike Christie 2021-02-02 591 goto put_task; a081c13e39b5c1 Mike Christie 2008-12-02 592 } a081c13e39b5c1 Mike Christie 2008-12-02 593 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 594 data_length = be32_to_cpu(rhdr->data_length); 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 595 if (data_length == 0) { a081c13e39b5c1 Mike Christie 2008-12-02 596 iscsi_conn_printk(KERN_ERR, conn, a081c13e39b5c1 Mike Christie 2008-12-02 597 "invalid R2T with zero data len\n"); f7dbf0662a0167 Mike Christie 2021-02-02 598 rc = ISCSI_ERR_DATALEN; f7dbf0662a0167 Mike Christie 2021-02-02 599 goto put_task; a081c13e39b5c1 Mike Christie 2008-12-02 600 } a081c13e39b5c1 Mike Christie 2008-12-02 601 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 602 if (data_length > session->max_burst) 0ab1c2529e6a70 Mike Christie 2009-03-05 603 ISCSI_DBG_TCP(conn, "invalid R2T with data len %u and max " 0ab1c2529e6a70 Mike Christie 2009-03-05 604 "burst %u. Attempting to execute request.\n", 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 605 data_length, session->max_burst); a081c13e39b5c1 Mike Christie 2008-12-02 606 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 607 data_offset = be32_to_cpu(rhdr->data_offset); ae3d56d81507c3 Christoph Hellwig 2019-01-29 608 if (data_offset + data_length > task->sc->sdb.length) { a081c13e39b5c1 Mike Christie 2008-12-02 609 iscsi_conn_printk(KERN_ERR, conn, a081c13e39b5c1 Mike Christie 2008-12-02 610 "invalid R2T with data len %u at offset %u " 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 611 "and total length %d\n", data_length, ae3d56d81507c3 Christoph Hellwig 2019-01-29 612 data_offset, task->sc->sdb.length); f7dbf0662a0167 Mike Christie 2021-02-02 613 rc = ISCSI_ERR_DATALEN; f7dbf0662a0167 Mike Christie 2021-02-02 614 goto put_task; a081c13e39b5c1 Mike Christie 2008-12-02 615 } a081c13e39b5c1 Mike Christie 2008-12-02 616 659743b02c4110 Shlomo Pongratz 2014-02-07 617 spin_lock(&tcp_task->pool2queue); 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 618 rc = kfifo_out(&tcp_task->r2tpool.queue, (void *)&r2t, sizeof(void *)); 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 619 if (!rc) { 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 620 iscsi_conn_printk(KERN_ERR, conn, "Could not allocate R2T. " 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 621 "Target has sent more R2Ts than it " 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 622 "negotiated for or driver has leaked.\n"); 659743b02c4110 Shlomo Pongratz 2014-02-07 623 spin_unlock(&tcp_task->pool2queue); f7dbf0662a0167 Mike Christie 2021-02-02 624 rc = ISCSI_ERR_PROTO; f7dbf0662a0167 Mike Christie 2021-02-02 625 goto put_task; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 626 } 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 627 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 628 r2t->exp_statsn = rhdr->statsn; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 629 r2t->data_length = data_length; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 630 r2t->data_offset = data_offset; 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 631 a081c13e39b5c1 Mike Christie 2008-12-02 632 r2t->ttt = rhdr->ttt; /* no flip */ a081c13e39b5c1 Mike Christie 2008-12-02 633 r2t->datasn = 0; a081c13e39b5c1 Mike Christie 2008-12-02 634 r2t->sent = 0; a081c13e39b5c1 Mike Christie 2008-12-02 635 a081c13e39b5c1 Mike Christie 2008-12-02 636 tcp_task->exp_datasn = r2tsn + 1; 7acd72eb85f1c7 Stefani Seibold 2009-12-21 637 kfifo_in(&tcp_task->r2tqueue, (void*)&r2t, sizeof(void*)); a081c13e39b5c1 Mike Christie 2008-12-02 638 conn->r2t_pdus_cnt++; 659743b02c4110 Shlomo Pongratz 2014-02-07 639 spin_unlock(&tcp_task->pool2queue); a081c13e39b5c1 Mike Christie 2008-12-02 640 a081c13e39b5c1 Mike Christie 2008-12-02 641 iscsi_requeue_task(task); a081c13e39b5c1 Mike Christie 2008-12-02 642 return 0; f7dbf0662a0167 Mike Christie 2021-02-02 643 f7dbf0662a0167 Mike Christie 2021-02-02 644 put_task: f7dbf0662a0167 Mike Christie 2021-02-02 645 iscsi_put_task(task); f7dbf0662a0167 Mike Christie 2021-02-02 646 return rc; a081c13e39b5c1 Mike Christie 2008-12-02 647 } --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 38567 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/9] libiscsi: drop taskqueuelock 2021-02-03 10:19 ` Dan Carpenter @ 2021-02-03 17:10 ` Mike Christie -1 siblings, 0 replies; 8+ messages in thread From: Mike Christie @ 2021-02-03 17:10 UTC (permalink / raw) To: Dan Carpenter, kbuild, lduncan, cleech, martin.petersen, linux-scsi, james.bottomley Cc: lkp, kbuild-all, lutianxiong, linfeilong, liuzhiqiang26, haowenchao On 2/3/21 4:19 AM, Dan Carpenter wrote: > Hi Mike, > > url: https://github.com/0day-ci/linux/commits/Mike-Christie/iscsi-fixes-and-cleanups/20210203-122757 > base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next > config: i386-randconfig-m021-20210202 (attached as .config) > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > smatch warnings: > drivers/scsi/libiscsi_tcp.c:586 iscsi_tcp_r2t_rsp() warn: variable dereferenced before check 'task->sc' (see line 547) > > vim +586 drivers/scsi/libiscsi_tcp.c > > f7dbf0662a0167 Mike Christie 2021-02-02 529 static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr) > a081c13e39b5c1 Mike Christie 2008-12-02 530 { > a081c13e39b5c1 Mike Christie 2008-12-02 531 struct iscsi_session *session = conn->session; > f7dbf0662a0167 Mike Christie 2021-02-02 532 struct iscsi_tcp_task *tcp_task; > f7dbf0662a0167 Mike Christie 2021-02-02 533 struct iscsi_tcp_conn *tcp_conn; > f7dbf0662a0167 Mike Christie 2021-02-02 534 struct iscsi_r2t_rsp *rhdr; > a081c13e39b5c1 Mike Christie 2008-12-02 535 struct iscsi_r2t_info *r2t; > f7dbf0662a0167 Mike Christie 2021-02-02 536 struct iscsi_task *task; > 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 537 u32 data_length; > 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 538 u32 data_offset; > f7dbf0662a0167 Mike Christie 2021-02-02 539 int r2tsn; > a081c13e39b5c1 Mike Christie 2008-12-02 540 int rc; > a081c13e39b5c1 Mike Christie 2008-12-02 541 > f7dbf0662a0167 Mike Christie 2021-02-02 542 spin_lock(&session->back_lock); > f7dbf0662a0167 Mike Christie 2021-02-02 543 task = iscsi_itt_to_ctask(conn, hdr->itt); > f7dbf0662a0167 Mike Christie 2021-02-02 544 if (!task) { > f7dbf0662a0167 Mike Christie 2021-02-02 545 spin_unlock(&session->back_lock); > f7dbf0662a0167 Mike Christie 2021-02-02 546 return ISCSI_ERR_BAD_ITT; > f7dbf0662a0167 Mike Christie 2021-02-02 @547 } else if (task->sc->sc_data_direction != DMA_TO_DEVICE) { > ^^^^^^^^ > New unchecked dereference. I see the issue. iscsi_itt_ctask checks task->sc and if NULL returns NULL. However, below in this function there is now a not needed task->sc check. The checker saw that and thinks the above line could be a invalid access. I'll fix the patch by removing the old check since it's confusing code that's also not needed since it's done for us now. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/9] libiscsi: drop taskqueuelock @ 2021-02-03 17:10 ` Mike Christie 0 siblings, 0 replies; 8+ messages in thread From: Mike Christie @ 2021-02-03 17:10 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 2744 bytes --] On 2/3/21 4:19 AM, Dan Carpenter wrote: > Hi Mike, > > url: https://github.com/0day-ci/linux/commits/Mike-Christie/iscsi-fixes-and-cleanups/20210203-122757 > base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next > config: i386-randconfig-m021-20210202 (attached as .config) > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > smatch warnings: > drivers/scsi/libiscsi_tcp.c:586 iscsi_tcp_r2t_rsp() warn: variable dereferenced before check 'task->sc' (see line 547) > > vim +586 drivers/scsi/libiscsi_tcp.c > > f7dbf0662a0167 Mike Christie 2021-02-02 529 static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr) > a081c13e39b5c1 Mike Christie 2008-12-02 530 { > a081c13e39b5c1 Mike Christie 2008-12-02 531 struct iscsi_session *session = conn->session; > f7dbf0662a0167 Mike Christie 2021-02-02 532 struct iscsi_tcp_task *tcp_task; > f7dbf0662a0167 Mike Christie 2021-02-02 533 struct iscsi_tcp_conn *tcp_conn; > f7dbf0662a0167 Mike Christie 2021-02-02 534 struct iscsi_r2t_rsp *rhdr; > a081c13e39b5c1 Mike Christie 2008-12-02 535 struct iscsi_r2t_info *r2t; > f7dbf0662a0167 Mike Christie 2021-02-02 536 struct iscsi_task *task; > 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 537 u32 data_length; > 5d0fddd0a72d30 Shlomo Pongratz 2014-02-07 538 u32 data_offset; > f7dbf0662a0167 Mike Christie 2021-02-02 539 int r2tsn; > a081c13e39b5c1 Mike Christie 2008-12-02 540 int rc; > a081c13e39b5c1 Mike Christie 2008-12-02 541 > f7dbf0662a0167 Mike Christie 2021-02-02 542 spin_lock(&session->back_lock); > f7dbf0662a0167 Mike Christie 2021-02-02 543 task = iscsi_itt_to_ctask(conn, hdr->itt); > f7dbf0662a0167 Mike Christie 2021-02-02 544 if (!task) { > f7dbf0662a0167 Mike Christie 2021-02-02 545 spin_unlock(&session->back_lock); > f7dbf0662a0167 Mike Christie 2021-02-02 546 return ISCSI_ERR_BAD_ITT; > f7dbf0662a0167 Mike Christie 2021-02-02 @547 } else if (task->sc->sc_data_direction != DMA_TO_DEVICE) { > ^^^^^^^^ > New unchecked dereference. I see the issue. iscsi_itt_ctask checks task->sc and if NULL returns NULL. However, below in this function there is now a not needed task->sc check. The checker saw that and thinks the above line could be a invalid access. I'll fix the patch by removing the old check since it's confusing code that's also not needed since it's done for us now. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-02-07 4:48 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-03 7:39 [PATCH 2/9] libiscsi: drop taskqueuelock kernel test robot -- strict thread matches above, loose matches on Subject: below -- 2021-02-07 4:45 [PATCH 0/9 V6] iscsi fixes and cleanups Mike Christie 2021-02-07 4:46 ` [PATCH 2/9] libiscsi: drop taskqueuelock Mike Christie 2021-02-03 1:33 [PATCH 0/9 V5] iscsi fixes and cleanups Mike Christie 2021-02-03 1:33 ` [PATCH 2/9] libiscsi: drop taskqueuelock Mike Christie 2021-02-03 10:19 ` Dan Carpenter 2021-02-03 10:19 ` Dan Carpenter 2021-02-03 10:19 ` Dan Carpenter 2021-02-03 17:10 ` Mike Christie 2021-02-03 17:10 ` Mike Christie
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.