From owner-freebsd-bugs@FreeBSD.ORG Sun Feb 7 01:20:04 2010 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 27CA51065679 for ; Sun, 7 Feb 2010 01:20:04 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id E05448FC16 for ; Sun, 7 Feb 2010 01:20:03 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.3/8.14.3) with ESMTP id o171K3uU096456 for ; Sun, 7 Feb 2010 01:20:03 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.3/8.14.3/Submit) id o171K3rG096453; Sun, 7 Feb 2010 01:20:03 GMT (envelope-from gnats) Resent-Date: Sun, 7 Feb 2010 01:20:03 GMT Resent-Message-Id: <201002070120.o171K3rG096453@freefall.freebsd.org> Resent-From: FreeBSD-gnats-submit@FreeBSD.org (GNATS Filer) Resent-To: freebsd-bugs@FreeBSD.org Resent-Reply-To: FreeBSD-gnats-submit@FreeBSD.org, Gleb Kurtsou Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 4234F106566C for ; Sun, 7 Feb 2010 01:11:38 +0000 (UTC) (envelope-from nobody@FreeBSD.org) Received: from www.freebsd.org (www.freebsd.org [IPv6:2001:4f8:fff6::21]) by mx1.freebsd.org (Postfix) with ESMTP id 31E978FC08 for ; Sun, 7 Feb 2010 01:11:38 +0000 (UTC) Received: from www.freebsd.org (localhost [127.0.0.1]) by www.freebsd.org (8.14.3/8.14.3) with ESMTP id o171BbBT048042 for ; Sun, 7 Feb 2010 01:11:37 GMT (envelope-from nobody@www.freebsd.org) Received: (from nobody@localhost) by www.freebsd.org (8.14.3/8.14.3/Submit) id o171BbEm048024; Sun, 7 Feb 2010 01:11:37 GMT (envelope-from nobody) Message-Id: <201002070111.o171BbEm048024@www.freebsd.org> Date: Sun, 7 Feb 2010 01:11:37 GMT From: Gleb Kurtsou To: freebsd-gnats-submit@FreeBSD.org X-Send-Pr-Version: www-3.1 Cc: Subject: kern/143622: [patch] unlock pfil lock while calling firewall hooks X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 07 Feb 2010 01:20:04 -0000 >Number: 143622 >Category: kern >Synopsis: [patch] unlock pfil lock while calling firewall hooks >Confidential: no >Severity: non-critical >Priority: low >Responsible: freebsd-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Sun Feb 07 01:20:03 UTC 2010 >Closed-Date: >Last-Modified: >Originator: Gleb Kurtsou >Release: >Organization: >Environment: FreeBSD sandbox9.vbox 9.0-CURRENT FreeBSD 9.0-CURRENT #2 r201894+71985df: Sun Feb 7 02:39:39 UTC 2010 root@sandbox9.vbox:/usr/obj/usr/src/sys/SANDBOX amd64 >Description: Unlock pfil lock while running hooks. All firewalls in the tree implement locking on there own not expecting to be called with non-sleepable lock held. Besides holding a lock while calling other subsystems is error prone, forbids firewall reentrants, etc. To grantee consistency add per hook reference count and lazily remove hooks from list. >How-To-Repeat: >Fix: Patch attached with submission follows: diff --git a/sys/net/pfil.c b/sys/net/pfil.c index a11950f..36196dc 100644 --- a/sys/net/pfil.c +++ b/sys/net/pfil.c @@ -34,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -56,7 +57,7 @@ static int pfil_list_add(pfil_list_t *, struct packet_filter_hook *, int); static int pfil_list_remove(pfil_list_t *, int (*)(void *, struct mbuf **, struct ifnet *, int, struct inpcb *), - void *); + void *, struct pfil_head *); LIST_HEAD(pfilheadhead, pfil_head); VNET_DEFINE(struct pfilheadhead, pfil_head_list); @@ -76,16 +77,24 @@ pfil_run_hooks(struct pfil_head *ph, struct mbuf **mp, struct ifnet *ifp, PFIL_RLOCK(ph, &rmpt); KASSERT(ph->ph_nhooks >= 0, ("Pfil hook count dropped < 0")); + refcount_acquire(&ph->ph_refcnt); for (pfh = pfil_hook_get(dir, ph); pfh != NULL; pfh = TAILQ_NEXT(pfh, pfil_link)) { - if (pfh->pfil_func != NULL) { + if (pfh->pfil_func != NULL && + (pfh->pfil_flags & PFIL_HOOK_DOOMED) == 0 ) { + PFIL_RUNLOCK(ph, &rmpt); rv = (*pfh->pfil_func)(pfh->pfil_arg, &m, ifp, dir, inp); - if (rv != 0 || m == NULL) - break; + if (rv != 0 || m == NULL) { + refcount_release(&ph->ph_refcnt); + goto out; + } + PFIL_RLOCK(ph, &rmpt); } } + refcount_release(&ph->ph_refcnt); PFIL_RUNLOCK(ph, &rmpt); +out: *mp = m; return (rv); } @@ -107,6 +116,7 @@ pfil_head_register(struct pfil_head *ph) return (EEXIST); } } + refcount_init(&ph->ph_refcnt, 0); PFIL_LOCK_INIT(ph); ph->ph_nhooks = 0; TAILQ_INIT(&ph->ph_in); @@ -186,9 +196,15 @@ pfil_add_hook(int (*func)(void *, struct mbuf **, struct ifnet *, int, } } PFIL_WLOCK(ph); + if (ph->ph_refcnt == 0) { + /* Remove doomed hooks */ + pfil_list_remove(&ph->ph_in, NULL, NULL, ph); + pfil_list_remove(&ph->ph_out, NULL, NULL, ph); + } if (flags & PFIL_IN) { pfh1->pfil_func = func; pfh1->pfil_arg = arg; + pfh1->pfil_flags = 0; err = pfil_list_add(&ph->ph_in, pfh1, flags & ~PFIL_OUT); if (err) goto locked_error; @@ -197,10 +213,11 @@ pfil_add_hook(int (*func)(void *, struct mbuf **, struct ifnet *, int, if (flags & PFIL_OUT) { pfh2->pfil_func = func; pfh2->pfil_arg = arg; + pfh2->pfil_flags = 0; err = pfil_list_add(&ph->ph_out, pfh2, flags & ~PFIL_IN); if (err) { if (flags & PFIL_IN) - pfil_list_remove(&ph->ph_in, func, arg); + pfil_list_remove(&ph->ph_in, func, arg, ph); goto locked_error; } ph->ph_nhooks++; @@ -229,12 +246,12 @@ pfil_remove_hook(int (*func)(void *, struct mbuf **, struct ifnet *, int, PFIL_WLOCK(ph); if (flags & PFIL_IN) { - err = pfil_list_remove(&ph->ph_in, func, arg); + err = pfil_list_remove(&ph->ph_in, func, arg, ph); if (err == 0) ph->ph_nhooks--; } if ((err == 0) && (flags & PFIL_OUT)) { - err = pfil_list_remove(&ph->ph_out, func, arg); + err = pfil_list_remove(&ph->ph_out, func, arg, ph); if (err == 0) ph->ph_nhooks--; } @@ -250,11 +267,13 @@ pfil_list_add(pfil_list_t *list, struct packet_filter_hook *pfh1, int flags) /* * First make sure the hook is not already there. */ - TAILQ_FOREACH(pfh, list, pfil_link) + TAILQ_FOREACH(pfh, list, pfil_link) { if (pfh->pfil_func == pfh1->pfil_func && - pfh->pfil_arg == pfh1->pfil_arg) + pfh->pfil_arg == pfh1->pfil_arg && + (pfh->pfil_flags & PFIL_HOOK_DOOMED) == 0) { return (EEXIST); - + } + } /* * Insert the input list in reverse order of the output list so that * the same path is followed in or out of the kernel. @@ -273,14 +292,19 @@ pfil_list_add(pfil_list_t *list, struct packet_filter_hook *pfh1, int flags) static int pfil_list_remove(pfil_list_t *list, int (*func)(void *, struct mbuf **, struct ifnet *, int, struct inpcb *), - void *arg) + void *arg, struct pfil_head *ph) { struct packet_filter_hook *pfh; TAILQ_FOREACH(pfh, list, pfil_link) - if (pfh->pfil_func == func && pfh->pfil_arg == arg) { - TAILQ_REMOVE(list, pfh, pfil_link); - free(pfh, M_IFADDR); + if ((pfh->pfil_func == func && pfh->pfil_arg == arg) || + (pfh->pfil_flags & PFIL_HOOK_DOOMED) != 0) { + if (ph->ph_refcnt == 0) { + TAILQ_REMOVE(list, pfh, pfil_link); + free(pfh, M_IFADDR); + } else { + pfh->pfil_flags |= PFIL_HOOK_DOOMED; + } return (0); } return (ENOENT); diff --git a/sys/net/pfil.h b/sys/net/pfil.h index 6ac750a..56d44d9 100644 --- a/sys/net/pfil.h +++ b/sys/net/pfil.h @@ -47,11 +47,15 @@ struct inpcb; * The packet filter hooks are designed for anything to call them to * possibly intercept the packet. */ + +#define PFIL_HOOK_DOOMED 0x00000001 + struct packet_filter_hook { TAILQ_ENTRY(packet_filter_hook) pfil_link; int (*pfil_func)(void *, struct mbuf **, struct ifnet *, int, struct inpcb *); void *pfil_arg; + int pfil_flags; }; #define PFIL_IN 0x00000001 @@ -69,6 +73,7 @@ struct pfil_head { pfil_list_t ph_out; int ph_type; int ph_nhooks; + volatile u_int ph_refcnt; struct rmlock ph_lock; union { u_long phu_val; >Release-Note: >Audit-Trail: >Unformatted: