DF-0726 / 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 115 116 117 | diff --git a/sys/net/if_clone.c b/sys/net/if_clone.c --- a/sys/net/if_clone.c +++ b/sys/net/if_clone.c @@ -144,11 +144,6 @@ int len, maxclone; int unit; - LIST_FOREACH(ifct, &if_cloners, ifc_list) { - if (strcmp(ifct->ifc_name, ifc->ifc_name) == 0) - return (EEXIST); - } - KASSERT(ifc->ifc_minifs - 1 <= ifc->ifc_maxunit, ("%s: %s requested more units then allowed (%d > %d)", __func__, ifc->ifc_name, ifc->ifc_minifs, @@ -163,10 +158,25 @@ ifc->ifc_units = kmalloc(len, M_CLONE, M_WAITOK | M_ZERO); ifc->ifc_bmlen = len; + /* + * Serialize all list/count mutations and lookups with ifnet_lock so + * that an unlocked reader in if_clone_list()/if_clone_lookup() cannot + * race a detach (which frees ifc->ifc_units and is followed by module + * unload unmapping the ifc backing store). See DF-0726. + */ + ifnet_lock(); + LIST_FOREACH(ifct, &if_cloners, ifc_list) { + if (strcmp(ifct->ifc_name, ifc->ifc_name) == 0) { + ifnet_unlock(); + kfree(ifc->ifc_units, M_CLONE); + ifc->ifc_units = NULL; + return (EEXIST); + } + } + LIST_INSERT_HEAD(&if_cloners, ifc, ifc_list); if_cloners_count++; - ifnet_lock(); for (unit = 0; unit < ifc->ifc_minifs; unit++) { if_clone_alloc_unit(ifc, &unit); if (if_clone_createif(ifc, unit, NULL, NULL) != 0) { @@ -189,9 +199,18 @@ if_clone_detach(struct if_clone *ifc) { + /* + * Serialize against if_clone_list()/if_clone_lookup() readers; without + * this, a reader can hold a pointer into the module's backing store + * across the LIST_REMOVE + kfree(ifc_units) below and the subsequent + * module unload that unmaps the ifc page. See DF-0726. + */ + ifnet_lock(); LIST_REMOVE(ifc, ifc_list); kfree(ifc->ifc_units, M_CLONE); + ifc->ifc_units = NULL; if_cloners_count--; + ifnet_unlock(); } /* @@ -204,14 +223,24 @@ struct if_clone *ifc; int count, error = 0; + /* + * if_cloners / if_cloners_count are mutated by if_clone_attach() and + * if_clone_detach() (module load/unload); take ifnet_lock to serialize + * against them so the LIST_FOREACH cannot chase a freed/unmapped ifc. + * See DF-0726. + */ + ifnet_lock(); ifcr->ifcr_total = if_cloners_count; if ((dst = ifcr->ifcr_buffer) == NULL) { /* Just asking how many there are. */ + ifnet_unlock(); return (0); } - if (ifcr->ifcr_count < 0) + if (ifcr->ifcr_count < 0) { + ifnet_unlock(); return (EINVAL); + } count = (if_cloners_count < ifcr->ifcr_count) ? if_cloners_count : ifcr->ifcr_count; @@ -225,6 +254,7 @@ if (error) break; } + ifnet_unlock(); return (error); } @@ -297,10 +327,20 @@ { struct if_clone *ifc; + /* + * Serialize against if_clone_attach()/if_clone_detach() writers; the + * LIST_FOREACH otherwise races module unload which frees the ifc backing + * store. See DF-0726. Callers (if_clone_create, if_clone_destroy) do + * NOT hold ifnet_lock at this point, so acquiring it here is safe. + */ + ifnet_lock(); LIST_FOREACH(ifc, &if_cloners, ifc_list) { - if (if_clone_match(ifc, name)) + if (if_clone_match(ifc, name)) { + ifnet_unlock(); return ifc; + } } + ifnet_unlock(); return (NULL); } |