--- a/sys/netbt/l2cap.h @@ -355,12 +355,16 @@ #ifdef _KERNEL #include +#include #include /* 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 #include #include +#include #include #include @@ -43,6 +44,18 @@ #include #include +/* + * 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 #include #include +#include #include #include @@ -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