⬢ DragonFlyBSD Kernel Audit
DF-3536 / fix.diff
← back to finding ↓ download raw 
 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
/* DF-3536 fix: identical framework defect as DF-3531 — two-layer fix
   (kern_timeout PREVENTED reporting + ng_uncallout ng_free_item), see VERDICT.md. */
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);