Hello,
On Tue, Jul 06, 2021 at 11:05:47PM +0200, Alexander Bluhm wrote:
> Hi,
>
> Thank a lot to Hrvoje Popovski for testing my diff and to sashan@
> and dlg@ for fixing all the fallout in pf and pseudo drivers.
>
> Are there any bugs left? I think everything has been fixed.
>
there is one more change we need to commit. Currently brdiges
assume call to pf_test() won't sleep. It's been discussed here [1].
we still need to agree on whether pf_test() can sleep (give up CPU),
when processing packet. I don't mind if pf_test() gives up CPU (sleeps),
when waiting for other packets to finish their business in pf(4).
diff below makes sure all bridges call pf_test() _outside_ of smr_read()
section. Hrvoje successfully tested the diff with tpmr/veb bridges.
this should be the only stopper I'm aware of.
thanks and
regards
sashan
[1] https://marc.info/?l=openbsd-tech&m=162291154406183&w=2
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/if_bridge.c b/sys/net/if_bridge.c
index 703be01648f..c41f3c93668 100644
--- a/sys/net/if_bridge.c
+++ b/sys/net/if_bridge.c
@@ -138,6 +138,8 @@ int bridge_ipsec(struct ifnet *, struct ether_header *,
int, struct llc *,
#endif
int bridge_clone_create(struct if_clone *, int);
int bridge_clone_destroy(struct ifnet *);
+void bridge_take(void *);
+void bridge_rele(void *);
#define ETHERADDR_IS_IP_MCAST(a) \
/* struct etheraddr *a; */ \
@@ -152,6 +154,8 @@ struct if_clone bridge_cloner =
const struct ether_brport bridge_brport = {
bridge_input,
+ bridge_take,
+ bridge_rele,
NULL,
};
@@ -1986,3 +1990,15 @@ bridge_send_icmp_err(struct ifnet *ifp,
dropit:
m_freem(n);
}
+
+void
+bridge_take(void *unused)
+{
+ return;
+}
+
+void
+bridge_rele(void *unused)
+{
+ return;
+}
diff --git a/sys/net/if_ethersubr.c b/sys/net/if_ethersubr.c
index c9493b7f857..4b975e2d4a1 100644
--- a/sys/net/if_ethersubr.c
+++ b/sys/net/if_ethersubr.c
@@ -426,14 +426,16 @@ ether_input(struct ifnet *ifp, struct mbuf *m)
smr_read_enter();
eb = SMR_PTR_GET(&ac->ac_brport);
+ if (eb != NULL)
+ eb->eb_port_take(eb->eb_port);
+ smr_read_leave();
if (eb != NULL) {
m = (*eb->eb_input)(ifp, m, dst, eb->eb_port);
+ eb->eb_port_rele(eb->eb_port);
if (m == NULL) {
- smr_read_leave();
return;
}
}
- smr_read_leave();
/*
* Fourth phase: drop service delimited packets.
diff --git a/sys/net/if_switch.c b/sys/net/if_switch.c
index 22aecdc6b75..ca24597d1f7 100644
--- a/sys/net/if_switch.c
+++ b/sys/net/if_switch.c
@@ -110,6 +110,8 @@ struct mbuf
void switch_flow_classifier_dump(struct switch_softc *,
struct switch_flow_classify *);
void switchattach(int);
+void switch_take(void *);
+void switch_rele(void *);
struct if_clone switch_cloner =
IF_CLONE_INITIALIZER("switch", switch_clone_create, switch_clone_destroy);
@@ -122,6 +124,8 @@ struct pool swfcl_pool;
const struct ether_brport switch_brport = {
switch_input,
+ switch_take,
+ switch_rele,
NULL,
};
@@ -1571,3 +1575,15 @@ ofp_split_mbuf(struct mbuf *m, struct mbuf **mtail)
return (0);
}
+
+void
+switch_take(void *unused)
+{
+ return;
+}
+
+void
+switch_rele(void *unused)
+{
+ return;
+}
diff --git a/sys/net/if_tpmr.c b/sys/net/if_tpmr.c
index f6eb99f347c..ddd56341e85 100644
--- a/sys/net/if_tpmr.c
+++ b/sys/net/if_tpmr.c
@@ -83,6 +83,8 @@ struct tpmr_port {
struct tpmr_softc *p_tpmr;
unsigned int p_slot;
+ unsigned int p_refcnt;
+
struct ether_brport p_brport;
};
@@ -125,6 +127,8 @@ static int tpmr_add_port(struct tpmr_softc *,
static int tpmr_del_port(struct tpmr_softc *,
const struct ifbreq *);
static int tpmr_port_list(struct tpmr_softc *, struct ifbifconf *);
+static void tpmr_p_take(void *);
+static void tpmr_p_rele(void *);
static struct if_clone tpmr_cloner =
IF_CLONE_INITIALIZER("tpmr", tpmr_clone_create, tpmr_clone_destroy);
@@ -375,16 +379,21 @@ tpmr_input(struct ifnet *ifp0, struct mbuf *m, uint64_t
dst, void *brport)
}
#endif
- SMR_ASSERT_CRITICAL(); /* ether_input calls us in a crit section */
+ smr_read_enter();
pn = SMR_PTR_GET(&sc->sc_ports[!p->p_slot]);
+ if (pn != NULL)
+ tpmr_p_take(pn);
+ smr_read_leave();
if (pn == NULL)
goto drop;
ifpn = pn->p_ifp0;
#if NPF > 0
if (!ISSET(iff, IFF_LINK1) &&
- (m = tpmr_pf(ifpn, PF_OUT, m)) == NULL)
+ (m = tpmr_pf(ifpn, PF_OUT, m)) == NULL) {
+ tpmr_p_rele(pn);
return (NULL);
+ }
#endif
if (if_enqueue(ifpn, m))
@@ -394,6 +403,8 @@ tpmr_input(struct ifnet *ifp0, struct mbuf *m, uint64_t
dst, void *brport)
ifc_opackets, ifc_obytes, len);
}
+ tpmr_p_rele(pn);
+
return (NULL);
drop:
@@ -532,6 +543,8 @@ tpmr_add_port(struct tpmr_softc *sc, const struct ifbreq
*req)
if_detachhook_add(ifp0, &p->p_dtask);
p->p_brport.eb_input = tpmr_input;
+ p->p_brport.eb_port_take = tpmr_p_take;
+ p->p_brport.eb_port_rele = tpmr_p_rele;
p->p_brport.eb_port = p;
/* commit */
@@ -547,6 +560,7 @@ tpmr_add_port(struct tpmr_softc *sc, const struct ifbreq
*req)
p->p_slot = i;
+ tpmr_p_take(p);
ether_brport_set(ifp0, &p->p_brport);
ifp0->if_ioctl = tpmr_p_ioctl;
ifp0->if_output = tpmr_p_output;
@@ -700,6 +714,26 @@ tpmr_p_output(struct ifnet *ifp0, struct mbuf *m, struct
sockaddr *dst,
return ((*p_output)(ifp0, m, dst, rt));
}
+static void
+tpmr_p_take(void *p)
+{
+ struct tpmr_port *port = p;
+
+ atomic_inc_int((int *)(&port->p_refcnt));
+}
+
+static void
+tpmr_p_rele(void *p)
+{
+ struct tpmr_port *port = p;
+ struct ifnet *ifp0 = port->p_ifp0;
+
+ if (atomic_dec_int_nv((int *)(&port->p_refcnt)) == 0) {
+ if_put(ifp0);
+ free(port, M_DEVBUF, sizeof(*port));
+ }
+}
+
static void
tpmr_p_dtor(struct tpmr_softc *sc, struct tpmr_port *p, const char *op)
{
@@ -725,10 +759,9 @@ tpmr_p_dtor(struct tpmr_softc *sc, struct tpmr_port *p,
const char *op)
if_detachhook_del(ifp0, &p->p_dtask);
if_linkstatehook_del(ifp0, &p->p_ltask);
- smr_barrier();
+ tpmr_p_rele(p);
- if_put(ifp0);
- free(p, M_DEVBUF, sizeof(*p));
+ smr_barrier();
if (ifp->if_link_state != LINK_STATE_DOWN) {
ifp->if_link_state = LINK_STATE_DOWN;
diff --git a/sys/net/if_veb.c b/sys/net/if_veb.c
index 71cc814fa54..cc8817d7be3 100644
--- a/sys/net/if_veb.c
+++ b/sys/net/if_veb.c
@@ -209,6 +209,9 @@ static void veb_eb_port_rele(void *, void *);
static size_t veb_eb_port_ifname(void *, char *, size_t, void *);
static void veb_eb_port_sa(void *, struct sockaddr_storage *, void *);
+static void veb_eb_brport_take(void *);
+static void veb_eb_brport_rele(void *);
+
static const struct etherbridge_ops veb_etherbridge_ops = {
veb_eb_port_cmp,
veb_eb_port_take,
@@ -1060,8 +1063,13 @@ veb_port_input(struct ifnet *ifp0, struct mbuf *m,
uint64_t dst, void *brport)
smr_read_enter();
tp = etherbridge_resolve(&sc->sc_eb, dst);
- m = veb_transmit(sc, p, tp, m, src, dst);
+ if (tp != NULL)
+ veb_eb_port_take(NULL, tp);
smr_read_leave();
+ if (tp != NULL) {
+ m = veb_transmit(sc, p, tp, m, src, dst);
+ veb_eb_port_rele(NULL, tp);
+ }
if (m == NULL)
return (NULL);
@@ -1291,6 +1299,9 @@ veb_add_port(struct veb_softc *sc, const struct ifbreq
*req, unsigned int span)
p->p_brport.eb_input = veb_port_input;
}
+ p->p_brport.eb_port_take = veb_eb_brport_take;
+ p->p_brport.eb_port_rele = veb_eb_brport_rele;
+
/* this might have changed if we slept for malloc or ifpromisc */
error = ether_brport_isset(ifp0);
if (error != 0)
@@ -1910,8 +1921,8 @@ veb_p_dtor(struct veb_softc *sc, struct veb_port *p,
const char *op)
SMR_TAILQ_REMOVE_LOCKED(&port_list->l_list, p, p_entry);
port_list->l_count--;
- smr_barrier();
refcnt_finalize(&p->p_refs, "vebpdtor");
+ smr_barrier();
veb_rule_list_free(TAILQ_FIRST(&p->p_vrl));
@@ -2016,6 +2027,18 @@ veb_eb_port_rele(void *arg, void *port)
refcnt_rele_wake(&p->p_refs);
}
+static void
+veb_eb_brport_take(void *port)
+{
+ veb_eb_port_take(NULL, port);
+}
+
+static void
+veb_eb_brport_rele(void *port)
+{
+ veb_eb_port_rele(NULL, port);
+}
+
static size_t
veb_eb_port_ifname(void *arg, char *dst, size_t len, void *port)
{
@@ -2181,6 +2204,9 @@ vport_enqueue(struct ifnet *ifp, struct mbuf *m)
smr_read_enter();
eb = SMR_PTR_GET(&ac->ac_brport);
+ if (eb != NULL)
+ eb->eb_port_take(eb->eb_port);
+ smr_read_leave();
if (eb != NULL) {
struct ether_header *eh;
uint64_t dst;
@@ -2199,8 +2225,9 @@ vport_enqueue(struct ifnet *ifp, struct mbuf *m)
m = (*eb->eb_input)(ifp, m, dst, eb->eb_port);
error = 0;
+
+ eb->eb_port_rele(eb->eb_port);
}
- smr_read_leave();
m_freem(m);
diff --git a/sys/netinet/if_ether.h b/sys/netinet/if_ether.h
index 56e6384f39b..611e0fa704b 100644
--- a/sys/netinet/if_ether.h
+++ b/sys/netinet/if_ether.h
@@ -220,6 +220,8 @@ do {
\
struct ether_brport {
struct mbuf *(*eb_input)(struct ifnet *, struct mbuf *,
uint64_t, void *);
+ void (*eb_port_take)(void *);
+ void (*eb_port_rele)(void *);
void *eb_port;
};
