Don't pass a user buffer pointer as the data pointer in a CCB.

[jhb]

Summary

Allocate a temporary buffer in the kernel to serve as the CCB data
pointer for a pass-through transaction and using copyin/copyout to
shuffle the data to/from the user buffer.

Test Plan

• compile tested only, found on CheriBSD where user and kernel pointers have different types, so conflating the two gives compile errors

[imp]

sys/cam/scsi/scsi_sg.c

633 ↗

(On Diff #70750)

Dir & CAM_DIR_OUT?

[brooks]

Modulo accepting or rejecting @imp's suggestion this looks good.

[jhb]

sys/cam/scsi/scsi_sg.c

582 ↗

(On Diff #70750)

That is why I changed this btw, because BOTH != IN | OUT.

633 ↗

(On Diff #70750)

If only that would work. :(

From cam.h (and I don't know why this is so dumb):

	CAM_DIR_BOTH		= 0x00000000,/* Data direction (00:IN/OUT)    */
	CAM_DIR_IN		= 0x00000040,/* Data direction (01:DATA IN)   */
	CAM_DIR_OUT		= 0x00000080,/* Data direction (10:DATA OUT)  */
	CAM_DIR_NONE		= 0x000000C0,/* Data direction (11:no data)   */
	CAM_DIR_MASK		= 0x000000C0,/* Data direction Mask	      */

[scottl]

sys/cam/scsi/scsi_sg.c

633 ↗

(On Diff #70750)

Then we should seriously consider changing this.

[jhb]

sys/cam/scsi/scsi_sg.c

633 ↗

(On Diff #70750)

Hmm, rS216088 is when the 0 value was changed from RESERVED to BOTH. Maybe @ken has some thoughts?

[jhb]

I do think if we change CAM_DIR_BOTH we should probably do that as a separate change from this though? Do we think this patch is ok given the current CAM_DIR_* values?

[scottl]

Yep, can tackle the CAM_DIR problem separately.

[ken]

sys/cam/scsi/scsi_sg.c

633 ↗

(On Diff #70750)

I added CAM_DIR_BOTH for SMP commands, because they are effectively bidirectional. The request was too big to allocate statically in the CCB.

The SCSI standard also defines bi-directional commands, at least with opcode 0x9D (Service Action Bi-Directional).

In other words, we can't remove bi-directional support, we need it in some form. This is also why it is typically written:

if ((ccb->ccb_h.flags & CAM_DIR_MASK) == CAM_DIR_X)

[scottl]

The problem isn't the existence of bi-directional commands, it's that DIR_BOTH == 0, not an OR'ing of DIR_IN and DIR_OUT, as would be expected.

[ken]

In D24489#539455, @scottl wrote:

The problem isn't the existence of bi-directional commands, it's that DIR_BOTH == 0, not an OR'ing of DIR_IN and DIR_OUT, as would be expected.

Well, CAM_DIR_NONE was defined as all 1's from the beginning. It was that way in the ANSI CAM spec. 0's was defined as CAM_DIR_RESV.

So that is why it is that way...

[scottl]

Blah, but thanks for the history on it, I guess I never noticed before. I would be surprised if this hasn't caused bugs. There's no longer an active standards group, and even if there was I'd advocate that this is wrong. Let's follow up with a change on it.

[imp]

In D24489#539551, @scottl wrote:

Blah, but thanks for the history on it, I guess I never noticed before. I would be surprised if this hasn't caused bugs. There's no longer an active standards group, and even if there was I'd advocate that this is wrong. Let's follow up with a change on it.

I'd love to change this too... but I worry about userland/kernel ABI changes being hard to transition...