Page MenuHomeFreeBSD

iscsi: Always free a cdw before its associated ctl_io.

Authored by jhb on May 14 2021, 10:55 PM.



cxgbei stores state about a target transfer in the ctl_private[] array
of a ctl_io that is freed when a target transfer (represented by the
cdw) is freed. As such, freeing a ctl_io before a cdw that references
it can result in a use after free in cxgbei. Two of the four places
freed the cdw first, and the other two freed the ctl_io first. Fix
the latter two places to free the cdw first.

Reported by: Jithesh Arakkan @ Chelsio

Diff Detail

rG FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jhb requested review of this revision.May 14 2021, 10:55 PM

The panics showed up after Chelsio QA started testing the fix in e894e3adb206815c2acff17a4011becb166c2f66. Specifically, in some cases due to the bug this change fixes, the write to clear the pointer to NULL in ctl_io in the cxgbei driver in that commit would write to ctl_io after it had been freed by ctl_datamove_done().


I would appreciate some thoughts on my questions here.

Looks good to me. Just remove the XXX comment.


No, cdw_io != io here. io is the abort request, while cdw_io is the original SCSI command being aborted. You've properly fixed CTL_FLAG_DMA_INPROG clear on wrong io.


above we set it to 42, here 43? I'm not super familiar with the code, but is that a problem?


These codes were never formalized. I've done some cleanup, but haven't completed. So anything non-zero is OK, unique values are used just to ease debugging. And since the command was aborted, this code goes nowhere really, but still it is better to indicate that the transfer was not successful.

jhb marked an inline comment as done.May 19 2021, 9:04 PM
jhb added inline comments.

Thanks, I've axed the comment.

jhb marked an inline comment as done.
  • Remove comment with questions.
This revision is now accepted and ready to land.May 19 2021, 10:56 PM