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
