DF-3436 / 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 | --- a/sys/net/ipfw3_basic/ip_fw3_state.c +++ b/sys/net/ipfw3_basic/ip_fw3_state.c @@ -315,9 +315,26 @@ } } if (*the_count <= the_max) { + s = RB_FIND(fw3_state_tree, the_tree, k); + if (s != NULL) { + /* + * Already tracked: limit-type rules (arg3 != 0) + * skip the RB_FIND above, so re-check here instead + * of allocating a duplicate the tree will reject + * (DF-3434: the rejected allocation was leaked and + * kprintf("oops") fired once per packet). + */ + s->timestamp = time_uptime; + goto done; + } (*the_count)++; s = kmalloc(LEN_FW3_STATE, M_IPFW3_STATE, M_INTWAIT | M_NULLOK | M_ZERO); + if (s == NULL) { + /* DF-3435: M_NULLOK permits NULL at the type + * limit; do not write through it */ + goto done; + } s->src_addr = k->src_addr; s->dst_addr = k->dst_addr; s->src_port = k->src_port; @@ -325,7 +342,7 @@ s->stub = *f; s->timestamp = time_uptime; if (RB_INSERT(fw3_state_tree, the_tree, s)) { - kprintf("oops\n"); + kfree(s, M_IPFW3_STATE); } } done: @@ -542,6 +559,11 @@ struct ipfw3_state_context *state_ctx = fw3_state_ctx[mycpuid]; struct ipfw3_state *s, *tmp; + if (state_ctx == NULL) { + /* DF-3436: fini_dispatch may have run first on this cpu */ + netisr_forwardmsg_all(&nmsg->base, mycpuid + 1); + return; + } RB_FOREACH_SAFE(s, fw3_state_tree, &state_ctx->rb_icmp_in, tmp) { if (time_uptime - s->timestamp > sysctl_var_icmp_timeout) { RB_REMOVE(fw3_state_tree, &state_ctx->rb_icmp_in, s); @@ -688,7 +710,9 @@ 0, ip_fw3_state_fini_dispatch); netisr_domsg(&msg, 0); - callout_stop(&ip_fw3_state_cleanup_callout); + /* DF-3436: the handler re-arms itself; cancel synchronously and + * disconnect the callout (re-initialized by the next MOD_LOAD) */ + callout_terminate(&ip_fw3_state_cleanup_callout); } void --- a/sys/net/ipfw3_basic/ip_fw3_table.c +++ b/sys/net/ipfw3_basic/ip_fw3_table.c @@ -127,7 +127,8 @@ table_ctx += ioc_tbl->id; table_ctx->count = 0; - rn_flush(table_ctx->node, flush_table_entry); + if (table_ctx->node != NULL) + rn_flush(table_ctx->node, flush_table_entry); /* XXX: should free the tree: rn_freehead(table_ctx->node) */ table_ctx->type = 0; netisr_forwardmsg_all(&nmsg->base, mycpuid + 1); @@ -578,12 +579,21 @@ struct ipfw3_table_context *table_ctx, *tmp_table; int id; table_ctx = fw3_ctx[mycpuid]->table_ctx; + if (table_ctx == NULL) { + /* DF-3436: already torn down (e.g. a refused unload that + * ran fini anyway); do not walk freed memory */ + netisr_forwardmsg_all(&nmsg->base, mycpuid + 1); + return; + } tmp_table = table_ctx; for (id = 0; id < IPFW_TABLES_MAX; id++, table_ctx++) { - rn_flush(table_ctx->node, flush_table_entry); + /* only configured tables ever ran rn_inithead */ + if (table_ctx->node != NULL) + rn_flush(table_ctx->node, flush_table_entry); /* XXX: should free the tree: rn_freehead(table_ctx->node) */ } kfree(tmp_table, M_IPFW3_TABLE); + fw3_ctx[mycpuid]->table_ctx = NULL; netisr_forwardmsg_all(&nmsg->base, mycpuid + 1); } --- a/sys/net/ipfw3_basic/ip_fw3_basic.c +++ b/sys/net/ipfw3_basic/ip_fw3_basic.c @@ -654,6 +654,15 @@ default: err = 1; } + if (type == MOD_UNLOAD && err != 0) { + /* + * DF-3436: the module stays loaded (rules still reference + * it); do NOT tear the state/table contexts down from a + * refused unload - that leaves the loaded module pointing + * at freed memory. + */ + return (EBUSY); + } ip_fw3_state_modevent(type); ip_fw3_table_modevent(type); return err; |