On Wed, Jan 22, 2025 at 05:25:33PM +1030, Jack Burton wrote:
> 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.

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