On Thu, Mar 16, 2017 at 08:32:42PM +0100, Bruno Flueckiger wrote: > >Synopsis: iscsid(8) cannot connect to newer Linux targets > >Category: system > >Environment: > System : OpenBSD 6.1 > Details : OpenBSD 6.1-beta (GENERIC.MP) #26: Wed Mar 15 22:22:37 > MDT 2017 > > [email protected]:/usr/src/sys/arch/amd64/compile/GENERIC.MP > > Architecture: OpenBSD.amd64 > Machine : amd64 > >Description: > The iSCSI targets of CentOS 7 and Synology DSM 6.0.2-8451 refuse to > establish a connection with iscsid(8). Both systems complain about > missing parameters during the security negotiation phase (CentOS) > or the operational parameter negotiation phase (DSM). > Besides Synology DSM and CentOS I've tested the connection to the > iSCSI targets of the following operating systems: FreeBSD 11, > NetBSD 7 and Debian 8 (has an old iSCSI implementation for Linux). > The connection to these three systems is established without pro- > blems.
Thanks for the report and the diff. I will rework your diff a bit and see how to fix the kvp memory leaks correctly. Guess I need to setup a centos 7 box for testing. > >How-To-Repeat: > Way 1: Install any Linux-based OS with a kernel that uses the LIO > iSCSI fabric module (http://linux-iscsi.org/wiki/ISCSI), configure > a LUN accroding to the instructions in the link and try to connect > to it using iscsid(8). > Way 2: Create a LUN on a Synology product running a recent version > of DSM and try to connect to it. > >Fix: > The following four diffs modify iscsid(8) in a way that the mis- > sing parameters are advertised during the security negotiation > phase and the operational parameter negotiation phase. > > Index: usr.sbin/iscsid/iscsid.c > =================================================================== > RCS file: /cvs/src/usr.sbin/iscsid/iscsid.c,v > retrieving revision 1.20 > diff -u -p -r1.20 iscsid.c > --- usr.sbin/iscsid/iscsid.c 23 Jan 2017 08:40:07 -0000 1.20 > +++ usr.sbin/iscsid/iscsid.c 2 Mar 2017 14:40:44 -0000 > @@ -254,6 +254,8 @@ iscsid_ctrl_dispatch(void *ch, struct pd > control_compose(ch, CTRL_FAILURE, NULL, 0); > goto done; > } > + s->config.HeaderDigest = SESSION_DIGEST_NONE; > + s->config.DataDigest = SESSION_DIGEST_NONE; > } > > session_config(s, sc); > > Index: usr.sbin/iscsid/iscsid.h > =================================================================== > RCS file: /cvs/src/usr.sbin/iscsid/iscsid.h,v > retrieving revision 1.16 > diff -u -p -r1.16 iscsid.h > --- usr.sbin/iscsid/iscsid.h 2 Sep 2016 16:22:31 -0000 1.16 > +++ usr.sbin/iscsid/iscsid.h 2 Mar 2017 14:40:54 -0000 > @@ -181,6 +181,9 @@ struct session_config { > u_int8_t disabled; > }; > > +#define SESSION_DIGEST_NONE 0 > +#define SESSION_DIGEST_CRC32 1 > + > #define SESSION_TYPE_NORMAL 0 > #define SESSION_TYPE_DISCOVERY 1 > > Index: usr.sbin/iscsid/initiator.c > =================================================================== > RCS file: /cvs/src/usr.sbin/iscsid/initiator.c,v > retrieving revision 1.15 > diff -u -p -r1.15 initiator.c > --- usr.sbin/iscsid/initiator.c 16 Jan 2015 15:57:06 -0000 1.15 > +++ usr.sbin/iscsid/initiator.c 3 Mar 2017 10:08:46 -0000 > @@ -250,39 +250,106 @@ initiator_nop_in_imm(struct connection * > conn_task_issue(c, t); > } > > +#define WRITE_BOOL(k, v) \ > +do { \ > + if (v) \ > + k = "Yes"; \ > + else \ > + k = "No"; \ > +} while (0) > + > +#define WRITE_NUM(k, v) \ > +do { \ > + if (asprintf(&k, "%hu", v) == -1) \ > + return NULL; \ > +} while (0) > + > +#define WRITE_INT(k, v) \ > +do { \ > + if (asprintf(&k, "%u", v) == -1) \ > + return NULL; \ > +} while (0) > + > +#define WRITE_DIGEST(k, v) \ > +do { \ > + if (v) \ > + k = "CRC32"; \ > + else \ > + k = "None"; \ > +} while (0) \ > + There are some memory leaks hiding here. At least the asprinf() version leak memory. Just realized that the current code does more or less the same :( > struct kvp * > initiator_login_kvp(struct connection *c, u_int8_t stage) > { > struct kvp *kvp; > - size_t nkvp; > + struct session *s; > > switch (stage) { > case ISCSI_LOGIN_STG_SECNEG: > - if (!(kvp = calloc(4, sizeof(*kvp)))) > + 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; > + kvp[2].key = "SessionType"; > > if (c->session->config.SessionType == SESSION_TYPE_DISCOVERY) { > - kvp[2].key = "SessionType"; > kvp[2].value = "Discovery"; > } else { > - kvp[2].key = "TargetName"; > - kvp[2].value = c->session->config.TargetName; > + kvp[2].value = "Normal"; > + kvp[3].key = "TargetName"; > + kvp[3].value = c->session->config.TargetName; > } > 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) { > - free(kvp); > + if (!(kvp = calloc(15, sizeof(*kvp)))) > return NULL; > - } > + > + s = c->session; > + > + kvp[0].key = "MaxConnections"; > + WRITE_NUM(kvp[0].value, s->mine.MaxConnections); > + > + kvp[1].key = "InitialR2T"; > + WRITE_BOOL(kvp[1].value, s->mine.InitialR2T); > + > + kvp[2].key = "ImmediateData"; > + WRITE_BOOL(kvp[2].value, s->mine.ImmediateData); > + > + kvp[3].key = "MaxRecvDataSegmentLength"; > + WRITE_INT(kvp[3].value, c->mine.MaxRecvDataSegmentLength); > + > + kvp[4].key = "MaxBurstLength"; > + WRITE_INT(kvp[4].value, s->mine.MaxBurstLength); > + > + kvp[5].key = "FirstBurstLength"; > + WRITE_INT(kvp[5].value, s->mine.FirstBurstLength); > + > + kvp[6].key = "HeaderDigest"; > + WRITE_DIGEST(kvp[6].value, s->config.HeaderDigest); > + > + kvp[7].key = "DataDigest"; > + WRITE_DIGEST(kvp[7].value, s->config.DataDigest); > + > + kvp[8].key = "MaxOutstandingR2T"; > + WRITE_NUM(kvp[8].value, s->mine.MaxOutstandingR2T); > + > + kvp[9].key = "DataPDUInOrder"; > + WRITE_BOOL(kvp[9].value, s->mine.DataPDUInOrder); > + > + kvp[10].key = "DataSequenceInOrder"; > + WRITE_BOOL(kvp[10].value, s->mine.DataSequenceInOrder); > + > + kvp[11].key = "ErrorRecoveryLevel"; > + WRITE_NUM(kvp[11].value, s->mine.ErrorRecoveryLevel); > + > + kvp[12].key = "DefaultTime2Wait"; > + WRITE_NUM(kvp[12].value, s->mine.DefaultTime2Wait); > + > + kvp[13].key = "DefaultTime2Retain"; > + WRITE_NUM(kvp[13].value, s->mine.DefaultTime2Retain); > + Hmm. If I remember the iscsi RFC correctly there is no need to send default values to the target. So either I missed something in the RFC or those targets are not behaving right. Do you know which params Linux is complaining about (or is it realy all of them...). > break; > default: > log_warnx("initiator_login_kvp: exit stage left"); > @@ -290,6 +357,11 @@ initiator_login_kvp(struct connection *c > } > return kvp; > } > + > +#undef WRITE_DIGEST > +#undef WRITE_INT > +#undef WRITE_NUM > +#undef WRITE_BOOL > > struct pdu * > initiator_login_build(struct connection *c, struct task_login *tl) > > Index: usr.sbin/iscsid/connection.c > =================================================================== > RCS file: /cvs/src/usr.sbin/iscsid/connection.c,v > retrieving revision 1.21 > diff -u -p -r1.21 connection.c > --- usr.sbin/iscsid/connection.c 5 Dec 2015 06:38:18 -0000 1.21 > +++ usr.sbin/iscsid/connection.c 2 Mar 2017 10:17:13 -0000 > @@ -316,44 +316,6 @@ log_debug("conn_parse_kvp: %s = %s", k-> > #undef SET_NUM > #undef SET_BOOL > > -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 = strdup("MaxConnections"); > - if (kvp[i].key == NULL) > - return -1; > - if (asprintf(&kvp[i].value, "%hu", > - s->mine.MaxConnections) == -1) { > - kvp[i].value = NULL; > - return -1; > - } > - } > - i++; > - } > - if (c->mine.MaxRecvDataSegmentLength != > - iscsi_conn_defaults.MaxRecvDataSegmentLength) { > - if (kvp && i < *nkvp) { > - kvp[i].key = strdup("MaxRecvDataSegmentLength"); > - if (kvp[i].key == NULL) > - return -1; > - if (asprintf(&kvp[i].value, "%u", > - c->mine.MaxRecvDataSegmentLength) == -1) { > - kvp[i].value = NULL; > - return -1; > - } > - } > - i++; > - } > - > - *nkvp = i; > - return 0; > -} > - > void > conn_pdu_write(struct connection *c, struct pdu *p) > { > -- :wq Claudio
