On Tue, 21 Jan 2025 17:21:33 +0100
Claudio Jeker <cjeker@diehard.n-r-g.com> wrote:
> 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.

That's a good start, but it's not enough to support lio targets.

To begin with, we'd also need to send SessionType even when it's normal
(my point 1 which you agreed with above).  This patch (one of my
earlier attempts) applied on top of your patch from yesterday will do
that:

cvs server: Diffing .
Index: initiator.c
===================================================================
RCS file: /cvs/src/usr.sbin/iscsid/initiator.c,v
diff -u -p -r1.15 initiator.c
--- initiator.c	16 Jan 2015 15:57:06 -0000	1.15
+++ initiator.c	29 Dec 2024 08:56:05 -0000
@@ -258,19 +258,20 @@ initiator_login_kvp(struct connection *c
 
 	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:


However, even with that iscsid still can't reach ffp with lio targets
because we're not responding to a bunch of proposals made by the target
(which only gets to make those proposals because we don't get in
first).  That was my point 3 above.

Still, I see no harm in using your latest patch (plus my trivial patch
above, needed for the first pdu) as the next step.  It doesn't break
anything new and it reduces the size of the outstanding issues nicely.

I agree that proposing defaults first every time is not ideal, but I
don't really see the point of implementing negotiation fully unless we
intend to expose a bunch more parameters to user-visible knobs, most
which I doubt would ever be needed anyway.  One of the strengths of
iscsi.conf(5) as it stands is its simplicity.

conn_gen_kvp() as it stands only proposes (at most) MaxConnections &
MaxRecvDataSegmentLength.  Either we need to get in first to propose a
bunch more params than that; or alternatively we need to start
*responding* to all (non-declarative) proposals made by the target
*and* implement robust handling of rejections.

The nice thing about proposing defaults is that RFC 7143 requires
participants to accept those defaults (except for auth, but we don't
support auth yet anyway).  So if we propose everything we're allowed to
propose and in each case we propose the defaults (unless there's a good
reason to choose something else, like we do already with
MaxRecvDataSegmentLength for example), then we can be assured that any
compliant target won't reject our proposals *and* won't send us any
other (non-declarative) proposals to deal with.

I think the "get in first proposing everything we're allowed to"
approach is at least a good start.  It is sufficient to enable using
lio targets, with the subset of iscsi features we support today.

On the other hand, if/when we add support for e.g. crc32c digests or
chap auth (the only two extra knobs to twiddle which might be useful in
some circumstances) then we'll need real negotiation to work.

So if you see adding either of those as desirable sometime soon, then
yes it's probably worth implementing negotiation fully (generating
appropriate responses to a target's proposals [which we get many of
because we don't get in first to propose most params] and gracefully
handling a target's rejections of our proposals [which right now can
only happen if the target requires auth]) now, so that it's in place
before we start implementing more of the standard.

Otherwise, "get in first" is likely to keep things much simpler.

What do you think?  I don't mind having a go at full negotiation if
that's the approach you prefer.  Only reason I avoided it to begin with
was that there didn't seem much point adding the extra complexity when
our current desired end state is all-defaults anyway (except for
MaxRecvDataSegmentLength, but that's declarative so negotiation
doesn't come into play).