--- Internalise handling of virtualised hook points inside --- hhook_{add|remove}_hook_lookup() so that khelp (and other potential API --- consumers) do not have to care when they attempt to (un)hook a particular --- hook point identified by id and type. --- --- Reviewed by: scottl --- MFC after: 1 week --- Index: kern/kern_hhook.c =================================================================== --- kern/kern_hhook.c (revision 251768) +++ kern/kern_hhook.c (working copy) @@ -1,12 +1,12 @@ /*- - * Copyright (c) 2010 Lawrence Stewart + * Copyright (c) 2010,2013 Lawrence Stewart * Copyright (c) 2010 The FreeBSD Foundation * All rights reserved. * * This software was developed by Lawrence Stewart while studying at the Centre * for Advanced Internet Architectures, Swinburne University of Technology, * made possible in part by grants from the FreeBSD Foundation and Cisco * University Research Program Fund at Community Foundation Silicon Valley. * * Portions of this software were developed at the Centre for Advanced * Internet Architectures, Swinburne University of Technology, Melbourne, @@ -62,20 +62,23 @@ static MALLOC_DEFINE(M_HHOOK, "hhook", " LIST_HEAD(hhookheadhead, hhook_head); struct hhookheadhead hhook_head_list; VNET_DEFINE(struct hhookheadhead, hhook_vhead_list); #define V_hhook_vhead_list VNET(hhook_vhead_list) static struct mtx hhook_head_list_lock; MTX_SYSINIT(hhookheadlistlock, &hhook_head_list_lock, "hhook_head list lock", MTX_DEF); +/* Protected by hhook_head_list_lock. */ +static uint32_t n_hhookheads; + /* Private function prototypes. */ static void hhook_head_destroy(struct hhook_head *hhh); #define HHHLIST_LOCK() mtx_lock(&hhook_head_list_lock) #define HHHLIST_UNLOCK() mtx_unlock(&hhook_head_list_lock) #define HHHLIST_LOCK_ASSERT() mtx_assert(&hhook_head_list_lock, MA_OWNED) #define HHH_LOCK_INIT(hhh) rm_init(&(hhh)->hhh_lock, "hhook_head rm lock") #define HHH_LOCK_DESTROY(hhh) rm_destroy(&(hhh)->hhh_lock) #define HHH_WLOCK(hhh) rm_wlock(&(hhh)->hhh_lock) @@ -158,35 +161,85 @@ hhook_add_hook(struct hhook_head *hhh, s hhh->hhh_nhooks++; } else free(hhk, M_HHOOK); HHH_WUNLOCK(hhh); return (error); } /* - * Lookup a helper hook point and register a new helper hook function with it. + * Register a helper hook function with a helper hook point (including all + * virtual instances of the hook point if it is virtualised). + * + * The logic is unfortunately far more complex than for + * hhook_remove_hook_lookup() because hhook_add_hook() can call malloc() with + * M_WAITOK and thus we cannot call hhook_add_hook() with the + * hhook_head_list_lock held. + * + * The logic assembles an array of hhook_head structs that correspond to the + * helper hook point being hooked and bumps the refcount on each (all done with + * the hhook_head_list_lock held). The hhook_head_list_lock is then dropped, and + * hhook_add_hook() is called and the refcount dropped for each hhook_head + * struct in the array. */ int hhook_add_hook_lookup(struct hookinfo *hki, uint32_t flags) { - struct hhook_head *hhh; - int error; + struct hhook_head **heads_to_hook, *hhh; + int error, i, n_heads_to_hook; - hhh = hhook_head_get(hki->hook_type, hki->hook_id); +tryagain: + error = i = 0; + /* + * Accessing n_hhookheads without hhook_head_list_lock held opens up a + * race with hhook_head_register() which we are unlikely to lose, but + * nonetheless have to cope with - hence the complex goto logic. + */ + n_heads_to_hook = n_hhookheads; + heads_to_hook = malloc(n_heads_to_hook * sizeof(struct hhook_head *), + M_HHOOK, flags & HHOOK_WAITOK ? M_WAITOK : M_NOWAIT); + if (heads_to_hook == NULL) + return (ENOMEM); - if (hhh == NULL) - return (ENOENT); + HHHLIST_LOCK(); + LIST_FOREACH(hhh, &hhook_head_list, hhh_next) { + if (hhh->hhh_type == hki->hook_type && + hhh->hhh_id == hki->hook_id) { + if (i < n_heads_to_hook) { + heads_to_hook[i] = hhh; + refcount_acquire(&heads_to_hook[i]->hhh_refcount); + i++; + } else { + /* + * We raced with hhook_head_register() which + * inserted a hhook_head that we need to hook + * but did not malloc space for. Abort this run + * and try again. + */ + for (i--; i >= 0; i--) + refcount_release(&heads_to_hook[i]->hhh_refcount); + free(heads_to_hook, M_HHOOK); + HHHLIST_UNLOCK(); + goto tryagain; + } + } + } + HHHLIST_UNLOCK(); - error = hhook_add_hook(hhh, hki, flags); - hhook_head_release(hhh); + for (i--; i >= 0; i--) { + if (!error) + error = hhook_add_hook(heads_to_hook[i], hki, flags); + refcount_release(&heads_to_hook[i]->hhh_refcount); + } + + free(heads_to_hook, M_HHOOK); return (error); } /* * Remove a helper hook function from a helper hook point. */ int hhook_remove_hook(struct hhook_head *hhh, struct hookinfo *hki) { @@ -204,34 +257,35 @@ hhook_remove_hook(struct hhook_head *hhh hhh->hhh_nhooks--; break; } } HHH_WUNLOCK(hhh); return (0); } /* - * Lookup a helper hook point and remove a helper hook function from it. + * Remove a helper hook function from a helper hook point (including all + * virtual instances of the hook point if it is virtualised). */ int hhook_remove_hook_lookup(struct hookinfo *hki) { struct hhook_head *hhh; - hhh = hhook_head_get(hki->hook_type, hki->hook_id); - - if (hhh == NULL) - return (ENOENT); - - hhook_remove_hook(hhh, hki); - hhook_head_release(hhh); + HHHLIST_LOCK(); + LIST_FOREACH(hhh, &hhook_head_list, hhh_next) { + if (hhh->hhh_type == hki->hook_type && + hhh->hhh_id == hki->hook_id) + hhook_remove_hook(hhh, hki); + } + HHHLIST_UNLOCK(); return (0); } /* * Register a new helper hook point. */ int hhook_head_register(int32_t hhook_type, int32_t hhook_id, struct hhook_head **hhh, uint32_t flags) @@ -267,43 +321,46 @@ hhook_head_register(int32_t hhook_type, HHHLIST_LOCK(); if (flags & HHOOK_HEADISINVNET) { tmphhh->hhh_flags |= HHH_ISINVNET; #ifdef VIMAGE KASSERT(curvnet != NULL, ("curvnet is NULL")); tmphhh->hhh_vid = (uintptr_t)curvnet; LIST_INSERT_HEAD(&V_hhook_vhead_list, tmphhh, hhh_vnext); #endif } LIST_INSERT_HEAD(&hhook_head_list, tmphhh, hhh_next); + n_hhookheads++; HHHLIST_UNLOCK(); return (0); } static void hhook_head_destroy(struct hhook_head *hhh) { struct hhook *tmp, *tmp2; HHHLIST_LOCK_ASSERT(); + KASSERT(n_hhookheads > 0, ("n_hhookheads should be > 0")); LIST_REMOVE(hhh, hhh_next); #ifdef VIMAGE if (hhook_head_is_virtualised(hhh) == HHOOK_HEADISINVNET) LIST_REMOVE(hhh, hhh_vnext); #endif HHH_WLOCK(hhh); STAILQ_FOREACH_SAFE(tmp, &hhh->hhh_hooks, hhk_next, tmp2) free(tmp, M_HHOOK); HHH_WUNLOCK(hhh); HHH_LOCK_DESTROY(hhh); free(hhh, M_HHOOK); + n_hhookheads--; } /* * Remove a helper hook point. */ int hhook_head_deregister(struct hhook_head *hhh) { int error; Index: kern/kern_khelp.c =================================================================== --- kern/kern_khelp.c (revision 251768) +++ kern/kern_khelp.c (working copy) @@ -1,12 +1,12 @@ /*- - * Copyright (c) 2010 Lawrence Stewart + * Copyright (c) 2010,2013 Lawrence Stewart * Copyright (c) 2010 The FreeBSD Foundation * All rights reserved. * * This software was developed by Lawrence Stewart while studying at the Centre * for Advanced Internet Architectures, Swinburne University of Technology, * made possible in part by grants from the FreeBSD Foundation and Cisco * University Research Program Fund at Community Foundation Silicon Valley. * * Portions of this software were developed at the Centre for Advanced * Internet Architectures, Swinburne University of Technology, Melbourne, @@ -77,26 +77,27 @@ khelp_register_helper(struct helper *h) error = 0; inserted = 0; refcount_init(&h->h_refcount, 0); h->h_id = osd_register(OSD_KHELP, NULL, NULL); /* It's only safe to add the hooks after osd_register(). */ if (h->h_nhooks > 0) { for (i = 0; i < h->h_nhooks && !error; i++) { /* We don't require the module to assign hook_helper. */ h->h_hooks[i].hook_helper = h; - error = khelp_add_hhook(&h->h_hooks[i], HHOOK_NOWAIT); + error = hhook_add_hook_lookup(&h->h_hooks[i], + HHOOK_WAITOK); } if (error) { for (i--; i >= 0; i--) - khelp_remove_hhook(&h->h_hooks[i]); + hhook_remove_hook_lookup(&h->h_hooks[i]); osd_deregister(OSD_KHELP, h->h_id); } } if (!error) { KHELP_LIST_WLOCK(); /* * Keep list of helpers sorted in descending h_id order. Due to * the way osd_set() works, a sorted list ensures @@ -137,21 +138,21 @@ khelp_deregister_helper(struct helper *h error = 0; break; } } } KHELP_LIST_WUNLOCK(); if (!error) { if (h->h_nhooks > 0) { for (i = 0; i < h->h_nhooks; i++) - khelp_remove_hhook(&h->h_hooks[i]); + hhook_remove_hook_lookup(&h->h_hooks[i]); } osd_deregister(OSD_KHELP, h->h_id); } return (error); } int khelp_init_osd(uint32_t classes, struct osd *hosd) { @@ -256,71 +257,41 @@ khelp_get_id(char *hname) } } KHELP_LIST_RUNLOCK(); return (id); } int khelp_add_hhook(struct hookinfo *hki, uint32_t flags) { - VNET_ITERATOR_DECL(vnet_iter); int error; - error = 0; - /* - * XXXLAS: If a helper is dynamically adding a helper hook function at - * runtime using this function, we should update the helper's h_hooks - * struct member to include the additional hookinfo struct. + * XXXLAS: Should probably include the functionality to update the + * helper's h_hooks struct member. */ - - VNET_LIST_RLOCK_NOSLEEP(); - VNET_FOREACH(vnet_iter) { - CURVNET_SET(vnet_iter); - error = hhook_add_hook_lookup(hki, flags); - CURVNET_RESTORE(); -#ifdef VIMAGE - if (error) - break; -#endif - } - VNET_LIST_RUNLOCK_NOSLEEP(); + error = hhook_add_hook_lookup(hki, flags); return (error); } int khelp_remove_hhook(struct hookinfo *hki) { - VNET_ITERATOR_DECL(vnet_iter); int error; - error = 0; - /* - * XXXLAS: If a helper is dynamically removing a helper hook function at - * runtime using this function, we should update the helper's h_hooks - * struct member to remove the defunct hookinfo struct. + * XXXLAS: Should probably include the functionality to update the + * helper's h_hooks struct member. */ - - VNET_LIST_RLOCK_NOSLEEP(); - VNET_FOREACH(vnet_iter) { - CURVNET_SET(vnet_iter); - error = hhook_remove_hook_lookup(hki); - CURVNET_RESTORE(); -#ifdef VIMAGE - if (error) - break; -#endif - } - VNET_LIST_RUNLOCK_NOSLEEP(); + error = hhook_remove_hook_lookup(hki); return (error); } int khelp_modevent(module_t mod, int event_type, void *data) { struct khelp_modevent_data *kmd; int error; Index: sys/hhook.h =================================================================== --- sys/hhook.h (revision 251768) +++ sys/hhook.h (working copy) @@ -1,12 +1,12 @@ /*- - * Copyright (c) 2010 Lawrence Stewart + * Copyright (c) 2010,2013 Lawrence Stewart * Copyright (c) 2010 The FreeBSD Foundation * All rights reserved. * * This software was developed by Lawrence Stewart while studying at the Centre * for Advanced Internet Architectures, Swinburne University of Technology, made * possible in part by grants from the FreeBSD Foundation and Cisco University * Research Program Fund at Community Foundation Silicon Valley. * * Portions of this software were developed at the Centre for Advanced * Internet Architectures, Swinburne University of Technology, Melbourne,