NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: kern/44418: FAST_IPSEC and if_wm kernel panic - may affect the whole network stack
The following reply was made to PR kern/44418; it has been noted by GNATS.
From: Wolfgang Stukenbrock <Wolfgang.Stukenbrock%nagler-company.com@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: kern-bug-people%NetBSD.org@localhost, gnats-admin%NetBSD.org@localhost,
netbsd-bugs%NetBSD.org@localhost
Subject: Re: kern/44418: FAST_IPSEC and if_wm kernel panic - may affect the
whole network stack
Date: Fri, 21 Jan 2011 14:40:30 +0100
Hi once again ...
sorry but according to my latest test I need to correct the last patch
aproch --- sorry
I've found my problen inside of the wm driver now.
The main problem is, that the network code is not able to handle
processing of more than one CPU at a time.
The introduction of the softnet_lock mutex does not realy help a lot
here ....
There is a very big problem with the device drivers!!!!
The network code does the following:
1. lock softnet_lock
2. lock KERNEL_LOCK
3. increase spl-level if required
The driver interrupt does the following:
1. enter a higher SPL-level
2. lock KERNEL_LOCK
3. call driver interrupt-routine
This is bad, because we now cannot lock the softnet_lock anymore - order
problem.
Next problem here is, that I've got the following calling sequence (in DDB):
db{0}> trace
breakpoint() at netbsd:breakpoint+0x5
panic() at netbsd:panic+0x24d
lockdebug_abort() at netbsd:lockdebug_abort+0x42
mutex_vector_enter() at netbsd:mutex_vector_enter+0x229
wm_intr() at netbsd:wm_intr+0x6ba
intr_biglock_wrapper() at netbsd:intr_biglock_wrapper+0x1d
Xintr_ioapic_level5() at netbsd:Xintr_ioapic_level5+0xf7
--- interrupt ---
Xspllower() at netbsd:Xspllower+0xe
crypto_getreq() at netbsd:crypto_getreq+0x25
esp_input() at netbsd:esp_input+0x2cf
ipsec_common_input() at netbsd:ipsec_common_input+0x643
ipsec4_common_input() at netbsd:ipsec4_common_input+0xc3
ip_input() at netbsd:ip_input+0x4a3
ipintr() at netbsd:ipintr+0x6a
softint_dispatch() at netbsd:softint_dispatch+0xc3
DDB lost frame for netbsd:Xsoftintr+0x50, trying 0xffff80005382bd70
Xsoftintr() at netbsd:Xsoftintr+0x50
--- interrupt ---
0:
db{0}>
It is possible, that we handle driver interrupts while we are processing
network-packets! And we have locked the softnet_lock in ipintr() and it
is still locked.
(Output comes from one of my aproches to avoid concurrent access of
ipsec-callback and the driver interrupts for the if_snd queue).
The current implementation of the mutex is a little bit problematic from
my point of view, because it cannot be used to synchronise critical
sections with interrupts - at least not if we are holding the lock while
we are processing pending interrupts ...
So currently the only way to avoid concurrent access in any
network/driver-data is to hold the global KERNEL_LOCK.
Not very smart! This is a performance issue ...
I've modified my previous patch, so that the KERNEL_LOCK is locked too
prior reinjection the packet into the network code.
I'm still not 100% shure if no changes to the interfaces drivers are
required.
I'm currently useing only the wm driver and there I've located the
following problem in wm_tick():
It updates some event-counters - OK should be safe as long as the
register access on the HW is MP-safe. Is it ??? I'm not shure ...
But it also calls wm_tbi_check_link() for some chips and that one
may call wm_start() and that accesses if_snd.
Now I'm not shure if the KERNEL_LOCK is hold while processing wm_tick().
A short look at it leads me to the oppinion that it is not!
And if not, there is unsynchronised MP-access to the queue !!!!!
For me, I've added KERNEL_LOCK's to my sources, but I won't issue a PR
for that, because I'm simply not shure if it is realy required.
(same for wm_82547_txfifo_stall() and wm_watchdog())
I think the maintainer of the wm-driver should think a little big about
it and if the synchronisation is not OK, then all drivers should have a
review.
Here the corrected patch for the xfrom_xxx.c files of the FAST_IPSEC
implementation:
--- xform_ah.c 2011/01/20 12:10:56 1.1
+++ xform_ah.c 2011/01/21 13:03:35
@@ -54,6 +54,8 @@
#include <sys/kernel.h>
#include <sys/sysctl.h>
+#include <sys/socketvar.h>
+
#include <net/if.h>
#include <netinet/in.h>
@@ -828,6 +830,8 @@
}
#endif
+ mutex_enter(softnet_lock);
+ KERNEL_LOCK(1,NULL);
s = splsoftnet();
sav = KEY_ALLOCSA(&tc->tc_dst, tc->tc_proto, tc->tc_spi, sport,
dport);
@@ -851,8 +855,12 @@
if (sav->tdb_cryptoid != 0)
sav->tdb_cryptoid = crp->crp_sid;
- if (crp->crp_etype == EAGAIN)
+ if (crp->crp_etype == EAGAIN) {
+ splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
return crypto_dispatch(crp);
+ }
AH_STATINC(AH_STAT_NOXFORM);
DPRINTF(("ah_input_cb: crypto error %d\n",
crp->crp_etype));
@@ -960,11 +968,15 @@
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
return error;
bad:
if (sav)
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
if (m != NULL)
m_freem(m);
if (tc != NULL)
@@ -1228,6 +1240,8 @@
ptr = (tc + 1);
m = (struct mbuf *) crp->crp_buf;
+ mutex_enter(softnet_lock);
+ KERNEL_LOCK(1,NULL);
s = splsoftnet();
isr = tc->tc_isr;
@@ -1248,6 +1262,8 @@
if (crp->crp_etype == EAGAIN) {
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
return crypto_dispatch(crp);
}
@@ -1294,11 +1310,15 @@
err = ipsec_process_done(m, isr);
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
return err;
bad:
if (sav)
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
if (m)
m_freem(m);
free(tc, M_XDATA);
diff -u -r1.1 xform_esp.c
--- xform_esp.c 2011/01/20 12:14:23 1.1
+++ xform_esp.c 2011/01/21 13:04:27
@@ -55,6 +55,8 @@
/*#include <sys/random.h>*/
#include <sys/sysctl.h>
+#include <sys/socketvar.h>
+
#include <net/if.h>
#include <netinet/in.h>
@@ -497,6 +499,8 @@
}
#endif
+ mutex_enter(softnet_lock);
+ KERNEL_LOCK(1,NULL);
s = splsoftnet();
sav = KEY_ALLOCSA(&tc->tc_dst, tc->tc_proto, tc->tc_spi, sport,
dport);
@@ -527,6 +531,8 @@
if (crp->crp_etype == EAGAIN) {
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
return crypto_dispatch(crp);
}
@@ -665,11 +671,15 @@
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
return error;
bad:
if (sav)
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
if (m != NULL)
m_freem(m);
if (tc != NULL)
@@ -934,6 +944,8 @@
IPSEC_ASSERT(tc != NULL, ("esp_output_cb: null opaque data
area!"));
m = (struct mbuf *) crp->crp_buf;
+ mutex_enter(softnet_lock);
+ KERNEL_LOCK(1,NULL);
s = splsoftnet();
isr = tc->tc_isr;
@@ -958,6 +970,8 @@
if (crp->crp_etype == EAGAIN) {
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
return crypto_dispatch(crp);
}
@@ -1004,11 +1018,15 @@
err = ipsec_process_done(m, isr);
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
return err;
bad:
if (sav)
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
if (m)
m_freem(m);
free(tc, M_XDATA);
--- xform_ipcomp.c 2010/05/04 13:25:55 1.1
+++ xform_ipcomp.c 2011/01/21 13:05:37
@@ -46,6 +46,8 @@
#include <sys/protosw.h>
#include <sys/sysctl.h>
+#include <sys/socketvar.h>
+
#include <netinet/in.h>
#include <netinet/in_systm.h>
#include <netinet/ip.h>
@@ -257,6 +259,8 @@
}
#endif
+ mutex_enter(softnet_lock);
+ KERNEL_LOCK(1,NULL);
s = splsoftnet();
sav = KEY_ALLOCSA(&tc->tc_dst, tc->tc_proto, tc->tc_spi, sport,
dport);
@@ -282,6 +286,8 @@
if (crp->crp_etype == EAGAIN) {
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
return crypto_dispatch(crp);
}
@@ -299,6 +305,9 @@
}
IPCOMP_STATINC(IPCOMP_STAT_HIST + sav->alg_comp);
+ /* Update the counters */
+ IPCOMP_STATADD(IPCOMP_STAT_IBYTES, m->m_pkthdr.len - skip);
+
clen = crp->crp_olen; /* Length of data after
processing */
/* Release the crypto descriptors */
@@ -336,11 +345,15 @@
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
return error;
bad:
if (sav)
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
if (m)
m_freem(m);
if (tc != NULL)
@@ -509,6 +522,8 @@
skip = tc->tc_skip;
rlen = crp->crp_ilen - skip;
+ mutex_enter(softnet_lock);
+ KERNEL_LOCK(1,NULL);
s = splsoftnet();
isr = tc->tc_isr;
@@ -530,6 +545,8 @@
if (crp->crp_etype == EAGAIN) {
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
return crypto_dispatch(crp);
}
IPCOMP_STATINC(IPCOMP_STAT_NOXFORM);
@@ -625,11 +642,15 @@
error = ipsec_process_done(m, isr);
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
return error;
bad:
if (sav)
KEY_FREESAV(&sav);
splx(s);
+ KERNEL_UNLOCK_ONE(NULL);
+ mutex_exit(softnet_lock);
if (m)
m_freem(m);
free(tc, M_XDATA);
Home |
Main Index |
Thread Index |
Old Index