iscsid: set max_recv_data_segment_length to what we advertise

[emaste]

Summary

Previously we updated the conection's conn_max_recv_data_segment_length
only when we received a response containing MaxRecvDataSegmentLength
from the target.  If the target did not send MaxRecvDataSegmentLength
then we left conn_max_recv_data_segment_length at the default (i.e.,
8192).

RFC 7143 does not require the target to send MaxRecvDataSegmentLength as
an ack.  Instead, just set conn_max_recv_data_segment_length to our
advertised value in login_negotiate().

PR: 259355
Fixes: R10:a15fbc904a4d ("Alike to r312190 decouple iSCSI...")
Sponsored by: The FreeBSD Foundation

[emaste]

@jhb points out that ctld has a similar issue, from usr.sbin/ctld/login.c:

} else if (strcmp(name, "MaxRecvDataSegmentLength") == 0) {
        tmp = strtoul(value, NULL, 10);
        if (tmp <= 0) {
                login_send_error(request, 0x02, 0x00);
                log_errx(1, "received invalid "
                    "MaxRecvDataSegmentLength");
        }

        /*
         * MaxRecvDataSegmentLength is a direction-specific parameter.
         * We'll limit our _send_ to what the initiator can handle but
         * our MaxRecvDataSegmentLength is not influenced by the
         * initiator in any way.
         */
        if ((int)tmp > conn->conn_max_send_data_segment_limit) {
                log_debugx("capping MaxRecvDataSegmentLength "
                    "from %zd to %d", tmp,
                    conn->conn_max_send_data_segment_limit);
                tmp = conn->conn_max_send_data_segment_limit;
        }
        conn->conn_max_send_data_segment_length = tmp;
        conn->conn_max_recv_data_segment_length =
            conn->conn_max_recv_data_segment_limit;
        keys_add_int(response_keys, name,
            conn->conn_max_recv_data_segment_length);

so conn_max_recv_data_segment_length is only set if the initiator provides a MaxRecvDataSegmentLength (and it's only in that case that we send MaxRecvDataSegmentLength back to the initiator). Presumably MaxRecvDataSegmentLength should be sent unconditionally. Same for all Declarative keys?

[emaste]

In any case I'll look at the ctld issue separately, later on. The change in this review addresses an issue we're currently encountering with the initiator and has been tested.

[mav]

This looks like a valid issue for initator. Remote side is not required to acknowledge the parameter, since instead it _may_ send back its own limit. It may try to reject the value, but not sure why would it, or why would we care about the reject. So once we announced our value, we must accept PDUs of that size. The only question is where to set it cleaner: as proposed or in connection_new(). Note that iSER's InitiatorRecvDataSegmentLength has different negotiation logic (uses minimum, and so it is acknowledged) for the same parameter, but I think it should fit either way.

For target though I don't think there is a bug. At most sub-optimal behavior in rare scenario: if initiator haven't sent MaxRecvDataSegmentLength, relying on the default value of 8K, then ctld won't announce its own limit either, that means also using default, that is consistent. There is a chance that initiator not supporting large PDUs receive (and for that cause not sending MaxRecvDataSegmentLength) may still be able to send them to the target if is proposed via its MaxRecvDataSegmentLength, for which case ctld could try to announce it, but I am not sure it is big.

[emaste]

The only question is where to set it cleaner: as proposed or in connection_new().

Yeah, that wasn't completely clear to me at first and is part of why I wasn't sure this was the correct fix at first. After some more research this location makes sense to me as it is only after we have advertised MaxRecvDataSegmentLength that the target can send that much.

Discussion about this behaviour in the target can carry on in the PR, but I agree with your logic.