From: Sepherosa Ziehau <sephe@dragonflybsd.org>
Date: Sun, 14 Sep 2008 11:13:00 +0000 (+0000)
Subject: Make pfil(4) MPSAFE using following way:
X-Git-Tag: v2.1.1~410
X-Git-Url: https://gitweb.dragonflybsd.org/dragonfly.git/commitdiff_plain/d66da3ad7f075fde1da4d78f84a9504d71cd66c9

Make pfil(4) MPSAFE using following way:
- The pfil hook lists alteration is serialized by netisr0 (rev 1.10)
- Duplicate the pfil hook lists to be altered.
- Alter the pfil hook lists' duplication
- Save the original pfil hook lists, install the altered pfil hook lists
- Sync all network msgports to make sure that no one sees the original
  lists after this syncing
- Free the original pfil hook lists
---

diff --git a/sys/net/pfil.c b/sys/net/pfil.c
index bfdcf037f4..82125c37cb 100644
--- a/sys/net/pfil.c
+++ b/sys/net/pfil.c
@@ -1,5 +1,5 @@
 /*	$NetBSD: pfil.c,v 1.20 2001/11/12 23:49:46 lukem Exp $	*/
-/* $DragonFly: src/sys/net/pfil.c,v 1.10 2008/09/14 08:58:55 sephe Exp $ */
+/* $DragonFly: src/sys/net/pfil.c,v 1.11 2008/09/14 11:13:00 sephe Exp $ */
 
 /*
  * Copyright (c) 1996 Matthew R. Green
@@ -52,11 +52,20 @@ struct netmsg_pfil {
 	struct pfil_head	*pfil_ph;
 };
 
-static int pfil_list_add(struct pfil_head *, pfil_func_t, void *, int);
-static int pfil_list_remove(struct pfil_head *, pfil_func_t, void *, int);
+static LIST_HEAD(, pfil_head) pfil_head_list =
+	LIST_HEAD_INITIALIZER(&pfil_head_list);
 
-LIST_HEAD(, pfil_head) pfil_head_list =
-    LIST_HEAD_INITIALIZER(&pfil_head_list);
+static pfil_list_t	*pfil_list_alloc(void);
+static void		pfil_list_free(pfil_list_t *);
+static void 		pfil_list_dup(const pfil_list_t *, pfil_list_t *,
+				      const struct packet_filter_hook *);
+static void 		pfil_list_add(pfil_list_t *, pfil_func_t, void *, int);
+static struct packet_filter_hook *
+			pfil_list_find(const pfil_list_t *, pfil_func_t,
+				       const void *);
+
+static void		pfil_remove_hook_dispatch(struct netmsg *);
+static void		pfil_add_hook_dispatch(struct netmsg *);
 
 /*
  * pfil_run_hooks() runs the specified packet filter hooks.
@@ -71,9 +80,9 @@ pfil_run_hooks(struct pfil_head *ph, struct mbuf **mp, struct ifnet *ifp,
 	int rv = 0;
 
 	if (dir == PFIL_IN)
-		list = &ph->ph_in;
+		list = ph->ph_in;
 	else if (dir == PFIL_OUT)
-		list = &ph->ph_out;
+		list = ph->ph_out;
 	else
 		return 0; /* XXX panic? */
 
@@ -104,8 +113,8 @@ pfil_head_register(struct pfil_head *ph)
 			return EEXIST;
 	}
 
-	TAILQ_INIT(&ph->ph_in);
-	TAILQ_INIT(&ph->ph_out);
+	ph->ph_in = pfil_list_alloc();
+	ph->ph_out = pfil_list_alloc();
 	ph->ph_hashooks = 0;
 
 	LIST_INSERT_HEAD(&pfil_head_list, ph, ph_list);
@@ -147,20 +156,74 @@ pfil_add_hook_dispatch(struct netmsg *nmsg)
 	void *arg = pfilmsg->pfil_arg;
 	int flags = pfilmsg->pfil_flags;
 	struct pfil_head *ph = pfilmsg->pfil_ph;
+	const struct packet_filter_hook *pfh;
+	pfil_list_t *list_in = NULL, *list_out = NULL;
+	pfil_list_t *old_list_in = NULL, *old_list_out = NULL;
 	int err = 0;
 
+	/* This probably should not happen ... */
+	if ((flags & (PFIL_IN | PFIL_OUT)) == 0)
+		goto reply; /* XXX panic? */
+
+	/*
+	 * If pfil hooks exist on any of the requested lists,
+	 * then bail out.
+	 */
 	if (flags & PFIL_IN) {
-		err = pfil_list_add(ph, func, arg, flags & ~PFIL_OUT);
-		if (err)
+		pfh = pfil_list_find(ph->ph_in, func, arg);
+		if (pfh != NULL) {
+			err = EEXIST;
 			goto reply;
+		}
 	}
 	if (flags & PFIL_OUT) {
-		err = pfil_list_add(ph, func, arg, flags & ~PFIL_IN);
-		if (err) {
-			if (flags & PFIL_IN)
-				pfil_list_remove(ph, func, arg, PFIL_IN);
+		pfh = pfil_list_find(ph->ph_out, func, arg);
+		if (pfh != NULL) {
+			err = EEXIST;
+			goto reply;
 		}
 	}
+
+	/*
+	 * Duplicate the requested lists, install new hooks
+	 * on the duplication
+	 */
+	if (flags & PFIL_IN) {
+		list_in = pfil_list_alloc();
+		pfil_list_dup(ph->ph_in, list_in, NULL);
+		pfil_list_add(list_in, func, arg, flags & ~PFIL_OUT);
+	}
+	if (flags & PFIL_OUT) {
+		list_out = pfil_list_alloc();
+		pfil_list_dup(ph->ph_out, list_out, NULL);
+		pfil_list_add(list_out, func, arg, flags & ~PFIL_IN);
+	}
+
+	/*
+	 * Switch list pointers, but keep the old ones
+	 */
+	if (list_in != NULL) {
+		old_list_in = ph->ph_in;
+		ph->ph_in = list_in;
+	}
+	if (list_out != NULL) {
+		old_list_out = ph->ph_out;
+		ph->ph_out = list_out;
+	}
+
+	/*
+	 * Wait until everyone has finished the old lists iteration
+	 */
+	netmsg_service_sync();
+	ph->ph_hashooks = 1;
+
+	/*
+	 * Now it is safe to free the old lists, since no one sees it
+	 */
+	if (old_list_in != NULL)
+		pfil_list_free(old_list_in);
+	if (old_list_out != NULL)
+		pfil_list_free(old_list_out);
 reply:
 	lwkt_replymsg(&nmsg->nm_lmsg, err);
 }
@@ -171,7 +234,6 @@ reply:
  *	PFIL_IN		call me on incoming packets
  *	PFIL_OUT	call me on outgoing packets
  *	PFIL_ALL	call me on all of the above
- *	PFIL_WAITOK	OK to call kmalloc with M_WAITOK.
  */
 int
 pfil_add_hook(pfil_func_t func, void *arg, int flags, struct pfil_head *ph)
@@ -189,58 +251,84 @@ pfil_add_hook(pfil_func_t func, void *arg, int flags, struct pfil_head *ph)
 	return lwkt_domsg(PFIL_CFGPORT, &nmsg->nm_lmsg, 0);
 }
 
-static int
-pfil_list_add(struct pfil_head *ph, pfil_func_t func, void *arg, int flags)
+static void
+pfil_remove_hook_dispatch(struct netmsg *nmsg)
 {
-	struct packet_filter_hook *pfh;
-	pfil_list_t *list;
-
-	KKASSERT(&curthread->td_msgport == PFIL_CFGPORT);
+	struct netmsg_pfil *pfilmsg = (struct netmsg_pfil *)nmsg;
+	pfil_func_t func = pfilmsg->pfil_func;
+	void *arg = pfilmsg->pfil_arg;
+	int flags = pfilmsg->pfil_flags;
+	struct pfil_head *ph = pfilmsg->pfil_ph;
+	struct packet_filter_hook *skip_in = NULL, *skip_out = NULL;
+	pfil_list_t *list_in = NULL, *list_out = NULL;
+	pfil_list_t *old_list_in = NULL, *old_list_out = NULL;
+	int err = 0;
 
-	list = (flags & PFIL_IN) ? &ph->ph_in : &ph->ph_out;
+	/* This probably should not happen ... */
+	if ((flags & (PFIL_IN | PFIL_OUT)) == 0)
+		goto reply; /* XXX panic? */
 
 	/*
-	 * First make sure the hook is not already there.
+	 * The pfil hook should exist on all requested lists,
+	 * if not just bail out
 	 */
-	TAILQ_FOREACH(pfh, list, pfil_link) {
-		if (pfh->pfil_func == func && pfh->pfil_arg == arg)
-			return EEXIST;
+	if (flags & PFIL_IN) {
+		skip_in = pfil_list_find(ph->ph_in, func, arg);
+		if (!skip_in) {
+			err = ENOENT;
+			goto reply;
+		}
+	}
+	if (flags & PFIL_OUT) {
+		skip_out = pfil_list_find(ph->ph_out, func, arg);
+		if (!skip_out) {
+			err = ENOENT;
+			goto reply;
+		}
 	}
 
-	pfh = (struct packet_filter_hook *)kmalloc(sizeof(*pfh), M_IFADDR,
-	    (flags & PFIL_WAITOK) ? M_WAITOK : M_NOWAIT);
-	if (pfh == NULL)
-		return ENOMEM;
-
-	pfh->pfil_func = func;
-	pfh->pfil_arg  = arg;
+	/*
+	 * Duplicate the requested lists, but the pfil hook to
+	 * be deleted is not copied
+	 */
+	if (flags & PFIL_IN) {
+		KKASSERT(skip_in != NULL);
+		list_in = pfil_list_alloc();
+		pfil_list_dup(ph->ph_in, list_in, skip_in);
+	}
+	if (flags & PFIL_OUT) {
+		KKASSERT(skip_out != NULL);
+		list_out = pfil_list_alloc();
+		pfil_list_dup(ph->ph_out, list_out, skip_out);
+	}
 
 	/*
-	 * insert the input list in reverse order of the output list
-	 * so that the same path is followed in or out of the kernel.
+	 * Switch list pointers, but keep the old ones
 	 */
-	if (flags & PFIL_IN)
-		TAILQ_INSERT_HEAD(list, pfh, pfil_link);
-	else
-		TAILQ_INSERT_TAIL(list, pfh, pfil_link);
-	ph->ph_hashooks = 1;
-	return (0);
-}
+	if (list_in != NULL) {
+		old_list_in = ph->ph_in;
+		ph->ph_in = list_in;
+	}
+	if (list_out != NULL) {
+		old_list_out = ph->ph_out;
+		ph->ph_out = list_out;
+	}
 
-static void
-pfil_remove_hook_dispatch(struct netmsg *nmsg)
-{
-	struct netmsg_pfil *pfilmsg = (struct netmsg_pfil *)nmsg;
-	pfil_func_t func = pfilmsg->pfil_func;
-	void *arg = pfilmsg->pfil_arg;
-	int flags = pfilmsg->pfil_flags;
-	struct pfil_head *ph = pfilmsg->pfil_ph;
-	int err = 0;
+	/*
+	 * Wait until everyone has finished the old lists iteration
+	 */
+	if (TAILQ_EMPTY(ph->ph_in) && TAILQ_EMPTY(ph->ph_out))
+		ph->ph_hashooks = 0;
+	netmsg_service_sync();
 
-	if (flags & PFIL_IN)
-		err = pfil_list_remove(ph, func, arg, PFIL_IN);
-	if ((err == 0) && (flags & PFIL_OUT))
-		err = pfil_list_remove(ph, func, arg, PFIL_OUT);
+	/*
+	 * Now it is safe to free the old lists, since no one sees it
+	 */
+	if (old_list_in != NULL)
+		pfil_list_free(old_list_in);
+	if (old_list_out != NULL)
+		pfil_list_free(old_list_out);
+reply:
 	lwkt_replymsg(&nmsg->nm_lmsg, err);
 }
 
@@ -264,28 +352,80 @@ pfil_remove_hook(pfil_func_t func, void *arg, int flags, struct pfil_head *ph)
 	return lwkt_domsg(PFIL_CFGPORT, &nmsg->nm_lmsg, 0);
 }
 
-/*
- * pfil_list_remove is an internal function that takes a function off the
- * specified list.  Clear ph_hashooks if no functions remain on any list.
- */
-static int
-pfil_list_remove(struct pfil_head *ph, pfil_func_t func, void *arg, int flags)
+static void
+pfil_list_add(pfil_list_t *list, pfil_func_t func, void *arg, int flags)
 {
 	struct packet_filter_hook *pfh;
-	pfil_list_t *list;
 
 	KKASSERT(&curthread->td_msgport == PFIL_CFGPORT);
 
-	list = (flags & PFIL_IN) ? &ph->ph_in : &ph->ph_out;
+	pfh = kmalloc(sizeof(*pfh), M_IFADDR, M_WAITOK);
+
+	pfh->pfil_func = func;
+	pfh->pfil_arg  = arg;
+
+	/*
+	 * Insert the input list in reverse order of the output list
+	 * so that the same path is followed in or out of the kernel.
+	 */
+	if (flags & PFIL_IN)
+		TAILQ_INSERT_HEAD(list, pfh, pfil_link);
+	else
+		TAILQ_INSERT_TAIL(list, pfh, pfil_link);
+}
+
+static void
+pfil_list_dup(const pfil_list_t *from, pfil_list_t *to,
+	      const struct packet_filter_hook *skip)
+{
+	struct packet_filter_hook *pfh_to, *pfh_from;
+
+	KKASSERT(&curthread->td_msgport == PFIL_CFGPORT);
+	KKASSERT(TAILQ_EMPTY(to));
+
+	TAILQ_FOREACH(pfh_from, from, pfil_link) {
+		if (pfh_from == skip)
+			continue;
+
+		pfh_to = kmalloc(sizeof(*pfh_to), M_IFADDR, M_WAITOK);
+		bcopy(pfh_from, pfh_to, sizeof(*pfh_to));
+
+		TAILQ_INSERT_TAIL(to, pfh_to, pfil_link);
+	}
+}
+
+static pfil_list_t *
+pfil_list_alloc(void)
+{
+	pfil_list_t *list;
+
+	list = kmalloc(sizeof(*list), M_IFADDR, M_WAITOK);
+	TAILQ_INIT(list);
+	return list;
+}
+
+static void
+pfil_list_free(pfil_list_t *list)
+{
+	struct packet_filter_hook *pfh;
+
+	while ((pfh = TAILQ_FIRST(list)) != NULL) {
+		TAILQ_REMOVE(list, pfh, pfil_link);
+		kfree(pfh, M_IFADDR);
+	}
+	kfree(list, M_IFADDR);
+}
+
+static struct packet_filter_hook *
+pfil_list_find(const pfil_list_t *list, pfil_func_t func, const void *arg)
+{
+	struct packet_filter_hook *pfh;
+
+	KKASSERT(&curthread->td_msgport == PFIL_CFGPORT);
 
 	TAILQ_FOREACH(pfh, list, pfil_link) {
-		if (pfh->pfil_func == func && pfh->pfil_arg == arg) {
-			TAILQ_REMOVE(list, pfh, pfil_link);
-			kfree(pfh, M_IFADDR);
-			if (TAILQ_EMPTY(&ph->ph_in) && TAILQ_EMPTY(&ph->ph_out))
-				ph->ph_hashooks = 0;
-			return 0;
-		}
+		if (pfh->pfil_func == func && pfh->pfil_arg == arg)
+			return pfh;
 	}
-	return ENOENT;
+	return NULL;
 }
diff --git a/sys/net/pfil.h b/sys/net/pfil.h
index 80b4f7f23e..fe5219c318 100644
--- a/sys/net/pfil.h
+++ b/sys/net/pfil.h
@@ -1,5 +1,5 @@
 /*	$NetBSD: pfil.h,v 1.22 2003/06/23 12:57:08 martin Exp $	*/
-/* $DragonFly: src/sys/net/pfil.h,v 1.7 2008/09/14 05:22:44 sephe Exp $ */
+/* $DragonFly: src/sys/net/pfil.h,v 1.8 2008/09/14 11:13:00 sephe Exp $ */
 
 /*
  * Copyright (c) 1996 Matthew R. Green
@@ -69,8 +69,8 @@ typedef	TAILQ_HEAD(pfil_list, packet_filter_hook) pfil_list_t;
 #define	PFIL_TYPE_IFNET		2	/* key is ifnet pointer */
 
 struct pfil_head {
-	pfil_list_t	ph_in;
-	pfil_list_t	ph_out;
+	pfil_list_t	*ph_in;
+	pfil_list_t	*ph_out;
 	int		ph_type;
 	int		ph_hashooks;	/* 0 if no hooks installed */
 	union {