On 20/12/16(Tue) 14:31, Alexander Bluhm wrote:
> On Tue, Dec 20, 2016 at 12:08:57PM +0100, Martin Pieuchot wrote:
> > I introduced a bug in carp(4), one code path tries to take the NET_LOCK()
> > twice:
> >
> > panic() at panic+0xfe
> > rw_enter() at rw_enter+0x1f8
> > carp_send_ad() at carp_send_ad+0x4d
> > carp_vhe_send_ad_all() at carp_vhe_send_ad_all+0x4b
> > carp_ioctl() at carp_ioctl+0x491
> > ifioctl() at ifioctl+0x7be
> > soo_ioctl() at soo_ioctl+0x260
> > sys_ioctl() at sys_ioctl+0x196
> > syscall() at syscall+0x27b
> >
> > Diff below fixes that by respecting the rule of always taking the
> > NET_LOCK() at the beginning of every timer.
> >
> > While here remove two recursive splsoftnet() dances.
> >
> > ok?
>
> What about
>
> carp_proto_input
> carp_proto_input_if
> carp_proto_input_c
> carp_master_down
> NET_LOCK
Good catch.
> May we take the NET_LOCK in the ip input path?
No we can't because we're already holding it. So carp_master_down()
must be split in two, diff below does that and only grab the lock in
the timer routine.
Index: netinet/ip_carp.c
===================================================================
RCS file: /cvs/src/sys/netinet/ip_carp.c,v
retrieving revision 1.297
diff -u -p -r1.297 ip_carp.c
--- netinet/ip_carp.c 19 Dec 2016 08:36:49 -0000 1.297
+++ netinet/ip_carp.c 20 Dec 2016 14:04:55 -0000
@@ -221,9 +221,11 @@ int carp_prepare_ad(struct mbuf *, struc
struct carp_header *);
void carp_send_ad_all(void);
void carp_vhe_send_ad_all(struct carp_softc *);
-void carp_send_ad(void *);
+void carp_timer_ad(void *);
+void carp_send_ad(struct carp_vhost_entry *);
void carp_send_arp(struct carp_softc *);
-void carp_master_down(void *);
+void carp_timer_down(void *);
+void carp_master_down(struct carp_vhost_entry *);
int carp_ioctl(struct ifnet *, u_long, caddr_t);
int carp_vhids_ioctl(struct carp_softc *, struct carpreq *);
int carp_check_dup_vhids(struct carp_softc *, struct carp_if *,
@@ -831,9 +833,9 @@ carp_new_vhost(struct carp_softc *sc, in
vhe->vhid = vhid;
vhe->advskew = advskew;
vhe->state = INIT;
- timeout_set_proc(&vhe->ad_tmo, carp_send_ad, vhe);
- timeout_set_proc(&vhe->md_tmo, carp_master_down, vhe);
- timeout_set_proc(&vhe->md6_tmo, carp_master_down, vhe);
+ timeout_set_proc(&vhe->ad_tmo, carp_timer_ad, vhe);
+ timeout_set_proc(&vhe->md_tmo, carp_timer_down, vhe);
+ timeout_set_proc(&vhe->md6_tmo, carp_timer_down, vhe);
KERNEL_ASSERT_LOCKED(); /* touching carp_vhosts */
@@ -1027,26 +1029,34 @@ carp_vhe_send_ad_all(struct carp_softc *
}
void
-carp_send_ad(void *v)
+carp_timer_ad(void *v)
+{
+ int s;
+
+ NET_LOCK(s);
+ carp_send_ad(v);
+ NET_UNLOCK(s);
+}
+
+void
+carp_send_ad(struct carp_vhost_entry *vhe)
{
struct carp_header ch;
struct timeval tv;
- struct carp_vhost_entry *vhe = v;
struct carp_softc *sc = vhe->parent_sc;
struct carp_header *ch_ptr;
-
struct mbuf *m;
- int error, len, advbase, advskew, s;
+ int error, len, advbase, advskew;
struct ifaddr *ifa;
struct sockaddr sa;
+ NET_ASSERT_LOCKED();
+
if (sc->sc_carpdev == NULL) {
sc->sc_if.if_oerrors++;
return;
}
- NET_LOCK(s);
-
/* bow out if we've gone to backup (the carp interface is going down) */
if (sc->sc_bow_out) {
advbase = 255;
@@ -1246,7 +1256,6 @@ carp_send_ad(void *v)
retry_later:
sc->cur_vhe = NULL;
- NET_UNLOCK(s);
if (advbase != 255 || advskew != 255)
timeout_add(&vhe->ad_tmo, tvtohz(&tv));
}
@@ -1261,7 +1270,6 @@ carp_send_arp(struct carp_softc *sc)
{
struct ifaddr *ifa;
in_addr_t in;
- int s = splsoftnet();
TAILQ_FOREACH(ifa, &sc->sc_if.if_addrlist, ifa_list) {
@@ -1272,7 +1280,6 @@ carp_send_arp(struct carp_softc *sc)
arprequest(&sc->sc_if, &in, &in, sc->sc_ac.ac_enaddr);
DELAY(1000); /* XXX */
}
- splx(s);
}
#ifdef INET6
@@ -1282,7 +1289,6 @@ carp_send_na(struct carp_softc *sc)
struct ifaddr *ifa;
struct in6_addr *in6;
static struct in6_addr mcast = IN6ADDR_LINKLOCAL_ALLNODES_INIT;
- int s = splsoftnet();
TAILQ_FOREACH(ifa, &sc->sc_if.if_addrlist, ifa_list) {
@@ -1295,7 +1301,6 @@ carp_send_na(struct carp_softc *sc)
(ip6_forwarding ? ND_NA_FLAG_ROUTER : 0), 1, NULL);
DELAY(1000); /* XXX */
}
- splx(s);
}
#endif /* INET6 */
@@ -1504,10 +1509,21 @@ done:
}
void
-carp_master_down(void *v)
+carp_timer_down(void *v)
+{
+ int s;
+
+ NET_LOCK(s);
+ carp_master_down(v);
+ NET_UNLOCK(s);
+}
+
+void
+carp_master_down(struct carp_vhost_entry *vhe)
{
- struct carp_vhost_entry *vhe = v;
struct carp_softc *sc = vhe->parent_sc;
+
+ NET_ASSERT_LOCKED();
switch (vhe->state) {
case INIT:
