Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Message-ID: <0abfcf5b-5ab8-4968-bf6d-eb4dee32e2f4@default>
Date:   Thu, 7 Jan 2021 15:48:02 +0000 (UTC)
From:   Gulam Mohamed <gulam.mohamed@oracle.com>
Sender: Gulam Mohamed <gulam.mohamed@oracle.com>
To:     Michael Christie <michael.christie@oracle.com>
Cc:     Lee Duncan <lduncan@suse.com>, Chris Leech <cleech@redhat.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        Martin Petersen <martin.petersen@oracle.com>,
        open-iscsi@googlegroups.com, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org, Junxiao Bi <junxiao.bi@oracle.com>
Subject: RE: [PATCH] iscsi: Do Not set param when sock is NULL
References: <1a8aaa17-b1a3-4d6a-b87a-ff49d61a0d0b@default>
 <9df96d73-015c-4de6-96fa-2f315b066909@default>
 <05277786-2E1F-432D-AE73-F39565C6BEA4@oracle.com>
In-Reply-To: <05277786-2E1F-432D-AE73-F39565C6BEA4@oracle.com>
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Michael,

             As per your suggestions in below mail, I have completed the su=
ggested changes and tested them. Can you please review and let me know your=
 comments? Here is the patch:

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

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

Fix
---

1. Create a flag "set_param_fail" in iscsi_cls_conn structure 2. On ep_disc=
onnect and stop_conn set this flag to indicate set_param calls for connecti=
on level settings should fail 3. This way, scsi_transport_iscsi can set and=
 check this bit for all drivers 2. On bind_conn clear the bit

Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>
---
 drivers/scsi/scsi_transport_iscsi.c | 6 ++++++  include/scsi/scsi_transpor=
t_iscsi.h | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transp=
ort_iscsi.c
index 2e68c0a87698..15c5a7223a40 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2473,6 +2473,8 @@ static void iscsi_if_stop_conn(struct iscsi_cls_conn =
*conn, int flag)
 =09 * it works.
 =09 */
 =09mutex_lock(&conn_mutex);
+=09if (!test_bit(ISCSI_SET_PARAM_FAIL_BIT, &conn->set_param_fail))
+=09=09set_bit(ISCSI_SET_PARAM_FAIL_BIT, &conn->set_param_fail);
 =09conn->transport->stop_conn(conn, flag);
 =09mutex_unlock(&conn_mutex);
=20
@@ -2895,6 +2897,8 @@ iscsi_set_param(struct iscsi_transport *transport, st=
ruct iscsi_uevent *ev)
 =09=09=09session->recovery_tmo =3D value;
 =09=09break;
 =09default:
+=09=09if (test_bit(ISCSI_SET_PARAM_FAIL_BIT, &conn->set_param_fail))
+=09=09=09return -ENOTCONN;
 =09=09err =3D transport->set_param(conn, ev->u.set_param.param,
 =09=09=09=09=09   data, ev->u.set_param.len);
 =09}
@@ -2956,6 +2960,7 @@ static int iscsi_if_ep_disconnect(struct iscsi_transp=
ort *transport,
 =09=09mutex_lock(&conn->ep_mutex);
 =09=09conn->ep =3D NULL;
 =09=09mutex_unlock(&conn->ep_mutex);
+=09=09set_bit(ISCSI_SET_PARAM_FAIL_BIT, &conn->set_param_fail);
 =09}
=20
 =09transport->ep_disconnect(ep);
@@ -3716,6 +3721,7 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghd=
r *nlh, uint32_t *group)
 =09=09ev->r.retcode =3D=09transport->bind_conn(session, conn,
 =09=09=09=09=09=09ev->u.b_conn.transport_eph,
 =09=09=09=09=09=09ev->u.b_conn.is_leading);
+=09=09clear_bit(ISCSI_SET_PARAM_FAIL_BIT, &conn->set_param_fail);
 =09=09mutex_unlock(&conn_mutex);
=20
 =09=09if (ev->r.retcode || !transport->ep_connect) diff --git a/include/sc=
si/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index 8a26a2ffa952..71b1952b913b 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -29,6 +29,8 @@ struct bsg_job;
 struct iscsi_bus_flash_session;
 struct iscsi_bus_flash_conn;
=20
+#define ISCSI_SET_PARAM_FAIL_BIT=091
+
 /**
  * struct iscsi_transport - iSCSI Transport template
  *
@@ -206,6 +208,7 @@ struct iscsi_cls_conn {
=20
 =09struct device dev;=09=09/* sysfs transport/container device */
 =09enum iscsi_connection_state state;
+=09unsigned long set_param_fail; /* set_param for connection should fail=
=20
+*/
 };
=20
 #define iscsi_dev_to_conn(_dev) \
--
2.27.0

Regards,
Gulam Mohamed.

-----Original Message-----
From: Michael Christie=20
Sent: Tuesday, November 24, 2020 4:37 AM
To: Gulam Mohamed <gulam.mohamed@oracle.com>
Cc: Lee Duncan <lduncan@suse.com>; Chris Leech <cleech@redhat.com>; James E=
.J. Bottomley <jejb@linux.ibm.com>; Martin K. Petersen <martin.petersen@ora=
cle.com>; open-iscsi@googlegroups.com; linux-scsi@vger.kernel.org; linux-ke=
rnel@vger.kernel.org; Junxiao Bi <junxiao.bi@oracle.com>
Subject: Re: [PATCH] iscsi: Do Not set param when sock is NULL


> On Nov 4, 2020, at 11:40 PM, Gulam Mohamed <gulam.mohamed@oracle.com> wro=
te:
>=20
> Description
> =3D=3D=3D=3D=3D=3D=3D=3D=3D
> 1. This Kernel panic could be due to a timing issue when there is a race =
between the sync thread and the initiator was processing of a login respons=
e from the target. The session re-open can be invoked from two places
>          a.=09Sessions sync thread when the iscsid restart
>          b.=09From iscsid through iscsi error handler
> 2. The session reopen sequence is as follows in user-space (iscsi-initiat=
or-utils)
>          a.=09Disconnect the connection
>          b.=09Then send the stop connection request to the kernel which r=
eleases the connection (releases the socket)
>          c.=09Queues the reopen for 2 seconds delay
>         d.=09Once the delay expires, create the TCP connection again by c=
alling the connect() call
>         e.=09Poll for the connection
>          f.=09When poll is successful i.e when the TCP connection is esta=
blished, it performs
> =09i. Creation of session and connection data structures
> =09ii. Bind the connection to the session. This is the place where we ass=
ign the sock to tcp_sw_conn->sock
> =09iii. Sets the parameters like target name, persistent address etc .
> =09iv. Creates the login pdu
> =09v. Sends the login pdu to kernel
> =09vi. Returns to the main loop to process further events. The kernel the=
n sends the login request over to the target node
> =09g. Once login response with success is received, it enters into full f=
eature phase and sets the negotiable parameters like max_recv_data_length, =
max_transmit_length, data_digest etc .
> 3. While setting the negotiable parameters by calling=20
> "iscsi_session_set_neg_params()", kernel panicked as sock was NULL
>=20
> What happened here is
> --------------------------------
> 1.=09Before initiator received the login response mentioned in above poin=
t 2.f.v, another reopen request was sent from the error handler/sync sessio=
n for the same session, as the initiator utils was in main loop to process =
further events (as=20
> =09mentioned in point 2.f.vi above).=20
> 2.=09While processing this reopen, it stopped the connection which releas=
ed the socket and queued this connection and at this point of time the logi=
n response was received for the earlier one


To make sure I got this you are saying before iscsi_sw_tcp_conn_stop calls =
iscsi_sw_tcp_release_conn that the recv path has called iscsi_recv_pdu righ=
t?


> 3.=09The kernel passed over this response to user-space which then sent t=
he set_neg_params request to kernel
> 4.=09As the connection was stopped, the sock was NULL and hence while the=
 kernel was processing the set param request from user-space, it panicked
>=20
> Fix
> ----
> 1.=09While setting the set_param in kernel, we need to check if sock is N=
ULL
> 2.=09If the sock is NULL, then return EPERM (operation not permitted)
> 3.=09Due to this error handler will be invoked in user-space again to rec=
over the session
>=20
> Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>
> Reviewed-by: Junxiao Bi <junxiao.bi@oracle.com>
> ---
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c index=20
> df47557a02a3..fd668a194053 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -711,6 +711,12 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_=
cls_conn *cls_conn,
>        struct iscsi_tcp_conn *tcp_conn =3D conn->dd_data;
>        struct iscsi_sw_tcp_conn *tcp_sw_conn =3D tcp_conn->dd_data;
>=20
> +       if (!tcp_sw_conn->sock) {
> +               iscsi_conn_printk(KERN_ERR, conn,
> +                               "Cannot set param as sock is NULL\n");
> +               return -ENOTCONN;
> +       }
> +

I think this might have 2 issues:

1. It only fixes iscsi_tcp. What about other drivers that free/null resourc=
es/fields in ep_disconnect that we layer need to access in the set_param ca=
llback (the cxgbi drivers)?

2. What about drivers that do not free/null fields (be2iscsi, bnx2i, ql4xxx=
 and qedi) so the set_param calls end up just working. What state will user=
space be in where we have logged in, but iscsid also thinks we are in the m=
iddle of trying to login?

I think we could do:

1. On ep_disconnect and stop_conn set some iscsi_cls_conn bit that indicate=
s set_param calls for connection level settings should fail. scsi_transport=
_iscsi could set and check this bit for all drivers.
2. On bind_conn clear the bit.