From: Claudio Jeker Subject: Re: [patch 3 of 3] iscsid(8): support a wider variety of iscsi targets To: Jack Burton Cc: tech@openbsd.org Date: Wed, 22 Jan 2025 10:56:27 +0100 On Wed, Jan 22, 2025 at 05:25:33PM +1030, Jack Burton wrote: > On Tue, 21 Jan 2025 17:21:33 +0100 > Claudio Jeker 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. Yes, I only picked one bit out of your diff. I will also commit the diff below afterwards. It is another bit of the puzzle. > 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. That is exactly what I try to do. Reduce the problem down. > 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. Agreed. I think the one that may make sense as a userland knob is MaxConnections and even that one is questionable. OK maybe the timeouts (DefaultTime2Wait and DefaultTime2Retain) but that is for later anyway. The problem I see is that InitialR2T, ImmediateData, DataPDUInOrder and DataSequenceInOrder are buttons we may need to negotiate. Also I would like to bump MaxRecvDataSegmentLength to 64k. OpenBSD only does up to 64k writes to a disk since that is MAXPHYS. > 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. That is a good point. > 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. Yes, when I hacked up iscsid I tried to dodge that bullet but at one point we need to implement proper negotiation. > 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). Right now I think we should get something in that allows to connect to more targets and then see how we can improve the negotiation. I also want to look at the kvp api and make that one better. I dislike the macros but have not come up with a better solution yet. -- :wq Claudio