On Mon, Mar 20, 2017 at 10:46:19PM +0100, Claudio Jeker wrote: > 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. >
I still have my test VMs around, so if I can help out with additional testing I'd happy to do so. > > >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 :( > I've taken the existing code as a template for my diff and I wasn't aware of the memory leaks. > > 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...). > Unfortunately the CentOS targets just returns the status code 0x0207 in the login response (according to RFC 7143 Section 11.13.5.). So I've checked the source of the Linux kernel 3.10 and found that it requires the following parameter to be set: - HeaderDigest - DataDigest - MaxOutstandingR2T - DataPDUInOrder - DataSequenceInOrder - ErrorRecoveryLevel Additionally the CentOs 7 target reuires the parameter Discovery to be set during the security negotiation phase, else it refuses the connection with an error message that this parameter is missing. On the Synology DSM target the situation is different due to modifications the manufacturer has made to the iSCSI target. In the tcpdump of the failed login I can see that it seems to require the following parameters: - HeaderDigest - DataDigest - MaxConnections - InitialR2T - ImmediateData - MaxRecvDataSegmentLength - MaxBurstLength - FirstBurstLength - MaxOutstandingR2T - DataPDUInOrder - DataSequenceInOrder - ErrorRecoveryLevel This list is based on a packet capture I've taken. You can get the pcap file from here: https://www.bsdhowto.ch/wp-content/uploads/2017/03/synology_target.pcap The RFC 7143 states in Section 6 that most of the parameters can be proposed by both initiator and target. Each proposal made by one side must be answered by the other side. The existing code in iscsid repeats the sending of the initial parameters if the T bit in the response is clear. So I saw to ways for patching: either modify the code to detect what parameters the targets wants to see or simply send all available parameters in the first login request even if they have default values. > > 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 Cheers, Bruno
