Re: svn commit: r254773 - head/sys/net

2013-09-08 Thread Andre Oppermann 

On 07.09.2013 17:30, Mikolaj Golub wrote:

Hi,

On Sat, Aug 24, 2013 at 11:17:25AM +, Andre Oppermann wrote:

Author: andre
Date: Sat Aug 24 11:17:25 2013
New Revision: 254773
URL: http://svnweb.freebsd.org/changeset/base/254773

Log:
   Resolve the confusion between the head_list and the hook list.

   The linked list of pfil hooks is changed to "chain" and this term
   is applied consistently.  The head_list remains with "list" term.

   Add KASSERT to vnet_pfil_uninit().


...


  vnet_pfil_uninit(const void *unused)
  {

-   /*  XXX should panic if list is not empty */
+   KASSERT(LIST_EMPTY(&V_pfil_head_list),
+   ("%s: pfil_head_list %p not empty", __func__, &V_pfil_head_list));
PFIL_LOCK_DESTROY_REAL(&V_pfil_lock);
return (0);
  }



It is triggered when destroying a vnet, due to inet/inet6 pfil hooks
are not being unregistered.

The attached patch fixes the issue for me. I am going to commit it if
there are no objections -- might the unregistration has been skipped
intentionally due to some unresolved issue?


There's no reason that I know of.  And if there were then unregistering
would be unsafe in any case.

--
Andre

___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r254773 - head/sys/net

2013-09-07 Thread Mikolaj Golub 
Hi,

On Sat, Aug 24, 2013 at 11:17:25AM +, Andre Oppermann wrote:
> Author: andre
> Date: Sat Aug 24 11:17:25 2013
> New Revision: 254773
> URL: http://svnweb.freebsd.org/changeset/base/254773
> 
> Log:
>   Resolve the confusion between the head_list and the hook list.
>   
>   The linked list of pfil hooks is changed to "chain" and this term
>   is applied consistently.  The head_list remains with "list" term.
>   
>   Add KASSERT to vnet_pfil_uninit().

...

>  vnet_pfil_uninit(const void *unused)
>  {
>  
> - /*  XXX should panic if list is not empty */
> + KASSERT(LIST_EMPTY(&V_pfil_head_list),
> + ("%s: pfil_head_list %p not empty", __func__, &V_pfil_head_list));
>   PFIL_LOCK_DESTROY_REAL(&V_pfil_lock);
>   return (0);
>  }
> 

It is triggered when destroying a vnet, due to inet/inet6 pfil hooks
are not being unregistered.

The attached patch fixes the issue for me. I am going to commit it if
there are no objections -- might the unregistration has been skipped
intentionally due to some unresolved issue?

-- 
Mikolaj Golub
Unregister inet/inet6 pfil hooks on vnet destroy.

Index: sys/netinet/ip_input.c
===
--- sys/netinet/ip_input.c	(revision 255362)
+++ sys/netinet/ip_input.c	(working copy)
@@ -363,7 +363,12 @@ ip_init(void)
 void
 ip_destroy(void)
 {
+	int i;
 
+	if ((i = pfil_head_unregister(&V_inet_pfil_hook)) != 0)
+		printf("%s: WARNING: unable to unregister pfil hook, "
+		"error %d\n", __func__, i);
+
 	/* Cleanup in_ifaddr hash table; should be empty. */
 	hashdestroy(V_in_ifaddrhashtbl, M_IFADDR, V_in_ifaddrhmask);
 
Index: sys/netinet6/ip6_input.c
===
--- sys/netinet6/ip6_input.c	(revision 255362)
+++ sys/netinet6/ip6_input.c	(working copy)
@@ -307,7 +307,11 @@ ip6proto_unregister(short ip6proto)
 void
 ip6_destroy()
 {
+	int i;
 
+	if ((i = pfil_head_unregister(&V_inet6_pfil_hook)) != 0)
+		printf("%s: WARNING: unable to unregister pfil hook, "
+		"error %d\n", __func__, i);
 	hashdestroy(V_in6_ifaddrhashtbl, M_IFADDR, V_in6_ifaddrhmask);
 	nd6_destroy();
 	callout_drain(&V_in6_tmpaddrtimer_ch);
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

svn commit: r254773 - head/sys/net

2013-08-24 Thread Andre Oppermann 
Author: andre
Date: Sat Aug 24 11:17:25 2013
New Revision: 254773
URL: http://svnweb.freebsd.org/changeset/base/254773

Log:
  Resolve the confusion between the head_list and the hook list.
  
  The linked list of pfil hooks is changed to "chain" and this term
  is applied consistently.  The head_list remains with "list" term.
  
  Add KASSERT to vnet_pfil_uninit().
  
  Update and extend comments.
  
  Reviewed by:  eri (previous version)

Modified:
  head/sys/net/pfil.c
  head/sys/net/pfil.h

Modified: head/sys/net/pfil.c
==
--- head/sys/net/pfil.c Sat Aug 24 10:38:02 2013(r254772)
+++ head/sys/net/pfil.c Sat Aug 24 11:17:25 2013(r254773)
@@ -52,9 +52,9 @@ static struct mtx pfil_global_lock;
 MTX_SYSINIT(pfil_heads_lock, &pfil_global_lock, "pfil_head_list lock",
   MTX_DEF);
 
-static struct packet_filter_hook *pfil_hook_get(int, struct pfil_head *);
-static int pfil_list_add(pfil_list_t *, struct packet_filter_hook *, int);
-static int pfil_list_remove(pfil_list_t *, pfil_func_t, void *);
+static struct packet_filter_hook *pfil_chain_get(int, struct pfil_head *);
+static int pfil_chain_add(pfil_chain_t *, struct packet_filter_hook *, int);
+static int pfil_chain_remove(pfil_chain_t *, pfil_func_t, void *);
 
 LIST_HEAD(pfilheadhead, pfil_head);
 VNET_DEFINE(struct pfilheadhead, pfil_head_list);
@@ -63,7 +63,7 @@ VNET_DEFINE(struct rmlock, pfil_lock);
 #defineV_pfil_lock VNET(pfil_lock)
 
 /*
- * pfil_run_hooks() runs the specified packet filter hooks.
+ * pfil_run_hooks() runs the specified packet filter hook chain.
  */
 int
 pfil_run_hooks(struct pfil_head *ph, struct mbuf **mp, struct ifnet *ifp,
@@ -76,8 +76,8 @@ pfil_run_hooks(struct pfil_head *ph, str
 
PFIL_RLOCK(ph, &rmpt);
KASSERT(ph->ph_nhooks >= 0, ("Pfil hook count dropped < 0"));
-   for (pfh = pfil_hook_get(dir, ph); pfh != NULL;
-pfh = TAILQ_NEXT(pfh, pfil_link)) {
+   for (pfh = pfil_chain_get(dir, ph); pfh != NULL;
+pfh = TAILQ_NEXT(pfh, pfil_chain)) {
if (pfh->pfil_func != NULL) {
rv = (*pfh->pfil_func)(pfh->pfil_arg, &m, ifp, dir,
inp);
@@ -91,7 +91,7 @@ pfil_run_hooks(struct pfil_head *ph, str
 }
 
 static struct packet_filter_hook *
-pfil_hook_get(int dir, struct pfil_head *ph)
+pfil_chain_get(int dir, struct pfil_head *ph)
 {
 
if (dir == PFIL_IN)
@@ -163,6 +163,7 @@ pfil_wowned(struct pfil_head *ph)
 
return (PFIL_WOWNED(ph));
 }
+
 /*
  * pfil_head_register() registers a pfil_head with the packet filter hook
  * mechanism.
@@ -202,9 +203,9 @@ pfil_head_unregister(struct pfil_head *p
PFIL_LIST_LOCK();
LIST_REMOVE(ph, ph_list);
PFIL_LIST_UNLOCK();
-   TAILQ_FOREACH_SAFE(pfh, &ph->ph_in, pfil_link, pfnext)
+   TAILQ_FOREACH_SAFE(pfh, &ph->ph_in, pfil_chain, pfnext)
free(pfh, M_IFADDR);
-   TAILQ_FOREACH_SAFE(pfh, &ph->ph_out, pfil_link, pfnext)
+   TAILQ_FOREACH_SAFE(pfh, &ph->ph_out, pfil_chain, pfnext)
free(pfh, M_IFADDR);
PFIL_LOCK_DESTROY(ph);
return (0);
@@ -261,7 +262,7 @@ pfil_add_hook(pfil_func_t func, void *ar
if (flags & PFIL_IN) {
pfh1->pfil_func = func;
pfh1->pfil_arg = arg;
-   err = pfil_list_add(&ph->ph_in, pfh1, flags & ~PFIL_OUT);
+   err = pfil_chain_add(&ph->ph_in, pfh1, flags & ~PFIL_OUT);
if (err)
goto locked_error;
ph->ph_nhooks++;
@@ -269,10 +270,10 @@ pfil_add_hook(pfil_func_t func, void *ar
if (flags & PFIL_OUT) {
pfh2->pfil_func = func;
pfh2->pfil_arg = arg;
-   err = pfil_list_add(&ph->ph_out, pfh2, flags & ~PFIL_IN);
+   err = pfil_chain_add(&ph->ph_out, pfh2, flags & ~PFIL_IN);
if (err) {
if (flags & PFIL_IN)
-   pfil_list_remove(&ph->ph_in, func, arg);
+   pfil_chain_remove(&ph->ph_in, func, arg);
goto locked_error;
}
ph->ph_nhooks++;
@@ -291,7 +292,7 @@ error:
 
 /*
  * pfil_remove_hook removes a specific function from the packet filter hook
- * list.
+ * chain.
  */
 int
 pfil_remove_hook(pfil_func_t func, void *arg, int flags, struct pfil_head *ph)
@@ -300,12 +301,12 @@ pfil_remove_hook(pfil_func_t func, void 
 
PFIL_WLOCK(ph);
if (flags & PFIL_IN) {
-   err = pfil_list_remove(&ph->ph_in, func, arg);
+   err = pfil_chain_remove(&ph->ph_in, func, arg);
if (err == 0)
ph->ph_nhooks--;
}
if ((err == 0) && (flags & PFIL_OUT)) {
-   err = pfil_list_remove(&ph->ph_out, func, arg);
+   err = pfil_chain_remove(&ph->ph_out, func, arg);