On Tue, 2014-09-02 at 17:50 -0400, Joern Engel wrote: > Code inspection indicates these are possible. At the very least we > should catch them and call a bug a bug. > I don't see how an vector overflow is possible here..? The three callers of iscsit_map_iovec() are each using validated lengths based upon the original CDB data-length, and iscsit_allocate_iovecs() is doing iovec allocation based upon same CDB data-length, plus an extra ISCSI_IOV_DATA_BUFFER of room.. > While at it, this also changes iscsit_map_iovec() to take an index into > cmd->iov_data instead of a pointer. Also fixes an off-by-one leading to > leaked kmap()s. iscsit_send_datain() called iscsit_map_iovec() with and > index of 1, resulting in cmd->kmapped_nents being one less than required > and not unmapping the last element in the vector. > I like the overall cleanup, but the bit about an off-by-one error in iscsit_send_datain() is incorrect. iscsit_send_datain()'s call to iscsit_map_iovec() has an index of 1 because the first iovec element is used by the outgoing ISCSI_OP_SCSI_DATA_IN header, and the actual payload containing pages that need to be kmapped starts in the subsequent vector. This was originally done to dispatch the PDU + payload with a single call to ->sendpage(). cmd->kmapped_nents is then used by iscsit_unmap_iovec() for kunmaping of the outgoing payload, but not the outgoing PDU header. That said, I don't see how iscsit_unmap_iovec() is leaking kunmap() for the last element if cmd->kmapped_nents matches what was kmap()'ed earlier in iscsit_map_iovec(). > The fact that noone seems to have noticed this bug yet implies that > target code doesn't get a lot of testing on 32bit platforms. > There at about 1/2 dozen vendors shipping on 32-bit ARM. Go visit your local Frys + Best-Buy and see. ;) Regardless, I do like the patch as a cleanup, but the commit message is wrong and AFAICT it doesn't actually fix a bug. Care to give a different commit message + reasoning for this cleanup..? --nab -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
