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;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
Subject: Re: [PATCH] iscsi: Do Not set param when sock is NULL
From:   Michael Christie <michael.christie@oracle.com>
In-Reply-To: <9df96d73-015c-4de6-96fa-2f315b066909@default>
Date:   Mon, 23 Nov 2020 17:07:14 -0600
Cc:     Lee Duncan <lduncan@suse.com>, Chris Leech <cleech@redhat.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. 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>
Content-Transfer-Encoding: quoted-printable
Message-Id: <05277786-2E1F-432D-AE73-F39565C6BEA4@oracle.com>
References: <1a8aaa17-b1a3-4d6a-b87a-ff49d61a0d0b@default>
 <9df96d73-015c-4de6-96fa-2f315b066909@default>
To:     Gulam Mohamed <gulam.mohamed@oracle.com>
Precedence: bulk


> On Nov 4, 2020, at 11:40 PM, Gulam Mohamed <gulam.mohamed@oracle.com> =
wrote:
>=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 =
response from the target. The session re-open can be invoked from two =
places
>          a.	Sessions sync thread when the iscsid restart
>          b.	=46rom 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 the connection (releases the socket)
>          c.	Queues the reopen for 2 seconds delay
>         d.	Once the delay expires, create the TCP connection again =
by calling the connect() call
>         e.	Poll for the connection
>          f.	When poll is successful i.e when the TCP connection is =
established, it performs
> 	i. Creation of session and connection data structures
> 	ii. Bind the connection to the session. This is the place where =
we assign 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 then sends the login request over to the target node
> 	g. Once login response with success is received, it enters into =
full feature 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 =
"iscsi_session_set_neg_params()", kernel panicked as sock was NULL
>=20
> 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 for the same session, as the initiator utils was in main loop to =
process further events (as=20
> 	mentioned in point 2.f.vi above).=20
> 2.	While processing this reopen, it stopped the connection which =
released the socket and queued this connection and at this point of time =
the login 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 right?


> 3.	The kernel passed over this response to user-space which then =
sent the set_neg_params request to kernel
> 4.	As 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.	While setting the set_param in kernel, we need to check if sock =
is NULL
> 2.	If the sock is NULL, then return EPERM (operation not permitted)
> 3.	Due to this error handler will be invoked in user-space again to =
recover 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 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 =
resources/fields in ep_disconnect that we layer need to access in the =
set_param callback (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 userspace be in where we have logged in, but iscsid also thinks we =
are in the middle of trying to login?

I think we could do:

1. On ep_disconnect and stop_conn set some iscsi_cls_conn bit that =
indicates 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.=