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

[Dmitriy Bogdanov <d.bogdanov@yadro.com>]

Hi Mike,

> > +{
> > +	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?

No, no. This piece of code is the same as it was.
An absence of ACL is some erroneous situation because the login must be
rejected earlier in __iscsi_target_login_thread -> iscsi_target_locate_portal

> > +	}
> > +
> > +	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?

conn->sess is set to NULL in iscsi_login_non_zero_tsih_s1 and  new session is created
in iscsi_login_non_zero_tsih_s2 which is before iscsi_target_start_negotiation, so
we are safe.

BR,
 Dmitry