Author: rwatson
Date: Tue Dec 16 17:03:22 2008
New Revision: 186187
URL: http://svn.freebsd.org/changeset/base/186187
Log:
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
Modified:
head/sys/net/pfil.c
Modified: head/sys/net/pfil.c
==============================================================================
--- head/sys/net/pfil.c Tue Dec 16 17:01:52 2008 (r186186)
+++ head/sys/net/pfil.c Tue Dec 16 17:03:22 2008 (r186187)
@@ -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 *p
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
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
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)
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
