⬢ DragonFlyBSD Kernel Audit
DF-0819 / blkmap_lock_trace.c
← 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
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
/*
 * DF-0819 — blkmap_lock leaked on hammer_bnew error paths
 *
 * BUG: In hammer_blockmap_alloc() and hammer_blockmap_reserve(), the
 * function acquires hammer_lock_ex(&hmp->blkmap_lock) then calls
 * hammer_bnew()/hammer_bnew_ext().  If that returns an error, the code
 * does `goto failed` which jumps PAST the hammer_unlock() — the lock is
 * permanently leaked.  Since blkmap_lock serializes ALL HAMMER metadata
 * allocation, a leaked lock deadlocks the entire filesystem (every
 * subsequent hammer metadata op blocks forever in D-state, unkillable).
 *
 * This is a CODE-LEVEL analysis harness — it prints the exact line ranges
 * proving the lock-acquire-without-matching-release.  The error path
 * (hammer_bnew failure) is effectively dead code under normal operation
 * because hammer_io_new() always returns 0, but the code pattern is
 * objectively wrong and is a latent deadlock waiting to happen.
 *
 * Run:   ./blkmap_lock_trace
 * (no kernel interaction — pure static analysis printout)
 */
#include <stdio.h>

static const char *analysis =
"=================================================================\n"
"DF-0819: blkmap_lock LEAK on hammer_bnew error paths\n"
"=================================================================\n\n"
"FILE: sys/vfs/hammer/hammer_blockmap.c\n\n"
"--- BUG 1: hammer_blockmap_alloc() [line 95] ---\n\n"
"  285:  hammer_lock_ex(&hmp->blkmap_lock);   // LOCK ACQUIRED\n"
"  ...\n"
"  372:  if ((next_offset & HAMMER_BUFMASK) == 0) {\n"
"  373:      hammer_bnew_ext(trans->hmp, next_offset, bytes,\n"
"  374:                      errorp, &buffer3);\n"
"  375:      if (*errorp) {\n"
"  376:          result_offset = 0;\n"
"  377:          goto failed;                   // <<<< JUMPS PAST UNLOCK!\n"
"  378:      }\n"
"  379:  }\n"
"  ...\n"
"  392:  hammer_unlock(&hmp->blkmap_lock);     // UNLOCK (SKIPPED by goto)\n"
"  393: failed:                                 // <<<< LABEL IS PAST UNLOCK\n"
"  398:  if (buffer1) hammer_rel_buffer(buffer1, 0);\n"
"  ...\n"
"  405:  return(result_offset);\n\n"
"  PROOF: line 377 `goto failed` targets label at line 393,\n"
"         which is AFTER the unlock at line 392.\n"
"         The lock acquired at line 285 is NEVER released on this path.\n\n"
"--- BUG 2: hammer_blockmap_reserve() [line 419] ---\n\n"
"  546:  hammer_lock_ex(&hmp->blkmap_lock);   // LOCK ACQUIRED\n"
"  ...\n"
"  602:  if (bytes < HAMMER_BUFSIZE && (next_offset & HAMMER_BUFMASK) == 0) {\n"
"  603:      if (!vm_paging_min_dnc(HAMMER_BUFSIZE / PAGE_SIZE)) {\n"
"  604:          hammer_bnew(hmp, next_offset, errorp, &buffer3);\n"
"  605:          if (*errorp)\n"
"  606:              goto failed;               // <<<< JUMPS PAST UNLOCK!\n"
"  607:      }\n"
"  608:  }\n"
"  ...\n"
"  611:  hammer_unlock(&hmp->blkmap_lock);     // UNLOCK (SKIPPED by goto)\n"
"  613: failed:                                 // <<<< LABEL IS PAST UNLOCK\n"
"  ...\n"
"  623:  return(resv);\n\n"
"  PROOF: line 606 `goto failed` targets label at line 613,\n"
"         which is AFTER the unlock at line 611.\n"
"         The lock acquired at line 546 is NEVER released on this path.\n\n"
"--- IMPACT ---\n\n"
"  blkmap_lock serializes ALL HAMMER blockmap (metadata/data) allocation.\n"
"  A leaked lock means every subsequent hammer_blockmap_alloc/reserve/\n"
"  finalize/free/dedup call blocks forever in hammer_lock_ex() on\n"
"  blkmap_lock.  This is an unkillable D-state deadlock affecting the\n"
"  entire mounted HAMMER filesystem.  Impact: permanent DoS of the\n"
"  filesystem; all processes touching it hang in D-state.\n\n"
"--- TRIGGER REACHABILITY ---\n\n"
"  The error path requires hammer_bnew()/hammer_bnew_ext() to return\n"
"  a non-zero *errorp.  Tracing the call chain:\n"
"    hammer_bnew_ext -> _hammer_bread -> hammer_get_buffer ->\n"
"    hammer_load_buffer(isnew=1) -> hammer_io_new() -> return(0)\n"
"  hammer_io_new() (hammer_io.c:437) ALWAYS returns 0 — it calls\n"
"  getblk() which either succeeds or panics (NULL deref at bp->b_ops).\n"
"  So under normal operation the error path is dead code.\n\n"
"  However, hammer_get_buffer() can set *errorp via hammer_get_volume()\n"
"  (ENOENT if zone2_offset decodes to a non-existent vol_no — filesystem\n"
"  corruption) or hammer_load_volume() (I/O error on backing device).\n"
"  These are realistic on a corrupted/worn device or crafted image.\n\n"
"  CLASSIFICATION: latent bug — real code defect, lock-leak pattern is\n"
"  objectively present. Would deadlock the filesystem if the error path\n"
"  is ever reached (corruption / I/O error / crafted image / future\n"
"  code changes adding real error returns to hammer_io_new).\n\n"
"--- FIX ---\n\n"
"  Add hammer_unlock(&hmp->blkmap_lock) before each `goto failed` that\n"
"  occurs after the lock is acquired (lines 377 and 606).  See fix.diff.\n"
"=================================================================\n";

int main(void)
{
    printf("%s", analysis);
    return 0;
}