From: Claudio Jeker Subject: Re: [patch 3 of 3] iscsid(8): support a wider variety of iscsi targets To: Jack Burton , tech@openbsd.org Date: Tue, 21 Jan 2025 17:21:33 +0100 On Tue, Jan 21, 2025 at 11:06:26AM +0100, Claudio Jeker wrote: > On Tue, Dec 31, 2024 at 06:51:39PM +1030, Jack Burton wrote: > > This patch allows iscsid(8) to connect successfully to & use a wider > > variety of iscsi targets. > > > > It solves three current issues and makes a start on a fourth one: > > > > 1. Some common iscsi targets refuse to complete SECNEG if we do not > > send a SessionType proposal, even for normal sessions (which > > according to RFC7143 is supposed to be the default). Currently > > iscsid(8) only sends a SessionType proposal for discovery sessions. > > I think it is fine to send this all the time if it makes some iscsi > targets happy. > > > 2. Some common iscsi targets complain if we send the same proposal > > more than once during login (in SECNEG and/or OPNEG). Currently, if > > iscsid(8) sends MaxConnections or MaxRecvDataSegmentLength at all > > then it keeps sending them over & over again throughout OPNEG until > > the login phase succeeds or fails (but if they're not accepted the > > first time, it will always end up failing). > > > > 3. RFC7143 requires a response to every (non-declarative) proposal and > > some common iscsi targets enforce that requirement. Most proposals can > > be made either by the initiator or by the target. Since iscsid(8) > > currently only sends between 0 and 2 proposals in OPNEG, many common > > iscsi targets end up sending proposals to us, which we don't handle. > > I think 2 and 3 are mainly a problem that the negotiation is not correctly > implemented. I never had many targets to test and most of them I used > always used defaults and did not like any other value. So I could never > properly play with the negotiation phase. > > At least reading your #2 makes me think that iscsid does not properly > handle rejected values. > > So if I understand you correctly we should send out every non-declarative > key=value pair so that the target can just reply to those. > Now for some of the parameters iscsid is very flexible and so it is maybe > tricky to define good initial values. > I really need to read Section 6 of RFC7143... right now I dislike to send > out default values all the time (feels like an inelegant solution to me). > > I will check out your diff anyway and see what I can cherry-pick already. I cherry-picked the bits to set the HeaderDigest and DataDigest. I did change a few things, most prominently the param merge can't be a simple MIN(). I refactored text_to_digest() a bit. Also the setting of the digests was moved to connection_new() plus some other cleanup around the various params. Please have a look if that works for you. -- :wq Claudio Index: connection.c =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/connection.c,v diff -u -p -r1.22 connection.c --- connection.c 16 Jan 2025 16:19:39 -0000 1.22 +++ connection.c 21 Jan 2025 15:55:43 -0000 @@ -66,10 +66,11 @@ conn_new(struct session *s, struct conne c->cid = arc4random(); c->config = *cc; c->mine = initiator_conn_defaults; - c->mine.HeaderDigest = s->config.HeaderDigest; - c->mine.DataDigest = s->config.DataDigest; + if (s->config.HeaderDigest != 0) + c->mine.HeaderDigest = s->config.HeaderDigest; + if (s->config.DataDigest != 0) + c->mine.DataDigest = s->config.DataDigest; c->his = iscsi_conn_defaults; - c->active = iscsi_conn_defaults; TAILQ_INIT(&c->pdu_w); TAILQ_INIT(&c->tasks); @@ -262,7 +263,6 @@ do { \ (p)->key, (p)->value, err); \ errors++; \ } \ -log_debug("SET_NUM: %s = %llu", #v, (u_int64_t)(x)->his.v); \ } \ } while (0) @@ -275,7 +275,18 @@ do { \ (p)->key, (p)->value, err); \ errors++; \ } \ -log_debug("SET_BOOL: %s = %u", #v, (int)(x)->his.v); \ + } \ +} while (0) + +#define SET_DIGEST(p, x, v) \ +do { \ + if (!strcmp((p)->key, #v)) { \ + (x)->his.v = text_to_digest((p)->value, &err); \ + if (err) { \ + log_warnx("bad param %s=%s: %s", \ + (p)->key, (p)->value, err); \ + errors++; \ + } \ } \ } while (0) @@ -304,6 +315,8 @@ log_debug("conn_parse_kvp: %s = %s", k-> SET_BOOL(k, s, DataSequenceInOrder); SET_NUM(k, s, ErrorRecoveryLevel, 0, 2); SET_NUM(k, c, MaxRecvDataSegmentLength, 512, 16777215); + SET_DIGEST(k, c, HeaderDigest); + SET_DIGEST(k, c, DataDigest); } if (errors) { @@ -315,6 +328,7 @@ log_debug("conn_parse_kvp: %s = %s", k-> #undef SET_NUM #undef SET_BOOL +#undef SET_DIGEST int conn_gen_kvp(struct connection *c, struct kvp *kvp, size_t *nkvp) Index: initiator.c =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/initiator.c,v diff -u -p -r1.16 initiator.c --- initiator.c 16 Jan 2025 16:19:39 -0000 1.16 +++ initiator.c 21 Jan 2025 14:24:07 -0000 @@ -58,7 +58,6 @@ void initiator_login_cb(struct connectio void initiator_discovery_cb(struct connection *, void *, struct pdu *); void initiator_logout_cb(struct connection *, void *, struct pdu *); - struct session_params initiator_sess_defaults; struct connection_params initiator_conn_defaults; Index: iscsid.c =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/iscsid.c,v diff -u -p -r1.22 iscsid.c --- iscsid.c 16 Apr 2021 14:37:06 -0000 1.22 +++ iscsid.c 21 Jan 2025 15:54:24 -0000 @@ -54,11 +54,13 @@ const struct session_params iscsi_sess_d .ImmediateData = 1, .DataPDUInOrder = 1, .DataSequenceInOrder = 1, - .ErrorRecoveryLevel = 0 + .ErrorRecoveryLevel = 0, }; const struct connection_params iscsi_conn_defaults = { - .MaxRecvDataSegmentLength = 8192 + .MaxRecvDataSegmentLength = 8192, + .HeaderDigest = DIGEST_NONE, + .DataDigest = DIGEST_NONE, }; int @@ -334,6 +336,8 @@ void iscsi_merge_sess_params(struct session_params *res, struct session_params *mine, struct session_params *his) { + memset(res, 0, sizeof(*res)); + MERGE_MIN(res, mine, his, MaxBurstLength); MERGE_MIN(res, mine, his, FirstBurstLength); MERGE_MAX(res, mine, his, DefaultTime2Wait); @@ -353,8 +357,26 @@ void iscsi_merge_conn_params(struct connection_params *res, struct connection_params *mine, struct connection_params *his) { + int mask; + + memset(res, 0, sizeof(*res)); + res->MaxRecvDataSegmentLength = his->MaxRecvDataSegmentLength; - /* XXX HeaderDigest and DataDigest */ + + /* for digest select first bit that is set in both his and mine */ + mask = mine->HeaderDigest & his->HeaderDigest; + mask = ffs(mask) - 1; + if (mask == -1) + res->HeaderDigest = 0; + else + res->HeaderDigest = 1 << mask; + + mask = mine->DataDigest & his->DataDigest; + mask = ffs(mask) - 1; + if (mask == -1) + res->DataDigest = 0; + else + res->DataDigest = 1 << mask; } #undef MERGE_MIN Index: iscsid.h =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/iscsid.h,v diff -u -p -r1.20 iscsid.h --- iscsid.h 16 Jan 2025 16:19:39 -0000 1.20 +++ iscsid.h 21 Jan 2025 15:34:48 -0000 @@ -181,6 +181,9 @@ struct session_config { u_int8_t disabled; }; +#define DIGEST_NONE 0x1 +#define DIGEST_CRC32C 0x2 + #define SESSION_TYPE_NORMAL 0 #define SESSION_TYPE_DISCOVERY 1 @@ -369,6 +372,7 @@ int text_to_pdu(struct kvp *, struct pdu struct kvp *pdu_to_text(char *, size_t); u_int64_t text_to_num(const char *, u_int64_t, u_int64_t, const char **); int text_to_bool(const char *, const char **); +int text_to_digest(const char *, const char **); void pdu_free_queue(struct pduq *); ssize_t pdu_read(struct connection *); ssize_t pdu_write(struct connection *); Index: pdu.c =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/pdu.c,v diff -u -p -r1.13 pdu.c --- pdu.c 12 Apr 2021 10:03:33 -0000 1.13 +++ pdu.c 21 Jan 2025 15:48:44 -0000 @@ -193,23 +193,42 @@ text_to_bool(const char *buf, const char { int val = 0; - if (!strcmp(buf, "Yes")) { + if (strcmp(buf, "Yes") == 0) val = 1; - errno = 0; - } else if (!strcmp(buf, "No")) - errno = 0; - else - errno = EINVAL; - - if (errstrp != NULL) { - if (errno == 0) - *errstrp = NULL; - else + else if (strcmp(buf, "No") != 0) { + if (errstrp != NULL) *errstrp = "invalid"; } return val; } +int +text_to_digest(const char *buf, const char **errstrp) +{ + int val = 0; + size_t len; + const char *p; + + while (buf != NULL) { + p = strchr(buf, ','); + if (p == NULL) + len = strlen(buf); + else + len = p++ - buf; + + if (strncmp(buf, "None", len) == 0) + val |= DIGEST_NONE; + else if (strncmp(buf, "CRC32C", len) == 0) + val |= DIGEST_CRC32C; + else { + if (errstrp != NULL) + *errstrp = "invalid"; + return 0; + } + buf = p; + } + return val; +} /* * Internal functions to send/recv pdus. Index: session.c =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/session.c,v diff -u -p -r1.10 session.c --- session.c 16 Jan 2025 16:17:32 -0000 1.10 +++ session.c 21 Jan 2025 15:55:33 -0000 @@ -289,7 +289,6 @@ sess_do_start(struct session *s, struct /* initialize the session params */ s->mine = initiator_sess_defaults; s->his = iscsi_sess_defaults; - s->active = iscsi_sess_defaults; if (s->config.SessionType != SESSION_TYPE_DISCOVERY && s->config.MaxConnections)