[bug report] scsi: iscsi: Remove iscsi_get_task back_lock requirement

[Dan Carpenter]

Hello Mike Christie,

The patch a01ff1e161ea: "scsi: iscsi: Remove iscsi_get_task back_lock
requirement" from May 18, 2022, leads to the following Smatch static
checker warning:

drivers/scsi/libiscsi_tcp.c:649 iscsi_tcp_r2t_rsp()
warn: inconsistent returns '&session->back_lock'.

drivers/scsi/libiscsi_tcp.c
529 static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
530 {
531 struct iscsi_session *session = conn->session;
532 struct iscsi_tcp_task *tcp_task;
533 struct iscsi_tcp_conn *tcp_conn;
534 struct iscsi_r2t_rsp *rhdr;
535 struct iscsi_r2t_info *r2t;
536 struct iscsi_task *task;
537 u32 data_length;
538 u32 data_offset;
539 int r2tsn;
540 int rc;
541
542 spin_lock(&session->back_lock);
543 task = iscsi_itt_to_ctask(conn, hdr->itt);
544 if (!task) {
545 spin_unlock(&session->back_lock);
546 return ISCSI_ERR_BAD_ITT;
547 } else if (task->sc->sc_data_direction != DMA_TO_DEVICE) {
548 spin_unlock(&session->back_lock);
549 return ISCSI_ERR_PROTO;
550 }
551 /*
552 * A bad target might complete the cmd before we have handled R2Ts
553 * so get a ref to the task that will be dropped in the xmit path.
554 */
555 if (task->state != ISCSI_TASK_RUNNING) {
556 spin_unlock(&session->back_lock);
557 /* Let the path that got the early rsp complete it */

This comment looks exactly like the new comment below but this path
drops the lock.

558 return 0;
559 }
560 task->last_xfer = jiffies;
561 if (!iscsi_get_task(task)) {
562 /* Let the path that got the early rsp complete it */

Reading through the commit log it looks like maybe this it's intentional
to not drop the lock here??? If so it needs a comment.

563 return 0;
564 }
565
566 tcp_conn = conn->dd_data;
567 rhdr = (struct iscsi_r2t_rsp *)tcp_conn->in.hdr;
568 /* fill-in new R2T associated with the task */
569 iscsi_update_cmdsn(session, (struct iscsi_nopin *)rhdr);
570 spin_unlock(&session->back_lock);
^^^^^^^^^^^^^^^^^^
Everything else drops the lock.

571
572 if (tcp_conn->in.datalen) {
573 iscsi_conn_printk(KERN_ERR, conn,
574 "invalid R2t with datalen %d\n",
575 tcp_conn->in.datalen);
576 rc = ISCSI_ERR_DATALEN;
577 goto put_task;

Causes a static checker warning in the caller as well:

drivers/scsi/libiscsi_tcp.c:825 iscsi_tcp_hdr_dissect() warn: '&conn->session->back_lock' both locked and unlocked.

regards,
dan carpenter

[Mike Christie]

On 5/23/22 1:58 AM, Dan Carpenter wrote:
> Hello Mike Christie,
>
> The patch a01ff1e161ea: "scsi: iscsi: Remove iscsi_get_task back_lock
> requirement" from May 18, 2022, leads to the following Smatch static
> checker warning:
>
> drivers/scsi/libiscsi_tcp.c:649 iscsi_tcp_r2t_rsp()
> warn: inconsistent returns '&session->back_lock'.
>

Thanks Dan. Yeah, it's a bug. I'm just waiting on Martin to see how he
wants to resolve it (new patchset, new patch, or patch on top of staging).