DF-5592 / fix.diff
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 | DF-5592 fix: pin the cam_sim while it is linked on cam_simq so the completion softint (camisr) can never race the driver's detach-time cam_sim_free(); make cam_sim_release() an atomic cmpset protocol like its xpt_release_bus()/xpt_release_device() neighbors. Protocol: * xpt_done() takes one sim reference (atomic_add_int under cam_simq_spin) when it links the sim onto cam_simq; the flag CAM_SIM_ON_DONEQ now exactly means 'linked on cam_simq'. * camisr() clears the flag for the sims it pops (under the same spin) and drops that reference only AFTER processing+unlock. * cam_sim_release() drops references with atomic_cmpset_int; the final 1->0 transition runs in xpt_sim_finalize() under cam_simq_spin so it cannot interleave with xpt_done()'s pin. A linked sim always has refcount >= 2 and can never hit the final kfree() while queued for / undergoing completion. * cam_dead_sim gets a permanently large refcount so camisr's pin drops can never 'finalize' the static structure. --- cam_sim.c.orig 2026-10-08 09:56:18.479887051 +0000 +++ cam_sim.c 2026-10-08 09:56:32.167711585 +0000 @@ -243,15 +243,34 @@ { if (flags & CAM_SIM_SOFTC) sim->softc = NULL; - if (sim->refcount == 1) { - if (sim->devq) { - cam_simq_release(sim->devq); - sim->devq = NULL; + + /* + * Atomic compare-and-set release protocol, mirroring the + * xpt_release_bus()/xpt_release_device() pattern in cam_xpt.c. + * A sim that is linked on cam_simq (pending completion processing + * by the swi_cambio softint) holds one extra reference (see + * xpt_done()), so it cannot reach the final kfree() here while + * camisr still operates on it. + */ + for (;;) { + int count = sim->refcount; + + cpu_ccfence(); + if (count == 1) { + if (xpt_sim_finalize(sim)) { + if (sim->devq) { + cam_simq_release(sim->devq); + sim->devq = NULL; + } + kfree(sim, M_CAMSIM); + return; + } + } else { + if (atomic_cmpset_int(&sim->refcount, + count, count - 1)) { + return; + } } - sim->refcount = 0; - kfree(sim, M_CAMSIM); - } else { - --sim->refcount; } } --- cam_xpt_sim.h.orig 2026-10-08 09:56:18.479887051 +0000 +++ cam_xpt_sim.h 2026-10-08 09:56:41.675589720 +0000 @@ -45,6 +45,7 @@ void xpt_release_devq(struct cam_path *path, u_int count, int run_queue); void xpt_done(union ccb *done_ccb); +int xpt_sim_finalize(struct cam_sim *sim); #endif #endif /* _CAM_CAM_XPT_SIM_H */ --- cam_xpt.c.orig 2026-10-08 09:56:18.479887051 +0000 +++ cam_xpt.c 2026-10-08 09:56:32.187711330 +0000 @@ -1466,6 +1466,12 @@ cam_dead_sim.lock = &cam_dead_lock; lockinit(&cam_dead_lock, "XPT dead_sim lock", 0, LK_CANRECURSE); cam_dead_sim.flags |= CAM_SIM_DEREGISTERED; + /* + * camisr() drops the cam_simq pin of every sim it processes, + * including dead_sim; keep its refcount permanently above one so + * it can never be finalized/freed. + */ + cam_dead_sim.refcount = 0x40000000; /* * The xpt layer is, itself, the equivelent of a SIM. @@ -4909,6 +4915,13 @@ if ((sim->flags & CAM_SIM_ON_DONEQ) == 0) { spin_lock(&cam_simq_spin); if ((sim->flags & CAM_SIM_ON_DONEQ) == 0) { + /* + * Pin the sim for as long as it + * is linked on cam_simq; camisr() + * drops this reference after it + * has finished processing. + */ + atomic_add_int(&sim->refcount, 1); TAILQ_INSERT_TAIL(&cam_simq, sim, links); sim->flags |= CAM_SIM_ON_DONEQ; @@ -7395,15 +7408,44 @@ spin_lock(&cam_simq_spin); TAILQ_INIT(&queue); TAILQ_CONCAT(&queue, &cam_simq, links); + /* + * Clear ON_DONEQ under cam_simq_spin so the flag exactly tracks + * "linked on cam_simq" (xpt_done() sets it under the same spin). + * The pin taken by xpt_done() stays held until processing ends. + */ + TAILQ_FOREACH(sim, &queue, links) + sim->flags &= ~CAM_SIM_ON_DONEQ; spin_unlock(&cam_simq_spin); while ((sim = TAILQ_FIRST(&queue)) != NULL) { TAILQ_REMOVE(&queue, sim, links); CAM_SIM_LOCK(sim); - sim->flags &= ~CAM_SIM_ON_DONEQ; camisr_runqueue(sim); CAM_SIM_UNLOCK(sim); + /* drop the cam_simq pin taken by xpt_done() */ + cam_sim_release(sim, 0); + } +} + +/* + * Perform the final 1->0 reference transition of a cam_sim under + * cam_simq_spin, so it cannot interleave with xpt_done()'s pin + * acquisition. Returns 1 on success (caller may free the sim); + * returns 0 if the sim is (or just got) linked on cam_simq -- i.e. + * camisr holds a pin -- or if the reference changed concurrently. + */ +int +xpt_sim_finalize(struct cam_sim *sim) +{ + int rv = 0; + + spin_lock(&cam_simq_spin); + if ((sim->flags & CAM_SIM_ON_DONEQ) == 0 && + atomic_cmpset_int(&sim->refcount, 1, 0)) { + rv = 1; } + spin_unlock(&cam_simq_spin); + return (rv); } static void |