Re: [PATCH 2/3] scsi: target: iscsi: extract auth functions

[Mike Christie <michael.christie@oracle.com>]

On 9/14/21 5:03 AM, Dmitry Bogdanov wrote:
> +bool iscsi_conn_auth_required(struct iscsi_conn *conn)
> +{
> +	struct se_node_acl *se_nacl;
> +
> +	if (conn->sess->sess_ops->SessionType) {
> +		/*
> +		 * For SessionType=Discovery
> +		 */
> +		return conn->tpg->tpg_attrib.authentication;
> +	}
> +	/*
> +	 * For SessionType=Normal
> +	 */
> +	se_nacl = conn->sess->se_sess->se_node_acl;
> +	if (!se_nacl) {
> +		pr_debug("Unknown ACL %s is trying to connect\n",
> +			 se_nacl->initiatorname);
> +		return true;

Before the patch, if we didn't have an ACL we would go by
conn->tpg->tpg_attrib.authentication. But with the patch, if
we don't have an ACL, then it looks like we always require authentication
which I don't think is right.

Is the code above supposed to return the value of 
conn->tpg->tpg_attrib.authentication?


> +	}
> +
> +	if (se_nacl->dynamic_node_acl) {
> +		pr_debug("Dynamic ACL %s is trying to connect\n",
> +			 se_nacl->initiatorname);
> +		return conn->tpg->tpg_attrib.authentication;
> +	}
> +
> +	pr_debug("Known ACL %s is trying to connect\n",
> +		 se_nacl->initiatorname);
> +	return conn->tpg->tpg_attrib.authentication;
> +}
> +
>  static int iscsi_target_handle_csg_zero(
>  	struct iscsi_conn *conn,
>  	struct iscsi_login *login)
> @@ -874,22 +903,26 @@ static int iscsi_target_handle_csg_zero(
>  		return -1;
>  
>  	if (!iscsi_check_negotiated_keys(conn->param_list)) {
> -		if (conn->tpg->tpg_attrib.authentication &&
> -		    !strncmp(param->value, NONE, 4)) {
> -			pr_err("Initiator sent AuthMethod=None but"
> -				" Target is enforcing iSCSI Authentication,"
> -					" login failed.\n");
> -			iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_INITIATOR_ERR,
> -					ISCSI_LOGIN_STATUS_AUTH_FAILED);
> -			return -1;
> -		}
> +		bool auth_required = iscsi_conn_auth_required(conn);
> +

In __iscsi_target_login_thread we have:

        if (conn->sess)
                conn->sess->se_sess->sup_prot_ops =
                        conn->conn_transport->iscsit_get_sup_prot_ops(conn);

before we call:

iscsi_target_start_negotiation -> iscsi_target_do_login- > iscsi_target_handle_csg_zero
and hit the code above.

If conn->sess can be NULL then iscsi_conn_auth_required will crash.

However, I can't tell how conn->sess can be NULL in that code path. Is the conn->sess
check in __iscsi_target_login_thread not needed?