Re: carp(4) fix vs NET_LOCK
On Tue, Dec 20, 2016 at 03:09:11PM +0100, Martin Pieuchot wrote:
> 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.
OK bluhm@
>
> 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 - 1.297
> +++ netinet/ip_carp.c 20 Dec 2016 14:04:55 -
> @@ -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(>ad_tmo, carp_send_ad, vhe);
> - timeout_set_proc(>md_tmo, carp_master_down, vhe);
> - timeout_set_proc(>md6_tmo, carp_master_down, vhe);
> + timeout_set_proc(>ad_tmo, carp_timer_ad, vhe);
> + timeout_set_proc(>md_tmo, carp_timer_down, vhe);
> + timeout_set_proc(>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(>ad_tmo, tvtohz());
> }
> @@ -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_if.if_addrlist, ifa_list) {
>
> @@ -1272,7 +1280,6 @@ carp_send_arp(struct carp_softc *sc)
> arprequest(>sc_if, , , 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_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:
Re: carp(4) fix vs NET_LOCK
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 - 1.297
+++ netinet/ip_carp.c 20 Dec 2016 14:04:55 -
@@ -221,9 +221,11 @@ intcarp_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 *);
intcarp_ioctl(struct ifnet *, u_long, caddr_t);
intcarp_vhids_ioctl(struct carp_softc *, struct carpreq *);
intcarp_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(>ad_tmo, carp_send_ad, vhe);
- timeout_set_proc(>md_tmo, carp_master_down, vhe);
- timeout_set_proc(>md6_tmo, carp_master_down, vhe);
+ timeout_set_proc(>ad_tmo, carp_timer_ad, vhe);
+ timeout_set_proc(>md_tmo, carp_timer_down, vhe);
+ timeout_set_proc(>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(>ad_tmo, tvtohz());
}
@@ -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_if.if_addrlist, ifa_list) {
@@ -1272,7 +1280,6 @@ carp_send_arp(struct carp_softc *sc)
arprequest(>sc_if, , , 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_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 =
Re: carp(4) fix vs NET_LOCK
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
May we take the NET_LOCK in the ip input path?
bluhm
>
> 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 - 1.297
> +++ netinet/ip_carp.c 20 Dec 2016 11:03:22 -
> @@ -221,7 +221,8 @@ 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 *);
> int carp_ioctl(struct ifnet *, u_long, caddr_t);
> @@ -831,7 +832,7 @@ carp_new_vhost(struct carp_softc *sc, in
> vhe->vhid = vhid;
> vhe->advskew = advskew;
> vhe->state = INIT;
> - timeout_set_proc(>ad_tmo, carp_send_ad, vhe);
> + timeout_set_proc(>ad_tmo, carp_timer_ad, vhe);
> timeout_set_proc(>md_tmo, carp_master_down, vhe);
> timeout_set_proc(>md6_tmo, carp_master_down, vhe);
>
> @@ -1027,26 +1028,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 +1255,6 @@ carp_send_ad(void *v)
>
> retry_later:
> sc->cur_vhe = NULL;
> - NET_UNLOCK(s);
> if (advbase != 255 || advskew != 255)
> timeout_add(>ad_tmo, tvtohz());
> }
> @@ -1261,7 +1269,6 @@ carp_send_arp(struct carp_softc *sc)
> {
> struct ifaddr *ifa;
> in_addr_t in;
> - int s = splsoftnet();
>
> TAILQ_FOREACH(ifa, >sc_if.if_addrlist, ifa_list) {
>
> @@ -1272,7 +1279,6 @@ carp_send_arp(struct carp_softc *sc)
> arprequest(>sc_if, , , sc->sc_ac.ac_enaddr);
> DELAY(1000);/* XXX */
> }
> - splx(s);
> }
>
> #ifdef INET6
> @@ -1282,7 +1288,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_if.if_addrlist, ifa_list) {
>
> @@ -1295,7 +1300,6 @@ carp_send_na(struct carp_softc *sc)
> (ip6_forwarding ? ND_NA_FLAG_ROUTER : 0), 1, NULL);
> DELAY(1000);/* XXX */
> }
> - splx(s);
> }
> #endif /* INET6 */
>
> @@ -1508,7 +1512,9 @@ carp_master_down(void *v)
> {
> struct carp_vhost_entry *vhe = v;
> struct carp_softc *sc = vhe->parent_sc;
> + int s;
>
> + NET_LOCK(s);
> switch (vhe->state) {
> case INIT:
> printf("%s: master_down event in INIT state\n",
> @@ -1531,6 +1537,7 @@ carp_master_down(void *v)
> carpstats.carps_preempt++;
> break;
> }
> + NET_UNLOCK(s);
> }
>
> void
carp(4) fix vs NET_LOCK
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?
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 - 1.297
+++ netinet/ip_carp.c 20 Dec 2016 11:03:22 -
@@ -221,7 +221,8 @@ 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 *);
intcarp_ioctl(struct ifnet *, u_long, caddr_t);
@@ -831,7 +832,7 @@ carp_new_vhost(struct carp_softc *sc, in
vhe->vhid = vhid;
vhe->advskew = advskew;
vhe->state = INIT;
- timeout_set_proc(>ad_tmo, carp_send_ad, vhe);
+ timeout_set_proc(>ad_tmo, carp_timer_ad, vhe);
timeout_set_proc(>md_tmo, carp_master_down, vhe);
timeout_set_proc(>md6_tmo, carp_master_down, vhe);
@@ -1027,26 +1028,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 +1255,6 @@ carp_send_ad(void *v)
retry_later:
sc->cur_vhe = NULL;
- NET_UNLOCK(s);
if (advbase != 255 || advskew != 255)
timeout_add(>ad_tmo, tvtohz());
}
@@ -1261,7 +1269,6 @@ carp_send_arp(struct carp_softc *sc)
{
struct ifaddr *ifa;
in_addr_t in;
- int s = splsoftnet();
TAILQ_FOREACH(ifa, >sc_if.if_addrlist, ifa_list) {
@@ -1272,7 +1279,6 @@ carp_send_arp(struct carp_softc *sc)
arprequest(>sc_if, , , sc->sc_ac.ac_enaddr);
DELAY(1000);/* XXX */
}
- splx(s);
}
#ifdef INET6
@@ -1282,7 +1288,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_if.if_addrlist, ifa_list) {
@@ -1295,7 +1300,6 @@ carp_send_na(struct carp_softc *sc)
(ip6_forwarding ? ND_NA_FLAG_ROUTER : 0), 1, NULL);
DELAY(1000);/* XXX */
}
- splx(s);
}
#endif /* INET6 */
@@ -1508,7 +1512,9 @@ carp_master_down(void *v)
{
struct carp_vhost_entry *vhe = v;
struct carp_softc *sc = vhe->parent_sc;
+ int s;
+ NET_LOCK(s);
switch (vhe->state) {
case INIT:
printf("%s: master_down event in INIT state\n",
@@ -1531,6 +1537,7 @@ carp_master_down(void *v)
carpstats.carps_preempt++;
break;
}
+ NET_UNLOCK(s);
}
void
