cam: enable kern.cam.ada.enable_uma_ccbs by default
ClosedPublic
Actions

We've had good luck with this one. I'd feel better if we could have a quick coccinelle script to find this... Ignore the "this is bogus" because they might not be...

// Script to find all the uses of ccb's allocated on the stack and change
// them to use the new setup function which bzeros before doing a normal
// setup of the CCB.
@@
union ccb *CCB1;
union ccb *CCB2;
expression E;
@@
- memcpy(CCB1,CCB2,E)
+ THIS_IS_BOGUS(CCB1,CCB2,E)

@@
union ccb *CCB1;
union ccb *CCB2;
expression E;
@@
- bcopy(CCB1,CCB2,E)
+ THIS_IS_BOGUS(CCB1,CCB2,E)

@@
union ccb *CCB1;
union ccb *CCB2;
@@
- *CCB1=*CCB2
+ THIS_IS_BOGUS(CCB1,CCB2)

Had the following hits:

diff -u -p a/cam_xpt.c b/cam_xpt.c
--- a/cam_xpt.c
+++ b/cam_xpt.c
@@ -487,7 +487,7 @@ xptdoioctl(struct cdev *dev, u_long cmd, caddr_t addr,
 			xpt_path_lock(ccb->ccb_h.path);
 			cam_periph_runccb(ccb, NULL, 0, 0, NULL);
 			xpt_path_unlock(ccb->ccb_h.path);
-			bcopy(ccb, inccb, sizeof(union ccb));
+			THIS_IS_BOGUS(ccb, inccb, sizeof(union ccb));
 			xpt_free_path(ccb->ccb_h.path);
 			xpt_free_ccb(ccb);
 			break;
@@ -518,7 +518,7 @@ xptdoioctl(struct cdev *dev, u_long cmd, caddr_t addr,
 				      inccb->ccb_h.pinfo.priority);
 			xpt_merge_ccb(&ccb, inccb);
 			xpt_action(&ccb);
-			bcopy(&ccb, inccb, sizeof(union ccb));
+			THIS_IS_BOGUS(&ccb, inccb, sizeof(union ccb));
 			xpt_free_path(ccb.ccb_h.path);
 			break;
 		}
diff -u -p a/scsi/scsi_pass.c b/scsi/scsi_pass.c
--- a/scsi/scsi_pass.c
+++ b/scsi/scsi_pass.c
@@ -2232,7 +2232,7 @@ passsendccb(struct cam_periph *periph, union ccb *ccb,

 	ccb->ccb_h.cbfcnp = NULL;
 	ccb->ccb_h.periph_priv = inccb->ccb_h.periph_priv;
-	bcopy(ccb, inccb, sizeof(union ccb));
+	THIS_IS_BOGUS(ccb, inccb, sizeof(union ccb));

 	return(0);
 }
diff -u -p a/cam_periph.c b/cam_periph.c
--- a/cam_periph.c
+++ b/cam_periph.c
@@ -1423,7 +1423,7 @@ camperiphdone(struct cam_periph *periph, union ccb *do
 	 * and the result will be the final one returned to the CCB owher.
 	 */
 	saved_ccb = (union ccb *)done_ccb->ccb_h.saved_ccb_ptr;
-	bcopy(saved_ccb, done_ccb, sizeof(*done_ccb));
+	THIS_IS_BOGUS(saved_ccb, done_ccb, sizeof(*done_ccb));
 	xpt_free_ccb(saved_ccb);
 	if (done_ccb->ccb_h.cbfcnp != camperiphdone)
 		periph->flags &= ~CAM_PERIPH_RECOVERY_INPROG;
@@ -1713,7 +1713,7 @@ camperiphscsisenseerror(union ccb *ccb, union ccb **or
 			 * this freeze will be dropped as part of ERESTART.
 			 */
 			ccb->ccb_h.status &= ~CAM_DEV_QFRZN;
-			bcopy(ccb, orig_ccb, sizeof(*orig_ccb));
+			THIS_IS_BOGUS(ccb, orig_ccb, sizeof(*orig_ccb));
 		}

 		switch (err_action & SS_MASK) {

There's a review for the cam_periph one. The two ioctls in cam_xpt.c I think are safe because we're copying like to like in terms of the CCB allocation. Ditto with the scsi_pass one.

Based on this, I think we're good, but wanted to "show my work" in case there's some flaw in it.

This revision is now accepted and ready to land.Jul 6 2021, 3:25 PM

The ioctl ones may be a copyout in disguise too, but if so, that's also safe.

In D30567#699123, @imp wrote:

The ioctl ones may be a copyout in disguise too, but if so, that's also safe.

Now that I've had time to look at the code in full context, the ioctl functions are definitely a copyout, so they are fine for sure. And the path that did trigger is only for da, not ada. So I'm quite convinced now.

Closed by commit rG6f147a0734e0: cam: enable kern.cam.ada.enable_uma_ccbs by default (authored by trasz). · Explain WhyJul 7 2021, 9:48 AM

This revision was automatically updated to reflect the committed changes.

trasz added a commit: rG6f147a0734e0: cam: enable kern.cam.ada.enable_uma_ccbs by default.