DF-5749 / 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 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171 172 173 174 175 176 177 178 179 180 181 182 183 184 185 186 187 188 189 190 191 192 193 194 195 196 197 198 199 200 201 202 203 204 205 206 207 208 209 210 211 212 213 214 215 216 217 218 219 220 221 222 223 224 225 226 227 228 229 230 231 232 | --- a/sys/netbt/l2cap.h @@ -355,12 +355,16 @@ #ifdef _KERNEL #include <vm/vm_zone.h> +#include <sys/spinlock.h> #include <net/if.h> /* for struct ifqueue */ LIST_HEAD(l2cap_channel_list, l2cap_channel); /* global variables */ +extern struct spinlock l2cap_lock; /* protects the request + * lists / channel close + * claim (DF-5749) */ extern struct l2cap_channel_list l2cap_active_list; extern struct l2cap_channel_list l2cap_listen_list; extern vm_zone_t l2cap_pdu_pool; @@ -416,6 +420,8 @@ #define L2CAP_SHUTDOWN (1<<0) /* channel is closing */ #define L2CAP_WAIT_CONFIG_REQ (1<<1) /* waiting for config request */ #define L2CAP_WAIT_CONFIG_RSP (1<<2) /* waiting for config response */ +#define L2CAP_CLOSING (1<<3) /* close body in progress + * (see l2cap_close, DF-5749) */ /* * L2CAP Request --- a/sys/netbt/l2cap_misc.c @@ -36,6 +36,7 @@ #include <sys/mbuf.h> #include <sys/proc.h> #include <sys/queue.h> +#include <sys/spinlock2.h> #include <sys/systm.h> #include <net/if.h> @@ -43,6 +44,18 @@ #include <netbt/hci.h> #include <netbt/l2cap.h> +/* + * DF-5749: the L2CAP request lists and the channel close transition are + * touched from the softclock thread (l2cap_rtx), the netisr protocol + * port (socket close / signal processing) and, on hci_link teardown, + * the event context -- with no serialization in the original code. + * l2cap_lock makes the request-list mutations and the single execution + * of the l2cap_close() body atomic. It is NEVER held across + * callout_stop() (which synchronously drains an in-flight l2cap_rtx() + * that may be spinning on this lock) nor across hci_* calls. + */ +struct spinlock l2cap_lock = SPINLOCK_INITIALIZER(l2cap_lock, 0); + struct l2cap_channel_list l2cap_active_list = LIST_HEAD_INITIALIZER(l2cap_active_list); struct l2cap_channel_list @@ -136,7 +149,11 @@ callout_reset(&req->lr_rtx, l2cap_response_timeout * hz, l2cap_rtx, req); + /* list insertion only under l2cap_lock; the callout is armed + * first so we never nest callout-internal spins inside it */ + spin_lock(&l2cap_lock); TAILQ_INSERT_TAIL(&link->hl_reqs, req, lr_next); + spin_unlock(&l2cap_lock); return 0; } @@ -163,13 +180,37 @@ void l2cap_request_free(struct l2cap_req *req) { - struct hci_link *link = req->lr_link; + /* + * DF-5749: callout_stop() synchronously drains an in-flight + * l2cap_rtx() callback before returning (kern_timeout.c), so the + * original "if (callout_active()) return" deferral was dead code + * and concurrent callers ran the TAILQ_REMOVE/zfree twice. + * + * Ownership is claimed BEFORE the drain: the test-and-clear of + * lr_chan under l2cap_lock decides exactly one caller as the + * owner. The owner then drains the callback -- an in-flight + * l2cap_rtx() reads lr_chan at its start, sees NULL (we cleared + * it) and does nothing: our own channel close subsumes the + * timeout teardown -- and finally pulls the request off the + * link list and frees it. Reading the request AFTER the drain + * is not possible safely: the callback would have freed and + * reused the request meanwhile (the vm_zone freelist linkage + * overlays lr_link/lr_chan). + */ + spin_lock(&l2cap_lock); + if (req->lr_chan == NULL) { + spin_unlock(&l2cap_lock); + return; + } + req->lr_chan = NULL; + spin_unlock(&l2cap_lock); callout_stop(&req->lr_rtx); - if (callout_active(&req->lr_rtx)) - return; - TAILQ_REMOVE(&link->hl_reqs, req, lr_next); + spin_lock(&l2cap_lock); + TAILQ_REMOVE(&req->lr_link->hl_reqs, req, lr_next); + spin_unlock(&l2cap_lock); + zfree(l2cap_req_pool, req); } @@ -186,8 +227,15 @@ struct l2cap_req *req = arg; struct l2cap_channel *chan; + /* + * DF-5749: the callback no longer frees its own request -- any + * concurrent l2cap_request_free() on this request blocks in its + * callout_stop() until this callback returns, and then owns the + * removal/free exclusively. The channel close (whose request + * walk frees this request) re-checks lc_state under l2cap_lock + * so the body executes exactly once. + */ chan = req->lr_chan; - l2cap_request_free(req); DPRINTF("cid %d, ident %d\n", (chan ? chan->lc_lcid : 0), req->lr_id); --- a/sys/netbt/l2cap_lower.c @@ -38,6 +38,7 @@ #include <sys/mbuf.h> #include <sys/proc.h> #include <sys/queue.h> +#include <sys/spinlock2.h> #include <sys/systm.h> #include <sys/endian.h> @@ -65,25 +66,53 @@ { struct l2cap_pdu *pdu; struct l2cap_req *req, *n; + const struct btproto *proto; + void *upper; + struct hci_link *link; if (chan->lc_state == L2CAP_CLOSED) return; /* - * Since any potential PDU could be half sent we just let it go, - * but disassociate ourselves from it as links deal with ownerless - * PDU's in any case. We could try harder to flush unsent packets - * but maybe its better to leave them in the queue? + * DF-5749 single-execution claim: softclock (l2cap_rtx), the + * netisr protocol port (socket close / signal handlers) and the + * hci event context can all reach this function on the same + * channel with no serialization in the original code, running + * the body twice (hci_acl_close(NULL) page fault, double free + * of the ACL link and of the request). Claiming L2CAP_CLOSED + * under l2cap_lock BEFORE touching lc_link guarantees exactly + * one body execution; losers return immediately. */ - TAILQ_FOREACH(pdu, &chan->lc_link->hl_txq, lp_next) { - if (pdu->lp_chan == chan) - pdu->lp_chan = NULL; + spin_lock(&l2cap_lock); + if (chan->lc_state == L2CAP_CLOSED) { + /* + * Another context is executing (or has executed) the + * close body. Wait until the L2CAP_CLOSING flag clears + * so that once l2cap_close() returns, no context is + * touching the channel -- l2cap_detach() kfrees it. + */ + while (chan->lc_flags & L2CAP_CLOSING) { + spin_unlock(&l2cap_lock); + cpu_ccfence(); + spin_lock(&l2cap_lock); + } + spin_unlock(&l2cap_lock); + return; } + chan->lc_state = L2CAP_CLOSED; + chan->lc_flags |= L2CAP_CLOSING; + link = chan->lc_link; /* exclusive from here */ + proto = chan->lc_proto; + upper = chan->lc_upper; + spin_unlock(&l2cap_lock); /* - * and clear any outstanding requests.. + * Clear any outstanding requests without l2cap_lock held: + * l2cap_request_free() calls callout_stop() which synchronously + * drains an in-flight l2cap_rtx() callback, and that callback + * needs l2cap_lock for its own l2cap_close() claim above. */ - req = TAILQ_FIRST(&chan->lc_link->hl_reqs); + req = TAILQ_FIRST(&link->hl_reqs); while (req != NULL) { n = TAILQ_NEXT(req, lr_next); if (req->lr_chan == chan) @@ -92,12 +121,29 @@ req = n; } + /* + * Since any potential PDU could be half sent we just let it go, + * but disassociate ourselves from it as links deal with ownerless + * PDU's in any case. We could try harder to flush unsent packets + * but maybe its better to leave them in the queue? + */ + TAILQ_FOREACH(pdu, &link->hl_txq, lp_next) { + if (pdu->lp_chan == chan) + pdu->lp_chan = NULL; + } + chan->lc_pending = 0; - chan->lc_state = L2CAP_CLOSED; - hci_acl_close(chan->lc_link, err); chan->lc_link = NULL; - (*chan->lc_proto->disconnected)(chan->lc_upper, err); + /* outside l2cap_lock: hci_acl_close() can reach hci_link_free() + * which closes other channels on the same link */ + hci_acl_close(link, err); + + (*proto->disconnected)(upper, err); + + /* body complete: wake any closer waiting on L2CAP_CLOSING + * (plain store: this context owns the body exclusively) */ + chan->lc_flags &= ~L2CAP_CLOSING; } /* --- a/sys/netbt/l2cap_upper.c |