Date: Sun, 7 Feb 2010 01:11:37 GMT From: Gleb Kurtsou <gleb.kurtsou@gmail.com> To: freebsd-gnats-submit@FreeBSD.org Subject: kern/143622: [patch] unlock pfil lock while calling firewall hooks Message-ID: <201002070111.o171BbEm048024@www.freebsd.org> Resent-Message-ID: <201002070120.o171K3rG096453@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>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 <sys/errno.h>
#include <sys/lock.h>
#include <sys/malloc.h>
+#include <sys/refcount.h>
#include <sys/rmlock.h>
#include <sys/socket.h>
#include <sys/socketvar.h>
@@ -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:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201002070111.o171BbEm048024>