LUN addressing compatible with libiscsi (number > 255)

[mizhka]

Summary

Hosts under test:

• FreeBSD 13/14 with ctld
• KVM/qemu virtual machines connected by iscsi to FreeBSD hosts

The libiscsi uses 2 bytes for LUN addressing:
https://github.com/sahlberg/libiscsi/blob/1017435ca9681236b6c993202a75b7b44d45d1ea/lib/pdu.c#L704

On other hand, FreeBSD CAM is compliant with SCSI Architecture model 5 (item 4.7)
https://github.com/freebsd/freebsd-src/blob/main/sys/cam/ctl/ctl.c#L3763

These 2 formats of LUN addressing are not compatible in general. It works under conditions:

• If LUN number is less than 256, libiscsi works fine with FreeBSD iscsi host.
• If LUN number is equal or more than 256, FreeBSD can't retrieve LUN number sent by libiscsi.

This patch is minimal changes to handle libiscsi LUNs higher than 256.

We will appreciate if patch will be accepted. If not, please advise how to implement correctly.

Thank you!

Test Plan

Create RAM disk or ZVOL.
Register it via ctladm with LUN number higher than 256 (ex. 400)
Connect to LUN from QEMU via libiscsi.

[imp]

sys/cam/ctl/ctl.c

3772

It would be simpler to test if either ofcthe bytes have bits set and use the formuls in both cases.

[mav]

@mizhka, if you look on ctl_encode_lun(), you may see it encodes LUNs above 255 as RPL_LUNDATA_ATYP_FLAT when reporting them in REPORT LUNS response Proper modern initiator should not decode the 8-byte LUN number, but use it as received. I suppose libiscsi decodes RPL_LUNDATA_ATYP_FLAT, but on request encodes it back as RPL_LUNDATA_ATYP_PERIPH, which from CTL perspective is a different LUN. This is a quote from SAM specification:

Subclause 4.7 defines the construction of LUNs to be used by SCSI target devices. Application clients should use only those LUNs returned by a REPORT LUNS command (see SPC-4). The task router shall respond to incorrect logical unit numbers (i.e., LUNs other than those reported by a REPORT LUNS command with the SELECT REPORTS field set to 02h) as described in 5.11 and 4.6.7.2.

That makes me think the fix should be on libiscsi side. I worry that if we apply the patch you request (with Warner's comment) we will allow the same LU to be represented by two different numbers, that is probably not great from security point.

[mav]

Though on a second throught, in ctl_decode_lun() we already allow lower LUNs to be decoded in several ways. So it may be not a big regression really. But I still wonder why not libiscsi.

[mizhka]

In D41632#948779, @mav wrote:

Though on a second throught, in ctl_decode_lun() we already allow lower LUNs to be decoded in several ways. So it may be not a big regression really. But I still wonder why not libiscsi.

I'm fully agree with you. libiscsi's patch was my first thought, but on other hand it's just another format of LUN numbering different from SAM specification. I suppose libiscsi was implemented before SAM specification. In case of libiscsi's patch it will be behavior change, may be break of compatibility between different version of libiscsi and so on.