From 5c70338585f48038834a790616289f6d5134f430 Mon Sep 17 00:00:00 2001 From: Matthew Dillon Date: Wed, 19 Jan 2005 17:30:54 +0000 Subject: [PATCH] When a PCMCIA networking card is removed the IF code may free() the network interface before processing has completed on pending packets, leaving a dangling pointer in the mbuf and causing a crash. The two solutions are to either ref-count the network interface on a per-packet basis or to synchronize against consumers of the packet. ref-counting is very expensive in an MP system so we have chosen to synchronize against consumers by sending a NOP message to all protocol processing threads and waiting for it to be replied. This only occurs when an interface is being brought down and is not expected to introduce any performance issues. Crash-Reported-by: Jonathon McKitrick --- sys/net/if.c | 11 ++++-- sys/net/netisr.c | 83 ++++++++++++++++++++++++++++++++++++++++-- sys/net/netisr.h | 5 ++- sys/netinet/ip_demux.c | 6 +-- 4 files changed, 93 insertions(+), 12 deletions(-) diff --git a/sys/net/if.c b/sys/net/if.c index c7ca41c669..2704641559 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -32,7 +32,7 @@ * * @(#)if.c 8.3 (Berkeley) 1/4/94 * $FreeBSD: src/sys/net/if.c,v 1.185 2004/03/13 02:35:03 brooks Exp $ - * $DragonFly: src/sys/net/if.c,v 1.24 2005/01/06 09:14:13 hsu Exp $ + * $DragonFly: src/sys/net/if.c,v 1.25 2005/01/19 17:30:52 dillon Exp $ */ #include "opt_compat.h" @@ -857,8 +857,12 @@ if_route(struct ifnet *ifp, int flag, int fam) } /* - * Mark an interface down and notify protocols of - * the transition. + * Mark an interface down and notify protocols of the transition. An + * interface going down is also considered to be a synchronizing event. + * We must ensure that all packet processing related to the interface + * has completed before we return so e.g. the caller can free the ifnet + * structure that the mbufs may be referencing. + * * NOTE: must be called at splnet or eqivalent. */ void @@ -866,6 +870,7 @@ if_down(struct ifnet *ifp) { if_unroute(ifp, IFF_UP, AF_UNSPEC); + netmsg_service_sync(); } /* diff --git a/sys/net/netisr.c b/sys/net/netisr.c index a792c01a7b..08d3df6587 100644 --- a/sys/net/netisr.c +++ b/sys/net/netisr.c @@ -35,7 +35,7 @@ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF * SUCH DAMAGE. * - * $DragonFly: src/sys/net/netisr.c,v 1.22 2004/09/10 18:23:56 dillon Exp $ + * $DragonFly: src/sys/net/netisr.c,v 1.23 2005/01/19 17:30:52 dillon Exp $ */ /* @@ -73,7 +73,15 @@ #include #include +static int netmsg_sync_func(struct netmsg *msg); + +struct netmsg_port_registration { + TAILQ_ENTRY(netmsg_port_registration) npr_entry; + lwkt_port_t npr_port; +}; + static struct netisr netisrs[NETISR_MAX]; +static TAILQ_HEAD(,netmsg_port_registration) netreglist; /* Per-CPU thread to handle any protocol. */ struct thread netisr_cpu[MAXCPU]; @@ -104,7 +112,7 @@ netisr_autofree_reply(lwkt_port_t port, lwkt_msg_t msg) * note: ms_target_port does not need to be set when returning a synchronous * error code. */ -int +static int netmsg_put_port(lwkt_port_t port, lwkt_msg_t lmsg) { int error; @@ -162,13 +170,15 @@ netisr_init(void) { int i; + TAILQ_INIT(&netreglist); + /* * Create default per-cpu threads for generic protocol handling. */ for (i = 0; i < ncpus; ++i) { lwkt_create(netmsg_service_loop, NULL, NULL, &netisr_cpu[i], 0, i, - "netisr_cpu %d", i); - netisr_cpu[i].td_msgport.mp_putport = netmsg_put_port; + "netisr_cpu %d", i); + netmsg_service_port_init(&netisr_cpu[i].td_msgport); } /* @@ -191,6 +201,71 @@ netisr_init(void) SYSINIT(netisr, SI_SUB_PROTO_BEGIN, SI_ORDER_FIRST, netisr_init, NULL); +/* + * Finish initializing the message port for a netmsg service. This also + * registers the port for synchronous cleanup operations such as when an + * ifnet is being destroyed. There is no deregistration API yet. + */ +void +netmsg_service_port_init(lwkt_port_t port) +{ + struct netmsg_port_registration *reg; + + /* + * Override the putport function. Our custom function checks for + * self-references and executes such commands synchronously. + */ + port->mp_putport = netmsg_put_port; + + /* + * Keep track of ports using the netmsg API so we can synchronize + * certain operations (such as freeing an ifnet structure) across all + * consumers. + */ + reg = malloc(sizeof(*reg), M_TEMP, M_WAITOK|M_ZERO); + reg->npr_port = port; + TAILQ_INSERT_TAIL(&netreglist, reg, npr_entry); +} + +/* + * This function synchronizes the caller with all netmsg services. For + * example, if an interface is being removed we must make sure that all + * packets related to that interface complete processing before the structure + * can actually be freed. This sort of synchronization is an alternative to + * ref-counting the netif, removing the ref counting overhead in favor of + * placing additional overhead in the netif freeing sequence (where it is + * inconsequential). + */ +void +netmsg_service_sync(void) +{ + struct netmsg_port_registration *reg; + struct netmsg smsg; + + lwkt_initmsg(&smsg.nm_lmsg, &curthread->td_msgport, 0, + lwkt_cmd_func((void *)netmsg_sync_func), lwkt_cmd_op_none); + + TAILQ_FOREACH(reg, &netreglist, npr_entry) { + lwkt_domsg(reg->npr_port, &smsg.nm_lmsg); + } +} + +/* + * The netmsg function simply replies the message. API semantics require + * EASYNC to be returned if the netmsg function disposes of the message. + */ +static +int +netmsg_sync_func(struct netmsg *msg) +{ + lwkt_replymsg(&msg->nm_lmsg, 0); + return(EASYNC); +} + +/* + * Generic netmsg service loop. Some protocols may roll their own but all + * must do the basic command dispatch function call done here. + */ void netmsg_service_loop(void *arg) { diff --git a/sys/net/netisr.h b/sys/net/netisr.h index 996e10cd6b..6426bdb14f 100644 --- a/sys/net/netisr.h +++ b/sys/net/netisr.h @@ -82,7 +82,7 @@ * * @(#)netisr.h 8.1 (Berkeley) 6/10/93 * $FreeBSD: src/sys/net/netisr.h,v 1.21.2.5 2002/02/09 23:02:39 luigi Exp $ - * $DragonFly: src/sys/net/netisr.h,v 1.20 2004/07/18 16:26:41 dillon Exp $ + * $DragonFly: src/sys/net/netisr.h,v 1.21 2005/01/19 17:30:52 dillon Exp $ */ #ifndef _NET_NETISR_H_ @@ -215,8 +215,9 @@ void netisr_dispatch(int, struct mbuf *); int netisr_queue(int, struct mbuf *); void netisr_register(int, lwkt_portfn_t, netisr_fn_t); int netisr_unregister(int); -int netmsg_put_port(lwkt_port_t, lwkt_msg_t); +void netmsg_service_port_init(lwkt_port_t); void netmsg_service_loop(void *arg); +void netmsg_service_sync(void); void schednetisr(int); #endif /* KERNEL */ diff --git a/sys/netinet/ip_demux.c b/sys/netinet/ip_demux.c index 5869e6818f..c9c8f4a796 100644 --- a/sys/netinet/ip_demux.c +++ b/sys/netinet/ip_demux.c @@ -30,7 +30,7 @@ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF * SUCH DAMAGE. * - * $DragonFly: src/sys/netinet/ip_demux.c,v 1.29 2004/12/21 02:54:15 hsu Exp $ + * $DragonFly: src/sys/netinet/ip_demux.c,v 1.30 2005/01/19 17:30:54 dillon Exp $ */ /* @@ -386,7 +386,7 @@ tcp_thread_init(void) for (cpu = 0; cpu < ncpus2; cpu++) { lwkt_create(tcpmsg_service_loop, NULL, NULL, &tcp_thread[cpu], 0, cpu, "tcp_thread %d", cpu); - tcp_thread[cpu].td_msgport.mp_putport = netmsg_put_port; + netmsg_service_port_init(&tcp_thread[cpu].td_msgport); } } @@ -398,6 +398,6 @@ udp_thread_init(void) for (cpu = 0; cpu < ncpus2; cpu++) { lwkt_create(netmsg_service_loop, NULL, NULL, &udp_thread[cpu], 0, cpu, "udp_thread %d", cpu); - udp_thread[cpu].td_msgport.mp_putport = netmsg_put_port; + netmsg_service_port_init(&udp_thread[cpu].td_msgport); } } -- 2.41.0