diff options
author | rwatson <rwatson@FreeBSD.org> | 2008-12-16 17:03:22 +0000 |
---|---|---|
committer | rwatson <rwatson@FreeBSD.org> | 2008-12-16 17:03:22 +0000 |
commit | 1d38ccff9412b97456b3869f294eda87234057c9 (patch) | |
tree | aa6c7a5689d761c4ddf4a01aeb74dc38124a4482 /sys/net | |
parent | a06a1075a6719e0d49495a44becc791ba9f37bbc (diff) | |
download | FreeBSD-src-1d38ccff9412b97456b3869f294eda87234057c9.zip FreeBSD-src-1d38ccff9412b97456b3869f294eda87234057c9.tar.gz |
A few locking fixes and cleanups to pfil hook registration,
unregistration, and execution:
- Add some brackets for clarity and trim a bit of vertical whitespace.
- Remove comments that may not contribute to clarity, such as "Lock"
before acquiring a lock and "Get memory" before allocating memory.
- During hook registration, don't drop pfil_list_lock between checking
for a duplicate and registering the hook, as this leaves a race
condition by failing to enforce the "no duplicate hooks" invariant.
- Don't lock the hook during registration, since it's not yet in use.
- Document assumption that hooks will be quiesced before being
unregistered.
- Don't write-lock hooks during removal because they are assumed
quiesced.
- Rename "done" label to "locked_error" to be clear that it's an error
path on the way out of hook execution.
MFC after: pretty soon
Diffstat (limited to 'sys/net')
-rw-r--r-- | sys/net/pfil.c | 42 |
1 files changed, 10 insertions, 32 deletions
diff --git a/sys/net/pfil.c b/sys/net/pfil.c index bb82bb3..3018eb9 100644 --- a/sys/net/pfil.c +++ b/sys/net/pfil.c @@ -97,33 +97,26 @@ pfil_head_register(struct pfil_head *ph) struct pfil_head *lph; PFIL_LIST_LOCK(); - LIST_FOREACH(lph, &pfil_head_list, ph_list) + LIST_FOREACH(lph, &pfil_head_list, ph_list) { if (ph->ph_type == lph->ph_type && ph->ph_un.phu_val == lph->ph_un.phu_val) { PFIL_LIST_UNLOCK(); return EEXIST; } - PFIL_LIST_UNLOCK(); - + } PFIL_LOCK_INIT(ph); - PFIL_WLOCK(ph); ph->ph_nhooks = 0; - TAILQ_INIT(&ph->ph_in); TAILQ_INIT(&ph->ph_out); - - PFIL_LIST_LOCK(); LIST_INSERT_HEAD(&pfil_head_list, ph, ph_list); PFIL_LIST_UNLOCK(); - - PFIL_WUNLOCK(ph); - return (0); } /* - * pfil_head_unregister() removes a pfil_head from the packet filter - * hook mechanism. + * pfil_head_unregister() removes a pfil_head from the packet filter hook + * mechanism. The producer of the hook promises that all outstanding + * invocations of the hook have completed before it unregisters the hook. */ int pfil_head_unregister(struct pfil_head *ph) @@ -131,21 +124,13 @@ pfil_head_unregister(struct pfil_head *ph) struct packet_filter_hook *pfh, *pfnext; PFIL_LIST_LOCK(); - /* - * LIST_REMOVE is safe for unlocked pfil_heads in ph_list. - * No need to WLOCK all of them. - */ LIST_REMOVE(ph, ph_list); PFIL_LIST_UNLOCK(); - - PFIL_WLOCK(ph); - TAILQ_FOREACH_SAFE(pfh, &ph->ph_in, pfil_link, pfnext) free(pfh, M_IFADDR); TAILQ_FOREACH_SAFE(pfh, &ph->ph_out, pfil_link, pfnext) free(pfh, M_IFADDR); PFIL_LOCK_DESTROY(ph); - return (0); } @@ -175,14 +160,13 @@ pfil_head_get(int type, u_long val) * PFIL_WAITOK OK to call malloc with M_WAITOK. */ int -pfil_add_hook(int (*func)(void *, struct mbuf **, struct ifnet *, int, struct inpcb *), - void *arg, int flags, struct pfil_head *ph) +pfil_add_hook(int (*func)(void *, struct mbuf **, struct ifnet *, int, + struct inpcb *), void *arg, int flags, struct pfil_head *ph) { struct packet_filter_hook *pfh1 = NULL; struct packet_filter_hook *pfh2 = NULL; int err; - /* Get memory */ if (flags & PFIL_IN) { pfh1 = (struct packet_filter_hook *)malloc(sizeof(*pfh1), M_IFADDR, (flags & PFIL_WAITOK) ? M_WAITOK : M_NOWAIT); @@ -199,17 +183,13 @@ pfil_add_hook(int (*func)(void *, struct mbuf **, struct ifnet *, int, struct in goto error; } } - - /* Lock */ PFIL_WLOCK(ph); - - /* Add */ if (flags & PFIL_IN) { pfh1->pfil_func = func; pfh1->pfil_arg = arg; err = pfil_list_add(&ph->ph_in, pfh1, flags & ~PFIL_OUT); if (err) - goto done; + goto locked_error; ph->ph_nhooks++; } if (flags & PFIL_OUT) { @@ -219,15 +199,13 @@ pfil_add_hook(int (*func)(void *, struct mbuf **, struct ifnet *, int, struct in if (err) { if (flags & PFIL_IN) pfil_list_remove(&ph->ph_in, func, arg); - goto done; + goto locked_error; } ph->ph_nhooks++; } - PFIL_WUNLOCK(ph); - return 0; -done: +locked_error: PFIL_WUNLOCK(ph); error: if (pfh1 != NULL) |