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: Thu, 23 Jan 2025 15:14:42 +1030 On Wed, 22 Jan 2025 22:23:39 +1030 Jack Burton wrote: > On Wed, 22 Jan 2025 10:56:27 +0100 > Claudio Jeker wrote: > > Right now I think we should get something in that allows to connect > > to more targets and then see how we can improve the negotiation. > > Sounds good. Once the next round of commits are in I'll put together > a revised (and now should be much smaller) version of that. Done: see diff below. Still longer than I would have liked, but at least not as big as my original #3. I can't really see how to split the rest of this bit up sensibly any further. This diff is essentially my original #3 rebased on the current tree, except for these 3 further changes: 1. in conn_parse_kvp(), initialised err to NULL. Without that I was seeing all sorts of bogus errors today (which didn't happen when I first tested #3 a few weeks back ... most likely that was just dumb luck the first time around, undone by random memory layout changes from an unrelated commit last night). 2. no longer allocating kvp keys at all, anywhere (as discussed on the list a few days ago). I've left the ability to do so in place, as there's a reasonable chance we'll need it again for full negotiation (if the kvp api survives until then). In kvp_set_from_mine() I'm still allocing kvp values only in GET_NUM_P. This could easily be changed to allocing all kvp values everywhere if you really want to get rid of the kvp flags member (but that probably belongs as a separate patch after this one). 3. in GET_DIGEST_P included ",None" as a fallback (which I really should have done in the first place). With this diff applied, iscsid(8) can establish sessions to LIO targets successfully. Also tested against IET (which has worked since a few commits ago) and netbsd-iscsi-target from ports (which has always worked); can't see any regressions there. Yes it's a bit ugly in places, but we'll probably end up throwing some of those bits away when we implement real negotiation and at least with this in place we support LIO targets (probably one of the most common today) and most likely some others that didn't work before too. Would appreciate test reports against other targets from anyone who has access to them. 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 23 Jan 2025 03:59:18 -0000 @@ -300,7 +300,7 @@ conn_parse_kvp(struct connection *c, str { struct kvp *k; struct session *s = c->session; - const char *err; + const char *err = NULL; int errors = 0; @@ -335,41 +335,106 @@ 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) -{ - struct session *s = c->session; - size_t i = 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++; +kvp_set_from_mine(struct kvp *kvp, const char *key, struct connection *c) { + 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; + + /* 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 23 Jan 2025 03:59:18 -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,48 +309,73 @@ initiator_nop_in_imm(struct connection * conn_task_issue(c, t); } +int +conn_is_leading(struct connection *c) +{ + struct connection *lead; + + if (c == NULL || c->session == NULL + || (lead = TAILQ_FIRST(&c->session->connections)) == NULL) + return 0; + return (c == lead) ? 1 : 0; +} + +#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 *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; switch (stage) { case ISCSI_LOGIN_STG_SECNEG: - if (!(kvp = calloc(5, sizeof(*kvp)))) + len = sizeof(secneg) / sizeof(*secneg); + if (!(kvp = calloc(len + 1, 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"; - } else { - kvp[2].key = "SessionType"; - kvp[2].value = "Normal"; - kvp[3].key = "TargetName"; - kvp[3].value = c->session->config.TargetName; - } + for (p = secneg; *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)))) - return NULL; - if (conn_gen_kvp(c, kvp, &nkvp) == -1) { - kvp_free(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; - } + 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 23 Jan 2025 03:59:18 -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); Index: session.c =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/session.c,v diff -u -p -r1.13 session.c --- session.c 22 Jan 2025 16:06:36 -0000 1.13 +++ session.c 23 Jan 2025 03:59:18 -0000 @@ -103,6 +103,10 @@ session_config(struct session *s, struct fatal("strdup"); } else s->config.InitiatorName = default_initiator_name(); + if (!s->config.HeaderDigest) + s->config.HeaderDigest = DIGEST_NONE; + if (!s->config.DataDigest) + s->config.DataDigest = DIGEST_NONE; } void