From dc7152bbcaa6dbc36fd32dc090c3cdb93210399e Mon Sep 17 00:00:00 2001
From: visa <visa@openbsd.org>
Date: Mon, 7 Mar 2022 13:02:53 +0000
Subject: [PATCH] Prevent deadlock in cad_down()

Introduce an rwlock to serialize cad(4) ioctl operations so that
cad_down() can release NET_LOCK() temporarily when invoking barriers.
This releasing prevents a possible deadlock with the taskq barrier.
The deadlock was pointed out by witness(4).

In addition, release NET_LOCK() when allocating memory in cad_up()
to reduce the risk of ill effects.
---
 sys/dev/fdt/if_cad.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/sys/dev/fdt/if_cad.c b/sys/dev/fdt/if_cad.c
index 250c18f56d9..146514318d7 100644
--- a/sys/dev/fdt/if_cad.c
+++ b/sys/dev/fdt/if_cad.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_cad.c,v 1.9 2022/01/27 17:34:51 visa Exp $	*/
+/*	$OpenBSD: if_cad.c,v 1.10 2022/03/07 13:02:53 visa Exp $	*/
 
 /*
  * Copyright (c) 2021-2022 Visa Hankala
@@ -30,6 +30,7 @@
 #include <sys/ioctl.h>
 #include <sys/mutex.h>
 #include <sys/kstat.h>
+#include <sys/rwlock.h>
 #include <sys/task.h>
 #include <sys/timeout.h>
 
@@ -297,6 +298,7 @@ struct cad_softc {
 	unsigned int		sc_rx_cons;
 	uint32_t		sc_netctl;
 
+	struct rwlock		sc_cfg_lock;
 	struct task		sc_statchg_task;
 	uint32_t		sc_tx_freq;
 
@@ -467,10 +469,13 @@ cad_attach(struct device *parent, struct device *self, void *aux)
 	if (OF_is_compatible(faa->fa_node, "cdns,zynq-gem"))
 		sc->sc_rxhang_erratum = 1;
 
+	rw_init(&sc->sc_cfg_lock, "cadcfg");
 	timeout_set(&sc->sc_tick, cad_tick, sc);
 	task_set(&sc->sc_statchg_task, cad_statchg_task, sc);
 
+	rw_enter_write(&sc->sc_cfg_lock);
 	cad_reset(sc);
+	rw_exit_write(&sc->sc_cfg_lock);
 
 	sc->sc_ih = fdt_intr_establish(faa->fa_node, IPL_NET | IPL_MPSAFE,
 	    cad_intr, sc, sc->sc_dev.dv_xname);
@@ -548,6 +553,9 @@ cad_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 	int error = 0;
 	int s;
 
+	NET_UNLOCK();
+	rw_enter_write(&sc->sc_cfg_lock);
+	NET_LOCK();
 	s = splnet();
 
 	switch (cmd) {
@@ -585,6 +593,7 @@ cad_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 	}
 
 	splx(s);
+	rw_exit_write(&sc->sc_cfg_lock);
 
 	return error;
 }
@@ -598,6 +607,8 @@ cad_reset(struct cad_softc *sc)
 	unsigned int freq, i;
 	uint32_t div, netcfg;
 
+	rw_assert_wrlock(&sc->sc_cfg_lock);
+
 	HWRITE4(sc, GEM_NETCTL, 0);
 	HWRITE4(sc, GEM_IDR, ~0U);
 	HWRITE4(sc, GEM_RXSR, 0);
@@ -655,6 +666,11 @@ cad_up(struct cad_softc *sc)
 	unsigned int i, nrxd, ntxd;
 	uint32_t val;
 
+	rw_assert_wrlock(&sc->sc_cfg_lock);
+
+	/* Release lock for memory allocation. */
+	NET_UNLOCK();
+
 	if (sc->sc_dma64)
 		flags |= BUS_DMA_64BIT;
 
@@ -810,6 +826,8 @@ cad_up(struct cad_softc *sc)
 		}
 	}
 
+	NET_LOCK();
+
 	/*
 	 * Set MAC address filters.
 	 */
@@ -905,11 +923,16 @@ cad_down(struct cad_softc *sc)
 	struct cad_buf *rxb, *txb;
 	unsigned int i, timeout;
 
+	rw_assert_wrlock(&sc->sc_cfg_lock);
+
 	ifp->if_flags &= ~IFF_RUNNING;
 
 	ifq_clr_oactive(&ifp->if_snd);
 	ifp->if_timer = 0;
 
+	/* Avoid lock order issues with barriers. */
+	NET_UNLOCK();
+
 	timeout_del_barrier(&sc->sc_tick);
 
 	/* Disable data transfer. */
@@ -981,6 +1004,8 @@ cad_down(struct cad_softc *sc)
 	cad_dmamem_free(sc, sc->sc_rxring);
 	sc->sc_rxring = NULL;
 	sc->sc_rxdesc = NULL;
+
+	NET_LOCK();
 }
 
 uint8_t
@@ -1011,6 +1036,8 @@ cad_iff(struct cad_softc *sc)
 	uint64_t hash;
 	uint32_t netcfg;
 
+	rw_assert_wrlock(&sc->sc_cfg_lock);
+
 	netcfg = HREAD4(sc, GEM_NETCFG);
 	netcfg &= ~GEM_NETCFG_UCASTHASHEN;
 
-- 
2.20.1