DF-3531 / fix.diff
DF-3531 requires the callout-stop contract fix (layer 1, kernel) plus the real item free in ng_uncallout (layer 2, ng7 module). Layer 1 must ALSO be dispatch-precise (see caveat in VERDICT.md): as validated here it eliminates the leak but can report PREVENTED for a callback that already dispatched, letting ng_uncallout free an in-flight item (double-free observed under close-vs-fire race stress). The ng_callout "oitem" hunk shipped in DF-3514/DF-3523's fix.diff is deliberately NOT included: for ng_tty-style 1-tick self-rearming timers (re-arm from inside the timeout callback) it frees the in-flight fired item on every re-arm and reproduced the same double-free signature immediately. --- a/sys/kern/kern_timeout.c +++ b/sys/kern/kern_timeout.c @@ -885,10 +885,28 @@ _callout_cancel_or_stop(struct callout *cc, uint32_t flags, int sync) * If the operation is still in-progress then re-acquire the spin-lock * and block if necessary. Also initiate the lock cancel. */ + /* + * If the operation completed inline above (the armed callout was + * removed from the wheel and will never execute), report whether + * execution was PREVENTED. Returning 0 for a successfully + * cancelled pending callout violates the callout_stop(9) contract + * and makes every conditional cleanup (`if (callout_stop(&c)) ...`) + * leak the resources paired with the callback -- e.g. netgraph's + * ng_callout/ng_uncallout timer items (ng_item + node/hook refs), + * which permanently pin netgraph nodes and modules. + * + * CAVEAT (DF-3531 verification): PREVENTED must only be reported + * when the callback never DISPATCHED. The early-exit below reads + * the PREVENTED flag after _callout_update_spinlocked(); for a + * callout whose trampoline already dequeued the argument and handed + * it to a consumer (ng_callout_trampoline -> ng_snd_item -> async), + * reporting PREVENTED makes that consumer's item get freed twice. + * Observed as 'memory chunk already free' slab panics. Upstream + * needs a dispatch-precise condition here (or trampoline-side arg + * clearance) before this is shippable. + */ if (sync == 0) { exis_drop_gd(gd); return 0; } + if ((c->flags & (CALLOUT_INPROG | CALLOUT_SET)) == 0) { + /* inline-completed stop/cancel: report PREVENTED, not 0 */ + res = ((c->flags & CALLOUT_PREVENTED) != 0); + exis_drop_gd(gd); + return (res); + } if (c->flags & CALLOUT_AUTOLOCK) lockmgr(c->lk, LK_CANCEL_BEG); --- a/sys/netgraph7/netgraph/ng_base.c +++ b/sys/netgraph7/netgraph/ng_base.c @@ -3284,11 +3284,12 @@ ng_uncallout(struct callout *c, node_p node) if ((rval > 0) && (callout_func(c) == &ng_callout_trampoline) && (NGI_NODE(item) == node)) { /* - * We successfully removed it from the queue before it ran - * So now we need to unreference everything that was - * given extra references. (NG_FREE_ITEM does this). + * We successfully removed it from the queue before it ran. + * The item was never sent, so its queue refcount is zero; + * free it directly (this also drops the node/hook refs it + * holds). NG_FREE_ITEM alone is flag-only in ng7 and + * frees nothing. */ NG_FREE_ITEM(item); + ng_free_item(item); } callout_set_arg(c, NULL); |