Details
- Reviewers
mav imp scottl - Group Reviewers
cam - Commits
- rG616a676a0535: cam: clear stack-allocated CCB in the target layer
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 39224 Build 36113: arc lint + arc unit
Event Timeline
| sys/cam/scsi/scsi_targ_bh.c | ||
|---|---|---|
| 268 | we should just use this on the stack, not malloc it. we're not freeing it anyway. | |
| 270 | This is leaked | |
| 310 | we shouldn't malloc this, but just use it on the stack imho. | |
| 312 | this is leaked | |
| 326 | well, if not leaked and the xpt_action(), but I don't see the couple of routines I checked do that. | |
There might be some deferred action going on here, but I'd rather not do any deeper changes simply because it's unrelated to the problem at hand. I'm not even sure how to test it properly - it probably involves an FC HBA (isp(4)?) in target mode.
I'm ok splitting the leaks out from this change.
You have to look at a lot more to know for sure if what I flagged is right or not.
The scsi_ctl.c part needs a lot of work. ctlferegster() is probably allocating the ccb on the stack because it's the peripheral constructor and is being called with locks held, so the original author didn't want to do a malloc(M_WAITOK). This is a pattern in CAM that needs to be fixed but is outside of the scope of this change. I think that that's probably a better to replace the stack allocations with malloc(M_NOWAIT|M_ZERO) and not overthink an out-of-memory situation. But if that's not acceptable, then consider that the ccb is only used for XPT_FE_LUN and doesn't need a full union ccb allocation on the stack. I'm also a bit confused and concerned as to why this function uses different allocation paradigms; pick one and stick with it.
| sys/cam/scsi/scsi_targ_bh.c | ||
|---|---|---|
| 268 | Absolutely disagree. The usage and completion happens in a different context. | |
| 310 | Again, this is incorrect advice. | |
| sys/cam/scsi/scsi_targ_bh.c | ||
|---|---|---|
| 270 | It is not leaked (at least here), but queued into HBA. It return back to be processed by targbhdone() after request arrival or if aborted. And obviously it should not be on stack for that to work. | |
| 312 |
| |
| 312 | Same as above, not leaked but queued. | |
| 326 | targbhdone() or something behind should free it. | |
struct ccb_en_lun should be sufficient there. scsi_target.c is using it already.
I'm also a bit confused and concerned as to why this function uses different allocation paradigms; pick one and stick with it.
XPT_EN_LUN is synchronous (I am not happy about it, but that is what it is), so it is fine to be on stack. Other functions are queued, so they have to be allocated.
| sys/cam/scsi/scsi_targ_bh.c | ||
|---|---|---|
| 326 | Ah, yes. that is true. I'd bogusly thought these were all sync CCBs, and since they are async, my observations (leaked + put on stack) are equally bogus, here and elsewhere... they should be ignored. | |
I think that most of the stack allocations and the added memset() should move to malloc(M_NOWAIT|M_ZERO). Among other reasons, allocating an entire CCB can create stack overflow problems in deep call stacks; it's been a recurring problem for 20 years. This should be done first to satisfy what you're trying to achieve. Next step, peripheral construction should move to a taskqueue or thread where calling locks can be dropped and the periph can allocate its memory with M_WAITOK (this suggests a private TQ thread is the best mechanism). That will require an async callback mechanism to indicate to the caller when the periph creation succeeds or fails.
I thought about this, and while I agree in principle with what this should eventually look like, I'd rather stay with the current patch for now. The deficiencies you've identified are real, but they are already there, and I'd prefer to first get finished with UMA CCBs, with minimally intrusive patches like this one, and only after that's done get back to more intrusive stuff. It's just easier to debug (and perhaps merge) this way. Not to mention I don't even have a way to test this code; I believe I'd need two physical isp(4) HBAs with a cable?
(Also: tinderboxed.)