On Fri, 2017-05-05 at 11:06 +0200, Christoph Hellwig wrote: > On Thu, May 04, 2017 at 03:50:48PM -0700, Bart Van Assche wrote: > > Avoid that the iSCSI target driver complains about "Initial page entry > > out-of-bounds" and closes the connection if the SCSI Data-Out buffer > > is larger than the buffer size specified through the CDB. This patch > > prevents that running the libiscsi regression tests against LIO trigger > > an infinite loop of libiscsi submitting a command, LIO closing the > > connection and libiscsi resubmitting the same command. > > Can you add a bit more of an explanation of why this happens? I've > just tried to verify the area, but at least while sitting in a conference > talk I can't quite make sense of the changes. Hello Christoph, The aspects of SCSI command processing that are relevant in this context are: * When the iSCSI target driver receives a SCSI command it calls transport_init_se_cmd() to initialize struct se_cmd. The iSCSI target driver passes the number of bytes that will be transferred into the "data_length" argument of transport_init_se_cmd(). That function stores the data length in the .data_length member of struct se_cmd. The value passed by target drivers to transport_init_se_cmd() is what is called the Expected Data Transfer Length (EDTL) in the iSCSI RFC. * After CDB parsing has finished target_cmd_size_check() is called. If EDTL exceeds the data buffer size extracted from the SCSI CDB (CDBL) then .data_length is reduced to CDBL. * Next target_alloc_sgl() allocates an sg-list for .data_length bytes (CDBL). * iscsit_allocate_iovecs() allocates a struct kvec (.iov_data) also for .data_length bytes (CDBL). * iscsit_get_dataout() calls rx_data() and tries to store EDTL bytes in the allocated struct kvec. If EDTL > CDBL then .iov_data overflows and this usually triggers a crash. With the patch that prevents .iov_data overflows the initiator is disconnected. In the case of libiscsi, it keeps retrying forever to resubmit SCSI commands for which EDTL > CDBL. In other words, initially EDTL is stored into .data_length and later on the value of .data_length changes to CDBL. My proposal to avoid the buffer overflows is to store both EDTL and CDBL in struct se_cmd and to allocate an sg-list per command that can store EDTL bytes instead of CDBL bytes. Bart.