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)