From: Jack Burton Subject: Re: [patch 3 of 3] iscsid(8): support a wider variety of iscsi targets To: Claudio Jeker Cc: tech@openbsd.org Date: Tue, 28 Jan 2025 12:58:42 +1030 On Sun, 26 Jan 2025 13:03:21 +0100 Claudio Jeker wrote: > On Fri, Jan 24, 2025 at 11:21:23AM +1030, Jack Burton wrote: > > On Thu, 23 Jan 2025 13:14:30 +0100 > > Claudio Jeker wrote: > > > More comments inline. > > > > Thanks. Have taken all your comments on board. Revised diff > > follows. > > > > All that's left in it is just replacing the two-shot conn_gen_kvp() > > with a one-shot-per-key kvp_set_from_mine() and its associated > > macros, then implementing "get in first" by adding the missing keys > > in initiator_login_kvp(), categorised according to the rules in > > section 13 of RFC 7143. > > So I finally setup a lio target and was able to see the same problems. > We get stuck because of the HeaderDigest and DataDigest fields. > Now your diff below blows up on discovery sessions since TargetName is > NULL. Fixing that is not hard but also a bit ugly. Well caught! Yes, sorry, discovery sessions were the one thing I hadn't been testing. Although RFC 7143 does allow for TargetName (optionally) to be sent in discovery sessions, our iscsictl(8) does not support named discovery sessions (and I don't see any real benefit in doing so), so I've stuck with the all-discovery-sessions-are-unnamed approach already in place. The slightly revised diff below now works for discovery sessions too. I figured that a separate list for discovery would be slightly less ugly than special-casing TargetName. > I try to spend some time on this on monday to see if I can make this > better. No worries. If you can come up with a better way I'm all ears. cvs server: Diffing . Index: connection.c =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/connection.c,v diff -u -p -r1.24 connection.c --- connection.c 22 Jan 2025 10:14:54 -0000 1.24 +++ connection.c 28 Jan 2025 02:10:14 -0000 @@ -335,41 +335,107 @@ log_debug("conn_parse_kvp: %s = %s", k-> #undef SET_BOOL #undef SET_DIGEST +#define GET_BOOL_P(dst, req, k, src, f) \ +do { \ + if (f == 0 && !strcmp(req, #k)) { \ + (dst)->key = #k; \ + (dst)->value = ((src)->mine.k) ? "Yes" : "No"; \ + f++; \ + } \ +} while (0) + +#define GET_DIGEST_P(dst, req, k, src, f) \ +do { \ + if (f == 0 && !strcmp(req, #k)) { \ + (dst)->key = #k; \ + (dst)->value = \ + ((src)->mine.k == DIGEST_NONE) ? "None" : "CRC32C,None";\ + f++; \ + } \ +} while (0) + +#define GET_NUM_P(dst, req, k, src, f, e) \ +do { \ + if (f == 0 && !strcmp(req, #k)) { \ + (dst)->key = #k; \ + if (asprintf(&((dst)->value), "%u", (src)->mine.k) == -1)\ + e++; \ + else \ + (dst)->flags |= KVP_VALUE_ALLOCED; \ + f++; \ + } \ +} while (0) + +#define GET_STR_C(dst, req, k, src, f) \ +do { \ + if (f == 0 && !strcmp(req, #k)) { \ + (dst)->key = #k; \ + (dst)->value = (src)->config.k; \ + f++; \ + } \ +} while (0) + +#define GET_STYPE_C(dst, req, k, src, f) \ +do { \ + if (f == 0 && !strcmp(req, #k)) { \ + (dst)->key = #k; \ + (dst)->value = ((src)->config.k == SESSION_TYPE_DISCOVERY)\ + ? "Discovery" : "Normal"; \ + f++; \ + } \ +} while (0) + int -conn_gen_kvp(struct connection *c, struct kvp *kvp, size_t *nkvp) +kvp_set_from_mine(struct kvp *kvp, const char *key, struct connection *c) { - struct session *s = c->session; - size_t i = 0; + int e = 0, f = 0; + + if (kvp->flags & KVP_KEY_ALLOCED) + free(kvp->key); + kvp->key = NULL; + if (kvp->flags & KVP_VALUE_ALLOCED) + free(kvp->value); + kvp->value = NULL; + kvp->flags = 0; - if (s->mine.MaxConnections != iscsi_sess_defaults.MaxConnections) { - if (kvp && i < *nkvp) { - kvp[i].key = "MaxConnections"; - if (asprintf(&kvp[i].value, "%hu", - s->mine.MaxConnections) == -1) { - kvp[i].value = NULL; - return -1; - } - kvp[i].flags |= KVP_VALUE_ALLOCED; - } - i++; + /* XXX handle at least CHAP */ + if (!strcmp(key, "AuthMethod")) { + kvp->key = "AuthMethod"; + kvp->value = "None"; + return 0; } - if (c->mine.MaxRecvDataSegmentLength != - iscsi_conn_defaults.MaxRecvDataSegmentLength) { - if (kvp && i < *nkvp) { - kvp[i].key = "MaxRecvDataSegmentLength"; - if (asprintf(&kvp[i].value, "%u", - c->mine.MaxRecvDataSegmentLength) == -1) { - kvp[i].value = NULL; - return -1; - } - kvp[i].flags |= KVP_VALUE_ALLOCED; - } - i++; + GET_DIGEST_P(kvp, key, HeaderDigest, c, f); + GET_DIGEST_P(kvp, key, DataDigest, c, f); + GET_NUM_P(kvp, key, MaxConnections, c->session, f, e); + GET_STR_C(kvp, key, TargetName, c->session, f); + GET_STR_C(kvp, key, InitiatorName, c->session, f); + GET_BOOL_P(kvp, key, InitialR2T, c->session, f); + GET_BOOL_P(kvp, key, ImmediateData, c->session, f); + GET_NUM_P(kvp, key, MaxRecvDataSegmentLength, c, f, e); + GET_NUM_P(kvp, key, MaxBurstLength, c->session, f, e); + GET_NUM_P(kvp, key, FirstBurstLength, c->session, f, e); + GET_NUM_P(kvp, key, DefaultTime2Wait, c->session, f, e); + GET_NUM_P(kvp, key, DefaultTime2Retain, c->session, f, e); + GET_NUM_P(kvp, key, MaxOutstandingR2T, c->session, f, e); + GET_BOOL_P(kvp, key, DataPDUInOrder, c->session, f); + GET_BOOL_P(kvp, key, DataSequenceInOrder, c->session, f); + GET_NUM_P(kvp, key, ErrorRecoveryLevel, c->session, f, e); + GET_STYPE_C(kvp, key, SessionType, c->session, f); + /* XXX handle TaskReporting */ + + if (f == 0) { + errno = EINVAL; + return 1; } - *nkvp = i; - return 0; + return e; } + +#undef GET_BOOL_P +#undef GET_DIGEST_P +#undef GET_NUM_P +#undef GET_STR_C +#undef GET_STYPE_C void conn_pdu_write(struct connection *c, struct pdu *p) Index: initiator.c =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/initiator.c,v diff -u -p -r1.20 initiator.c --- initiator.c 22 Jan 2025 16:06:36 -0000 1.20 +++ initiator.c 28 Jan 2025 02:10:14 -0000 @@ -48,6 +48,7 @@ struct task_logout { u_int8_t reason; }; +int conn_is_leading(struct connection *); struct kvp *initiator_login_kvp(struct connection *, u_int8_t); struct pdu *initiator_login_build(struct connection *, struct task_login *); @@ -308,49 +309,76 @@ initiator_nop_in_imm(struct connection * conn_task_issue(c, t); } +int +conn_is_leading(struct connection *c) +{ + return c == TAILQ_FIRST(&c->session->connections); +} + +#define MINE_NOT_DEFAULT(c, k) ((c)->mine.k != iscsi_conn_defaults.k) + struct kvp * initiator_login_kvp(struct connection *c, u_int8_t stage) { - struct kvp *kvp; - size_t nkvp; + struct kvp *kvp = NULL; + size_t i = 0, len; + const char *discovery[] = {"SessionType", "InitiatorName", + "AuthMethod", NULL}; + const char *leading_only[] = {"MaxConnections", "InitialR2T", + "ImmediateData", "MaxBurstLength", "FirstBurstLength", + "DefaultTime2Wait", "DefaultTime2Retain", "MaxOutstandingR2T", + "DataPDUInOrder", "DataSequenceInOrder", "ErrorRecoveryLevel", + NULL}; + const char *opneg_always[] = {"HeaderDigest", "DataDigest", NULL}; + const char *secneg[] = {"SessionType", "InitiatorName", "TargetName", + "AuthMethod", NULL}; + const char **p, **q; switch (stage) { case ISCSI_LOGIN_STG_SECNEG: - if (!(kvp = calloc(5, sizeof(*kvp)))) - return NULL; - kvp[0].key = "AuthMethod"; - kvp[0].value = "None"; - kvp[1].key = "InitiatorName"; - kvp[1].value = c->session->config.InitiatorName; - if (c->session->config.SessionType == SESSION_TYPE_DISCOVERY) { - kvp[2].key = "SessionType"; - kvp[2].value = "Discovery"; + len = sizeof(discovery) / sizeof(*discovery); + q = discovery; } else { - kvp[2].key = "SessionType"; - kvp[2].value = "Normal"; - kvp[3].key = "TargetName"; - kvp[3].value = c->session->config.TargetName; + len = sizeof(secneg) / sizeof(*secneg); + q = secneg; } + if (!(kvp = calloc(len + 1, sizeof(*kvp)))) + return NULL; + for (p = q; *p != NULL; i++, p++) + if (kvp_set_from_mine(&kvp[i], *p, c)) + goto fail; break; case ISCSI_LOGIN_STG_OPNEG: - if (conn_gen_kvp(c, NULL, &nkvp) == -1) - return NULL; - nkvp += 1; /* add slot for terminator */ - if (!(kvp = calloc(nkvp, sizeof(*kvp)))) + len = sizeof(opneg_always) / sizeof(*opneg_always); + if (conn_is_leading(c)) + len += sizeof(leading_only) / sizeof(*leading_only); + if (MINE_NOT_DEFAULT(c, MaxRecvDataSegmentLength)) + len++; + if (!(kvp = calloc(len + 1, sizeof(*kvp)))) return NULL; - if (conn_gen_kvp(c, kvp, &nkvp) == -1) { - kvp_free(kvp); - return NULL; - } + for (p = opneg_always; *p != NULL; i++, p++) + if (kvp_set_from_mine(&kvp[i], *p, c)) + goto fail; + if (conn_is_leading(c)) + for (p = leading_only; *p != NULL; i++, p++) + if (kvp_set_from_mine(&kvp[i], *p, c)) + goto fail; + if (MINE_NOT_DEFAULT(c, MaxRecvDataSegmentLength) && + kvp_set_from_mine(&kvp[i], "MaxRecvDataSegmentLength", c)) + goto fail; break; default: log_warnx("initiator_login_kvp: exit stage left"); return NULL; } return kvp; +fail: + kvp_free(kvp); + return NULL; } +#undef MINE_NOT_DEFAULT struct pdu * initiator_login_build(struct connection *c, struct task_login *tl) { Index: iscsid.h =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/iscsid.h,v diff -u -p -r1.23 iscsid.h --- iscsid.h 22 Jan 2025 16:06:36 -0000 1.23 +++ iscsid.h 28 Jan 2025 02:10:14 -0000 @@ -367,7 +367,7 @@ void conn_task_issue(struct connection * void conn_task_schedule(struct connection *); void conn_task_cleanup(struct connection *, struct task *); int conn_parse_kvp(struct connection *, struct kvp *); -int conn_gen_kvp(struct connection *, struct kvp *, size_t *); +int kvp_set_from_mine(struct kvp *, const char *, struct connection *); void conn_pdu_write(struct connection *, struct pdu *); void conn_fail(struct connection *); void conn_fsm(struct connection *, enum c_event);