Bug 259439 - ctld: MaxRecvDataSegmentLength is sent only in response to initiator's MaxRecvDataSegmentLength
Summary: ctld: MaxRecvDataSegmentLength is sent only in response to initiator's MaxRec...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: John Baldwin
URL: https://reviews.freebsd.org/D32651
Keywords:
Depends on:
Blocks:
 
Reported: 2021-10-25 14:58 UTC by Ed Maste
Modified: 2022-05-25 19:04 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ed Maste freebsd_committer freebsd_triage 2021-10-25 14:58:22 UTC
PR259355 reports an issue with the iSCSI initiator relating to MaxRecvDataSegmentLength. jhb@ reports ctld has a similar issue - some more detail in https://reviews.freebsd.org/D32605. Quoting from that review:

---
} 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?
---
Comment 1 Alexander Motin freebsd_committer freebsd_triage 2021-10-25 20:07:14 UTC
As I have told in https://reviews.freebsd.org/D32605, unlike the initiator, for target this is not a bug, at most a sub-optimal parameter negotiation.  We may indeed try to unconditionally announce our MaxRecvDataSegmentLength value, but what if initiator not sending its own value will try to reject the received one?  Our minimalistic negotiation code is not ready for real disputes.  I haven't see many initiators not sending MaxRecvDataSegmentLength, so can't guess what can be on their minds.
Comment 2 Ed Maste freebsd_committer freebsd_triage 2021-10-25 21:03:58 UTC
(In reply to Alexander Motin from comment #1)
Thanks, this makes sense and I agree it should pose no functional issue (although I can't see a reason why an initiator would want to reject MaxRecvDataSegmentLength).

I guess we could assume that an initiator that does not send MaxRecvDataSegmentLength does not particularly care about performance, so it doesn't really matter.
Comment 3 commit-hook freebsd_committer freebsd_triage 2021-10-26 21:56:12 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=7ef7b252adc0152e5f726d00640124c5de0909a9

commit 7ef7b252adc0152e5f726d00640124c5de0909a9
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2021-10-26 21:52:40 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2021-10-26 21:52:40 +0000

    ctld: Always declare MaxRecvDataSegmentLength.

    This key is Declarative and should always be sent even if the
    initiator did not send it's own limit.  This is similar to the fix in
    fc79cf4fea72 but for the target side.  However, unlike that fix,
    failure to send the key simply results in reduced performance.

    PR:             259439
    Reviewed by:    mav, emaste
    Sponsored by:   Chelsio Communications
    Differential Revision:  https://reviews.freebsd.org/D32651

 usr.sbin/ctld/login.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
Comment 4 commit-hook freebsd_committer freebsd_triage 2021-11-23 23:14:25 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=b3d02f0be3ecd4a08fcd4b9ca366f9cf56418356

commit b3d02f0be3ecd4a08fcd4b9ca366f9cf56418356
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2021-10-26 21:52:40 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2021-11-23 23:11:44 +0000

    ctld: Always declare MaxRecvDataSegmentLength.

    This key is Declarative and should always be sent even if the
    initiator did not send it's own limit.  This is similar to the fix in
    fc79cf4fea72 but for the target side.  However, unlike that fix,
    failure to send the key simply results in reduced performance.

    PR:             259439
    Reviewed by:    mav, emaste
    Sponsored by:   Chelsio Communications
    Differential Revision:  https://reviews.freebsd.org/D32651

    (cherry picked from commit 7ef7b252adc0152e5f726d00640124c5de0909a9)

 usr.sbin/ctld/login.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)