Page MenuHomeFreeBSD

[1/2] mps(4): Fix lifetime of command buffer for mpssas_get_sata_identify
ClosedPublic

Authored by cem on Dec 20 2018, 12:25 AM.

Details

Summary

In the event that the ID command timed out, mps(4) did not free the command
until it could be cancelled. However, it freed the associated buffer
(cm_data). Fix the lifetime issue by freeing the associated buffer at the
same time as the command.

Test Plan

I have not tested this at all, other than compiling.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

cem created this revision.Dec 20 2018, 12:25 AM
cem added a comment.Dec 20 2018, 12:42 AM

My remaining concern is this may leak if the controller is reinited for some unrelated reason. That should not be the ordinary case, but maybe it is possible. I think that could be fixed by something like the following in mps_sas.c:mpssas_complete_all_commands():

+if (cm->cm_flags & MPS_CM_FLAGS_SATA_ID_TIMEOUT)
+    free(cm->cm_data, ...);

Does that seem ok?

Can you do the same for mpr?

cem added a comment.Dec 20 2018, 4:46 AM

Can you do the same for mpr?

Ditto the other review — I'd be happy to, but I have a weak preference for getting to "accept" quality on mps alone first and then copying to mpr once, rather than reviewing two sets of changes and applying any fixups twice. It's just less work for me. If you still prefer both at the same time, I am willing to do that.

scottl accepted this revision.Dec 20 2018, 5:04 AM
This revision is now accepted and ready to land.Dec 20 2018, 5:04 AM
cem updated this revision to Diff 52207.Dec 21 2018, 12:19 AM

Copy to mpr(4); add seatbelt check in mp?sas_complete_all_commands() in case of reinit

This revision now requires review to proceed.Dec 21 2018, 12:19 AM
scottl accepted this revision.Dec 21 2018, 8:23 PM
This revision is now accepted and ready to land.Dec 21 2018, 8:23 PM
This revision was automatically updated to reflect the committed changes.