--- //depot/vendor/freebsd/src/sys/conf/files 2004/11/20 23:40:44 +++ //depot/user/rwatson/netperf/sys/conf/files 2004/11/25 10:56:11 @@ -1130,6 +1130,7 @@ kern/subr_kobj.c standard kern/subr_log.c standard kern/subr_mbpool.c optional libmbpool +kern/subr_mbufqueue.c standard kern/subr_mchain.c optional libmchain kern/subr_module.c standard kern/subr_msgbuf.c standard @@ -1680,6 +1681,7 @@ security/mac_seeotheruids/mac_seeotheruids.c optional mac_seeotheruids security/mac_stub/mac_stub.c optional mac_stub security/mac_test/mac_test.c optional mac_test +test/test_synch_timing.c standard ufs/ffs/ffs_alloc.c optional ffs ufs/ffs/ffs_balloc.c optional ffs ufs/ffs/ffs_inode.c optional ffs --- //depot/vendor/freebsd/src/sys/dev/em/if_em.c 2004/11/14 20:20:42 +++ //depot/user/rwatson/netperf/sys/dev/em/if_em.c 2004/11/14 21:41:20 @@ -571,7 +571,64 @@ return(0); } +/* + * Alternative transmit entry point: accept an mbuf as well as a network + * interface, avoiding queuing overhead if possible. Several paths, depending + * on whether other packets are already queued or not. We must either hand + * off or free the mbuf, as the caller no longer owns it. + * + * XXXRW: For now, simply replicate if_handoff(), avoiding two mutex + * operations, but eventually we want a fast path here with no ifq mutex + * operations for when the ifq is full. I left it with one pair for now to + * maintain consistency on the if send counters. + */ +static int +em_start_mbuf(struct ifnet *ifp, struct mbuf *m, int adjust) +{ + struct adapter *adapter = ifp->if_softc; + struct ifqueue *ifq; + + /* + * First, do a lot of work to make it look as though the packet was + * queued by if_handoff(), stats, etc. + */ + EM_LOCK(adapter); + ifq = (struct ifqueue *)&ifp->if_snd; + IF_LOCK(ifq); + if (_IF_QFULL(ifq)) { + _IF_DROP(ifq); + IF_UNLOCK(ifq); + EM_UNLOCK(adapter); + m_freem(m); + return (0); + } + ifp->if_obytes += m->m_pkthdr.len + adjust; + if (m->m_flags & (M_BCAST|M_MCAST)) + ifp->if_omcasts++; + /* + * Now we see if the queue is empty, in which case we can begin a + * direct dispatch of our packet. Otherwise, we enqueue it as + * if_handoff() would have. NOTE: Not ALTQ-safe. + */ + if (IFQ_IS_EMPTY(ifq)) { + if (em_encap(adapter, &m)) + goto enqueue; + ifp->if_flags |= IFF_OACTIVE; + BPF_MTAP(ifp, m); + ifp->if_timer = EM_TX_TIMEOUT; + IF_UNLOCK(ifq); + EM_UNLOCK(adapter); + return (1); + } +enqueue: + if (m != NULL) + _IF_ENQUEUE(ifq, m); + IF_UNLOCK(ifq); + EM_UNLOCK(adapter); + return (1); +} + /********************************************************************* * Transmit entry point * @@ -1930,6 +1987,7 @@ ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST; ifp->if_ioctl = em_ioctl; ifp->if_start = em_start; + ifp->if_start_mbuf = em_start_mbuf; ifp->if_watchdog = em_watchdog; IFQ_SET_MAXLEN(&ifp->if_snd, adapter->num_tx_desc - 1); ifp->if_snd.ifq_drv_maxlen = adapter->num_tx_desc - 1; --- //depot/vendor/freebsd/src/sys/dev/random/randomdev_soft.c 2004/11/05 20:15:29 +++ //depot/user/rwatson/netperf/sys/dev/random/randomdev_soft.c 2004/11/06 12:44:50 @@ -54,7 +54,7 @@ #include #include -#define RANDOM_FIFO_MAX 256 /* How many events to queue up */ +#define RANDOM_FIFO_MAX 4 /* How many events to queue up */ static void random_kthread(void *); static void --- //depot/vendor/freebsd/src/sys/fs/portalfs/portal_vnops.c 2004/12/01 23:20:51 +++ //depot/user/rwatson/netperf/sys/fs/portalfs/portal_vnops.c 2004/12/04 17:43:45 @@ -194,6 +194,7 @@ unp2 = sotounpcb(so2); unp3 = sotounpcb(so3); + /* XXXRW: Locking? */ if (unp2->unp_addr) unp3->unp_addr = (struct sockaddr_un *) sodupsockaddr((struct sockaddr *)unp2->unp_addr, --- //depot/vendor/freebsd/src/sys/i386/conf/GENERIC 2004/11/02 21:01:10 +++ //depot/user/rwatson/netperf/sys/i386/conf/GENERIC 2004/11/03 14:10:39 @@ -70,6 +70,7 @@ options INVARIANT_SUPPORT # Extra sanity checks of internal structures, required by INVARIANTS options WITNESS # Enable checks to detect deadlocks and cycles options WITNESS_SKIPSPIN # Don't run witness on spinlocks for speed +options BREAK_TO_DEBUGGER # To make an SMP kernel, the next two are needed options SMP # Symmetric MultiProcessor Kernel --- //depot/vendor/freebsd/src/sys/kern/kern_descrip.c 2004/12/03 21:30:38 +++ //depot/user/rwatson/netperf/sys/kern/kern_descrip.c 2004/12/04 17:43:45 @@ -1468,6 +1468,9 @@ /* * Release a filedesc structure. + * + * XXXRW: Requires Giant because it calls into VFS. Status of p_fdtol is + * unclear to me. */ void fdfree(struct thread *td) @@ -1603,10 +1606,16 @@ * * Since setugidsafety calls this only for fd 0, 1 and 2, this check is * sufficient. We also don't for check setugidness since we know we are. + * + * XXXRW: Requires Giant due to check of v_vflag. Actually, this can + * probably become a lockless read. */ static int is_unsafe(struct file *fp) { + + GIANT_REQUIRED; + if (fp->f_type == DTYPE_VNODE) { struct vnode *vp = fp->f_vnode; @@ -1618,6 +1627,8 @@ /* * Make this setguid thing safe, if at all possible. + * + * XXXRW: MPSAFE, as knote_fdclose() and closef() are both now MPSAFE. */ void setugidsafety(struct thread *td) @@ -1675,6 +1686,8 @@ /* * Close any files on exec? + * + * XXXRW: Now MPSAFE, as knode_fdclosed() and closef() are now MPSAFE. */ void fdcloseexec(struct thread *td) @@ -1721,6 +1734,8 @@ * significance in the Standard C library. fdcheckstd() will create a * descriptor referencing /dev/null for each of stdin, stdout, and * stderr that is not already open. + * + * XXXRW: Requires Giant for VFS access. do_dup is MPSAFE. */ int fdcheckstd(struct thread *td) @@ -1945,6 +1960,7 @@ * error). The returned vnode will be vref()d. * * XXX: what about the unused flags ? + * XXXRW: _fgetvp() is not MPSAFE because of VFS access. */ static __inline int _fgetvp(struct thread *td, int fd, struct vnode **vpp, int flags) @@ -1996,6 +2012,8 @@ * * We bump the ref count on the returned socket. XXX Also obtain the SX * lock in the future. + * + * XXXRW: fgetsock() is MPSAFE. */ int fgetsock(struct thread *td, int fd, struct socket **spp, u_int *fflagp) @@ -2050,6 +2068,9 @@ * Drop reference on struct file passed in, may call closef if the * reference hits zero. * Expects struct file locked, and will unlock it. + * + * XXXRW: fdrop_locked() is MPSAFE, as fo_close() will acquire Giant if it + * needs it. */ int fdrop_locked(struct file *fp, struct thread *td) --- //depot/vendor/freebsd/src/sys/kern/kern_poll.c 2004/07/03 02:40:38 +++ //depot/user/rwatson/netperf/sys/kern/kern_poll.c 2004/10/22 22:57:09 @@ -31,6 +31,8 @@ #include #include #include +#include +#include #include /* needed by net/if.h */ #include @@ -41,12 +43,6 @@ #include #include -#ifdef SMP -#ifndef COMPILING_LINT -#error DEVICE_POLLING is not compatible with SMP -#endif -#endif - static void netisr_poll(void); /* the two netisr handlers */ static void netisr_pollmore(void); @@ -183,12 +179,15 @@ static struct pollrec pr[POLL_LIST_LEN]; +static struct mtx pollmtx; + static void init_device_poll(void) { - netisr_register(NETISR_POLL, (netisr_t *)netisr_poll, NULL, 0); - netisr_register(NETISR_POLLMORE, (netisr_t *)netisr_pollmore, NULL, 0); + mtx_init(&pollmtx, "pollmtx", NULL, MTX_DEF); + netisr_register(NETISR_POLL, (netisr_t *)netisr_poll, NULL, NETISR_MPSAFE); + netisr_register(NETISR_POLLMORE, (netisr_t *)netisr_pollmore, NULL, NETISR_MPSAFE); } SYSINIT(device_poll, SI_SUB_CLOCKS, SI_ORDER_MIDDLE, init_device_poll, NULL) @@ -215,6 +214,7 @@ if (poll_handlers == 0) return; + mtx_lock(&pollmtx); microuptime(&t); delta = (t.tv_usec - prev_t.tv_usec) + (t.tv_sec - prev_t.tv_sec)*1000000; @@ -242,6 +242,7 @@ } if (pending_polls++ > 0) lost_polls++; + mtx_unlock(&pollmtx); } /* @@ -252,15 +253,14 @@ { int i; - mtx_lock(&Giant); - + mtx_lock(&pollmtx); if (count > poll_each_burst) count = poll_each_burst; for (i = 0 ; i < poll_handlers ; i++) if (pr[i].handler && (IFF_UP|IFF_RUNNING) == (pr[i].ifp->if_flags & (IFF_UP|IFF_RUNNING)) ) pr[i].handler(pr[i].ifp, 0, count); /* quick check */ - mtx_unlock(&Giant); + mtx_unlock(&pollmtx); } /* @@ -286,8 +286,8 @@ { struct timeval t; int kern_load; - /* XXX run at splhigh() or equivalent */ + mtx_lock(&pollmtx); phase = 5; if (residual_burst > 0) { schednetisrbits(1 << NETISR_POLL | 1 << NETISR_POLLMORE); @@ -322,6 +322,7 @@ schednetisrbits(1 << NETISR_POLL | 1 << NETISR_POLLMORE); phase = 6; } + mtx_unlock(&pollmtx); } /* @@ -335,8 +336,8 @@ static int reg_frac_count; int i, cycles; enum poll_cmd arg = POLL_ONLY; - mtx_lock(&Giant); + mtx_lock(&pollmtx); phase = 3; if (residual_burst == 0) { /* first call in this tick */ microuptime(&poll_start_t); @@ -396,7 +397,7 @@ } /* on -stable, schednetisr(NETISR_POLLMORE); */ phase = 4; - mtx_unlock(&Giant); + mtx_unlock(&pollmtx); } /* @@ -410,7 +411,6 @@ int ether_poll_register(poll_handler_t *h, struct ifnet *ifp) { - int s; if (polling == 0) /* polling disabled, cannot register */ return 0; @@ -421,7 +421,7 @@ if (ifp->if_flags & IFF_POLLING) /* already polling */ return 0; - s = splhigh(); + mtx_lock(&pollmtx); if (poll_handlers >= POLL_LIST_LEN) { /* * List full, cannot register more entries. @@ -431,7 +431,7 @@ * anyways, so just report a few times and then give up. */ static int verbose = 10 ; - splx(s); + mtx_unlock(&pollmtx); if (verbose >0) { printf("poll handlers list full, " "maybe a broken driver ?\n"); @@ -444,9 +444,9 @@ pr[poll_handlers].ifp = ifp; poll_handlers++; ifp->if_flags |= IFF_POLLING; - splx(s); if (idlepoll_sleeping) wakeup(&idlepoll_sleeping); + mtx_unlock(&pollmtx); return 1; /* polling enabled in next call */ } @@ -461,9 +461,9 @@ { int i; - mtx_lock(&Giant); + mtx_lock(&pollmtx); if ( !ifp || !(ifp->if_flags & IFF_POLLING) ) { - mtx_unlock(&Giant); + mtx_unlock(&pollmtx); return 0; } for (i = 0 ; i < poll_handlers ; i++) @@ -471,7 +471,7 @@ break; ifp->if_flags &= ~IFF_POLLING; /* found or not... */ if (i == poll_handlers) { - mtx_unlock(&Giant); + mtx_unlock(&pollmtx); printf("ether_poll_deregister: ifp not found!!!\n"); return 0; } @@ -480,7 +480,7 @@ pr[i].handler = pr[poll_handlers].handler; pr[i].ifp = pr[poll_handlers].ifp; } - mtx_unlock(&Giant); + mtx_unlock(&pollmtx); return 1; } @@ -498,21 +498,24 @@ pri = td->td_priority; mtx_unlock_spin(&sched_lock); + mtx_lock(&pollmtx); for (;;) { if (poll_in_idle_loop && poll_handlers > 0) { idlepoll_sleeping = 0; - mtx_lock(&Giant); ether_poll(poll_each_burst); - mtx_unlock(&Giant); mtx_assert(&Giant, MA_NOTOWNED); + mtx_unlock(&pollmtx); mtx_lock_spin(&sched_lock); mi_switch(SW_VOL, NULL); mtx_unlock_spin(&sched_lock); + mtx_lock(&pollmtx); } else { idlepoll_sleeping = 1; - tsleep(&idlepoll_sleeping, pri, "pollid", hz * 3); + msleep(&idlepoll_sleeping, &pollmtx, pri, "pollid", + hz * 3); } } + mtx_unlock(&pollmtx); } static struct proc *idlepoll; --- //depot/vendor/freebsd/src/sys/kern/sched_4bsd.c 2004/10/05 22:05:40 +++ //depot/user/rwatson/netperf/sys/kern/sched_4bsd.c 2004/10/09 21:40:52 @@ -1141,9 +1141,13 @@ */ kg = td->td_ksegrp; if (td->td_priority != kg->kg_user_pri) { +#if 0 mtx_lock_spin(&sched_lock); +#endif td->td_priority = kg->kg_user_pri; +#if 0 mtx_unlock_spin(&sched_lock); +#endif } } --- //depot/vendor/freebsd/src/sys/kern/subr_log.c 2004/06/16 09:52:13 +++ //depot/user/rwatson/netperf/sys/kern/subr_log.c 2004/06/18 00:24:39 @@ -83,7 +83,12 @@ struct callout sc_callout; /* callout to wakeup syslog */ } logsoftc; -int log_open; /* also used in log() */ +/* + * log_mtx protects logsoftc, log_open. Note that log_mtx does *not* + * protect the structures associated with msgbuf, which require Giant. + */ +struct mtx log_mtx; +int log_open; /* also used in log() */ /* Times per second to check for a pending syslog wakeup. */ static int log_wakeups_per_second = 5; @@ -94,17 +99,24 @@ static int logopen(struct cdev *dev, int flags, int mode, struct thread *td) { - if (log_open) + + mtx_lock(&log_mtx); + if (log_open) { + mtx_unlock(&log_mtx); return (EBUSY); + } log_open = 1; - callout_init(&logsoftc.sc_callout, 0); + callout_init(&logsoftc.sc_callout, CALLOUT_MPSAFE); + mtx_unlock(&log_mtx); fsetown(td->td_proc->p_pid, &logsoftc.sc_sigio); /* signal process only */ + mtx_lock(&log_mtx); if (log_wakeups_per_second < 1) { printf("syslog wakeup is less than one. Adjusting to 1.\n"); log_wakeups_per_second = 1; } callout_reset(&logsoftc.sc_callout, hz / log_wakeups_per_second, logtimeout, NULL); + mtx_unlock(&log_mtx); return (0); } @@ -113,9 +125,11 @@ logclose(struct cdev *dev, int flag, int mode, struct thread *td) { + mtx_lock(&log_mtx); log_open = 0; callout_stop(&logsoftc.sc_callout); logsoftc.sc_state = 0; + mtx_unlock(&log_mtx); funsetown(&logsoftc.sc_sigio); return (0); } @@ -134,14 +148,18 @@ splx(s); return (EWOULDBLOCK); } + mtx_lock(&log_mtx); logsoftc.sc_state |= LOG_RDWAIT; + mtx_unlock(&log_mtx); if ((error = tsleep(mbp, LOG_RDPRI | PCATCH, "klog", 0))) { splx(s); return (error); } } splx(s); + mtx_lock(&log_mtx); logsoftc.sc_state &= ~LOG_RDWAIT; + mtx_unlock(&log_mtx); while (uio->uio_resid > 0) { l = imin(sizeof(buf), uio->uio_resid); @@ -178,8 +196,11 @@ logtimeout(void *arg) { - if (!log_open) + mtx_lock(&log_mtx); + if (!log_open) { + mtx_unlock(&log_mtx); return; + } if (log_wakeups_per_second < 1) { printf("syslog wakeup is less than one. Adjusting to 1.\n"); log_wakeups_per_second = 1; @@ -187,6 +208,7 @@ if (msgbuftrigger == 0) { callout_reset(&logsoftc.sc_callout, hz / log_wakeups_per_second, logtimeout, NULL); + mtx_unlock(&log_mtx); return; } msgbuftrigger = 0; @@ -199,6 +221,7 @@ } callout_reset(&logsoftc.sc_callout, hz / log_wakeups_per_second, logtimeout, NULL); + mtx_unlock(&log_mtx); } /*ARGSUSED*/ @@ -217,10 +240,12 @@ break; case FIOASYNC: + mtx_lock(&log_mtx); if (*(int *)data) logsoftc.sc_state |= LOG_ASYNC; else logsoftc.sc_state &= ~LOG_ASYNC; + mtx_unlock(&log_mtx); break; case FIOSETOWN: @@ -249,6 +274,7 @@ log_drvinit(void *unused) { + mtx_init(&log_mtx, "log_mtx", NULL, MTX_DEF); make_dev(&log_cdevsw, 0, UID_ROOT, GID_WHEEL, 0600, "klog"); } --- //depot/vendor/freebsd/src/sys/kern/subr_prf.c 2004/07/10 21:40:38 +++ //depot/user/rwatson/netperf/sys/kern/subr_prf.c 2004/07/10 22:08:23 @@ -84,6 +84,9 @@ size_t remain; }; +/* + * XXXRW: We access subr_log.c's log_open variable unlocked. + */ extern int log_open; static void msglogchar(int c, int pri); --- //depot/vendor/freebsd/src/sys/kern/subr_witness.c 2004/11/09 06:50:43 +++ //depot/user/rwatson/netperf/sys/kern/subr_witness.c 2004/11/14 11:53:24 @@ -317,6 +317,7 @@ */ { "ddp_list_mtx", &lock_class_mtx_sleep }, { "ddp_mtx", &lock_class_mtx_sleep }, + { "at_ifaddr", &lock_class_mtx_sleep }, { NULL, NULL }, /* * BPF --- //depot/vendor/freebsd/src/sys/kern/sys_pipe.c 2004/11/23 22:15:29 +++ //depot/user/rwatson/netperf/sys/kern/sys_pipe.c 2004/11/24 14:41:38 @@ -409,6 +409,9 @@ * This routine will 'realloc' the size of a pipe safely, if it fails * it will retain the old buffer. * If it fails it will return ENOMEM. + * + * This version of the call should not be called directly except via + * pipe_create(), as it doesn't assert the pipe lock. */ static int pipespace_new(cpipe, size) --- //depot/vendor/freebsd/src/sys/kern/uipc_socket.c 2004/11/29 23:15:36 +++ //depot/user/rwatson/netperf/sys/kern/uipc_socket.c 2004/12/04 18:39:08 @@ -299,6 +299,7 @@ * SO_ACCEPTCONN before the call to pru_listen()? * XXXRW: General atomic test-and-set concerns here also. */ + /* Unlocked read. */ if (so->so_state & (SS_ISCONNECTED | SS_ISCONNECTING | SS_ISDISCONNECTING)) return (EINVAL); @@ -338,6 +339,7 @@ ACCEPT_LOCK_ASSERT(); SOCK_LOCK_ASSERT(so); + /* XXXRW: Why would SS_NOFDREF be unset here? so_count is 0. */ if (so->so_pcb != NULL || (so->so_state & SS_NOFDREF) == 0 || so->so_count != 0) { SOCK_UNLOCK(so); @@ -400,6 +402,45 @@ sodealloc(so); } +#if 0 +static void +dump_socket(struct socket *so) +{ + + printf("so = %p\n", so); + printf(" so_count = %d\n", so->so_count); + printf(" so_type = %d\n", so->so_type); + printf(" so_options = %d\n", so->so_options); + printf(" so_linger = %d\n", so->so_linger); + printf(" so_state = %d\n", so->so_state); + printf(" so_qstate = %d\n", so->so_qstate); + printf(" so_pcb = %p\n", so->so_pcb); + printf(" so_proto = %p\n", so->so_proto); + printf(" pr_type = %d\n", so->so_proto->pr_type); + printf(" pr_domain = %p\n", so->so_proto->pr_domain); + printf(" dom_family = %d\n", so->so_proto->pr_domain->dom_family); + printf(" dom_name = %s\n", so->so_proto->pr_domain->dom_name); + printf(" pr_protocol = %d\n", so->so_proto->pr_protocol); + printf(" pr_flags = %d\n", so->so_proto->pr_flags); + printf(" so_head = %p\n", so->so_head); + printf(" TAILQ_FIRST(so_incomp) = %p\n", TAILQ_FIRST(&so->so_incomp)); + printf(" TAILQ_FIRST(so_comp) = %p\n", TAILQ_FIRST(&so->so_comp)); + printf(" so_qlen = %d\n", so->so_qlen); + printf(" so_incqlen = %d\n", so->so_incqlen); + printf(" so_qlimit = %d\n", so->so_qlimit); + printf(" so_timeo = %d\n", so->so_timeo); + printf(" so_error = %d\n", so->so_error); + printf(" so_sigio = %p\n", so->so_sigio); + printf(" so_oobmark = %lu\n", so->so_oobmark); + printf(" so_rcv.sb_state = %d\n", so->so_rcv.sb_state); + printf(" so_snd.sb_state = %d\n", so->so_snd.sb_state); + printf(" so_upcall = %p\n", so->so_upcall); + printf(" so_upcallarg = %p\n", so->so_upcallarg); + printf(" so_cred = %p\n", so->so_cred); + printf(" so_gencnt = %d\n", (int)so->so_gencnt); +} +#endif + /* * Close a socket on last file table reference removal. * Initiate disconnect if connected. @@ -418,6 +459,23 @@ KASSERT(!(so->so_state & SS_NOFDREF), ("soclose: SS_NOFDREF on enter")); funsetown(&so->so_sigio); + /*- + * XXXRW: Lots of locking problems here. For one thing, + * we should probably clear the SO_ACCEPTCONN flag and + * push that down to the protocol layer. Also, the locking + * for the queue draining is probably not right either: + * what prevents new ones from being inserted after we get + * past this? Ideally, the downcall would prevent this, but + * there isn't one. + */ + /* + * XXXRW: soclose() should be split into two parts: one to handle + * listen sockets, and the other to handle the remainder. To + * prevent races with the protocol code, we need to detach before + * draining the queues. For non-listen sockets, the disconnect + * has to happen before the detach. + */ + /* Unlocked read. */ if (so->so_options & SO_ACCEPTCONN) { struct socket *sp; ACCEPT_LOCK(); @@ -443,6 +501,7 @@ } if (so->so_pcb == NULL) goto discard; + /* XXXRW: so_state locking? */ if (so->so_state & SS_ISCONNECTED) { if ((so->so_state & SS_ISDISCONNECTING) == 0) { error = sodisconnect(so); @@ -529,6 +588,7 @@ * This allows user to disconnect by connecting to, e.g., * a null address. */ + /* Unlocked read. */ if (so->so_state & (SS_ISCONNECTED|SS_ISCONNECTING) && ((so->so_proto->pr_flags & PR_CONNREQUIRED) || (error = sodisconnect(so)))) @@ -553,6 +613,7 @@ { int error; + /* Unlocked read. */ if ((so->so_state & SS_ISCONNECTED) == 0) return (ENOTCONN); if (so->so_state & SS_ISDISCONNECTING) @@ -643,8 +704,6 @@ #define snderr(errno) { error = (errno); goto release; } SOCKBUF_LOCK(&so->so_snd); -restart: - SOCKBUF_LOCK_ASSERT(&so->so_snd); error = sblock(&so->so_snd, SBLOCKWAIT(flags)); if (error) goto out_locked; @@ -683,11 +742,10 @@ (atomic || space < so->so_snd.sb_lowat || space < clen)) { if ((so->so_state & SS_NBIO) || (flags & MSG_NBIO)) snderr(EWOULDBLOCK); - sbunlock(&so->so_snd); error = sbwait(&so->so_snd); if (error) - goto out_locked; - goto restart; + goto release; + continue; } SOCKBUF_UNLOCK(&so->so_snd); mp = ⊤ @@ -799,6 +857,15 @@ break; } } while (space > 0 && atomic); + /* + * XXXRW: There may be a race condition here regarding + * SO_DONTROUTE. We hold the sblock() so in theory + * there won't be other consumers of the socket doing + * a pru_send(), but SO_DONTROUTE is also consumed + * from the protocol side. It sounds like we might + * want to use MSG_DONTROUTE here, if the semantics are + * right. + */ if (dontroute) { SOCK_LOCK(so); so->so_options |= SO_DONTROUTE; @@ -827,6 +894,10 @@ /* If there is more to send set PRUS_MORETOCOME */ (resid > 0 && space > 0) ? PRUS_MORETOCOME : 0, top, addr, control, td); + /* + * XXXRW: Second half of above comment on SO_DONTROUTE. + * and so_options. + */ if (dontroute) { SOCK_LOCK(so); so->so_options &= ~SO_DONTROUTE; @@ -997,16 +1068,17 @@ return (soreceive_rcvoob(so, uio, flags)); if (mp != NULL) *mp = NULL; + /* Unlocked read. */ if (so->so_state & SS_ISCONFIRMING && uio->uio_resid) (*pr->pr_usrreqs->pru_rcvd)(so, 0); SOCKBUF_LOCK(&so->so_rcv); -restart: - SOCKBUF_LOCK_ASSERT(&so->so_rcv); error = sblock(&so->so_rcv, SBLOCKWAIT(flags)); if (error) goto out; +restart: + SOCKBUF_LOCK_ASSERT(&so->so_rcv); m = so->so_rcv.sb_mb; /* * If we have less data than requested, block awaiting more @@ -1047,6 +1119,7 @@ m = so->so_rcv.sb_mb; goto dontblock; } + /* XXXRW: so_state locking? */ if ((so->so_state & (SS_ISCONNECTED|SS_ISCONNECTING)) == 0 && (so->so_proto->pr_flags & PR_CONNREQUIRED)) { error = ENOTCONN; @@ -1054,6 +1127,7 @@ } if (uio->uio_resid == 0) goto release; + /* XXXRW: so_state locking? */ if ((so->so_state & SS_NBIO) || (flags & (MSG_DONTWAIT|MSG_NBIO))) { error = EWOULDBLOCK; @@ -1061,10 +1135,9 @@ } SBLASTRECORDCHK(&so->so_rcv); SBLASTMBUFCHK(&so->so_rcv); - sbunlock(&so->so_rcv); error = sbwait(&so->so_rcv); if (error) - goto out; + goto release; goto restart; } dontblock: @@ -1093,10 +1166,25 @@ if (pr->pr_flags & PR_ADDR) { KASSERT(m->m_type == MT_SONAME, ("m->m_type == %d", m->m_type)); - orig_resid = 0; - if (psa != NULL) + if (psa != NULL) { *psa = sodupsockaddr(mtod(m, struct sockaddr *), M_NOWAIT); + if (*psa == NULL) { + error = ENOMEM; + goto release; + } + /* + * XXXRW: In the rwatson_netperf branch, we don't + * release the socket buffer lock here because + * we always use M_NOWAIT. In the main tree, + * or if we restore conditional waiting, we + * need to refresh nextpacket from the socket + * buffer version of the head mbuf, or we may + * have a stale value if it was previously NULL + * and now isn't. + */ + nextrecord = m->m_nextpkt; + } if (flags & MSG_PEEK) { m = m->m_next; } else { @@ -1105,6 +1193,7 @@ m = so->so_rcv.sb_mb; sockbuf_pushsync(&so->so_rcv, nextrecord); } + orig_resid = 0; } /* @@ -1119,6 +1208,14 @@ do { if (flags & MSG_PEEK) { + /* + * XXXRW: In the BSD/OS version of this + * code, m_copym() is called with M_TRYWAIT, + * and we catch allcation failures and + * jump to release with error of ENOBUFS. + * For consistency with the current + * implementation, we don't. + */ if (controlp != NULL) { *controlp = m_copy(m, 0, m->m_len); controlp = &(*controlp)->m_next; @@ -1133,6 +1230,12 @@ m = so->so_rcv.sb_mb; } } while (m != NULL && m->m_type == MT_CONTROL); + /* + * XXXRW: Since we're dropping the socket buffer locks, and + * may have modified the mbuf list on the socket buffer, push + * out the latest version so it can be seen by any other code + * that accesses the buffer in the mean time. + */ if ((flags & MSG_PEEK) == 0) sockbuf_pushsync(&so->so_rcv, nextrecord); while (cm != NULL) { @@ -1154,10 +1257,21 @@ } cm = cmn; } + /* + * XXXRW: During externalization, additional mbuf chains + * may have been added to the socket buffer. Update our + * local cache to avoid using a stale value and corrupting + * the list. + */ nextrecord = so->so_rcv.sb_mb->m_nextpkt; orig_resid = 0; } if (m != NULL) { + /* + * XXXRW: Before the advent of sockbuf_pushsync() above, it + * was necessary to do the equivilent work here. Now, we + * just assert that it is the case. + */ if ((flags & MSG_PEEK) == 0) { KASSERT(m->m_nextpkt == nextrecord, ("soreceive: post-control, nextrecord !sync")); @@ -1172,6 +1286,11 @@ if (type == MT_OOBDATA) flags |= MSG_OOB; } else { + /* + * XXXRW: Before the advent of sockbuf_pushsync() above, it + * was necessary to update sb_mb to nextrecord here. Now, we + * just assert that is the case. + */ if ((flags & MSG_PEEK) == 0) { KASSERT(so->so_rcv.sb_mb == nextrecord, ("soreceive: sb_mb != nextrecord")); @@ -1275,14 +1394,7 @@ so->so_rcv.sb_mb = m_free(m); m = so->so_rcv.sb_mb; } - if (m != NULL) { - m->m_nextpkt = nextrecord; - if (nextrecord == NULL) - so->so_rcv.sb_lastrecord = m; - } else { - so->so_rcv.sb_mb = nextrecord; - SB_EMPTY_FIXUP(&so->so_rcv); - } + sockbuf_pushsync(&so->so_rcv, nextrecord); SBLASTRECORDCHK(&so->so_rcv); SBLASTMBUFCHK(&so->so_rcv); } @@ -1349,6 +1461,12 @@ /* * Notify the protocol that some data has been * drained before blocking. + * + * XXXRW: We drop the socket buffer lock here + * because we know the protocol will need to do its + * own locking. However, after calling pru_rcvd() + * and re-grabbing the buffer mutex, we don't + * re-check the condition before sleeping. */ if (pr->pr_flags & PR_WANTRCVD && so->so_pcb != NULL) { SOCKBUF_UNLOCK(&so->so_rcv); @@ -1358,8 +1476,10 @@ SBLASTRECORDCHK(&so->so_rcv); SBLASTMBUFCHK(&so->so_rcv); error = sbwait(&so->so_rcv); - if (error) + if (error) { + error = 0; goto release; + } m = so->so_rcv.sb_mb; if (m != NULL) nextrecord = m->m_nextpkt; @@ -1392,6 +1512,10 @@ * If soreceive() is being done from the socket callback, then * don't need to generate ACK to peer to update window, since * ACK will be generated on return to TCP. + * + * XXXRW: We drop the socket buffer lock before calling + * down into the protocol. Is that OK in the calling + * context? */ if (!(flags & MSG_SOCALLBCK) && (pr->pr_flags & PR_WANTRCVD) && so->so_pcb) { @@ -1402,10 +1526,8 @@ } SOCKBUF_LOCK_ASSERT(&so->so_rcv); if (orig_resid == uio->uio_resid && orig_resid && - (flags & MSG_EOR) == 0 && (so->so_rcv.sb_state & SBS_CANTRCVMORE) == 0) { - sbunlock(&so->so_rcv); - goto restart; - } + (flags & MSG_EOR) == 0 && (so->so_rcv.sb_state & SBS_CANTRCVMORE) == 0) + goto restart; /* XXX multi-counts msgs */ if (flagsp != NULL) *flagsp |= flags; @@ -1473,6 +1595,7 @@ sizeof(*sb) - offsetof(struct sockbuf, sb_startzero)); SOCKBUF_UNLOCK(sb); + /* XXXRW: is passing in sb_mb this way really safe? */ SOCKBUF_LOCK_INIT(&asb, "so_rcv"); if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose != NULL) (*pr->pr_domain->dom_dispose)(asb.sb_mb); @@ -1924,6 +2047,7 @@ case SO_TIMESTAMP: case SO_BINTIME: case SO_NOSIGPIPE: + /* Unlocked read. */ optval = so->so_options & sopt->sopt_name; integer: error = sooptcopyout(sopt, &optval, sizeof optval); @@ -2129,6 +2253,10 @@ { int revents = 0; + /* + * XXXRW: We can probably trim this down to acquire only one lock at + * a time, and do it conditional on (events). + */ SOCKBUF_LOCK(&so->so_snd); SOCKBUF_LOCK(&so->so_rcv); if (events & (POLLIN | POLLRDNORM)) @@ -2175,6 +2303,7 @@ switch (kn->kn_filter) { case EVFILT_READ: + /* Unlocked read. */ if (so->so_options & SO_ACCEPTCONN) kn->kn_fop = &solisten_filtops; else @@ -2272,6 +2401,7 @@ { struct socket *so = kn->kn_fp->f_data; + /* Unlocked read. */ kn->kn_data = so->so_qlen; return (! TAILQ_EMPTY(&so->so_comp)); } --- //depot/vendor/freebsd/src/sys/kern/uipc_socket2.c 2004/10/27 05:40:41 +++ //depot/user/rwatson/netperf/sys/kern/uipc_socket2.c 2004/11/03 14:10:39 @@ -106,21 +106,39 @@ { SOCK_LOCK(so); + /* + * XXXRW: so_state locking? + * + * XXXRW: Why not clear SS_ISDISCONNECTED, SS_ISCONFIRMING? + */ so->so_state &= ~(SS_ISCONNECTED|SS_ISDISCONNECTING); so->so_state |= SS_ISCONNECTING; SOCK_UNLOCK(so); } +/* + * soisconnected() transitions a socket to a fully connected state. If + * so->so_head is non-NULL, we transition it from the incompletely + * connected queue to the completely connected queue. Otherwise, we + * just wake up the socket since it was an out-bound connection. + */ void soisconnected(so) struct socket *so; { struct socket *head; + /* XXXRW: so_state locking? */ SOCK_LOCK(so); so->so_state &= ~(SS_ISCONNECTING|SS_ISDISCONNECTING|SS_ISCONFIRMING); so->so_state |= SS_ISCONNECTED; SOCK_UNLOCK(so); + + /* + * XXXRW: Maybe an unlocked read of so_head would be desirable + * here? Unlocked read of so_qstate probably not a good idea + * regardless. so_options handling here is a little unsavory. + */ ACCEPT_LOCK(); head = so->so_head; if (head != NULL && (so->so_qstate & SQ_INCOMP)) { @@ -135,15 +153,37 @@ sorwakeup(head); wakeup_one(&head->so_timeo); } else { - ACCEPT_UNLOCK(); + void (*so_upcall)(struct socket *, void *, int); + void *so_upcallarg; + /* + * XXXRW: Should probably copy the upcall fields to + * local stack variables while holding the socket + * lock (and clear the option), then release the + * lock and make the call. This will prevent lock + * order reversals/recursion between this code and + * the upcall implementation, which will likely + * also want to frob the socket using locks. + * + * NOTE: We keep a local copy of the function + * pointer so we can invoke the upcall without + * holding locks. However, we also have to copy + * in the upcall fields from the head because the + * filter may need to be called more than once. + */ SOCK_LOCK(so); - so->so_upcall = + so_upcall = so->so_upcall = head->so_accf->so_accept_filter->accf_callback; - so->so_upcallarg = head->so_accf->so_accept_filter_arg; + so_upcallarg = so->so_upcallarg = + head->so_accf->so_accept_filter_arg; so->so_rcv.sb_flags |= SB_UPCALL; so->so_options &= ~SO_ACCEPTFILTER; SOCK_UNLOCK(so); - so->so_upcall(so, so->so_upcallarg, M_TRYWAIT); + ACCEPT_UNLOCK(); + /* + * Call with our existing reference, but without + * any locks. + */ + so_upcall(so, so_upcallarg, M_TRYWAIT); } return; } @@ -233,6 +273,8 @@ so->so_type = head->so_type; so->so_options = head->so_options &~ SO_ACCEPTCONN; so->so_linger = head->so_linger; + /* XXXRW: so_state locking? */ + /* XXXRW: should mask additional head->so_state bits here? */ so->so_state = head->so_state | SS_NOFDREF; so->so_proto = head->so_proto; so->so_timeo = head->so_timeo; @@ -279,6 +321,7 @@ } ACCEPT_UNLOCK(); if (connstatus) { + /* XXXRW: so_state locking? */ so->so_state |= connstatus; sorwakeup(head); wakeup_one(&head->so_timeo); @@ -407,6 +450,10 @@ } KNOTE_LOCKED(&sb->sb_sel.si_note, 0); SOCKBUF_UNLOCK(sb); + /* + * XXXRW: What locks should be held over what parts of the + * following? Currently we hold none. + */ if ((so->so_state & SS_ASYNC) && so->so_sigio != NULL) pgsigio(&so->so_sigio, SIGIO, 0); if (sb->sb_flags & SB_UPCALL) @@ -453,7 +500,7 @@ register struct socket *so; u_long sndcc, rcvcc; { - struct thread *td = curthread; + struct thread *td = curthread; /* XXX */ SOCKBUF_LOCK(&so->so_snd); SOCKBUF_LOCK(&so->so_rcv); @@ -517,6 +564,13 @@ /* * td will only be NULL when we're in an interrupt * (e.g. in tcp_input()) + * + * XXXRW: This comment is true, but only because the caller passed + * in NULL, not for the 4.x reason that there is no thread + * available. Need to be careful of callers that do this wrong; + * I suspect many do it wrong, and therefore many socket buffers + * end up with the wrong limits, especially via the soreserve() + * path. */ if (cc > sb_max_adj) return (0); @@ -1447,6 +1501,7 @@ xso->so_type = so->so_type; xso->so_options = so->so_options; xso->so_linger = so->so_linger; + /* Unlocked read. */ xso->so_state = so->so_state; xso->so_pcb = so->so_pcb; xso->xso_protocol = so->so_proto->pr_protocol; --- //depot/vendor/freebsd/src/sys/kern/uipc_syscalls.c 2004/11/13 11:56:08 +++ //depot/user/rwatson/netperf/sys/kern/uipc_syscalls.c 2004/11/24 14:41:38 @@ -311,6 +311,7 @@ error = fgetsock(td, uap->s, &head, &fflag); if (error) goto done2; + /* Unlocked read. */ if ((head->so_options & SO_ACCEPTCONN) == 0) { error = EINVAL; goto done; @@ -318,6 +319,10 @@ error = falloc(td, &nfp, &fd); if (error) goto done; + /* + * Unlocked reads. + * XXXRW: Dubious use of so->so_error. + */ ACCEPT_LOCK(); if ((head->so_state & SS_NBIO) && TAILQ_EMPTY(&head->so_comp)) { ACCEPT_UNLOCK(); @@ -366,6 +371,11 @@ /* An extra reference on `nfp' has been held for us by falloc(). */ td->td_retval[0] = fd; + /* + * XXXRW: Might make life simpler to grab the socket buffer lock + * here so it's held when KNOTE() calls back into the socket + * code. + */ /* connection has been removed from the listen queue */ KNOTE_UNLOCKED(&head->so_rcv.sb_sel.si_note, 0); @@ -509,6 +519,7 @@ if (error) goto done2; so = fp->f_data; + /* XXXRW: so_state locking? */ if (so->so_state & SS_ISCONNECTING) { error = EALREADY; goto done1; @@ -523,12 +534,14 @@ error = soconnect(so, sa, td); if (error) goto bad; + /* XXXRW: so_state locking? */ if ((so->so_state & SS_NBIO) && (so->so_state & SS_ISCONNECTING)) { error = EINPROGRESS; goto done1; } s = splnet(); SOCK_LOCK(so); + /* XXXRW: so_state locking? */ while ((so->so_state & SS_ISCONNECTING) && so->so_error == 0) { error = msleep(&so->so_timeo, SOCK_MTX(so), PSOCK | PCATCH, "connec", 0); @@ -545,6 +558,7 @@ SOCK_UNLOCK(so); splx(s); bad: + /* XXXRW: so_state locking? */ if (!interrupted) so->so_state &= ~SS_ISCONNECTING; if (error == ERESTART) @@ -1513,6 +1527,7 @@ if (error) goto done2; so = fp->f_data; + /* XXXRW: so_state locking? */ if ((so->so_state & (SS_ISCONNECTED|SS_ISCONFIRMING)) == 0) { error = ENOTCONN; goto done1; @@ -1754,6 +1769,7 @@ error = EINVAL; goto done; } + /* XXXRW: so_state locking? */ if ((so->so_state & SS_ISCONNECTED) == 0) { error = ENOTCONN; goto done; @@ -1846,6 +1862,7 @@ * before going to the extra work of constituting the sf_buf. */ SOCKBUF_LOCK(&so->so_snd); + /* XXXRW: so_state locking? */ if ((so->so_state & SS_NBIO) && sbspace(&so->so_snd) <= 0) { if (so->so_snd.sb_state & SBS_CANTSENDMORE) error = EPIPE; @@ -1910,6 +1927,7 @@ * Get the page from backing store. */ bsize = vp->v_mount->mnt_stat.f_iosize; + mtx_lock(&Giant); /* VFS */ vn_lock(vp, LK_SHARED | LK_NOPAUSE | LK_RETRY, td); /* * XXXMAC: Because we don't have fp->f_cred here, @@ -1921,6 +1939,7 @@ IO_VMIO | ((MAXBSIZE / bsize) << IO_SEQSHIFT), td->td_ucred, NOCRED, &resid, td); VOP_UNLOCK(vp, 0, td); + mtx_unlock(&Giant); /* VFS */ VM_OBJECT_LOCK(obj); vm_page_lock_queues(); vm_page_io_finish(pg); @@ -2031,6 +2050,7 @@ * after checking the connection state above in order to avoid * a race condition with sbwait(). */ + /* XXXRW: so_state locking? */ if (sbspace(&so->so_snd) < so->so_snd.sb_lowat) { if (so->so_state & SS_NBIO) { m_freem(m); --- //depot/vendor/freebsd/src/sys/kern/uipc_usrreq.c 2004/12/01 09:25:37 +++ //depot/user/rwatson/netperf/sys/kern/uipc_usrreq.c 2004/12/04 17:43:45 @@ -180,6 +180,9 @@ { struct unpcb *unp = sotounpcb(so); + /* + * XXXRW: How could unp be non-NULL here? + */ if (unp != NULL) return (EISCONN); return (unp_attach(so)); @@ -354,6 +357,14 @@ (void)chgsbsize(so2->so_cred->cr_uidinfo, &so2->so_snd.sb_hiwat, newhiwat, RLIM_INFINITY); unp->unp_cc = so->so_rcv.sb_cc; + + /* + * XXXRW: It would be desirable to release the UNP lock + * before doing the socket wakeup. However, to do that we + * need to acquire a reference to so2 so it doesn't evaporate + * if the peer closes it; this may be more expensive than the + * wakeup causing contention on the socket mutex. + */ SOCKBUF_UNLOCK(&so->so_rcv); sowwakeup_locked(so2); break; @@ -376,6 +387,12 @@ struct socket *so2; u_long newhiwat; + /* + * XXXRW: We do the basic checks before acquiring the UNP lock in + * order to avoid paying the cost if it's not needed. We have to + * re-do the sotounpcb() call once we do acquire it since that may + * have changed. + */ unp = sotounpcb(so); if (unp == NULL) { error = EINVAL; @@ -441,6 +458,7 @@ * Note: A better implementation would complain * if not equal to the peer's address. */ + /* Unlocked read. */ if ((so->so_state & SS_ISCONNECTED) == 0) { if (nam != NULL) { error = unp_connect(so, nam, td); @@ -452,6 +470,7 @@ } } + /* Unlocked read. */ if (so->so_snd.sb_state & SBS_CANTSENDMORE) { error = EPIPE; break; @@ -464,6 +483,9 @@ * Send to paired receive port, and then reduce * send buffer hiwater marks to maintain backpressure. * Wake up readers. + * + * XXXRW: Does it matter that we don't check return value of + * sbappend_locked() here? */ if (control != NULL) { if (sbappendcontrol_locked(&so2->so_rcv, m, control)) @@ -524,6 +546,7 @@ sb->st_blksize = so->so_snd.sb_hiwat; if (so->so_type == SOCK_STREAM && unp->unp_conn != NULL) { so2 = unp->unp_conn->unp_socket; + /* Unlocked read. */ sb->st_blksize += so2->so_rcv.sb_cc; } sb->st_dev = NODEV; @@ -705,6 +728,7 @@ unp->unp_socket = so; UNP_LOCK(); + KASSERT(so->so_pcb == NULL, ("unp_attach(): so->so_pcb != NULL")); unp->unp_gencnt = ++unp_gencnt; unp_count++; LIST_INSERT_HEAD(so->so_type == SOCK_DGRAM ? &unp_dhead @@ -844,6 +868,12 @@ ASSERT_VOP_LOCKED(vp, "unp_bind"); soun = (struct sockaddr_un *)sodupsockaddr(nam, M_WAITOK); UNP_LOCK(); + /* + * XXXRW: we actually need to retest unp here and make sure that we + * didn't race with another thread operating on this socket. In + * particular, we must check unp != NULL, and that unp->unp_vnode == + * NULL. + */ vp->v_socket = unp->unp_socket; unp->unp_vnode = vp; unp->unp_addr = soun; @@ -902,6 +932,11 @@ goto bad; mtx_unlock(&Giant); UNP_LOCK(); + /* + * XXXRW: We actually need to retest our assumptions here to make + * sure we didn't race with another thread operating on this UNIX + * domain socket. In particular, that unp != NULL. + */ unp = sotounpcb(so); if (unp == NULL) { /* @@ -927,6 +962,10 @@ * w/o locks; this avoids a recursive lock * of the head and holding sleep locks across * a (potentially) blocking malloc. + * + * XXXRW: This may introduce potential races if + * close() is called on (so) while the lock is + * releasd. */ UNP_UNLOCK(); so3 = sonewconn(so2, 0); @@ -1606,6 +1645,9 @@ * of a signal. If sbwait does return * an error, we'll go into an infinite * loop. Delete all of this for now. + * + * XXXRW: We will need to lock the socket + * buffer here if we enable this code. */ (void) sbwait(&so->so_rcv); goto restart; @@ -1617,6 +1659,9 @@ * to see if we hold any file descriptors in its * message buffers. Follow those links and mark them * as accessible too. + * + * XXXRW: We hold the socket buffer lock to prevent + * the mbuf chain of interest from changing. */ SOCKBUF_LOCK(&so->so_rcv); unp_scan(so->so_rcv.sb_mb, unp_mark); --- //depot/vendor/freebsd/src/sys/kern/vfs_aio.c 2004/11/04 08:02:12 +++ //depot/user/rwatson/netperf/sys/kern/vfs_aio.c 2004/11/06 12:44:50 @@ -170,6 +170,9 @@ SYSCTL_INT(_vfs_aio, OID_AUTO, max_buf_aio, CTLFLAG_RW, &max_buf_aio, 0, "Maximum buf aio requests per process (stored in the process)"); +/* + * struct aiocblist describes a specific AIO I/O request (job). + */ struct aiocblist { TAILQ_ENTRY(aiocblist) list; /* List of jobs */ TAILQ_ENTRY(aiocblist) plist; /* List of jobs for proc */ @@ -198,6 +201,10 @@ #define AIOP_FREE 0x1 /* proc on free queue */ #define AIOP_SCHED 0x2 /* proc explicitly scheduled */ +/* + * struct aiothreadlist is the per-worker thread data structure, and is + * allocated by the thread when it is created. + */ struct aiothreadlist { int aiothreadflags; /* AIO proc flags */ TAILQ_ENTRY(aiothreadlist) list; /* List of processes */ @@ -221,7 +228,9 @@ #define LIOJ_SIGNAL_POSTED 0x2 /* signal has been posted */ /* - * per process aio data structure + * struct kaiinfo is the per-process aio data structure. This is allocated + * and hung off of p->p_aioinfo by aio_init_aioinfo() the first time a + * process makes use of the AIO facility. */ struct kaioinfo { int kaio_flags; /* per process kaio flags */ @@ -1228,6 +1237,8 @@ /* * Wake up aio requests that may be serviceable now. + * + * XXXRW: This doesn't look MPSAFE at all. */ static void aio_swake_cb(struct socket *so, struct sockbuf *sb) --- //depot/vendor/freebsd/src/sys/net/bpf.c 2004/10/06 04:30:44 +++ //depot/user/rwatson/netperf/sys/net/bpf.c 2004/10/09 21:40:52 @@ -273,6 +273,10 @@ struct bpf_if *bp; struct ifnet *ifp; + /* + * XXXRW: Watch out for bpfdetach() in the other direction: can + * d->bd_bif become NULL here? + */ bp = d->bd_bif; BPFIF_LOCK(bp); BPFD_LOCK(d); @@ -296,6 +300,9 @@ /* * Check if this descriptor had requested promiscuous mode. * If so, turn it off. + * + * XXXRW: Note, ifp might be stale here if the ifnet has been + * removed via bpfdetach(). */ if (d->bd_promisc) { d->bd_promisc = 0; @@ -344,7 +351,6 @@ make_dev(&bpf_cdevsw, minor(dev), UID_ROOT, GID_WHEEL, 0600, "bpf%d", dev2unit(dev)); MALLOC(d, struct bpf_d *, sizeof(*d), M_BPF, M_WAITOK | M_ZERO); - dev->si_drv1 = d; d->bd_bufsize = bpf_bufsize; d->bd_sig = SIGIO; d->bd_seesent = 1; @@ -355,6 +361,7 @@ mtx_init(&d->bd_mtx, devtoname(dev), "bpf cdev lock", MTX_DEF); callout_init(&d->bd_callout, debug_mpsafenet ? CALLOUT_MPSAFE : 0); knlist_init(&d->bd_sel.si_note, &d->bd_mtx); + dev->si_drv1 = d; return (0); } @@ -373,6 +380,9 @@ { struct bpf_d *d = dev->si_drv1; + /* + * XXXRW: Should this be a drain not just a callout? + */ BPFD_LOCK(d); if (d->bd_state == BPF_WAITING) callout_stop(&d->bd_callout); @@ -499,6 +509,9 @@ * Move data from hold buffer into user space. * We know the entire buffer is transferred since * we checked above that the read buffer is bpf_bufsize bytes. + * + * XXXRW: What protects bd_hbuf and bd_hlen from changing during this + * operation? Especially given that uiomove() can page fault. */ error = uiomove(d->bd_hbuf, d->bd_hlen, uio); @@ -519,6 +532,10 @@ bpf_wakeup(d) struct bpf_d *d; { + + /* + * XXXRW: Assert descriptor lock for (d)? + */ if (d->bd_state == BPF_WAITING) { callout_stop(&d->bd_callout); d->bd_state = BPF_IDLE; @@ -531,6 +548,10 @@ KNOTE_LOCKED(&d->bd_sel.si_note, 0); } +/* + * XXXRW: What about races against bpf_timed_out() in close? Need to ensure + * that (d) is still valid. + */ static void bpf_timed_out(arg) void *arg; @@ -559,6 +580,11 @@ struct sockaddr dst; int datlen; + /* + * XXXRW: Need to hold some sort of lock here to prevent races with a + * change of bd_bif? We might want to cache a bunch of these values + * locally so we don't keep dereferencing (d). + */ if (d->bd_bif == NULL) return (ENXIO); @@ -583,6 +609,10 @@ mac_create_mbuf_from_bpfdesc(d, m); BPFD_UNLOCK(d); #endif + /* + * XXXRW: How do we know that (ifp) is still valid here, and also + * still the right type and same interface we had before? + */ NET_LOCK_GIANT(); error = (*ifp->if_output)(ifp, m, &dst, NULL); NET_UNLOCK_GIANT(); @@ -645,6 +675,10 @@ struct bpf_d *d = dev->si_drv1; int error = 0; + /* + * XXXRW: This is a bit odd. Does this properly account for possible + * multiple simultaenous access from more than one thread/process? + */ BPFD_LOCK(d); if (d->bd_state == BPF_WAITING) callout_stop(&d->bd_callout); @@ -678,6 +712,10 @@ { struct ifnet *ifp; + /* + * XXXRW: need to make sure ther interface doesn't + * change during this operation? + */ if (d->bd_bif == NULL) error = EINVAL; else { @@ -698,6 +736,11 @@ * Set buffer length. */ case BIOCSBLEN: + /* + * XXXRW: Probably ought to hold a mutex over the test of + * bd_bif and the set of bd_bufsize to prevent test-and-set + * races. + */ if (d->bd_bif != NULL) error = EINVAL; else { @@ -731,6 +774,12 @@ * Put interface into promiscuous mode. */ case BIOCPROMISC: + /* + * XXXRW: Some atomic test-and-set issues here -- need to + * prevent bd_bif changing simultaneously, as well as deal + * with the call to ifpromisc() safely. Can ifpromisc() + * sleep? + */ if (d->bd_bif == NULL) { /* * No interface attached yet. @@ -751,6 +800,10 @@ * Get current data link type. */ case BIOCGDLT: + /* + * XXXRW: Need to hold a mutex over test and then use of + * bd_bif. + */ if (d->bd_bif == NULL) error = EINVAL; else @@ -761,6 +814,10 @@ * Get a list of supported data link types. */ case BIOCGDLTLIST: + /* + * XXXRW: Need to hold a mutex over test of bd_bif and then + * pass into bpf_getdltlist() to prevent races on bd_bif? + */ if (d->bd_bif == NULL) error = EINVAL; else @@ -771,6 +828,10 @@ * Set data link type. */ case BIOCSDLT: + /* + * XXXRW: Need to hold a mutex over test of bd_bif and then + * pass into bpf_setfltlist() to prevent races on bd_bif? + */ if (d->bd_bif == NULL) error = EINVAL; else @@ -781,6 +842,10 @@ * Get interface name. */ case BIOCGETIF: + /* + * XXXRW: Need to hold a mutex over test of bd_bif and then + * dereferencing of it. + */ if (d->bd_bif == NULL) error = EINVAL; else { @@ -822,6 +887,10 @@ { struct timeval *tv = (struct timeval *)addr; + /* + * XXXRW: Need to hold a mutex over multiple reads to + * prevent inconsistency. + */ tv->tv_sec = d->bd_rtout / hz; tv->tv_usec = (d->bd_rtout % hz) * tick; break; @@ -834,6 +903,10 @@ { struct bpf_stat *bs = (struct bpf_stat *)addr; + /* + * XXXRW: Need to hold a mutex over multiple reads to + * prevent inconsistency. + */ bs->bs_recv = d->bd_rcount; bs->bs_drop = d->bd_dcount; break; @@ -891,20 +964,36 @@ break; case FIOSETOWN: + /* + * XXXRW: This is protected by sigio locking at the next + * layer down, right? + */ error = fsetown(*(int *)addr, &d->bd_sigio); break; case FIOGETOWN: + /* + * XXXRW: This is protected by sigio locking at the next + * layer down, right? + */ *(int *)addr = fgetown(&d->bd_sigio); break; /* This is deprecated, FIOSETOWN should be used instead. */ case TIOCSPGRP: + /* + * XXXRW: This is protected by sigio locking at the next + * layer down, right? + */ error = fsetown(-(*(int *)addr), &d->bd_sigio); break; /* This is deprecated, FIOGETOWN should be used instead. */ case TIOCGPGRP: + /* + * XXXRW: This is protected by sigio locking at the next + * layer down, right? + */ *(int *)addr = -fgetown(&d->bd_sigio); break; @@ -930,6 +1019,10 @@ /* * Set d's packet filter program to fp. If this file already has a filter, * free it and replace it. Returns EINVAL for bogus requests. + * + * XXXRW: There seem to be a number of races in here. Perhaps the copyin and + * malloc should be done up front to make it easier to hold the lock over the + * duration? */ static int bpf_setf(d, fp) @@ -986,6 +1079,10 @@ int error; struct ifnet *theywant; + /* + * XXXRW: While this might be more efficient, it does open the door + * for more racing. + */ theywant = ifunit(ifr->ifr_name); if (theywant == NULL) return ENXIO; @@ -1014,6 +1111,10 @@ if ((ifp->if_flags & IFF_UP) == 0) return (ENETDOWN); + /* + * XXXRW: This section appears to be a bit racey. Should it + * be protected by the descriptor mutex? + */ if (d->bd_sbuf == NULL) { error = bpf_allocbufs(d); if (error != 0) @@ -1053,6 +1154,10 @@ struct bpf_d *d; int revents; + /* + * XXXRW: mutex should probably be grabbed before the test of bd_bif + * or we might race. + */ d = dev->si_drv1; if (d->bd_bif == NULL) return (ENXIO); @@ -1090,6 +1195,10 @@ if (kn->kn_filter != EVFILT_READ) return (1); + /* + * XXXRW: Shouldn't we hold the bpf descriptor lock over this? Or + * maybe not. + */ kn->kn_fop = &bpfread_filtops; kn->kn_hook = d; knlist_add(&d->bd_sel.si_note, kn, 0); @@ -1103,6 +1212,9 @@ { struct bpf_d *d = (struct bpf_d *)kn->kn_hook; + /* + * XXXRW: Anything extra locking-wise need to happen here? + */ knlist_remove(&d->bd_sel.si_note, kn, 0); } @@ -1114,6 +1226,9 @@ struct bpf_d *d = (struct bpf_d *)kn->kn_hook; int ready; + /* + * XXXRW: Wow, this locking actually looks mostly correct! + */ BPFD_LOCK(d); ready = bpf_ready(d); if (ready) { @@ -1149,6 +1264,9 @@ /* * Lockless read to avoid cost of locking the interface if there are * no descriptors attached. + * + * XXXRW: Is there a risk that the (bp) pointer here might become + * invalid during or before dereference? */ if (LIST_EMPTY(&bp->bif_dlist)) return; @@ -1210,6 +1328,8 @@ /* * Lockless read to avoid cost of locking the interface if there are * no descriptors attached. + * + * XXXRW: See bpf_tap() comment. */ if (LIST_EMPTY(&bp->bif_dlist)) return; @@ -1256,6 +1376,8 @@ /* * Lockless read to avoid cost of locking the interface if there are * no descriptors attached. + * + * XXXRW: See bpf_tap() comment. */ if (LIST_EMPTY(&bp->bif_dlist)) return; @@ -1273,6 +1395,10 @@ BPFIF_LOCK(bp); LIST_FOREACH(d, &bp->bif_dlist, bd_next) { + /* + * XXXRW: For test-and-set reasons, shouldn't bpf descriptor + * be locked *before* this test? + */ if (!d->bd_seesent && (m->m_pkthdr.rcvif == NULL)) continue; BPFD_LOCK(d); @@ -1305,7 +1431,12 @@ { struct bpf_hdr *hp; int totlen, curlen; - int hdrlen = d->bd_bif->bif_hdrlen; + int hdrlen; + + /* + * XXXRW: Assert bpf descriptor lock here? + */ + hdrlen = d->bd_bif->bif_hdrlen; /* * Figure out how many bytes to move. If the packet is @@ -1363,6 +1494,9 @@ /* * Initialize all nonzero fields of a descriptor. + * + * XXXRW: Shouldn't some sort of locking be going on here? This can be + * called "live" by an ioctl(). */ static int bpf_allocbufs(d) @@ -1443,6 +1577,11 @@ bp->bif_dlt = dlt; mtx_init(&bp->bif_mtx, "bpf interface lock", NULL, MTX_DEF); + /* + * XXXRW: Seems like this should really happen down at the bottom to + * avoid exposing a mostly initialized bpf interface structure to the + * world. + */ mtx_lock(&bpf_mtx); LIST_INSERT_HEAD(&bpf_iflist, bp, bif_next); mtx_unlock(&bpf_mtx); @@ -1474,6 +1613,12 @@ struct bpf_if *bp; struct bpf_d *d; + /* + * XXXRW: Check to make sure races between this code and other code + * aren't too extreme. In particular, that other code doesn't assume + * that a descriptor will point to an interface in the global + * interface list? + */ /* Locate BPF interface information */ mtx_lock(&bpf_mtx); LIST_FOREACH(bp, &bpf_iflist, bif_next) { @@ -1492,6 +1637,12 @@ mtx_unlock(&bpf_mtx); while ((d = LIST_FIRST(&bp->bif_dlist)) != NULL) { + /* + * XXXRW: Should bpf descriptor lock be edged above the + * detach call? How come bpf_detachd() doesn't always + * perform a wakeup? Once detached, don't we risk a race with + * wakeup()? + */ bpf_detachd(d); BPFD_LOCK(d); bpf_wakeup(d); @@ -1514,6 +1665,9 @@ struct ifnet *ifp; struct bpf_if *bp; + /* + * XXXRW: Assert descriptor lock here? Why can't we use d->bd_bif? + */ ifp = d->bd_bif->bif_ifp; n = 0; error = 0; @@ -1548,6 +1702,10 @@ struct ifnet *ifp; struct bpf_if *bp; + /* + * XXXRW: Should acquire mutex earlier to prevent the ifnet from + * changing out from under us? + */ if (d->bd_bif->bif_dlt == dlt) return (0); ifp = d->bd_bif->bif_ifp; @@ -1557,6 +1715,9 @@ break; } mtx_unlock(&bpf_mtx); + /* + * XXXRW: Dropping the mutex here might open up a race? + */ if (bp != NULL) { opromisc = d->bd_promisc; bpf_detachd(d); --- //depot/vendor/freebsd/src/sys/net/if.c 2004/11/30 22:40:38 +++ //depot/user/rwatson/netperf/sys/net/if.c 2004/12/04 17:43:45 @@ -265,6 +265,14 @@ if_clone_init(); } +/* + * Grow the global ifindex_table by a power of two over the current side. + * Note that this call assumes ifindex_table (and hence the if_indexentry + * entries in it) can be relocated in memory without causing a problem. + * + * XXXRW: Locking assertion needed here? What protects against untimely + * indirection through ifindex_table? Note M_WAITOK. + */ static void if_grow(void) { @@ -281,6 +289,10 @@ ifindex_table = e; } +/* + * if_check() scans the list of registered network interfaces to check that + * initialization-time invariants are met. + */ /* ARGSUSED*/ static void if_check(void *dummy __unused) @@ -307,6 +319,19 @@ if_slowtimo(0); } +/* + * if_findindex ... XXXRW: what? + * + * Given a partially initialized ifnet reference, identify an available ifnet + * unit number that can be assigned to it. Try searching for any user + * preferences or prior unit allocations; if there's no match or a collision, + * iterate through searching for the next available unit. Note that the + * returned unit can be beyond the end of the index array, requiring the + * caller to extend the array by calling if_grow(). + * + * XXXRW: Note that no reservation takes place, so as soon as the caller + * releases the lock, the unit returned may be reused. + */ static int if_findindex(struct ifnet *ifp) { @@ -314,12 +339,18 @@ char eaddr[18], devname[32]; const char *name, *p; + IFNET_WASSERT(); + switch (ifp->if_type) { case IFT_ETHER: /* these types use struct arpcom */ case IFT_FDDI: case IFT_XETHER: case IFT_ISO88025: case IFT_L2VLAN: + /* + * XXXRW: Assumes that arpcom always has its ifnet at the + * head of its structure. + */ snprintf(eaddr, 18, "%6D", IFP2AC(ifp)->ac_enaddr, ":"); break; default: @@ -368,12 +399,13 @@ struct sockaddr_dl *sdl; struct ifaddr *ifa; + /* + * XXXRW: Shouldn't we add to the global list only once the ifnet + * is ready for use? + */ TASK_INIT(&ifp->if_starttask, 0, if_start_deferred, ifp); IF_AFDATA_LOCK_INIT(ifp); ifp->if_afdata_initialized = 0; - IFNET_WLOCK(); - TAILQ_INSERT_TAIL(&ifnet, ifp, if_link); - IFNET_WUNLOCK(); /* * XXX - * The old code would work if the interface passed a pre-existing @@ -393,11 +425,14 @@ mac_create_ifnet(ifp); #endif + IFNET_WLOCK(); + TAILQ_INSERT_TAIL(&ifnet, ifp, if_link); ifp->if_index = if_findindex(ifp); if (ifp->if_index > if_index) if_index = ifp->if_index; if (if_index >= if_indexlim) if_grow(); + IFNET_WUNLOCK(); ifp->if_data.ifi_datalen = sizeof(struct if_data); ifnet_byindex(ifp->if_index) = ifp; @@ -474,6 +509,14 @@ SYSINIT(domainifattach, SI_SUB_PROTO_IFATTACHDOMAIN, SI_ORDER_SECOND, if_attachdomain, NULL); +/* + * XXXRW: if_attachdomain1() can only be called on an interface after it has + * an ifindex assigned, as IPv6 assumes that the index is allocated and will + * attempt to use it. + * + * XXXRW: How come we need to check for multiple attachments of domain- + * specific data? + */ static void if_attachdomain1(struct ifnet *ifp) { @@ -1950,10 +1993,38 @@ } int +ifq_handoff(struct ifnet *ifp, struct mbuf *m, int adjust) +{ + int error, len; + short mflags; + + if (ifp->if_start_mbuf != NULL) { + if (ifp->if_start_mbuf(ifp, m, adjust)) + return (0); + else + return (ENOBUFS); + } + + len = m->m_pkthdr.len; + mflags = m->m_flags; + IFQ_ENQUEUE(&ifp->if_snd, m, error); + if (error) + return (error); + ifp->if_obytes += len + adjust; + if (mflags & M_MCAST) + ifp->if_omcasts++; + if ((ifp->if_flags & IFF_OACTIVE) == 0) + if_start(ifp); + return (0); +} + +int if_handoff(struct ifqueue *ifq, struct mbuf *m, struct ifnet *ifp, int adjust) { int active = 0; + if (ifp != NULL && ifp->if_start_mbuf != NULL) + return (ifp->if_start_mbuf(ifp, m, adjust)); IF_LOCK(ifq); if (_IF_QFULL(ifq)) { _IF_DROP(ifq); --- //depot/vendor/freebsd/src/sys/net/if_ethersubr.c 2004/10/12 10:35:23 +++ //depot/user/rwatson/netperf/sys/net/if_ethersubr.c 2004/10/19 18:01:38 @@ -570,9 +570,6 @@ if ((m = bridge_in_ptr(ifp, m)) == NULL) return; - /* First chunk of an mbuf contains good entropy */ - if (harvest.ethernet) - random_harvest(m, 16, 3, 0, RANDOM_NET); ether_demux(ifp, m); } --- //depot/vendor/freebsd/src/sys/net/if_gif.c 2004/07/15 08:30:31 +++ //depot/user/rwatson/netperf/sys/net/if_gif.c 2004/07/15 22:19:17 @@ -89,6 +89,10 @@ * gif_mtx protects the global gif_softc_list. * XXX: Per-softc locking is still required. */ +/* + * XXXRW: Note that gif_mtx only protects global gif-related data, not + * per-softc data. See also netinet/in_gif.c for locking needs. + */ static struct mtx gif_mtx; static MALLOC_DEFINE(M_GIF, "gif", "Generic Tunnel Interface"); static LIST_HEAD(, gif_softc) gif_softc_list; @@ -501,6 +505,9 @@ } /* XXX how should we handle IPv6 scope on SIOC[GS]IFPHYADDR? */ +/* + * XXXRW: per-gif softc locking required. + */ int gif_ioctl(ifp, cmd, data) struct ifnet *ifp; @@ -757,8 +764,10 @@ int s; int error = 0; + /* + * XXXRW: per-gif softc locking required. + */ s = splnet(); - mtx_lock(&gif_mtx); LIST_FOREACH(sc2, &gif_softc_list, gif_list) { if (sc2 == sc) @@ -787,6 +796,9 @@ } mtx_unlock(&gif_mtx); + /* + * XXXRW: Lock gif softc fields. + */ /* XXX we can detach from both, but be polite just in case */ if (sc->gif_psrc) switch (sc->gif_psrc->sa_family) { --- //depot/vendor/freebsd/src/sys/net/if_gre.c 2004/08/05 08:15:37 +++ //depot/user/rwatson/netperf/sys/net/if_gre.c 2004/08/05 21:29:14 @@ -95,7 +95,8 @@ /* * gre_mtx protects all global variables in if_gre.c. - * XXX: gre_softc data not protected yet. + * + * XXXRW: It does not protect softc-specific data. */ struct mtx gre_mtx; static MALLOC_DEFINE(M_GRE, GRENAME, "Generic Routing Encapsulation"); --- //depot/vendor/freebsd/src/sys/net/if_gre.h 2004/03/22 16:06:54 +++ //depot/user/rwatson/netperf/sys/net/if_gre.h 2004/03/22 16:18:59 @@ -54,6 +54,12 @@ WCCP_V2 } wccp_ver_t; +/* + * XXXRW: softc fields need locking. + * + * XXXRW: gre's notion of a 'called' count is not MP-safe, as it assumes + * only one packet can be processed at a time. + */ struct gre_softc { struct ifnet sc_if; LIST_ENTRY(gre_softc) sc_list; --- //depot/vendor/freebsd/src/sys/net/if_sl.c 2004/11/07 14:40:30 +++ //depot/user/rwatson/netperf/sys/net/if_sl.c 2004/11/14 11:53:24 @@ -162,6 +162,7 @@ #define ABT_WINDOW (ABT_COUNT*2+2) /* in seconds - time to count */ static LIST_HEAD(sl_list, sl_softc) sl_list; +static struct mtx slip_mtx; #define FRAME_END 0xc0 /* Frame End */ #define FRAME_ESCAPE 0xdb /* Frame Esc */ @@ -204,6 +205,7 @@ { switch (type) { case MOD_LOAD: + mtx_init(&slip_mtx, "slip_mtx", NULL, MTX_DEF); ldisc_register(SLIPDISC, &slipdisc); LIST_INIT(&sl_list); break; @@ -225,6 +227,7 @@ DECLARE_MODULE(if_sl, sl_mod, SI_SUB_PSEUDO, SI_ORDER_ANY); +/* Locked using slip_mtx. */ static int *st_unit_list; static size_t st_unit_max = 0; @@ -233,6 +236,7 @@ { struct sl_softc *nc; + mtx_assert(&slip_mtx, MA_OWNED); LIST_FOREACH(nc, &sl_list, sl_next) { if (nc->sc_if.if_dunit == unit) return (0); @@ -256,6 +260,7 @@ { size_t i; + mtx_assert(&slip_mtx, MA_OWNED); for (i = 0; i < st_unit_max; i++) if (st_unit_list[i] == unit) return 1; @@ -267,6 +272,7 @@ { int *t; + mtx_assert(&slip_mtx, MA_OWNED); if (slisstatic(unit)) return; @@ -330,10 +336,12 @@ sc->sc_if.if_linkmib = sc; sc->sc_if.if_linkmiblen = sizeof *sc; mtx_init(&sc->sc_fastq.ifq_mtx, "sl_fastq", NULL, MTX_DEF); + mtx_init(&sc->sc_mtx, "slip sc_mtx", NULL, MTX_DEF); /* * Find a suitable unit number. */ + mtx_lock(&slip_mtx); for (unit=0; ; unit++) { if (slisstatic(unit)) continue; @@ -343,6 +351,7 @@ } if_initname(&sc->sc_if, "sl", unit); LIST_INSERT_HEAD(&sl_list, sc, sl_next); + mtx_unlock(&slip_mtx); if_attach(&sc->sc_if); bpfattach(&sc->sc_if, DLT_SLIP, SLIP_HDRLEN); @@ -399,10 +408,20 @@ static void sldestroy(struct sl_softc *sc) { + + /* + * XXXRW: Slight race here: we may detach bpf/if before we + * attach. This appears to be a property of the unit selection + * process, which might be better handled by the interface + * cloning subsystem? + */ bpfdetach(&sc->sc_if); if_detach(&sc->sc_if); + mtx_lock(&slip_mtx); LIST_REMOVE(sc, sl_next); + mtx_unlock(&slip_mtx); m_free(sc->sc_mbuf); + mtx_destroy(&sc->sc_mtx); mtx_destroy(&sc->sc_fastq.ifq_mtx); if (sc->bpfbuf) free(sc->bpfbuf, M_SL); @@ -429,6 +448,10 @@ clist_free_cblocks(&tp->t_outq); sc = sl_for_tty(tp); if (sc != NULL) { + /* + * XXXRW: tear-down race between timeout and slclose()? + */ + mtx_lock(&sc->sc_mtx); if (sc->sc_outfill) { sc->sc_outfill = 0; untimeout(sl_outfill, sc, sc->sc_ofhandle); @@ -437,6 +460,7 @@ sc->sc_keepalive = 0; untimeout(sl_keepalive, sc, sc->sc_kahandle); } + mtx_unlock(&sc->sc_mtx); if_down(&sc->sc_if); sc->sc_ttyp = NULL; sldestroy(sc); @@ -469,12 +493,21 @@ splx(s); return (ENXIO); } + /* + * XXXRW: we hold the mutex over all of this to protect + * the unit change and global list consistency. However, + * some of these functions probably sleep, making this + * wrong. If we have to support renumbering, we probably + * need a way to reserve both numbers to prevent them + * from being reused during the change, or a way to sleep + * waiting for a change to end (i.e., a CV). + */ + mtx_lock(&slip_mtx); if (sc->sc_if.if_dunit != unit) { if (!slisunitfree(unit)) { - splx(s); + mtx_unlock(&slip_mtx); return (ENXIO); } - wasup = sc->sc_if.if_flags & IFF_UP; bpfdetach(&sc->sc_if); if_detach(&sc->sc_if); @@ -492,9 +525,11 @@ SLIP_HIWAT + 2 * sc->sc_if.if_mtu + 1); } slmarkstatic(unit); + mtx_unlock(&slip_mtx); break; case SLIOCSKEEPAL: + mtx_lock(&sc->sc_mtx); sc->sc_keepalive = *(u_int *)data * hz; if (sc->sc_keepalive) { sc->sc_flags |= SC_KEEPALIVE; @@ -506,6 +541,7 @@ sc->sc_flags &= ~SC_KEEPALIVE; } } + mtx_unlock(&sc->sc_mtx); break; case SLIOCGKEEPAL: @@ -513,6 +549,7 @@ break; case SLIOCSOUTFILL: + mtx_lock(&sc->sc_mtx); sc->sc_outfill = *(u_int *)data * hz; if (sc->sc_outfill) { sc->sc_flags |= SC_OUTWAIT; @@ -524,9 +561,11 @@ sc->sc_flags &= ~SC_OUTWAIT; } } + mtx_unlock(&sc->sc_mtx); break; case SLIOCGOUTFILL: + /* Unlocked read. */ *(int *)data = sc->sc_outfill / hz; break; @@ -601,9 +640,10 @@ } /* - * Start output on interface. Get another datagram - * to send from the interface queue and map it to - * the interface before starting output. + * Start output on interface. Get another datagram to send from the + * interface queue and map it to the interface before starting output. Due + * to IFF_NEEDSGIANT, this code runs with Giant and can safely interface + * with the tty code. */ static int sltstart(struct tty *tp) @@ -626,8 +666,11 @@ (*tp->t_oproc)(tp); if (tp->t_outq.c_cc != 0) { - if (sc != NULL) + if (sc != NULL) { + mtx_lock(&sc->sc_mtx); sc->sc_flags &= ~SC_OUTWAIT; + mtx_unlock(&sc->sc_mtx); + } if (tp->t_outq.c_cc > SLIP_HIWAT) return 0; } @@ -657,6 +700,7 @@ * queueing, and the connection id compression will get * munged when this happens. */ + mtx_lock(&sc->sc_mtx); if (sc->sc_if.if_bpf) { /* * We need to save the TCP/IP header before it's @@ -687,9 +731,10 @@ } ip = mtod(m, struct ip *); if (ip->ip_v == IPVERSION && ip->ip_p == IPPROTO_TCP) { - if (sc->sc_if.if_flags & SC_COMPRESS) + if (sc->sc_if.if_flags & SC_COMPRESS) { *mtod(m, u_char *) |= sl_compress_tcp(m, ip, &sc->sc_comp, 1); + } } if (sc->sc_if.if_bpf && sc->bpfbuf) { /* @@ -701,6 +746,7 @@ bcopy(mtod(m, caddr_t), &sc->bpfbuf[SLX_CHDR], CHDR_LEN); BPF_TAP(&sc->sc_if, sc->bpfbuf, len + SLIP_HDRLEN); } + mtx_unlock(&sc->sc_mtx); /* * If system is getting low on clists, just flush our @@ -716,7 +762,9 @@ continue; } + mtx_lock(&sc->sc_mtx); sc->sc_flags &= ~SC_OUTWAIT; + mtx_unlock(&sc->sc_mtx); /* * The extra FRAME_END will start up a new packet, and thus * will flush any accumulated garbage. We do this whenever @@ -804,6 +852,8 @@ { struct mbuf *m, *newm; + mtx_assert(&sc->sc_mtx, MA_OWNED); + MGETHDR(m, M_DONTWAIT, MT_DATA); if (m == NULL) return (NULL); @@ -857,13 +907,16 @@ if (sc == NULL) return 0; if (c & TTY_ERRORMASK || (tp->t_state & TS_CONNECTED) == 0) { + mtx_lock(&sc->sc_mtx); sc->sc_flags |= SC_ERROR; + mtx_unlock(&sc->sc_mtx); return 0; } c &= TTY_CHARMASK; ++sc->sc_if.if_ibytes; + mtx_lock(&sc->sc_mtx); if (sc->sc_if.if_flags & IFF_DEBUG) { if (c == ABT_ESC) { /* @@ -882,6 +935,7 @@ sc->sc_starttime = time_second; if (sc->sc_abortcount >= ABT_COUNT) { slclose(tp,0); + mtx_unlock(&sc->sc_mtx); return 0; } } @@ -904,6 +958,7 @@ case FRAME_ESCAPE: sc->sc_escape = 1; + mtx_unlock(&sc->sc_mtx); return 0; case FRAME_END: @@ -988,6 +1043,7 @@ if (sc->sc_mp < sc->sc_ep) { *sc->sc_mp++ = c; sc->sc_escape = 0; + mtx_unlock(&sc->sc_mtx); return 0; } @@ -999,6 +1055,7 @@ newpack: sc->sc_mp = sc->sc_buf = sc->sc_ep - SLRMAX; sc->sc_escape = 0; + mtx_unlock(&sc->sc_mtx); return 0; } @@ -1078,6 +1135,7 @@ { struct sl_softc *sc = chan; + mtx_lock(&sc->sc_mtx); if (sc->sc_keepalive) { if (sc->sc_flags & SC_KEEPALIVE) { if (sc->sc_ttyp->t_pgrp != NULL) { @@ -1091,6 +1149,7 @@ } else { sc->sc_flags &= ~SC_KEEPALIVE; } + mtx_unlock(&sc->sc_mtx); } static void @@ -1100,6 +1159,7 @@ register struct tty *tp = sc->sc_ttyp; int s; + mtx_lock(&sc->sc_mtx); if (sc->sc_outfill && tp != NULL) { if (sc->sc_flags & SC_OUTWAIT) { s = splimp (); @@ -1113,4 +1173,5 @@ } else { sc->sc_flags &= ~SC_OUTWAIT; } + mtx_unlock(&sc->sc_mtx); } --- //depot/vendor/freebsd/src/sys/net/if_slvar.h 2004/04/07 20:52:05 +++ //depot/user/rwatson/netperf/sys/net/if_slvar.h 2004/08/06 22:33:57 @@ -34,13 +34,16 @@ #ifndef _NET_IF_SLVAR_H_ #define _NET_IF_SLVAR_H_ +#include #include /* * Definitions for SLIP interface data structures * * (This exists so programs like slstats can get at the definition - * of sl_softc.) + * of sl_softc.) Fields owned by the SLIP subsystem are protected + * using sc_mtx, with the exception of sc_next, which is protected + * by the global slip_mtx. */ struct sl_softc { struct ifnet sc_if; /* network-visible interface */ @@ -66,6 +69,7 @@ struct slcompress sc_comp; /* tcp compression data */ LIST_ENTRY(sl_softc) sl_next; u_char *bpfbuf; /* hang buffer for bpf here */ + struct mtx sc_mtx; }; /* internal flags */ --- //depot/vendor/freebsd/src/sys/net/if_spppsubr.c 2004/08/27 18:35:34 +++ //depot/user/rwatson/netperf/sys/net/if_spppsubr.c 2004/08/28 14:27:28 @@ -92,12 +92,8 @@ #include #if defined(__FreeBSD__) && __FreeBSD__ >= 3 -# define UNTIMEOUT(fun, arg, handle) untimeout(fun, arg, handle) -# define TIMEOUT(fun, arg1, arg2, handle) handle = timeout(fun, arg1, arg2) # define IOCTL_CMD_T u_long #else -# define UNTIMEOUT(fun, arg, handle) untimeout(fun, arg) -# define TIMEOUT(fun, arg1, arg2, handle) timeout(fun, arg1, arg2) # define IOCTL_CMD_T int #endif @@ -259,10 +255,11 @@ void (*scr)(struct sppp *sp); }; +struct mtx sppp_mtx; +MTX_SYSINIT(sppp_mtx, &sppp_mtx, "sppp_mtx", MTX_DEF); + static struct sppp *spppq; -#if defined(__FreeBSD__) && __FreeBSD__ >= 3 -static struct callout_handle keepalive_ch; -#endif +static struct callout keepalive_callout; #if defined(__FreeBSD__) && __FreeBSD__ >= 3 && __FreeBSD_version < 501113 #define SPP_FMT "%s%d: " @@ -964,13 +961,18 @@ { struct sppp *sp = (struct sppp*) ifp; + mtx_lock(&sppp_mtx); /* Initialize keepalive handler. */ - if (spppq == NULL) - TIMEOUT(sppp_keepalive, 0, hz * 10, keepalive_ch); + if (spppq == NULL) { + callout_init(&keepalive_callout, 0); + callout_reset(&keepalive_callout, hz * 10, sppp_keepalive, + NULL); + } /* Insert new entry into the keepalive list. */ sp->pp_next = spppq; spppq = sp; + mtx_unlock(&sppp_mtx); sp->pp_if.if_mtu = PP_MTU; sp->pp_if.if_flags = IFF_POINTOPOINT | IFF_MULTICAST; @@ -1016,6 +1018,7 @@ struct sppp **q, *p, *sp = (struct sppp*) ifp; int i; + mtx_lock(&sppp_mtx); /* Remove the entry from the keepalive list. */ for (q = &spppq; (p = *q); q = &p->pp_next) if (p == sp) { @@ -1025,11 +1028,12 @@ /* Stop keepalive handler. */ if (spppq == NULL) - UNTIMEOUT(sppp_keepalive, 0, keepalive_ch); + callout_stop(&keepalive_callout); + mtx_unlock(&sppp_mtx); for (i = 0; i < IDX_COUNT; i++) - UNTIMEOUT((cps[i])->TO, (void *)sp, sp->ch[i]); - UNTIMEOUT(sppp_pap_my_TO, (void *)sp, sp->pap_my_to_ch); + untimeout((cps[i])->TO, (void *)sp, sp->ch[i]); + untimeout(sppp_pap_my_TO, (void *)sp, sp->pap_my_to_ch); mtx_destroy(&sp->pp_cpq.ifq_mtx); mtx_destroy(&sp->pp_fastq.ifq_mtx); } @@ -2008,8 +2012,8 @@ case STATE_STOPPING: sppp_cp_send(sp, cp->proto, TERM_REQ, ++sp->pp_seq[cp->protoidx], 0, 0); - TIMEOUT(cp->TO, (void *)sp, sp->lcp.timeout, - sp->ch[cp->protoidx]); + sp->ch[cp->protoidx] = timeout(cp->TO, (void *)sp, + sp->lcp.timeout); break; case STATE_REQ_SENT: case STATE_ACK_RCVD: @@ -2019,8 +2023,8 @@ break; case STATE_ACK_SENT: (cp->scr)(sp); - TIMEOUT(cp->TO, (void *)sp, sp->lcp.timeout, - sp->ch[cp->protoidx]); + sp->ch[cp->protoidx] = timeout(cp->TO, (void *)sp, + sp->lcp.timeout); break; } @@ -2036,7 +2040,7 @@ { sp->state[cp->protoidx] = newstate; - UNTIMEOUT(cp->TO, (void *)sp, sp->ch[cp->protoidx]); + untimeout(cp->TO, (void *)sp, sp->ch[cp->protoidx]); switch (newstate) { case STATE_INITIAL: case STATE_STARTING: @@ -2049,8 +2053,8 @@ case STATE_REQ_SENT: case STATE_ACK_RCVD: case STATE_ACK_SENT: - TIMEOUT(cp->TO, (void *)sp, sp->lcp.timeout, - sp->ch[cp->protoidx]); + sp->ch[cp->protoidx] = timeout(cp->TO, (void *)sp, + sp->lcp.timeout); break; } } @@ -4147,7 +4151,7 @@ * a number between 300 and 810 seconds. */ i = 300 + ((unsigned)(random() & 0xff00) >> 7); - TIMEOUT(chap.TO, (void *)sp, i * hz, sp->ch[IDX_CHAP]); + sp->ch[IDX_CHAP] = timeout(chap.TO, (void *)sp, i * hz); } if (debug) { @@ -4191,7 +4195,7 @@ if (debug) log(LOG_DEBUG, SPP_FMT "chap tld\n", SPP_ARGS(ifp)); - UNTIMEOUT(chap.TO, (void *)sp, sp->ch[IDX_CHAP]); + untimeout(chap.TO, (void *)sp, sp->ch[IDX_CHAP]); sp->lcp.protos &= ~(1 << IDX_CHAP); lcp.Close(sp); @@ -4327,7 +4331,7 @@ /* ack and nak are his authproto */ case PAP_ACK: - UNTIMEOUT(sppp_pap_my_TO, (void *)sp, sp->pap_my_to_ch); + untimeout(sppp_pap_my_TO, (void *)sp, sp->pap_my_to_ch); if (debug) { log(LOG_DEBUG, SPP_FMT "pap success", SPP_ARGS(ifp)); @@ -4356,7 +4360,7 @@ break; case PAP_NAK: - UNTIMEOUT(sppp_pap_my_TO, (void *)sp, sp->pap_my_to_ch); + untimeout(sppp_pap_my_TO, (void *)sp, sp->pap_my_to_ch); if (debug) { log(LOG_INFO, SPP_FMT "pap failure", SPP_ARGS(ifp)); @@ -4413,8 +4417,8 @@ if (sp->myauth.proto == PPP_PAP) { /* we are peer, send a request, and start a timer */ pap.scr(sp); - TIMEOUT(sppp_pap_my_TO, (void *)sp, sp->lcp.timeout, - sp->pap_my_to_ch); + sp->pap_my_to_ch = timeout(sppp_pap_my_TO, (void *)sp, + sp->lcp.timeout); } } @@ -4517,8 +4521,8 @@ if (debug) log(LOG_DEBUG, SPP_FMT "pap tld\n", SPP_ARGS(ifp)); - UNTIMEOUT(pap.TO, (void *)sp, sp->ch[IDX_PAP]); - UNTIMEOUT(sppp_pap_my_TO, (void *)sp, sp->pap_my_to_ch); + untimeout(pap.TO, (void *)sp, sp->ch[IDX_PAP]); + untimeout(sppp_pap_my_TO, (void *)sp, sp->pap_my_to_ch); sp->lcp.protos &= ~(1 << IDX_PAP); lcp.Close(sp); @@ -4645,7 +4649,12 @@ struct sppp *sp; int s; + /* + * XXXRW: It would be nice to avoid calling all this stuff while + * holding sppp_mtx, or we risk lock order reversals. + */ s = splimp(); + mtx_lock(&sppp_mtx); for (sp=spppq; sp; sp=sp->pp_next) { struct ifnet *ifp = &sp->pp_if; @@ -4684,8 +4693,9 @@ sp->lcp.echoid, 4, &nmagic); } } + callout_reset(&keepalive_callout, hz * 10, sppp_keepalive, NULL); + mtx_unlock(&sppp_mtx); splx(s); - TIMEOUT(sppp_keepalive, 0, hz * 10, keepalive_ch); } /* --- //depot/vendor/freebsd/src/sys/net/if_stf.c 2004/07/15 08:30:31 +++ //depot/user/rwatson/netperf/sys/net/if_stf.c 2004/07/15 22:19:17 @@ -139,13 +139,11 @@ #define sc_ro __sc_ro46.__sc_ro4 const struct encaptab *encap_cookie; LIST_ENTRY(stf_softc) sc_list; /* all stf's are linked */ + struct mtx sc_mtx; /* protect sc_ro */ }; /* * All mutable global variables in if_stf.c are protected by stf_mtx. - * XXXRW: Note that mutable fields in the softc are not currently locked: - * in particular, sc_ro needs to be protected from concurrent entrance - * of stf_output(). */ static struct mtx stf_mtx; static LIST_HEAD(, stf_softc) stf_softc_list; @@ -231,6 +229,7 @@ ifc_free_unit(ifc, unit); return (ENOMEM); } + mtx_init(&sc->sc_mtx, "stf sc_mtx", NULL, MTX_DEF); ifp->if_mtu = IPV6_MMTU; ifp->if_ioctl = stf_ioctl; @@ -255,6 +254,7 @@ bpfdetach(&sc->sc_if); if_detach(&sc->sc_if); + mtx_destroy(&sc->sc_mtx); free(sc, M_STF); } @@ -430,9 +430,10 @@ struct ip *ip; struct ip6_hdr *ip6; struct in6_ifaddr *ia6; -#ifdef MAC + struct route ro; int error; +#ifdef MAC error = mac_check_ifnet_transmit(ifp, m); if (error) { m_freem(m); @@ -534,9 +535,7 @@ else ip_ecn_ingress(ECN_NOCARE, &ip->ip_tos, &tos); - /* - * XXXRW: Locking of sc_ro required. - */ + mtx_lock(&sc->sc_mtx); dst4 = (struct sockaddr_in *)&sc->sc_ro.ro_dst; if (dst4->sin_family != AF_INET || bcmp(&dst4->sin_addr, &ip->ip_dst, sizeof(ip->ip_dst)) != 0) { @@ -555,12 +554,21 @@ if (sc->sc_ro.ro_rt == NULL) { m_freem(m); ifp->if_oerrors++; + mtx_unlock(&sc->sc_mtx); return ENETUNREACH; } } + /* + * XXXRW: Holding mutex over call to ip_output(): potential lock + * order issue? Hard to resolve cleanly with the current route + * caching model, as we have to synchronize access to shared softc + * state. + */ ifp->if_opackets++; - return ip_output(m, NULL, &sc->sc_ro, 0, NULL, NULL); + error = ip_output(m, NULL, &ro, 0, NULL, NULL); + mtx_unlock(&sc->sc_mtx); + return (error); } static int --- //depot/vendor/freebsd/src/sys/net/if_tap.c 2004/10/31 17:40:43 +++ //depot/user/rwatson/netperf/sys/net/if_tap.c 2004/11/03 14:10:39 @@ -113,6 +113,10 @@ * All global variables in if_tap.c are locked with tapmtx, with the * exception of tapdebug, which is accessed unlocked; tapclones is * static at runtime. + * + * XXXRW: si_flags appears not to be protected from concurrent access, + * and is written at run-time. + * XXXRW: si_drv1 is also used for test-and-set, and isn't synchronized. */ static struct mtx tapmtx; static int tapdebug = 0; /* debug flag */ @@ -162,6 +166,7 @@ * The EBUSY algorithm here can't quite atomically * guarantee that this is race-free since we have to * release the tap mtx to deregister the clone handler. + * XXXRW: is this true? */ mtx_lock(&tapmtx); SLIST_FOREACH(tp, &taphead, tap_next) { @@ -701,6 +706,7 @@ case SIOCSIFADDR: /* set MAC address of the remote side */ mtx_lock(&tp->tap_mtx); + /* XXXRW: Does this actually do anything? */ bcopy(data, tp->ether_addr, sizeof(tp->ether_addr)); mtx_unlock(&tp->tap_mtx); break; @@ -755,6 +761,7 @@ if (flag & IO_NDELAY) return (EWOULDBLOCK); + /* This looks like a wanna-be condition variable. */ mtx_lock(&tp->tap_mtx); tp->tap_flags |= TAP_RWAIT; mtx_unlock(&tp->tap_mtx); --- //depot/vendor/freebsd/src/sys/net/if_tun.c 2004/10/31 17:40:43 +++ //depot/user/rwatson/netperf/sys/net/if_tun.c 2004/11/03 14:10:39 @@ -59,6 +59,12 @@ * tun_list is protected by global tunmtx. Other mutable fields are * protected by tun->tun_mtx, or by their owning subsystem. tun_dev is * static for the duration of a tunnel interface. + * + * XXXRW: we allocate si_drv1 for the dev_t on demand, rather than when + * the dev_t is instantiated. Nothing serializes the test/set of that + * field. + * + * XXXRW: what serializes access to si_flags? */ struct tun_softc { TAILQ_ENTRY(tun_softc) tun_list; @@ -121,6 +127,9 @@ static d_ioctl_t tunioctl; static d_poll_t tunpoll; +/* + * XXXRW: can remove D_NEEDGIANT? Probably not because of si_drv1 for now. + */ static struct cdevsw tun_cdevsw = { .d_version = D_VERSION, .d_flags = D_PSEUDO | D_NEEDGIANT, @@ -382,6 +391,9 @@ ifp->if_flags |= IFF_UP | IFF_RUNNING; getmicrotime(&ifp->if_lastchange); + /* + * XXXRW: interface locking. + */ for (ifa = TAILQ_FIRST(&ifp->if_addrhead); ifa; ifa = TAILQ_NEXT(ifa, ifa_link)) { if (ifa->ifa_addr == NULL) --- //depot/vendor/freebsd/src/sys/net/if_var.h 2004/11/09 21:30:37 +++ //depot/user/rwatson/netperf/sys/net/if_var.h 2004/11/15 11:58:11 @@ -175,7 +175,8 @@ (void *); int (*if_resolvemulti) /* validate/resolve multicast */ (struct ifnet *, struct sockaddr **, struct sockaddr *); - void *if_spare1; /* spare pointer 1 */ + int (*if_start_mbuf) /* Hand off mbuf to driver for send */ + (struct ifnet *, struct mbuf *, int); void *if_spare2; /* spare pointer 2 */ void *if_spare3; /* spare pointer 3 */ u_int if_spare_flags1; /* spare flags 1 */ @@ -284,9 +285,12 @@ } while (0) #define IF_DEQUEUE(ifq, m) do { \ - IF_LOCK(ifq); \ - _IF_DEQUEUE(ifq, m); \ - IF_UNLOCK(ifq); \ + if ((ifq)->ifq_head != NULL) { \ + IF_LOCK(ifq); \ + _IF_DEQUEUE(ifq, m); \ + IF_UNLOCK(ifq); \ + } else \ + (m) = NULL; \ } while (0) #define _IF_POLL(ifq, m) ((m) = (ifq)->ifq_head) @@ -303,9 +307,11 @@ } while (0) #define IF_DRAIN(ifq) do { \ - IF_LOCK(ifq); \ - _IF_DRAIN(ifq); \ - IF_UNLOCK(ifq); \ + if ((ifq)->ifq_head != NULL) { \ + IF_LOCK(ifq); \ + _IF_DRAIN(ifq); \ + IF_UNLOCK(ifq); \ + } \ } while(0) #ifdef _KERNEL @@ -425,22 +431,11 @@ #define IFQ_INC_DROPS(ifq) ((ifq)->ifq_drops++) #define IFQ_SET_MAXLEN(ifq, len) ((ifq)->ifq_maxlen = (len)) +int ifq_handoff(struct ifnet *ifp, struct mbuf *m, int adjust); #define IFQ_HANDOFF_ADJ(ifp, m, adj, err) \ do { \ - int len; \ - short mflags; \ - \ - len = (m)->m_pkthdr.len; \ - mflags = (m)->m_flags; \ - IFQ_ENQUEUE(&(ifp)->if_snd, m, err); \ - if ((err) == 0) { \ - (ifp)->if_obytes += len + (adj); \ - if (mflags & M_MCAST) \ - (ifp)->if_omcasts++; \ - if (((ifp)->if_flags & IFF_OACTIVE) == 0) \ - if_start(ifp); \ - } \ -} while (0) + err = ifq_handoff(ifp, m, adj); \ +} while (0) \ #define IFQ_HANDOFF(ifp, m, err) \ IFQ_HANDOFF_ADJ(ifp, m, 0, err) @@ -598,8 +593,10 @@ mtx_init(&ifnet_lock, "ifnet", NULL, MTX_DEF | MTX_RECURSE) #define IFNET_WLOCK() mtx_lock(&ifnet_lock) #define IFNET_WUNLOCK() mtx_unlock(&ifnet_lock) +#define IFNET_WASSERT() mtx_assert(&ifnet_lock, MA_OWNED) #define IFNET_RLOCK() IFNET_WLOCK() #define IFNET_RUNLOCK() IFNET_WUNLOCK() +#define IFNET_RASSERT() IFNET_WASSERT() struct ifindex_entry { struct ifnet *ife_ifnet; --- //depot/vendor/freebsd/src/sys/net/netisr.c 2004/10/11 20:05:57 +++ //depot/user/rwatson/netperf/sys/net/netisr.c 2004/11/28 13:01:02 @@ -1,4 +1,5 @@ /*- + * Copyright (c) 2004 Robert N. M. Watson * Copyright (c) 2001,2002,2003 Jonathan Lemon * Copyright (c) 1997, Stefan Esser * All rights reserved. @@ -48,6 +49,7 @@ #include #include +#include #include #include @@ -199,11 +201,19 @@ static struct isrstat isrstat; SYSCTL_NODE(_net, OID_AUTO, isr, CTLFLAG_RW, 0, "netisr counters"); +SYSCTL_NODE(_net_isr, OID_AUTO, dispatch, CTLFLAG_RW, 0, "direct dispatch"); + +static int netisr_dispatch_enable = 0; +SYSCTL_INT(_net_isr_dispatch, OID_AUTO, enable, CTLFLAG_RW, + &netisr_dispatch_enable, 0, "enable direct dispatch"); +TUNABLE_INT("net.isr.dispatch.enable", &netisr_dispatch_enable); -static int netisr_enable = 0; -SYSCTL_INT(_net_isr, OID_AUTO, enable, CTLFLAG_RW, - &netisr_enable, 0, "enable direct dispatch"); -TUNABLE_INT("net.isr.enable", &netisr_enable); +/* + * XXXRW: net.isr.enable is a compatibility interface, to be removed. + */ +SYSCTL_INT(_net_isr, OID_AUTO, enable, CTLFLAG_RW, &netisr_dispatch_enable, 0, + "enable direct dispatch"); +TUNABLE_INT("net.isr.enable", &netisr_dispatch_enable); SYSCTL_INT(_net_isr, OID_AUTO, count, CTLFLAG_RD, &isrstat.isrs_count, 0, ""); @@ -226,14 +236,21 @@ static void netisr_processqueue(struct netisr *ni) { + struct mbufqueue mbq; + struct ifqueue *ifq; struct mbuf *m; - for (;;) { - IF_DEQUEUE(ni->ni_queue, m); - if (m == NULL) - break; + ifq = ni->ni_queue; + if (ifq->ifq_head == NULL) + return; + mbq_init(&mbq); + IF_LOCK(ifq); + mbq_enqueue_from_ifq(&mbq, ifq); + IF_UNLOCK(ifq); + + while ((m = mbq_dequeue_one(&mbq)) != NULL) ni->ni_handler(m); - } + mbq_assert_empty(&mbq); } /* @@ -265,7 +282,7 @@ * between multiple places in the system (e.g. IP * dispatched from interfaces vs. IP queued from IPSec). */ - if (netisr_enable && (ni->ni_flags & NETISR_MPSAFE)) { + if (netisr_dispatch_enable && (ni->ni_flags & NETISR_MPSAFE)) { isrstat.isrs_directed++; /* * NB: We used to drain the queue before handling --- //depot/vendor/freebsd/src/sys/net/raw_cb.c 2004/10/18 22:20:34 +++ //depot/user/rwatson/netperf/sys/net/raw_cb.c 2004/10/19 18:01:38 @@ -124,6 +124,7 @@ m_freem(dtom(rp->rcb_faddr)); rp->rcb_faddr = 0; #endif + /* Unlocked read. */ if (rp->rcb_socket->so_state & SS_NOFDREF) raw_detach(rp); } --- //depot/vendor/freebsd/src/sys/net/raw_usrreq.c 2004/11/08 14:45:39 +++ //depot/user/rwatson/netperf/sys/net/raw_usrreq.c 2004/11/14 11:53:24 @@ -76,6 +76,10 @@ register struct mbuf *m = m0; struct socket *last; + /* + * XXXRW: Potential lock order issues due to holding the + * rawcb_mtx across all this stuff. Need to revisit. + */ last = 0; mtx_lock(&rawcb_mtx); LIST_FOREACH(rp, &rawcb_list, list) { --- //depot/vendor/freebsd/src/sys/net/rtsock.c 2004/11/08 14:45:39 +++ //depot/user/rwatson/netperf/sys/net/rtsock.c 2004/11/14 11:53:24 @@ -54,10 +54,18 @@ MALLOC_DEFINE(M_RTABLE, "routetbl", "routing tables"); /* NB: these are not modified */ +/* + * XXXRW: It would be really nice to add const to these, but that may + * not be possible due to where they are passed in. We might need + * to const-poison a whole boatload of APIs...? + */ static struct sockaddr route_dst = { 2, PF_ROUTE, }; static struct sockaddr route_src = { 2, PF_ROUTE, }; static struct sockaddr sa_zero = { sizeof(sa_zero), AF_INET, }; +/* + * XXXRW: These fields are locked by RTSOCK_LOCK(). + */ static struct { int ip_count; /* attached w/ AF_INET */ int ip6_count; /* attached w/ AF_INET6 */ --- //depot/vendor/freebsd/src/sys/netatalk/aarp.c 2004/08/10 03:25:35 +++ //depot/user/rwatson/netperf/sys/netatalk/aarp.c 2004/08/11 02:33:29 @@ -109,10 +109,6 @@ #define AARPT_KILLC 20 #define AARPT_KILLI 3 -# if !defined(__FreeBSD__) -extern u_char etherbroadcastaddr[6]; -# endif /* __FreeBSD__ */ - static const u_char atmulticastaddr[ 6 ] = { 0x09, 0x00, 0x07, 0xff, 0xff, 0xff, }; @@ -124,6 +120,9 @@ 0x00, 0x00, 0x00, }; +/* + * XXXRW: Make use callouts, not timeouts. + */ static struct callout_handle aarptimer_ch = CALLOUT_HANDLE_INITIALIZER(&aarptimer_ch); @@ -158,6 +157,7 @@ struct at_ifaddr *aa; struct sockaddr_at *sat2; + AT_IFADDR_LIST_LOCK(); for (aa = at_ifaddr_list; aa != NULL; aa = aa->aa_next) { sat2 = &(aa->aa_addr); if (sat2->sat_addr.s_net == sat->sat_addr.s_net) { @@ -169,6 +169,8 @@ break; } } + IFAREF((struct ifaddr *)aa); + AT_IFADDR_LIST_UNLOCK(); return (aa); } @@ -251,6 +253,7 @@ ntohs(AA_SAT(aa)->sat_addr.s_net), AA_SAT(aa)->sat_addr.s_node); #endif /* NETATALKDEBUG */ + IFAFREE((struct ifaddr *)aa); sa.sa_len = sizeof(struct sockaddr); sa.sa_family = AF_UNSPEC; @@ -279,6 +282,7 @@ bcopy(ifp->if_broadcastaddr, (caddr_t)desten, sizeof(ifp->if_addrlen)); } + IFAFREE((struct ifaddr *)aa); return (1); } @@ -388,6 +392,8 @@ /* * Since we don't know the net, we just look for the first * phase 1 address on the interface. + * + * XXXRW: Note, if_addrhead not locked here. */ for (aa = (struct at_ifaddr *)TAILQ_FIRST(&ifp->if_addrhead); aa; aa = (struct at_ifaddr *)aa->aa_ifa.ifa_link.tqe_next) { @@ -421,6 +427,7 @@ untimeout(aarpprobe, ifp, aa->aa_ch); wakeup(aa); m_freem(m); + IFAFREE((struct ifaddr *)aa); return; } else if (op != AARPOP_PROBE) { /* @@ -433,6 +440,7 @@ ea->aarp_sha[ 0 ], ea->aarp_sha[ 1 ], ea->aarp_sha[ 2 ], ea->aarp_sha[ 3 ], ea->aarp_sha[ 4 ], ea->aarp_sha[ 5 ]); m_freem(m); + IFAFREE((struct ifaddr *)aa); return; } } @@ -449,6 +457,7 @@ aarptfree(aat); AARPTAB_UNLOCK(); m_freem(m); + IFAFREE((struct ifaddr *)aa); return; } @@ -484,6 +493,7 @@ if (tpa.s_net != ma.s_net || tpa.s_node != ma.s_node || op == AARPOP_RESPONSE || (aa->aa_flags & AFA_PROBING)) { m_freem(m); + IFAFREE((struct ifaddr *)aa); return; } @@ -502,6 +512,7 @@ sizeof(struct ether_aarp)); M_PREPEND(m, sizeof(struct llc), M_DONTWAIT); if (m == NULL) { + IFAFREE((struct ifaddr *)aa); return; } llc = mtod(m, struct llc *); @@ -515,6 +526,7 @@ } else { eh->ether_type = htons(ETHERTYPE_AARP); } + IFAFREE((struct ifaddr *)aa); ea->aarp_tpnode = ea->aarp_spnode; ea->aarp_spnode = ma.s_node; @@ -680,6 +692,9 @@ struct aarptab *aat; int i; + /* + * XXXRW: Should grab mutex before untimeout? + */ untimeout(aarptimer, 0, aarptimer_ch); AARPTAB_LOCK(); for (i = 0, aat = aarptab; i < AARPTAB_SIZE; i++, aat++) { --- //depot/vendor/freebsd/src/sys/netatalk/at_control.c 2004/07/19 17:20:31 +++ //depot/user/rwatson/netperf/sys/netatalk/at_control.c 2004/07/20 01:38:51 @@ -21,7 +21,15 @@ #include #include +/* + * Access to at_ifaddr_list and the aa_next pointer in struct at_ifaddr are + * locked with this global mutex. + * + * XXXRW: We'll need something better in the future. + */ +struct mtx at_ifaddr_mtx; struct at_ifaddr *at_ifaddr_list; +MTX_SYSINIT(at_ifaddr_mtx, &at_ifaddr_mtx, "at_ifaddr_mtx", MTX_DEF); static int aa_dorangeroute(struct ifaddr *ifa, u_int first, u_int last, int cmd); @@ -53,10 +61,12 @@ struct at_ifaddr *aa0; struct at_ifaddr *aa = NULL; struct ifaddr *ifa, *ifa0; + int error; /* * If we have an ifp, then find the matching at_ifaddr if it exists */ + mtx_lock(&at_ifaddr_mtx); if (ifp != NULL) { for (aa = at_ifaddr_list; aa != NULL; aa = aa->aa_next) { if (aa->aa_ifp == ifp) @@ -90,16 +100,20 @@ * If we a retrying to delete an addres but didn't find such, * then rewurn with an error */ - if (cmd == SIOCDIFADDR && aa == NULL) + if (cmd == SIOCDIFADDR && aa == NULL) { + mtx_unlock(&at_ifaddr_mtx); return (EADDRNOTAVAIL); + } /*FALLTHROUGH*/ case SIOCSIFADDR: /* * If we are not superuser, then we don't get to do these ops. */ - if (suser(td)) + if (suser(td)) { + mtx_unlock(&at_ifaddr_mtx); return (EPERM); + } sat = satosat(&ifr->ifr_addr); nr = (struct netrange *)sat->sat_zero; @@ -127,8 +141,7 @@ } } - if (ifp == NULL) - panic("at_control"); + KASSERT(ifp != NULL, ("at_control: ifp == NULL")); /* * If we failed to find an existing at_ifaddr entry, then we @@ -193,6 +206,9 @@ /* * If we DID find one then we clobber any routes * dependent on it.. + * + * XXXRW: Do we really want to hold at_ifaddr_mtx + * over at_scrub()? */ at_scrub(ifp, aa); } @@ -222,8 +238,10 @@ } } - if (aa == NULL) + if (aa == NULL) { + mtx_unlock(&at_ifaddr_mtx); return (EADDRNOTAVAIL); + } break; } @@ -231,6 +249,7 @@ * By the time this switch is run we should be able to assume that * the "aa" pointer is valid when needed. */ + error = 0; switch (cmd) { case SIOCGIFADDR: @@ -252,18 +271,30 @@ break; case SIOCSIFADDR: - return (at_ifinit(ifp, aa, - (struct sockaddr_at *)&ifr->ifr_addr)); + /* + * XXXRW: Do we really want to hold at_ifaddr_mtx over + * at_ifinit(). + */ + error = at_ifinit(ifp, aa, + (struct sockaddr_at *)&ifr->ifr_addr); + break; case SIOCAIFADDR: if (sateqaddr(&ifra->ifra_addr, &aa->aa_addr)) - return (0); - return (at_ifinit(ifp, aa, - (struct sockaddr_at *)&ifr->ifr_addr)); + break; + /* + * XXXRW: Do we really want to hold at_ifaddr_mtx over + * at_ifinit(). + */ + error = at_ifinit(ifp, aa, + (struct sockaddr_at *)&ifr->ifr_addr); + break; case SIOCDIFADDR: /* * scrub all routes.. didn't we just DO this? XXX yes, del it + * + * XXXRW: Do we want to hold at_ifaddr_mtx over at_scrub()? */ at_scrub(ifp, aa); @@ -287,10 +318,9 @@ /* * if we found it, remove it, otherwise we screwed up. */ - if (aa->aa_next) - aa->aa_next = aa0->aa_next; - else - panic("at_control"); + KASSERT(aa->aa_next != NULL, + ("at_control: aa->aa_next == NULL")); + aa->aa_next = aa0->aa_next; } /* @@ -300,11 +330,14 @@ break; default: - if (ifp == NULL || ifp->if_ioctl == NULL) - return (EOPNOTSUPP); - return ((*ifp->if_ioctl)(ifp, cmd, data)); + if (ifp == NULL || ifp->if_ioctl == NULL) { + error = EOPNOTSUPP; + break; + } + error = (*ifp->if_ioctl)(ifp, cmd, data); } - return (0); + mtx_unlock(&at_ifaddr_mtx); + return (error); } /* @@ -318,6 +351,14 @@ { int error; + /* + * XXXRW: We don't asser the at_ifaddr_mtx here, but we do rely on + * the caller holding a reference to 'aa' in such a way that it won't + * go away as at_scrub() runs. In practice this means that either + * the caller has to hold an ifaddr reference or at_ifaddr_mtx. + * + * mtx_assert(&at_ifaddr_mtx, MA_OWNED); + */ if (aa->aa_flags & AFA_ROUTE) { if (ifp->if_flags & IFF_LOOPBACK) { if ((error = aa_delsingleroute(&aa->aa_ifa, @@ -352,6 +393,7 @@ int netinc, nodeinc, nnets; u_short net; + mtx_assert(&at_ifaddr_mtx, MA_OWNED); /* * save the old addresses in the at_ifaddr just in case we need them. */ @@ -495,11 +537,15 @@ /* * start off the probes as an asynchronous * activity. though why wait 200mSec? + * + * XXXRW: This probably opens up a race, but + * the race already existed it and so I + * haven't fixed it. */ aa->aa_ch = timeout(aarpprobe, (caddr_t)ifp, hz / 5); - if (tsleep(aa, PPAUSE|PCATCH, "at_ifinit", - 0)) { + if (msleep(aa, &at_ifaddr_mtx, PPAUSE|PCATCH, + "at_ifinit", 0)) { /* * theoretically we shouldn't time * out here so if we returned with an @@ -547,6 +593,9 @@ /* * Now that we have selected an address, we need to tell the interface * about it, just in case it needs to adjust something. + * + * XXXRW: Should we release at_ifaddr_mtx before calling into the + * interface ioctl() code? */ if (ifp->if_ioctl != NULL && (error = (*ifp->if_ioctl)(ifp, SIOCSIFADDR, (caddr_t)aa))) { @@ -619,6 +668,10 @@ * getting risky by now XXX */ if (error) { + /* + * XXXRW: Do we want to hold at_ifaddr_mtx over calls to + * at_scrub()? + */ at_scrub(ifp, aa); aa->aa_addr = oldaddr; aa->aa_firstnet = onr.nr_firstnet; @@ -641,6 +694,7 @@ at_broadcast(struct sockaddr_at *sat) { struct at_ifaddr *aa; + int ret; /* * If the node is not right, it can't be a broadcast @@ -657,13 +711,18 @@ /* * failing that, if the net is one we have, it's a broadcast as well. */ + ret = 0; + mtx_lock(&at_ifaddr_mtx); for (aa = at_ifaddr_list; aa != NULL; aa = aa->aa_next) { if ((aa->aa_ifp->if_flags & IFF_BROADCAST) && (ntohs(sat->sat_addr.s_net) >= ntohs(aa->aa_firstnet) - && ntohs(sat->sat_addr.s_net) <= ntohs(aa->aa_lastnet))) - return (1); + && ntohs(sat->sat_addr.s_net) <= ntohs(aa->aa_lastnet))) { + ret = 1; + break; + } } - return (0); + mtx_unlock(&at_ifaddr_mtx); + return (ret); } /* @@ -785,6 +844,10 @@ struct ifaddr *ifa; struct ifnet *ifp; + /* + * XXXRW: #if 0, and hence untested. + */ + mtx_lock(&at_ifaddr_mtx); while ((aa = at_ifaddr_list) != NULL) { ifp = aa->aa_ifp; at_scrub(ifp, aa); @@ -795,13 +858,12 @@ while (ifa->ifa_next && (ifa->ifa_next != (struct ifaddr *)aa)) ifa = ifa->ifa_next; - if (ifa->ifa_next) - ifa->ifa_next = - ((struct ifaddr *)aa)->ifa_next; - else - panic("at_entry"); + KASSERT(ifa->ifa_next != NULL, + ("aa_clean: ifa_next == NULL")); + ifa->ifa_next = ((struct ifaddr *)aa)->ifa_next; } } + mtx_unlock(&at_ifaddr_mtx); } #endif --- //depot/vendor/freebsd/src/sys/netatalk/at_var.h 2004/03/22 04:56:26 +++ //depot/user/rwatson/netperf/sys/netatalk/at_var.h 2004/07/20 01:38:51 @@ -60,6 +60,11 @@ #define AFA_PHASE2 0x0004 #ifdef _KERNEL +extern struct mtx at_ifaddr_mtx; +#define AT_IFADDR_LIST_ASSERT() mtx_assert(&at_ifaddr_mtx, MA_OWNED) +#define AT_IFADDR_LIST_LOCK() mtx_lock(&at_ifaddr_mtx) +#define AT_IFADDR_LIST_UNLOCK() mtx_unlock(&at_ifaddr_mtx) + extern struct at_ifaddr *at_ifaddr_list; #endif --- //depot/vendor/freebsd/src/sys/netatalk/ddp_input.c 2004/08/10 03:25:35 +++ //depot/user/rwatson/netperf/sys/netatalk/ddp_input.c 2004/08/11 02:33:29 @@ -76,6 +76,11 @@ static volatile int ddp_firewall = 0; static struct ddpstat ddpstat; +/* + * XXXRW: If we're going to keep this cached route data, we'll need to lock it + * down, and change later function-local use of it to grab an extra reference + * after deciding it is useful. + */ static struct route forwro; static void ddp_input(struct mbuf *, struct ifnet *, struct elaphdr *, int); @@ -170,6 +175,7 @@ * Make sure that we point to the phase1 ifaddr info * and that it's valid for this packet. */ + AT_IFADDR_LIST_LOCK(); for (aa = at_ifaddr_list; aa != NULL; aa = aa->aa_next) { if ((aa->aa_ifp == ifp) && ((aa->aa_flags & AFA_PHASE2) == 0) @@ -185,6 +191,8 @@ m_freem(m); return; } + IFAREF((struct ifaddr *)aa); + AT_IFADDR_LIST_UNLOCK(); } else { /* * There was no 'elh' passed on. This could still be @@ -229,6 +237,7 @@ * this node number will match (which may NOT be what we want, * but it's probably safe in 99.999% of cases. */ + AT_IFADDR_LIST_LOCK(); for (aa = at_ifaddr_list; aa != NULL; aa = aa->aa_next) { if (phase == 1 && (aa->aa_flags & AFA_PHASE2)) { continue; @@ -243,11 +252,15 @@ break; } } + if (aa != NULL) + IFAREF((struct ifaddr *)aa); + AT_IFADDR_LIST_UNLOCK(); } else { /* * A destination network was given. We just try to find * which ifaddr info matches it. */ + AT_IFADDR_LIST_LOCK(); for (aa = at_ifaddr_list; aa != NULL; aa = aa->aa_next) { /* * This is a kludge. Accept packets that are @@ -281,6 +294,9 @@ } break; } + if (aa != NULL) + IFAREF((struct ifaddr *)aa); + AT_IFADDR_LIST_UNLOCK(); } } @@ -293,6 +309,8 @@ if (mlen < dlen) { ddpstat.ddps_toosmall++; m_freem(m); + if (aa != NULL) + IFAFREE((struct ifaddr *)aa); return; } if (mlen > dlen) { @@ -315,6 +333,8 @@ */ if (ddp_forward == 0) { m_freem(m); + if (aa != NULL) + IFAFREE((struct ifaddr *)aa); return; } /* @@ -359,6 +379,8 @@ && ((forwro.ro_rt == NULL) || (forwro.ro_rt->rt_ifp != ifp))) { m_freem(m); + if (aa != NULL) + IFAFREE((struct ifaddr *)aa); return; } @@ -376,6 +398,8 @@ } else { ddpstat.ddps_forward++; } + if (aa != NULL) + IFAFREE((struct ifaddr *)aa); return; } @@ -438,6 +462,8 @@ DDP_LIST_SUNLOCK(); if (m != NULL) m_freem(m); + if (aa != NULL) + IFAFREE((struct ifaddr *)aa); } #if 0 --- //depot/vendor/freebsd/src/sys/netatalk/ddp_output.c 2004/06/13 02:50:44 +++ //depot/user/rwatson/netperf/sys/netatalk/ddp_output.c 2004/07/20 01:38:51 @@ -151,6 +151,7 @@ && (ro->ro_rt->rt_ifa) && (ifp = ro->ro_rt->rt_ifa->ifa_ifp)) { net = ntohs(satosat(ro->ro_rt->rt_gateway)->sat_addr.s_net); + AT_IFADDR_LIST_LOCK(); for (aa = at_ifaddr_list; aa != NULL; aa = aa->aa_next) { if (((net == 0) || (aa->aa_ifp == ifp)) && net >= ntohs(aa->aa_firstnet) && @@ -158,6 +159,8 @@ break; } } + IFAREF((struct ifaddr *)aa); + AT_IFADDR_LIST_UNLOCK(); } else { m_freem(m); #ifdef NETATALK_DEBUG @@ -177,6 +180,8 @@ ifp->if_xname); #endif m_freem(m); + if (aa != NULL) + IFAFREE((struct ifaddr *)aa); return (ENETUNREACH); } @@ -203,6 +208,8 @@ MGET(m0, M_TRYWAIT, MT_HEADER); if (m0 == NULL) { m_freem(m); + if (aa != NULL) + IFAFREE((struct ifaddr *)aa); printf("ddp_route: no buffers\n"); return (ENOBUFS); } @@ -236,9 +243,13 @@ if ((satosat(&aa->aa_addr)->sat_addr.s_net == satosat(&ro->ro_dst)->sat_addr.s_net) && (satosat(&aa->aa_addr)->sat_addr.s_node == satosat(&ro->ro_dst)->sat_addr.s_node)) { + if (aa != NULL) + IFAFREE((struct ifaddr *)aa); return (if_simloop(ifp, m, gate.sat_family, 0)); } + if (aa != NULL) + IFAFREE((struct ifaddr *)aa); return ((*ifp->if_output)(ifp, m, (struct sockaddr *)&gate, NULL)); /* XXX */ } --- //depot/vendor/freebsd/src/sys/netatalk/ddp_pcb.c 2004/10/18 22:20:34 +++ //depot/user/rwatson/netperf/sys/netatalk/ddp_pcb.c 2004/10/19 18:01:38 @@ -44,6 +44,7 @@ struct sockaddr_at lsat, *sat; struct at_ifaddr *aa; struct ddpcb *ddpp; + int error; /* * We read and write both the ddp passed in, and also ddp_ports. @@ -51,37 +52,46 @@ DDP_LIST_XLOCK_ASSERT(); DDP_LOCK_ASSERT(ddp); - if (ddp->ddp_lsat.sat_port != ATADDR_ANYPORT) { /* shouldn't be bound */ - return (EINVAL); + error = 0; + if (ddp->ddp_lsat.sat_port != ATADDR_ANYPORT) { + /* shouldn't be bound */ + error = EINVAL; + goto out; } if (addr != NULL) { /* validate passed address */ sat = (struct sockaddr_at *)addr; if (sat->sat_family != AF_APPLETALK) { - return (EAFNOSUPPORT); + error = EAFNOSUPPORT; + goto out; } if (sat->sat_addr.s_node != ATADDR_ANYNODE || sat->sat_addr.s_net != ATADDR_ANYNET) { + AT_IFADDR_LIST_LOCK(); for (aa = at_ifaddr_list; aa != NULL; aa = aa->aa_next) { if ((sat->sat_addr.s_net == AA_SAT(aa)->sat_addr.s_net) && (sat->sat_addr.s_node == AA_SAT(aa)->sat_addr.s_node)) { break; } } - if (!aa) { - return (EADDRNOTAVAIL); + AT_IFADDR_LIST_UNLOCK(); + if (aa != NULL) { + error = EADDRNOTAVAIL; + goto out; } } if (sat->sat_port != ATADDR_ANYPORT) { if (sat->sat_port < ATPORT_FIRST || sat->sat_port >= ATPORT_LAST) { - return (EINVAL); + error = EINVAL; + goto out; } if (sat->sat_port < ATPORT_RESERVED && suser(td)) { - return (EACCES); + error = EACCES; + goto out; } } } else { @@ -96,7 +106,8 @@ if (sat->sat_addr.s_node == ATADDR_ANYNODE && sat->sat_addr.s_net == ATADDR_ANYNET) { if (at_ifaddr_list == NULL) { - return (EADDRNOTAVAIL); + error = EADDRNOTAVAIL; + goto out; } sat->sat_addr = AA_SAT(at_ifaddr_list)->sat_addr; } @@ -113,7 +124,8 @@ } } if (sat->sat_port == ATPORT_LAST) { - return (EADDRNOTAVAIL); + error = EADDRNOTAVAIL; + goto out; } ddp->ddp_lsat.sat_port = sat->sat_port; ddp_ports[ sat->sat_port - 1 ] = ddp; @@ -126,7 +138,8 @@ } } if (ddpp != NULL) { - return (EADDRINUSE); + error = EADDRINUSE; + goto out; } ddp->ddp_pnext = ddp_ports[ sat->sat_port - 1 ]; ddp_ports[ sat->sat_port - 1 ] = ddp; @@ -135,7 +148,8 @@ } } - return (0); +out: + return (error); } int @@ -181,6 +195,7 @@ } aa = NULL; if ((ifp = ro->ro_rt->rt_ifp) != NULL) { + AT_IFADDR_LIST_LOCK(); for (aa = at_ifaddr_list; aa != NULL; aa = aa->aa_next) { if (aa->aa_ifp == ifp && ntohs(net) >= ntohs(aa->aa_firstnet) && @@ -188,6 +203,7 @@ break; } } + AT_IFADDR_LIST_UNLOCK(); } if (aa == NULL || (satosat(&ro->ro_dst)->sat_addr.s_net != (hintnet ? hintnet : sat->sat_addr.s_net) || @@ -218,11 +234,13 @@ */ aa = NULL; if (ro->ro_rt && (ifp = ro->ro_rt->rt_ifp)) { + AT_IFADDR_LIST_LOCK(); for (aa = at_ifaddr_list; aa != NULL; aa = aa->aa_next) { if (aa->aa_ifp == ifp) { break; } } + AT_IFADDR_LIST_UNLOCK(); } if (aa == NULL) { return (ENETUNREACH); @@ -257,6 +275,10 @@ DDP_LOCK_INIT(ddp); ddp->ddp_lsat.sat_port = ATADDR_ANYPORT; + /* + * XXXRW: Is this unlocked assignment payer for socket and + * back-pointer something that needs to be protected? + */ ddp->ddp_socket = so; so->so_pcb = (caddr_t)ddp; --- //depot/vendor/freebsd/src/sys/netatalk/ddp_usrreq.c 2004/11/08 14:45:39 +++ //depot/user/rwatson/netperf/sys/netatalk/ddp_usrreq.c 2004/11/14 11:53:24 @@ -23,6 +23,11 @@ #include #include +/* + * XXXRW: These structures are currently not mutable. They're not marked + * with 'const' because I suspect that consumers of this code would like for + * them to be mutable someday. + */ static u_long ddp_sendspace = DDP_MAXSZ; /* Max ddp size + 1 (ddp_type) */ static u_long ddp_recvspace = 10 * (587 + sizeof(struct sockaddr_at)); @@ -149,6 +154,13 @@ return (0); } +/* + * XXXRW: If an explicit address is specified, then we temporarily change + * the address on the pcb for sending. This is inefficient because it + * requires us to perform global rather than pcb-local operations. It + * may also create a race if other users of the socket are simultaneously + * sending. + */ static int ddp_send(struct socket *so, int flags, struct mbuf *m, struct sockaddr *addr, struct mbuf *control, struct thread *td) @@ -218,9 +230,9 @@ mtx_init(&atintrq2.ifq_mtx, "at2_inq", NULL, MTX_DEF); mtx_init(&aarpintrq.ifq_mtx, "aarp_inq", NULL, MTX_DEF); DDP_LIST_LOCK_INIT(); - netisr_register(NETISR_ATALK1, at1intr, &atintrq1, 0); - netisr_register(NETISR_ATALK2, at2intr, &atintrq2, 0); - netisr_register(NETISR_AARP, aarpintr, &aarpintrq, 0); + netisr_register(NETISR_ATALK1, at1intr, &atintrq1, NETISR_MPSAFE); + netisr_register(NETISR_ATALK2, at2intr, &atintrq2, NETISR_MPSAFE); + netisr_register(NETISR_AARP, aarpintr, &aarpintrq, NETISR_MPSAFE); } #if 0 --- //depot/vendor/freebsd/src/sys/netgraph/ng_base.c 2004/12/01 12:01:13 +++ //depot/user/rwatson/netperf/sys/netgraph/ng_base.c 2004/12/04 17:43:45 @@ -2969,7 +2969,8 @@ MTX_DEF); s = splimp(); /* XXX could use NETISR_MPSAFE but need to verify code */ - netisr_register(NETISR_NETGRAPH, (netisr_t *)ngintr, NULL, 0); + netisr_register(NETISR_NETGRAPH, (netisr_t *)ngintr, NULL, + NETISR_MPSAFE); splx(s); break; case MOD_UNLOAD: --- //depot/vendor/freebsd/src/sys/netgraph/ng_ksocket.c 2004/09/02 19:55:34 +++ //depot/user/rwatson/netperf/sys/netgraph/ng_ksocket.c 2004/09/05 16:49:28 @@ -998,6 +998,9 @@ * before dereferencing the socket pointer. */ +/* + * XXXRW: ng_ksocket_incoming() is called without Giant. Is that OK? + */ static void ng_ksocket_incoming(struct socket *so, void *arg, int waitflag) { --- //depot/vendor/freebsd/src/sys/netinet/accf_http.c 2004/06/14 18:20:38 +++ //depot/user/rwatson/netperf/sys/netinet/accf_http.c 2004/06/14 19:35:13 @@ -161,6 +161,7 @@ sohashttpget(struct socket *so, void *arg, int waitflag) { + /* Unlocked read. */ if ((so->so_rcv.sb_state & SBS_CANTRCVMORE) == 0 && !sbfull(&so->so_rcv)) { struct mbuf *m; char *cmp; @@ -214,6 +215,7 @@ struct mbuf *m, *n; int i, cc, spaces, inspaces; + /* Unlocked read. */ if ((so->so_rcv.sb_state & SBS_CANTRCVMORE) != 0 || sbfull(&so->so_rcv)) goto fallout; @@ -301,6 +303,7 @@ int ccleft, copied; DPRINT("start"); + /* Unlocked read. */ if ((so->so_rcv.sb_state & SBS_CANTRCVMORE) != 0 || sbfull(&so->so_rcv)) goto gotit; --- //depot/vendor/freebsd/src/sys/netinet/if_ether.c 2004/10/26 03:50:56 +++ //depot/user/rwatson/netperf/sys/netinet/if_ether.c 2004/11/03 14:10:39 @@ -96,6 +96,11 @@ #define la_timer la_rt->rt_rmx.rmx_expire /* deletion time in seconds */ }; +/* + * XXXRW: Need to document (and/or fix) locking for this. We always + * seem to hold a lock (and assert) when referencing this list, but it's + * not clear it's always the same lock. + */ static LIST_HEAD(, llinfo_arp) llinfo_arp; static struct ifqueue arpintrq; --- //depot/vendor/freebsd/src/sys/netinet/igmp.c 2004/06/11 03:45:34 +++ //depot/user/rwatson/netperf/sys/netinet/igmp.c 2004/06/11 13:24:13 @@ -99,6 +99,9 @@ static u_long igmp_all_hosts_group; static u_long igmp_all_rtrs_group; +/* + * XXXRW: These variables make me vaguely nervous. + */ static struct mbuf *router_alert; static struct route igmprt; @@ -123,6 +126,7 @@ /* * Construct a Router Alert option to use in outgoing packets + * XXXRW: This might actually need a MAC label. */ MGET(router_alert, M_DONTWAIT, MT_DATA); ra = mtod(router_alert, struct ipoption *); --- //depot/vendor/freebsd/src/sys/netinet/in_gif.c 2004/06/18 02:06:42 +++ //depot/user/rwatson/netperf/sys/netinet/in_gif.c 2004/06/18 03:27:04 @@ -174,6 +174,9 @@ } bcopy(&iphdr, mtod(m, struct ip *), sizeof(struct ip)); + /* + * XXXRW: locking of gif's softc. + */ if (dst->sin_family != sin_dst->sin_family || dst->sin_addr.s_addr != sin_dst->sin_addr.s_addr) { /* cache route doesn't match */ @@ -321,6 +324,10 @@ case 0: case 127: case 255: return 0; } + + /* + * XXXRW: Lock in_ifaddrhead walking. + */ /* reject packets with broadcast on source */ TAILQ_FOREACH(ia4, &in_ifaddrhead, ia_link) { if ((ia4->ia_ifa.ifa_ifp->if_flags & IFF_BROADCAST) == 0) @@ -329,6 +336,7 @@ return 0; } + /* XXXRW: unlocked read. */ /* ingress filters on outer source */ if ((sc->gif_if.if_flags & IFF_LINK2) == 0 && ifp) { struct sockaddr_in sin; @@ -384,6 +392,11 @@ in_gif_attach(sc) struct gif_softc *sc; { + + /* + * XXXRW: Technically, NULL can also be returned for ENOMEM, + * not just EEXIST. + */ sc->encap_cookie4 = encap_attach_func(AF_INET, -1, gif_encapcheck, &in_gif_protosw, sc); if (sc->encap_cookie4 == NULL) --- //depot/vendor/freebsd/src/sys/netinet/in_pcb.c 2004/10/18 22:20:34 +++ //depot/user/rwatson/netperf/sys/netinet/in_pcb.c 2004/10/19 18:01:38 @@ -668,6 +668,7 @@ #ifdef IPSEC ipsec_pcbdisconn(inp->inp_sp); #endif + /* Unlocked read. */ if (inp->inp_socket->so_state & SS_NOFDREF) in_pcbdetach(inp); } @@ -739,6 +740,10 @@ struct in_addr addr; in_port_t port; + /* + * XXXRW: Why not use the inpcb lock instead of the info lock + * here? + */ s = splnet(); INP_INFO_RLOCK(pcbinfo); inp = sotoinpcb(so); @@ -747,10 +752,8 @@ splx(s); return ECONNRESET; } - INP_LOCK(inp); port = inp->inp_lport; addr = inp->inp_laddr; - INP_UNLOCK(inp); INP_INFO_RUNLOCK(pcbinfo); splx(s); @@ -772,6 +775,10 @@ struct in_addr addr; in_port_t port; + /* + * XXXRW: Why not use the inpcb lock instead of the info lock + * here? + */ s = splnet(); INP_INFO_RLOCK(pcbinfo); inp = sotoinpcb(so); @@ -780,10 +787,8 @@ splx(s); return ECONNRESET; } - INP_LOCK(inp); port = inp->inp_fport; addr = inp->inp_faddr; - INP_UNLOCK(inp); INP_INFO_RUNLOCK(pcbinfo); splx(s); --- //depot/vendor/freebsd/src/sys/netinet/ip_divert.c 2004/11/18 13:51:13 +++ //depot/user/rwatson/netperf/sys/netinet/ip_divert.c 2004/11/24 14:41:38 @@ -368,6 +368,9 @@ m->m_pkthdr.rcvif = ifa->ifa_ifp; } #ifdef MAC + /* + * XXXRW: perhaps should be mac_create_mbuf_from_inpcb()? + */ SOCK_LOCK(so); mac_create_mbuf_from_socket(so, m); SOCK_UNLOCK(so); @@ -416,6 +419,7 @@ inp->inp_vflag |= INP_IPV4; inp->inp_flags |= INP_HDRINCL; INP_UNLOCK(inp); + /* XXXRW: Not clear if this is adequate. */ return 0; } --- //depot/vendor/freebsd/src/sys/netinet/ip_encap.c 2004/03/10 02:50:38 +++ //depot/user/rwatson/netperf/sys/netinet/ip_encap.c 2004/03/10 03:47:45 @@ -1,4 +1,4 @@ -/* $FreeBSD: src/sys/netinet/ip_encap.c,v 1.19 2004/03/10 02:48:50 rwatson Exp $ */ +/* $FreeBSD: src/sys/netinet/ip_encap.c,v 1.18 2003/06/01 09:20:38 phk Exp $ */ /* $KAME: ip_encap.c,v 1.41 2001/03/15 08:35:08 itojun Exp $ */ /* @@ -106,8 +106,7 @@ LIST_HEAD(, encaptab) encaptab = LIST_HEAD_INITIALIZER(&encaptab); /* - * We currently keey encap_init() for source code compatibility reasons -- - * it's referenced by KAME pieces in netinet6. + * XXXRW: encap_init() was entirely useless, so I deleted it. */ void encap_init() @@ -185,6 +184,10 @@ } mtx_unlock(&encapmtx); + /* + * XXXRW: Need drain mechanism to prevent the encapsulation + * entry from being released while in use. + */ if (match) { /* found a match, "match" has the best one */ psw = match->psw; @@ -255,6 +258,10 @@ } mtx_unlock(&encapmtx); + /* + * XXXRW: Need drain mechanism so the encap entry isn't freed + * while in use. + */ if (match) { /* found a match */ psw = (const struct ip6protosw *)match->psw; --- //depot/vendor/freebsd/src/sys/netinet/ip_encap.h 2002/03/19 21:30:39 +++ //depot/user/rwatson/netperf/sys/netinet/ip_encap.h 2004/03/12 06:46:13 @@ -35,6 +35,15 @@ #ifdef _KERNEL +/* + * This structure is entirely static after registration, and other than + * its entry in the encapsulation table, requires no locking. The chain + * field is locked using the global encapmtx. + * + * XXXRW: Need to add a refcount/drain mechanism so that encapsulation + * entries can't be removed while in use. This likely requires a + * refcount and cv to wait for it to drain, or an sx lock. + */ struct encaptab { LIST_ENTRY(encaptab) chain; int af; --- //depot/vendor/freebsd/src/sys/netinet/ip_fastfwd.c 2004/11/09 09:40:44 +++ //depot/user/rwatson/netperf/sys/netinet/ip_fastfwd.c 2004/11/14 11:53:24 @@ -482,6 +482,7 @@ * "ours"-label. */ m->m_flags |= M_FASTFWD_OURS; + /* XXX statistic */ if (ro.ro_rt) RTFREE(ro.ro_rt); return 0; --- //depot/vendor/freebsd/src/sys/netinet/ip_fw2.c 2004/11/02 22:25:41 +++ //depot/user/rwatson/netperf/sys/netinet/ip_fw2.c 2004/11/03 14:10:39 @@ -1553,7 +1553,8 @@ } static int -check_uidgid(ipfw_insn_u32 *insn, +check_uidgid(struct ip_fw_chain *chain, + ipfw_insn_u32 *insn, int proto, struct ifnet *oif, struct in_addr dst_ip, u_int16_t dst_port, struct in_addr src_ip, u_int16_t src_port, @@ -1595,6 +1596,7 @@ match = 0; if (*lookup == 0) { INP_INFO_RLOCK(pi); + IPFW_LOCK(chain); pcb = (oif) ? in_pcblookup_hash(pi, dst_ip, htons(dst_port), @@ -1969,7 +1971,7 @@ break; if (proto == IPPROTO_TCP || proto == IPPROTO_UDP) - match = check_uidgid( + match = check_uidgid(chain, (ipfw_insn_u32 *)cmd, proto, oif, dst_ip, dst_port, --- //depot/vendor/freebsd/src/sys/netinet/ip_id.c 2004/08/16 18:35:35 +++ //depot/user/rwatson/netperf/sys/netinet/ip_id.c 2004/08/18 02:45:50 @@ -77,6 +77,9 @@ 2729 }; +/* + * XXXRW: Locking? + */ static u_int16_t ru_x; static u_int16_t ru_seed, ru_seed2; static u_int16_t ru_a, ru_b; --- //depot/vendor/freebsd/src/sys/netinet/ip_output.c 2004/09/29 04:55:44 +++ //depot/user/rwatson/netperf/sys/netinet/ip_output.c 2004/11/28 11:34:00 @@ -97,11 +97,9 @@ static struct ifnet *ip_multicast_if(struct in_addr *, int *); static void ip_mloopback (struct ifnet *, struct mbuf *, struct sockaddr_in *, int); -static int ip_getmoptions - (struct sockopt *, struct ip_moptions *); -static int ip_pcbopts(int, struct mbuf **, struct mbuf *); -static int ip_setmoptions - (struct sockopt *, struct ip_moptions **); +static int ip_getmoptions(struct inpcb *, struct sockopt *); +static int ip_pcbopts(struct inpcb *, int, struct mbuf *); +static int ip_setmoptions(struct inpcb *, struct sockopt *); int ip_optcopy(struct ip *, struct ip *); @@ -1147,7 +1145,8 @@ struct sockopt *sopt; { struct inpcb *inp = sotoinpcb(so); - int error, optval; + struct mbuf *m, *mopt; + int error, optval, tmp; error = optval = 0; if (sopt->sopt_level != IPPROTO_IP) { @@ -1162,7 +1161,6 @@ case IP_RETOPTS: #endif { - struct mbuf *m; if (sopt->sopt_valsize > MLEN) { error = EMSGSIZE; break; @@ -1175,9 +1173,10 @@ m->m_len = sopt->sopt_valsize; error = sooptcopyin(sopt, mtod(m, char *), m->m_len, m->m_len); - - return (ip_pcbopts(sopt->sopt_name, &inp->inp_options, - m)); + INP_LOCK(inp); + error = ip_pcbopts(inp, sopt->sopt_name, m); + INP_UNLOCK(inp); + return (error); } case IP_TOS: @@ -1248,7 +1247,7 @@ case IP_MULTICAST_LOOP: case IP_ADD_MEMBERSHIP: case IP_DROP_MEMBERSHIP: - error = ip_setmoptions(sopt, &inp->inp_moptions); + error = ip_setmoptions(inp, sopt); break; case IP_PORTRANGE: @@ -1287,7 +1286,6 @@ caddr_t req; size_t len = 0; int priv; - struct mbuf *m; int optname; if ((error = soopt_getm(sopt, &m)) != 0) /* XXX */ @@ -1299,7 +1297,9 @@ req = mtod(m, caddr_t); len = m->m_len; optname = sopt->sopt_name; + INP_LOCK(inp); error = ipsec4_set_policy(inp, optname, req, len, priv); + INP_UNLOCK(inp); m_freem(m); break; } @@ -1315,13 +1315,39 @@ switch (sopt->sopt_name) { case IP_OPTIONS: case IP_RETOPTS: - if (inp->inp_options) - error = sooptcopyout(sopt, - mtod(inp->inp_options, - char *), - inp->inp_options->m_len); - else + /* + * Make a temporary copy of the options in a local + * mbuf while holding the lock so that the copyout() + * can be done without a lock. Optimistically assume + * options will be present, so always allocate the + * mbuf and grab the lock. If there's no data, it + * doesn't matter if the allocation fails. + */ + MGET(m, sopt->sopt_td ? M_TRYWAIT : M_DONTWAIT, + MT_HEADER); + INP_LOCK(inp); + mopt = inp->inp_options; + if (mopt == NULL) { + INP_UNLOCK(inp); + if (m != NULL) + m_freem(m); + sopt->sopt_valsize = 0; + break; + } + if (m == NULL) { + INP_UNLOCK(inp); sopt->sopt_valsize = 0; + error = ENOBUFS; + break; + } + KASSERT(mopt->m_len <= MLEN, + ("ip_ctloutput: m_len > MLEN")); + bcopy(mtod(mopt, char *), mtod(m, char *), + mopt->m_len); + m->m_len = mopt->m_len; + INP_UNLOCK(inp); + error = sooptcopyout(sopt, mtod(m, char *), m->m_len); + m_freem(m); break; case IP_TOS: @@ -1367,9 +1393,14 @@ break; case IP_PORTRANGE: - if (inp->inp_flags & INP_HIGHPORT) + /* + * We make a local copy to avoid multiple + * reads without locking. + */ + tmp = inp->inp_flags; + if (tmp & INP_HIGHPORT) optval = IP_PORTRANGE_HIGH; - else if (inp->inp_flags & INP_LOWPORT) + else if (tmp & INP_LOWPORT) optval = IP_PORTRANGE_LOW; else optval = 0; @@ -1392,7 +1423,7 @@ case IP_MULTICAST_LOOP: case IP_ADD_MEMBERSHIP: case IP_DROP_MEMBERSHIP: - error = ip_getmoptions(sopt, inp->inp_moptions); + error = ip_getmoptions(inp, sopt); break; #if defined(IPSEC) || defined(FAST_IPSEC) @@ -1430,24 +1461,26 @@ * with destination address if source routed. */ static int -ip_pcbopts(optname, pcbopt, m) - int optname; - struct mbuf **pcbopt; - register struct mbuf *m; +ip_pcbopts(struct inpcb *inp, int optname, struct mbuf *m) { register int cnt, optlen; register u_char *cp; + struct mbuf **pcbopt; u_char opt; + INP_LOCK_ASSERT(inp); + + pcbopt = &inp->inp_options; + /* turn off any old options */ if (*pcbopt) (void)m_free(*pcbopt); *pcbopt = 0; - if (m == (struct mbuf *)0 || m->m_len == 0) { + if (m == NULL || m->m_len == 0) { /* * Only turning off any previous options. */ - if (m) + if (m != NULL) (void)m_free(m); return (0); } @@ -1564,38 +1597,46 @@ * Set the IP multicast options in response to user setsockopt(). */ static int -ip_setmoptions(sopt, imop) - struct sockopt *sopt; +ip_setmoptions(struct inpcb *inp, struct sockopt *sopt) +{ struct ip_moptions **imop; -{ int error = 0; int i; struct in_addr addr; struct ip_mreq mreq; struct ifnet *ifp; - struct ip_moptions *imo = *imop; + struct ip_moptions *imo; struct route ro; struct sockaddr_in *dst; int ifindex; int s; + imop = &inp->inp_moptions; + imo = *imop; if (imo == NULL) { /* - * No multicast option buffer attached to the pcb; - * allocate one and initialize to default values. + * No multicast option buffer attached to the pcb; allocate + * one and initialize to default values. Make sure to handle + * losing this race, as malloc() may sleep. */ imo = (struct ip_moptions*)malloc(sizeof(*imo), M_IPMOPTS, M_WAITOK); - if (imo == NULL) return (ENOBUFS); - *imop = imo; - imo->imo_multicast_ifp = NULL; - imo->imo_multicast_addr.s_addr = INADDR_ANY; - imo->imo_multicast_vif = -1; - imo->imo_multicast_ttl = IP_DEFAULT_MULTICAST_TTL; - imo->imo_multicast_loop = IP_DEFAULT_MULTICAST_LOOP; - imo->imo_num_memberships = 0; + INP_LOCK(inp); + if (*imop == NULL) { + *imop = imo; + imo->imo_multicast_ifp = NULL; + imo->imo_multicast_addr.s_addr = INADDR_ANY; + imo->imo_multicast_vif = -1; + imo->imo_multicast_ttl = IP_DEFAULT_MULTICAST_TTL; + imo->imo_multicast_loop = IP_DEFAULT_MULTICAST_LOOP; + imo->imo_num_memberships = 0; + } else { + free(imo, M_IPMOPTS); + imo = *imop; + } + INP_UNLOCK(inp); } switch (sopt->sopt_name) { @@ -1643,11 +1684,13 @@ error = EADDRNOTAVAIL; break; } + INP_LOCK(inp); imo->imo_multicast_ifp = ifp; if (ifindex) imo->imo_multicast_addr = addr; else imo->imo_multicast_addr.s_addr = INADDR_ANY; + INP_UNLOCK(inp); splx(s); break; @@ -1750,6 +1793,7 @@ * See if the membership already exists or if all the * membership slots are full. */ + INP_LOCK(inp); for (i = 0; i < imo->imo_num_memberships; ++i) { if (imo->imo_membership[i]->inm_ifp == ifp && imo->imo_membership[i]->inm_addr.s_addr @@ -1757,11 +1801,13 @@ break; } if (i < imo->imo_num_memberships) { + INP_UNLOCK(inp); error = EADDRINUSE; splx(s); break; } if (i == IP_MAX_MEMBERSHIPS) { + INP_UNLOCK(inp); error = ETOOMANYREFS; splx(s); break; @@ -1770,8 +1816,10 @@ * Everything looks good; add a new record to the multicast * address list for the given interface. */ + INP_UNLOCK(inp); /* XXXRW: in_addmulti() may sleep? */ if ((imo->imo_membership[i] = in_addmulti(&mreq.imr_multiaddr, ifp)) == NULL) { + error = ENOBUFS; splx(s); break; @@ -1812,6 +1860,7 @@ /* * Find the membership in the membership array. */ + INP_LOCK(inp); for (i = 0; i < imo->imo_num_memberships; ++i) { if ((ifp == NULL || imo->imo_membership[i]->inm_ifp == ifp) && @@ -1820,6 +1869,7 @@ break; } if (i == imo->imo_num_memberships) { + INP_UNLOCK(inp); error = EADDRNOTAVAIL; splx(s); break; @@ -1828,6 +1878,7 @@ * Give up the multicast address record to which the * membership points. */ + INP_UNLOCK(inp); /* XXXRW: in_delmulti() may sleep? */ in_delmulti(imo->imo_membership[i]); /* * Remove the gap in the membership array. @@ -1846,6 +1897,7 @@ /* * If all options have default values, no need to keep the mbuf. */ + INP_LOCK(inp); if (imo->imo_multicast_ifp == NULL && imo->imo_multicast_vif == -1 && imo->imo_multicast_ttl == IP_DEFAULT_MULTICAST_TTL && @@ -1854,6 +1906,7 @@ free(*imop, M_IPMOPTS); *imop = NULL; } + INP_UNLOCK(inp); return (error); } @@ -1862,10 +1915,9 @@ * Return the IP multicast options in response to user getsockopt(). */ static int -ip_getmoptions(sopt, imo) - struct sockopt *sopt; +ip_getmoptions(struct inpcb *inp, struct sockopt *sopt) +{ register struct ip_moptions *imo; -{ struct in_addr addr; struct in_ifaddr *ia; int error, optval; @@ -1873,15 +1925,20 @@ error = 0; switch (sopt->sopt_name) { - case IP_MULTICAST_VIF: + case IP_MULTICAST_VIF: + INP_LOCK(inp); + imo = inp->inp_moptions; if (imo != NULL) optval = imo->imo_multicast_vif; else optval = -1; + INP_UNLOCK(inp); error = sooptcopyout(sopt, &optval, sizeof optval); break; case IP_MULTICAST_IF: + INP_LOCK(inp); + imo = inp->inp_moptions; if (imo == NULL || imo->imo_multicast_ifp == NULL) addr.s_addr = INADDR_ANY; else if (imo->imo_multicast_addr.s_addr) { @@ -1892,14 +1949,18 @@ addr.s_addr = (ia == NULL) ? INADDR_ANY : IA_SIN(ia)->sin_addr.s_addr; } + INP_UNLOCK(inp); error = sooptcopyout(sopt, &addr, sizeof addr); break; case IP_MULTICAST_TTL: - if (imo == 0) + INP_LOCK(inp); + imo = inp->inp_moptions; + if (imo == NULL) optval = coptval = IP_DEFAULT_MULTICAST_TTL; else optval = coptval = imo->imo_multicast_ttl; + INP_UNLOCK(inp); if (sopt->sopt_valsize == 1) error = sooptcopyout(sopt, &coptval, 1); else @@ -1907,10 +1968,13 @@ break; case IP_MULTICAST_LOOP: - if (imo == 0) + INP_LOCK(inp); + imo = inp->inp_moptions; + if (imo == NULL) optval = coptval = IP_DEFAULT_MULTICAST_LOOP; else optval = coptval = imo->imo_multicast_loop; + INP_UNLOCK(inp); if (sopt->sopt_valsize == 1) error = sooptcopyout(sopt, &coptval, 1); else --- //depot/vendor/freebsd/src/sys/netinet/raw_ip.c 2004/11/08 14:45:39 +++ //depot/user/rwatson/netperf/sys/netinet/raw_ip.c 2004/11/14 11:53:24 @@ -86,6 +86,9 @@ * so leave them not initialized and rely on BSS being set to 0. */ +/* + * XXXRW: Locking for mrouter bits? + */ /* The socket used to communicate with the multicast routing daemon. */ struct socket *ip_mrouter; @@ -659,6 +662,7 @@ } INP_LOCK(inp); soisdisconnected(so); + /* Unlocked read. */ if (so->so_state & SS_NOFDREF) rip_pcbdetach(so, inp); else @@ -670,6 +674,8 @@ static int rip_disconnect(struct socket *so) { + + /* Unlocked read. */ if ((so->so_state & SS_ISCONNECTED) == 0) return ENOTCONN; return rip_abort(so); @@ -766,6 +772,7 @@ INP_INFO_WLOCK(&ripcbinfo); inp = sotoinpcb(so); + /* Unlocked read. */ if (so->so_state & SS_ISCONNECTED) { if (nam) { INP_INFO_WUNLOCK(&ripcbinfo); --- //depot/vendor/freebsd/src/sys/netinet/tcp_debug.c 2004/04/07 20:52:05 +++ //depot/user/rwatson/netperf/sys/netinet/tcp_debug.c 2004/04/08 03:11:34 @@ -50,6 +50,7 @@ #include #include #include +#include #include #include --- //depot/vendor/freebsd/src/sys/netinet/tcp_hostcache.c 2004/11/02 22:25:41 +++ //depot/user/rwatson/netperf/sys/netinet/tcp_hostcache.c 2004/11/25 02:50:29 @@ -250,7 +250,8 @@ * Set up periodic cache cleanup. */ callout_init(&tcp_hc_callout, CALLOUT_MPSAFE); - callout_reset(&tcp_hc_callout, TCP_HOSTCACHE_PRUNE * hz, tcp_hc_purge, 0); + callout_reset(&tcp_hc_callout, TCP_HOSTCACHE_PRUNE * hz, tcp_hc_purge, + NULL); } /* @@ -287,6 +288,9 @@ /* * circle through entries in bucket row looking for a match + * + * XXXRW: Wouldn't it be faster to let the compiler do the comparison + * if the data types are approrpiate, rather than using memcmp()? */ TAILQ_FOREACH(hc_entry, &hc_head->hch_bucket, rmx_q) { if (inc->inc_isipv6) { @@ -435,7 +439,7 @@ /* * External function: lookup an entry in the hostcache and return the - * discovered path mtu. Returns null if no entry found or value not is set. + * discovered path mtu. Returns null if no entry found or value is not set. */ u_long tcp_hc_getmtu(struct in_conninfo *inc) --- //depot/vendor/freebsd/src/sys/netinet/tcp_input.c 2004/11/28 11:10:41 +++ //depot/user/rwatson/netperf/sys/netinet/tcp_input.c 2004/12/04 17:43:45 @@ -1232,6 +1232,7 @@ tcp_timer_rexmt, tp); sowwakeup(so); + /* Unlocked read. */ if (so->so_snd.sb_cc) (void) tcp_output(tp); goto check_delack; @@ -1645,6 +1646,7 @@ * If new data are received on a connection after the * user processes are gone, then RST the other end. */ + /* Unlocked read. */ if ((so->so_state & SS_NOFDREF) && tp->t_state > TCPS_CLOSE_WAIT && tlen) { tp = tcp_close(tp); @@ -2110,6 +2112,7 @@ * we should release the tp also, and use a * compressed state. */ + /* Unlocked read. */ if (so->so_rcv.sb_state & SBS_CANTRCVMORE) { soisdisconnected(so); callout_reset(tp->tt_2msl, tcp_maxidle, @@ -2481,6 +2484,11 @@ if (tp) INP_UNLOCK(inp); + /* + * XXXRW: Added assertion at dropwithreset label because I believe + * the head should always be locked here. If true, this can be + * changed to an unconditional unlock. + */ if (headlocked) INP_INFO_WUNLOCK(&tcbinfo); return; --- //depot/vendor/freebsd/src/sys/netinet/tcp_output.c 2004/11/29 18:50:26 +++ //depot/user/rwatson/netperf/sys/netinet/tcp_output.c 2004/12/04 17:43:45 @@ -275,6 +275,7 @@ * to send then the probe will be the FIN * itself. */ + /* Unlocked read of sb_cc. */ if (off < so->so_snd.sb_cc) flags &= ~TH_FIN; sendwin = 1; @@ -371,6 +372,7 @@ len = tp->t_maxseg; sendalot = 1; } + /* Unlocked read of sb_cc. */ if (sack_rxmit) { if (SEQ_LT(p->rxmit + len, tp->snd_una + so->so_snd.sb_cc)) flags &= ~TH_FIN; @@ -403,6 +405,7 @@ * * note: the len + off check is almost certainly unnecessary. */ + /* Unlocked read of sb_cc. */ if (!(tp->t_flags & TF_MORETOCOME) && /* normal case */ (idle || (tp->t_flags & TF_NODELAY)) && len + off >= so->so_snd.sb_cc && @@ -494,6 +497,7 @@ * if window is nonzero, transmit what we can, * otherwise force out a byte. */ + /* Unlocked read of sb_cc. */ if (so->so_snd.sb_cc && !callout_active(tp->tt_rexmt) && !callout_active(tp->tt_persist)) { tp->t_rxtshift = 0; @@ -737,6 +741,7 @@ * give data to the user when a buffer fills or * a PUSH comes in.) */ + /* Unlocked read of sb_cc. */ if (off + len == so->so_snd.sb_cc) flags |= TH_PUSH; SOCKBUF_UNLOCK(&so->so_snd); --- //depot/vendor/freebsd/src/sys/netinet/tcp_subr.c 2004/11/23 17:25:33 +++ //depot/user/rwatson/netperf/sys/netinet/tcp_subr.c 2004/11/24 14:41:38 @@ -319,6 +319,8 @@ { struct tcphdr *th = (struct tcphdr *)tcp_ptr; + INP_LOCK_ASSERT(inp); + #ifdef INET6 if ((inp->inp_vflag & INP_IPV6) != 0) { struct ip6_hdr *ip6; @@ -647,6 +649,7 @@ struct socket *so = tp->t_inpcb->inp_socket; INP_LOCK_ASSERT(tp->t_inpcb); + if (TCPS_HAVERCVDSYN(tp->t_state)) { tp->t_state = TCPS_CLOSED; (void) tcp_output(tp); @@ -670,6 +673,8 @@ int isipv6 = (inp->inp_vflag & INP_IPV6) != 0; #endif /* INET6 */ + INP_LOCK_ASSERT(inp); + /* * Make sure that all of our timers are stopped before we * delete the PCB. @@ -765,6 +770,8 @@ struct socket *so = inp->inp_socket; #endif + INP_LOCK_ASSERT(inp); + tcp_discardcb(tp); #ifdef INET6 if (INP_CHECK_SOCKAF(so, AF_INET6)) @@ -829,6 +836,8 @@ { struct tcpcb *tp = (struct tcpcb *)inp->inp_ppcb; + INP_LOCK_ASSERT(inp); + /* * Ignore some errors if we are hooked up. * If connection hasn't completed, has retransmitted several times, @@ -1305,6 +1314,7 @@ tcp_seq new_isn; INP_INFO_WLOCK_ASSERT(&tcbinfo); + INP_LOCK_ASSERT(tp->t_inpcb); /* Seed if this is the first use, reseed if requested. */ if ((isn_last_reseed == 0) || ((tcp_isn_reseed_interval > 0) && @@ -1375,6 +1385,7 @@ struct tcpcb *tp = intotcpcb(inp); INP_LOCK_ASSERT(inp); + if (tp != NULL) tp->snd_cwnd = tp->t_maxseg; return (inp); @@ -1393,11 +1404,12 @@ struct tcpcb *tp = intotcpcb(inp); INP_LOCK_ASSERT(inp); + if (tp != NULL && tp->t_state == TCPS_SYN_SENT) { tcp_drop(tp, errno); - return (struct inpcb *)0; + return (NULL); } - return inp; + return (inp); } /* @@ -1421,6 +1433,10 @@ #endif /* INET6 */ INP_LOCK_ASSERT(inp); + + /* + * XXXRW: How often and why is it that tp could be non-NULL here? + */ if (tp != NULL) { #ifdef INET6 isipv6 = (tp->t_inpcb->inp_vflag & INP_IPV6) != 0; @@ -1612,7 +1628,7 @@ /* * Move a TCP connection into TIME_WAIT state. - * tcbinfo is unlocked. + * tcbinfo is locked. * inp is locked, and is unlocked before returning. */ void @@ -1898,6 +1914,8 @@ u_long bwnd; int save_ticks; + INP_LOCK_ASSERT(tp->t_inpcb); + /* * If inflight_enable is disabled in the middle of a tcp connection, * make sure snd_bwnd is effectively disabled. --- //depot/vendor/freebsd/src/sys/netinet/tcp_syncache.c 2004/11/02 22:25:41 +++ //depot/user/rwatson/netperf/sys/netinet/tcp_syncache.c 2004/11/03 14:10:39 @@ -560,6 +560,9 @@ goto abort2; } #ifdef MAC + /* + * XXXRW: Would prefer inpcb. + */ SOCK_LOCK(so); mac_set_socket_peer_from_mbuf(m, so); SOCK_UNLOCK(so); @@ -723,6 +726,14 @@ abort: INP_UNLOCK(inp); abort2: + /* + * XXXRW: Technically speaking, this soabort() likely doesn't + * do anything, since we insert sockets into the accept queues + * in an already completed state, and soabort() leaves it to + * the close() on the listen socket to remove completed + * connections. However, this means a TCP socket without + * full TCP state could slip through...? + */ if (so != NULL) (void) soabort(so); return (NULL); --- //depot/vendor/freebsd/src/sys/netinet/tcp_timer.c 2004/11/23 17:25:33 +++ //depot/user/rwatson/netperf/sys/netinet/tcp_timer.c 2004/11/24 14:41:38 @@ -280,10 +280,15 @@ struct tcptw *tw_tail; INP_INFO_WLOCK_ASSERT(&tcbinfo); + INP_LOCK_ASSERT(tw->tw_inpcb); + if (tw->tw_time != 0) LIST_REMOVE(tw, tw_2msl); tw->tw_time = timeo + ticks; i = timeo > tcp_msl ? 1 : 0; + /* + * XXXRW: Locking? + */ tw_tail = &twl_2msl[i].tw_tail; LIST_INSERT_BEFORE(tw_tail, tw, tw_2msl); } @@ -293,6 +298,7 @@ { INP_INFO_WLOCK_ASSERT(&tcbinfo); + /* XXXRW: Assert tw lock? */ if (tw->tw_time != 0) LIST_REMOVE(tw, tw_2msl); } --- //depot/vendor/freebsd/src/sys/netinet/tcp_usrreq.c 2004/11/27 20:23:41 +++ //depot/user/rwatson/netperf/sys/netinet/tcp_usrreq.c 2004/11/27 21:43:45 @@ -146,6 +146,50 @@ } /* + * Common code to setup and teardown locking. Most + * code begins with a COMMON_START macro and finishes + * with COMMON_END. You indicate whether the inpcb + * and enclosing head are to be locked read or write. + */ +#define INI_NOLOCK 0 /* no head lock */ +#define INI_READ 1 /* read head lock */ +#define INI_WRITE 2 /* write head lock */ + +#define COMMON_START0(_headrw) do { \ + if (_headrw == INI_READ) \ + INP_INFO_RLOCK(&tcbinfo); \ + else if (_headrw == INI_WRITE) \ + INP_INFO_WLOCK(&tcbinfo); \ + inp = sotoinpcb(so); \ + if (inp == 0) { \ + if (_headrw == INI_READ) \ + INP_INFO_RUNLOCK(&tcbinfo); \ + else if (_headrw == INI_WRITE) \ + INP_INFO_WUNLOCK(&tcbinfo); \ + return EINVAL; \ + } \ + INP_LOCK(inp); \ + if (_headrw == INI_READ) \ + INP_INFO_RUNLOCK(&tcbinfo); \ + tp = intotcpcb(inp); \ + TCPDEBUG1(); \ +} while(0) + +#define COMMON_START(_headrw) do { \ + TCPDEBUG0; \ + COMMON_START0(_headrw); \ +} while (0) + +#define COMMON_END(_headrw, req) \ + TCPDEBUG2(req); \ + do { \ + if (tp) \ + INP_UNLOCK(inp); \ + if (_headrw == INI_WRITE) \ + INP_INFO_WUNLOCK(&tcbinfo); \ + } while(0) + +/* * pru_detach() detaches the TCP protocol from the socket. * If the protocol state is non-embryonic, then can't * do this directly: have to initiate a pru_disconnect(), @@ -158,63 +202,13 @@ int error = 0; struct inpcb *inp; struct tcpcb *tp; - TCPDEBUG0; - INP_INFO_WLOCK(&tcbinfo); - inp = sotoinpcb(so); - if (inp == NULL) { - INP_INFO_WUNLOCK(&tcbinfo); - return error; - } - INP_LOCK(inp); - tp = intotcpcb(inp); - TCPDEBUG1(); + COMMON_START(INI_WRITE); tp = tcp_disconnect(tp); - - TCPDEBUG2(PRU_DETACH); - if (tp) - INP_UNLOCK(inp); - INP_INFO_WUNLOCK(&tcbinfo); + COMMON_END(INI_WRITE, PRU_DETACH); return error; } -#define INI_NOLOCK 0 -#define INI_READ 1 -#define INI_WRITE 2 - -#define COMMON_START() \ - TCPDEBUG0; \ - do { \ - if (inirw == INI_READ) \ - INP_INFO_RLOCK(&tcbinfo); \ - else if (inirw == INI_WRITE) \ - INP_INFO_WLOCK(&tcbinfo); \ - inp = sotoinpcb(so); \ - if (inp == 0) { \ - if (inirw == INI_READ) \ - INP_INFO_RUNLOCK(&tcbinfo); \ - else if (inirw == INI_WRITE) \ - INP_INFO_WUNLOCK(&tcbinfo); \ - return EINVAL; \ - } \ - INP_LOCK(inp); \ - if (inirw == INI_READ) \ - INP_INFO_RUNLOCK(&tcbinfo); \ - tp = intotcpcb(inp); \ - TCPDEBUG1(); \ -} while(0) - -#define COMMON_END(req) \ -out: TCPDEBUG2(req); \ - do { \ - if (tp) \ - INP_UNLOCK(inp); \ - if (inirw == INI_WRITE) \ - INP_INFO_WUNLOCK(&tcbinfo); \ - return error; \ - goto out; \ -} while(0) - /* * Give the socket an address. */ @@ -225,7 +219,6 @@ struct inpcb *inp; struct tcpcb *tp; struct sockaddr_in *sinp; - const int inirw = INI_WRITE; sinp = (struct sockaddr_in *)nam; if (nam->sa_len != sizeof (*sinp)) @@ -238,11 +231,10 @@ IN_MULTICAST(ntohl(sinp->sin_addr.s_addr))) return (EAFNOSUPPORT); - COMMON_START(); + COMMON_START(INI_WRITE); error = in_pcbbind(inp, nam, td->td_ucred); - if (error) - goto out; - COMMON_END(PRU_BIND); + COMMON_END(INI_WRITE, PRU_BIND); + return error; } #ifdef INET6 @@ -253,7 +245,6 @@ struct inpcb *inp; struct tcpcb *tp; struct sockaddr_in6 *sin6p; - const int inirw = INI_WRITE; sin6p = (struct sockaddr_in6 *)nam; if (nam->sa_len != sizeof (*sin6p)) @@ -266,7 +257,7 @@ IN6_IS_ADDR_MULTICAST(&sin6p->sin6_addr)) return (EAFNOSUPPORT); - COMMON_START(); + COMMON_START(INI_WRITE); inp->inp_vflag &= ~INP_IPV4; inp->inp_vflag |= INP_IPV6; if ((inp->inp_flags & IN6P_IPV6_V6ONLY) == 0) { @@ -284,9 +275,9 @@ } } error = in6_pcbbind(inp, nam, td->td_ucred); - if (error) - goto out; - COMMON_END(PRU_BIND); +out: + COMMON_END(INI_WRITE, PRU_BIND); + return error; } #endif /* INET6 */ @@ -299,14 +290,14 @@ int error = 0; struct inpcb *inp; struct tcpcb *tp; - const int inirw = INI_WRITE; - COMMON_START(); + COMMON_START(INI_WRITE); if (inp->inp_lport == 0) error = in_pcbbind(inp, (struct sockaddr *)0, td->td_ucred); if (error == 0) tp->t_state = TCPS_LISTEN; - COMMON_END(PRU_LISTEN); + COMMON_END(INI_WRITE, PRU_LISTEN); + return error; } #ifdef INET6 @@ -316,9 +307,8 @@ int error = 0; struct inpcb *inp; struct tcpcb *tp; - const int inirw = INI_WRITE; - COMMON_START(); + COMMON_START(INI_WRITE); if (inp->inp_lport == 0) { inp->inp_vflag &= ~INP_IPV4; if ((inp->inp_flags & IN6P_IPV6_V6ONLY) == 0) @@ -327,7 +317,8 @@ } if (error == 0) tp->t_state = TCPS_LISTEN; - COMMON_END(PRU_LISTEN); + COMMON_END(INI_WRITE, PRU_LISTEN); + return error; } #endif /* INET6 */ @@ -345,7 +336,6 @@ struct inpcb *inp; struct tcpcb *tp; struct sockaddr_in *sinp; - const int inirw = INI_WRITE; sinp = (struct sockaddr_in *)nam; if (nam->sa_len != sizeof (*sinp)) @@ -359,11 +349,13 @@ if (td && jailed(td->td_ucred)) prison_remote_ip(td->td_ucred, 0, &sinp->sin_addr.s_addr); - COMMON_START(); + COMMON_START(INI_WRITE); if ((error = tcp_connect(tp, nam, td)) != 0) goto out; error = tcp_output(tp); - COMMON_END(PRU_CONNECT); +out: + COMMON_END(INI_WRITE, PRU_CONNECT); + return error; } #ifdef INET6 @@ -374,7 +366,6 @@ struct inpcb *inp; struct tcpcb *tp; struct sockaddr_in6 *sin6p; - const int inirw = INI_WRITE; sin6p = (struct sockaddr_in6 *)nam; if (nam->sa_len != sizeof (*sin6p)) @@ -386,7 +377,7 @@ && IN6_IS_ADDR_MULTICAST(&sin6p->sin6_addr)) return (EAFNOSUPPORT); - COMMON_START(); + COMMON_START(INI_WRITE); if (IN6_IS_ADDR_V4MAPPED(&sin6p->sin6_addr)) { struct sockaddr_in sin; @@ -409,7 +400,9 @@ if ((error = tcp6_connect(tp, nam, td)) != 0) goto out; error = tcp_output(tp); - COMMON_END(PRU_CONNECT); +out: + COMMON_END(INI_WRITE, PRU_CONNECT); + return error; } #endif /* INET6 */ @@ -430,11 +423,11 @@ int error = 0; struct inpcb *inp; struct tcpcb *tp; - const int inirw = INI_WRITE; - COMMON_START(); + COMMON_START(INI_WRITE); tp = tcp_disconnect(tp); - COMMON_END(PRU_DISCONNECT); + COMMON_END(INI_WRITE, PRU_DISCONNECT); + return error; } /* @@ -452,33 +445,24 @@ in_port_t port = 0; TCPDEBUG0; + /* Unlocked read. */ if (so->so_state & SS_ISDISCONNECTED) { error = ECONNABORTED; - goto out; + goto out; /* NB: ok 'cuz tp is NULL */ } - INP_INFO_RLOCK(&tcbinfo); - inp = sotoinpcb(so); - if (!inp) { - INP_INFO_RUNLOCK(&tcbinfo); - return (EINVAL); - } - INP_LOCK(inp); - INP_INFO_RUNLOCK(&tcbinfo); - tp = intotcpcb(inp); - TCPDEBUG1(); + COMMON_START0(INI_READ); /* - * We inline in_setpeeraddr and COMMON_END here, so that we can - * copy the data of interest and defer the malloc until after we - * release the lock. + * We inline in_setpeeraddr so that we can copy the + * data of interest and defer the malloc until after + * we release the lock. */ port = inp->inp_fport; addr = inp->inp_faddr; -out: TCPDEBUG2(PRU_ACCEPT); - if (tp) - INP_UNLOCK(inp); +out: + COMMON_END(INI_READ, PRU_ACCEPT); if (error == 0) *nam = in_sockaddr(port, &addr); return error; @@ -497,25 +481,17 @@ int v4 = 0; TCPDEBUG0; + /* Unlocked read. */ if (so->so_state & SS_ISDISCONNECTED) { error = ECONNABORTED; - goto out; + goto out; /* NB: ok 'cuz tp is NULL */ } - INP_INFO_RLOCK(&tcbinfo); - inp = sotoinpcb(so); - if (inp == 0) { - INP_INFO_RUNLOCK(&tcbinfo); - return (EINVAL); - } - INP_LOCK(inp); - INP_INFO_RUNLOCK(&tcbinfo); - tp = intotcpcb(inp); - TCPDEBUG1(); + COMMON_START0(INI_READ); /* - * We inline in6_mapped_peeraddr and COMMON_END here, so that we can - * copy the data of interest and defer the malloc until after we - * release the lock. + * We inline in6_mapped_peeraddr so that we can + * copy the data of interest and defer the malloc + * until after we release the lock. */ if (inp->inp_vflag & INP_IPV4) { v4 = 1; @@ -526,9 +502,8 @@ addr6 = inp->in6p_faddr; } -out: TCPDEBUG2(PRU_ACCEPT); - if (tp) - INP_UNLOCK(inp); +out: + COMMON_END(INI_READ, PRU_ACCEPT); if (error == 0) { if (v4) *nam = in6_v4mapsin6_sockaddr(port, &addr); @@ -569,14 +544,14 @@ int error = 0; struct inpcb *inp; struct tcpcb *tp; - const int inirw = INI_WRITE; - COMMON_START(); + COMMON_START(INI_WRITE); socantsendmore(so); tp = tcp_usrclosed(tp); if (tp) error = tcp_output(tp); - COMMON_END(PRU_SHUTDOWN); + COMMON_END(INI_WRITE, PRU_SHUTDOWN); + return error; } /* @@ -588,11 +563,11 @@ int error = 0; struct inpcb *inp; struct tcpcb *tp; - const int inirw = INI_READ; - COMMON_START(); + COMMON_START(INI_READ); tcp_output(tp); - COMMON_END(PRU_RCVD); + COMMON_END(INI_READ, PRU_RCVD); + return error; } /* @@ -609,7 +584,6 @@ int error = 0; struct inpcb *inp; struct tcpcb *tp; - const int inirw = INI_WRITE; #ifdef INET6 int isipv6; #endif @@ -692,7 +666,9 @@ tp->t_flags &= ~TF_MORETOCOME; } } else { + SOCKBUF_LOCK(&so->so_snd); if (sbspace(&so->so_snd) < -512) { + SOCKBUF_UNLOCK(&so->so_snd); m_freem(m); error = ENOBUFS; goto out; @@ -705,7 +681,8 @@ * of data past the urgent section. * Otherwise, snd_up should be one lower. */ - sbappendstream(&so->so_snd, m); + sbappendstream_locked(&so->so_snd, m); + SOCKBUF_UNLOCK(&so->so_snd); if (nam && tp->t_state < TCPS_SYN_SENT) { /* * Do implied connect if not yet connected, @@ -724,13 +701,16 @@ tp->snd_wnd = TTCP_CLIENT_SND_WND; tcp_mss(tp, -1); } + /* Unlocked read of sb_cc. */ tp->snd_up = tp->snd_una + so->so_snd.sb_cc; tp->t_force = 1; error = tcp_output(tp); tp->t_force = 0; } - COMMON_END((flags & PRUS_OOB) ? PRU_SENDOOB : +out: + COMMON_END(INI_WRITE, (flags & PRUS_OOB) ? PRU_SENDOOB : ((flags & PRUS_EOF) ? PRU_SEND_EOF : PRU_SEND)); + return error; } /* @@ -742,11 +722,11 @@ int error = 0; struct inpcb *inp; struct tcpcb *tp; - const int inirw = INI_WRITE; - COMMON_START(); + COMMON_START(INI_WRITE); tp = tcp_drop(tp, ECONNABORTED); - COMMON_END(PRU_ABORT); + COMMON_END(INI_WRITE, PRU_ABORT); + return error; } /* @@ -758,9 +738,9 @@ int error = 0; struct inpcb *inp; struct tcpcb *tp; - const int inirw = INI_READ; - COMMON_START(); + COMMON_START(INI_READ); + /* Unlocked read. */ if ((so->so_oobmark == 0 && (so->so_rcv.sb_state & SBS_RCVATMARK) == 0) || so->so_options & SO_OOBINLINE || @@ -776,7 +756,9 @@ *mtod(m, caddr_t) = tp->t_iobc; if ((flags & MSG_PEEK) == 0) tp->t_oobflags ^= (TCPOOB_HAVEDATA | TCPOOB_HADDATA); - COMMON_END(PRU_RCVOOB); +out: + COMMON_END(INI_READ, PRU_RCVOOB); + return error; } struct pr_usrreqs tcp_usrreqs = { @@ -1182,17 +1164,31 @@ #endif inp->inp_vflag |= INP_IPV4; tp = tcp_newtcpcb(inp); - if (tp == 0) { - int nofd = so->so_state & SS_NOFDREF; /* XXX */ - - so->so_state &= ~SS_NOFDREF; /* don't free the socket yet */ + if (tp == NULL) { + int nofd; + /* + * XXXRW: This is a potentially racy scenario: we perform + * a read-update-write on so_state but don't hold a lock, + * not to mention calling out to external code that may + * grab locks. This section requires attention. + * + * XXXRW: Or not, this is in a local socket attach? + */ + SOCK_LOCK(so); + nofd = so->so_state & SS_NOFDREF; + /* don't free the socket yet */ + if (nofd) + so->so_state &= ~SS_NOFDREF; + SOCK_UNLOCK(so); #ifdef INET6 if (isipv6) in6_pcbdetach(inp); else #endif in_pcbdetach(inp); + SOCK_LOCK(so); so->so_state |= nofd; + SOCK_UNLOCK(so); return (ENOBUFS); } tp->t_state = TCPS_CLOSED; @@ -1272,4 +1268,3 @@ } return (tp); } - --- //depot/vendor/freebsd/src/sys/netinet/udp_usrreq.c 2004/11/08 14:45:39 +++ //depot/user/rwatson/netperf/sys/netinet/udp_usrreq.c 2004/11/14 11:53:24 @@ -701,10 +701,10 @@ int unlock_udbinfo; /* - * udp_output() may need to temporarily bind or connect the current - * inpcb. As such, we don't know up front what inpcb locks we will - * need. Do any work to decide what is needed up front before - * acquiring locks. + * XXXRW: udp_output() may need to temporarily bind or connect the + * current inpcb. As such, we don't know up front what inpcb locks + * we will need. Do any work to decide what is needed up front + * before acquiring locks. */ if (len + sizeof(struct udpiphdr) > IP_MAXPACKET) { if (control) @@ -747,6 +747,9 @@ src.sin_family = AF_INET; src.sin_len = sizeof(src); src.sin_port = inp->inp_lport; + /* + * XXXRW: alignment? + */ src.sin_addr = *(struct in_addr *)CMSG_DATA(cm); break; default: @@ -763,6 +766,12 @@ return error; } + /* + * XXXRW: We now have all the information we need in order to + * determine what locks are required. The temporary binding behavior + * here is expensive and undesirable. The current use of a local + * variable to track lock state is also unpretty. + */ if (src.sin_addr.s_addr != INADDR_ANY || addr != NULL) { INP_INFO_WLOCK(&udbinfo); @@ -904,24 +913,50 @@ SYSCTL_INT(_net_inet_udp, UDPCTL_RECVSPACE, recvspace, CTLFLAG_RW, &udp_recvspace, 0, "Maximum space for incoming UDP datagrams"); +/* + * Common code to setup and teardown locking. Most + * code begins with a COMMON_START macro and finishes + * with COMMON_END. You indicate whether the inpcb + * and enclosing head are to be locked read or write . + */ +#define INI_NOLOCK 0 /* no head lock */ +#define INI_READ 1 /* read head lock */ +#define INI_WRITE 2 /* write head lock */ + +#define COMMON_START(_headrw) do { \ + if (_headrw == INI_READ) \ + INP_INFO_RLOCK(&udbinfo); \ + else if (_headrw == INI_WRITE) \ + INP_INFO_WLOCK(&udbinfo); \ + inp = sotoinpcb(so); \ + if (inp == 0) { \ + if (_headrw == INI_READ) \ + INP_INFO_RUNLOCK(&udbinfo); \ + else if (_headrw == INI_WRITE) \ + INP_INFO_WUNLOCK(&udbinfo); \ + return EINVAL; \ + } \ + INP_LOCK(inp); \ + if (_headrw == INI_READ) \ + INP_INFO_RUNLOCK(&udbinfo); \ +} while(0) + +#define COMMON_END(_headrw) \ + do { \ + INP_UNLOCK(inp); \ + if (_headrw == INI_WRITE) \ + INP_INFO_WUNLOCK(&udbinfo); \ + } while(0) + static int udp_abort(struct socket *so) { struct inpcb *inp; - int s; - INP_INFO_WLOCK(&udbinfo); - inp = sotoinpcb(so); - if (inp == 0) { - INP_INFO_WUNLOCK(&udbinfo); - return EINVAL; /* ??? possible? panic instead? */ - } - INP_LOCK(inp); + COMMON_START(INI_WRITE); soisdisconnected(so); - s = splnet(); in_pcbdetach(inp); INP_INFO_WUNLOCK(&udbinfo); - splx(s); return 0; } @@ -963,20 +998,11 @@ udp_bind(struct socket *so, struct sockaddr *nam, struct thread *td) { struct inpcb *inp; - int s, error; + int error; - INP_INFO_WLOCK(&udbinfo); - inp = sotoinpcb(so); - if (inp == 0) { - INP_INFO_WUNLOCK(&udbinfo); - return EINVAL; - } - INP_LOCK(inp); - s = splnet(); + COMMON_START(INI_WRITE); error = in_pcbbind(inp, nam, td->td_ucred); - splx(s); - INP_UNLOCK(inp); - INP_INFO_WUNLOCK(&udbinfo); + COMMON_END(INI_WRITE); return error; } @@ -984,31 +1010,22 @@ udp_connect(struct socket *so, struct sockaddr *nam, struct thread *td) { struct inpcb *inp; - int s, error; + int error; struct sockaddr_in *sin; - INP_INFO_WLOCK(&udbinfo); - inp = sotoinpcb(so); - if (inp == 0) { - INP_INFO_WUNLOCK(&udbinfo); - return EINVAL; - } - INP_LOCK(inp); + COMMON_START(INI_WRITE); if (inp->inp_faddr.s_addr != INADDR_ANY) { - INP_UNLOCK(inp); - INP_INFO_WUNLOCK(&udbinfo); - return EISCONN; + error = EISCONN; + goto out; } - s = splnet(); sin = (struct sockaddr_in *)nam; if (td && jailed(td->td_ucred)) prison_remote_ip(td->td_ucred, 0, &sin->sin_addr.s_addr); error = in_pcbconnect(inp, nam, td->td_ucred); - splx(s); if (error == 0) soisconnected(so); - INP_UNLOCK(inp); - INP_INFO_WUNLOCK(&udbinfo); +out: + COMMON_END(INI_WRITE); return error; } @@ -1016,49 +1033,31 @@ udp_detach(struct socket *so) { struct inpcb *inp; - int s; - INP_INFO_WLOCK(&udbinfo); - inp = sotoinpcb(so); - if (inp == 0) { - INP_INFO_WUNLOCK(&udbinfo); - return EINVAL; - } - INP_LOCK(inp); - s = splnet(); + COMMON_START(INI_WRITE); in_pcbdetach(inp); INP_INFO_WUNLOCK(&udbinfo); - splx(s); return 0; } static int udp_disconnect(struct socket *so) { + int error = 0; struct inpcb *inp; - int s; - INP_INFO_WLOCK(&udbinfo); - inp = sotoinpcb(so); - if (inp == 0) { - INP_INFO_WUNLOCK(&udbinfo); - return EINVAL; - } - INP_LOCK(inp); + COMMON_START(INI_WRITE); if (inp->inp_faddr.s_addr == INADDR_ANY) { - INP_INFO_WUNLOCK(&udbinfo); - INP_UNLOCK(inp); - return ENOTCONN; + error = ENOTCONN; + goto out; } - s = splnet(); in_pcbdisconnect(inp); inp->inp_laddr.s_addr = INADDR_ANY; - INP_UNLOCK(inp); - INP_INFO_WUNLOCK(&udbinfo); - splx(s); + COMMON_END(INI_WRITE); so->so_state &= ~SS_ISCONNECTED; /* XXX */ - return 0; +out: + return error; } static int @@ -1076,16 +1075,9 @@ { struct inpcb *inp; - INP_INFO_RLOCK(&udbinfo); - inp = sotoinpcb(so); - if (inp == 0) { - INP_INFO_RUNLOCK(&udbinfo); - return EINVAL; - } - INP_LOCK(inp); - INP_INFO_RUNLOCK(&udbinfo); + COMMON_START(INI_READ); socantsendmore(so); - INP_UNLOCK(inp); + COMMON_END(INI_READ); return 0; } --- //depot/vendor/freebsd/src/sys/netinet6/in6_gif.c 2003/10/29 15:10:52 +++ //depot/user/rwatson/netperf/sys/netinet6/in6_gif.c 2004/03/09 23:48:47 @@ -82,6 +82,11 @@ &rip6_usrreqs }; +/* + * XXXRW: in6_gif per-softc locking required. Need to lock both the + * members, and also prevent the softc from disappearing during use + * including the route). + */ int in6_gif_output(ifp, family, m) struct ifnet *ifp; @@ -379,6 +384,10 @@ in6_gif_attach(sc) struct gif_softc *sc; { + + /* + * XXXRW: Technically, encap_attach() can return NULL due to ENOMEM? + */ sc->encap_cookie6 = encap_attach_func(AF_INET6, -1, gif_encapcheck, (struct protosw *)&in6_gif_protosw, sc); if (sc->encap_cookie6 == NULL) --- //depot/vendor/freebsd/src/sys/netinet6/in6_ifattach.c 2004/08/23 03:01:16 +++ //depot/user/rwatson/netperf/sys/netinet6/in6_ifattach.c 2004/08/25 20:28:45 @@ -226,8 +226,8 @@ struct sockaddr_dl *sdl; u_int8_t *addr; size_t addrlen; - static u_int8_t allzero[8] = { 0, 0, 0, 0, 0, 0, 0, 0 }; - static u_int8_t allone[8] = + static const u_int8_t allzero[8] = { 0, 0, 0, 0, 0, 0, 0, 0 }; + static const u_int8_t allone[8] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; for (ifa = ifp->if_addrlist.tqh_first; --- //depot/vendor/freebsd/src/sys/netinet6/in6_pcb.c 2004/10/18 22:20:34 +++ //depot/user/rwatson/netperf/sys/netinet6/in6_pcb.c 2004/10/19 18:01:38 @@ -414,6 +414,7 @@ #ifdef IPSEC ipsec_pcbdisconn(inp->inp_sp); #endif + /* Unlocked read. */ if (inp->inp_socket->so_state & SS_NOFDREF) in6_pcbdetach(inp); } --- //depot/vendor/freebsd/src/sys/netinet6/in6_proto.c 2004/08/23 03:01:16 +++ //depot/user/rwatson/netperf/sys/netinet6/in6_proto.c 2004/08/25 20:28:45 @@ -66,6 +66,8 @@ #include "opt_ipsec.h" #include +#include +#include #include #include #include --- //depot/vendor/freebsd/src/sys/netinet6/in6_rmx.c 2004/10/06 03:35:30 +++ //depot/user/rwatson/netperf/sys/netinet6/in6_rmx.c 2004/10/09 21:40:52 @@ -79,6 +79,8 @@ #include #include #include +#include +#include #include #include #include --- //depot/vendor/freebsd/src/sys/netinet6/ip6_input.c 2004/10/19 14:30:36 +++ //depot/user/rwatson/netperf/sys/netinet6/ip6_input.c 2004/10/19 18:23:01 @@ -195,7 +195,7 @@ ip6intrq.ifq_maxlen = ip6qmaxlen; mtx_init(&ip6intrq.ifq_mtx, "ip6_inq", NULL, MTX_DEF); - netisr_register(NETISR_IPV6, ip6_input, &ip6intrq, 0); + netisr_register(NETISR_IPV6, ip6_input, &ip6intrq, NETISR_MPSAFE); scope6_init(); addrsel_policy_init(); nd6_init(); @@ -241,7 +241,6 @@ struct in6_addr odst; int srcrt = 0; - GIANT_REQUIRED; /* XXX for now */ #ifdef IPSEC /* * should the inner packet be considered authentic? --- //depot/vendor/freebsd/src/sys/netinet6/ip6_output.c 2004/10/02 23:45:34 +++ //depot/user/rwatson/netperf/sys/netinet6/ip6_output.c 2004/10/09 21:40:52 @@ -2075,6 +2075,7 @@ bzero(&sro, sizeof(sro)); + /* Unlocked read. */ if (!(so->so_state & SS_ISCONNECTED)) return (ENOTCONN); /* --- //depot/vendor/freebsd/src/sys/netinet6/nd6.c 2004/10/03 00:50:32 +++ //depot/user/rwatson/netperf/sys/netinet6/nd6.c 2004/10/09 21:40:52 @@ -97,6 +97,9 @@ /* for debugging? */ static int nd6_inuse, nd6_allocated; +/* + * XXXRW: What follows requires locking. + */ struct llinfo_nd6 llinfo_nd6 = {&llinfo_nd6, &llinfo_nd6}; struct nd_drhead nd_defrouter; struct nd_prhead nd_prefix = { 0 }; @@ -1798,6 +1801,13 @@ nd6_slowtimo, NULL); IFNET_RLOCK(); for (ifp = TAILQ_FIRST(&ifnet); ifp; ifp = TAILQ_NEXT(ifp, if_list)) { + /* + * XXXRW: Temporary work-around for the fact that the timeout + * can fire during interface initialization, racing with the + * code that is intended to initialize this field. + */ + if (ifp->if_afdata[AF_INET6] == NULL) + continue; nd6if = ND_IFINFO(ifp); if (nd6if->basereachable && /* already initialized */ (nd6if->recalctm -= ND6_SLOWTIMER_INTERVAL) <= 0) { --- //depot/vendor/freebsd/src/sys/netinet6/raw_ip6.c 2004/11/08 14:45:39 +++ //depot/user/rwatson/netperf/sys/netinet6/raw_ip6.c 2004/11/14 11:53:24 @@ -640,6 +640,7 @@ { struct inpcb *inp = sotoinpcb(so); + /* Unlocked read. */ if ((so->so_state & SS_ISCONNECTED) == 0) return ENOTCONN; inp->in6p_faddr = in6addr_any; --- //depot/vendor/freebsd/src/sys/netipsec/keysock.c 2004/11/08 14:45:39 +++ //depot/user/rwatson/netperf/sys/netipsec/keysock.c 2004/11/14 11:53:24 @@ -306,6 +306,11 @@ pfkeystat.in_msgtype[msg->sadb_msg_type]++; } + /* + * XXXRW: This will almost certainly cause lock order problems; + * we may need a netisr solution similar to that used in rtsock. + */ + mtx_lock(&rawcb_mtx); LIST_FOREACH(rp, &rawcb_list, list) { if (rp->rcb_proto.sp_family != PF_KEY) @@ -354,18 +359,21 @@ continue; if ((n = m_copy(m, 0, (int)M_COPYALL)) == NULL) { + mtx_unlock(&rawcb_mtx); m_freem(m); pfkeystat.in_nomem++; return ENOBUFS; } if ((error = key_sendup0(rp, n, 0)) != 0) { + mtx_unlock(&rawcb_mtx); m_freem(m); return error; } n = NULL; } + mtx_lock(&rawcb_mtx); if (so) { error = key_sendup0(sotorawcb(so), m, 0); --- //depot/vendor/freebsd/src/sys/netipx/ipx_input.c 2003/11/08 22:30:39 +++ //depot/user/rwatson/netperf/sys/netipx/ipx_input.c 2004/03/19 10:34:30 @@ -72,23 +72,39 @@ SYSCTL_INT(_net_ipx, OID_AUTO, ipxnetbios, CTLFLAG_RW, &ipxnetbios, 0, ""); -union ipx_net ipx_zeronet; -union ipx_host ipx_zerohost; +const union ipx_net ipx_zeronet; +const union ipx_host ipx_zerohost; -union ipx_net ipx_broadnet; -union ipx_host ipx_broadhost; +const union ipx_net ipx_broadnet = { .s_net[0] = 0xffff, + .s_net[1] = 0xffff }; +const union ipx_host ipx_broadhost = { .s_host[0] = 0xffff, + .s_host[1] = 0xffff, + .s_host[2] = 0xffff }; +/* + * XXXRW: Locking needed here. + */ struct ipxstat ipxstat; + +/* + * XXXRW: These should/could also be const, since they're set only at + * init time. + */ struct sockaddr_ipx ipx_netmask, ipx_hostmask; -static u_short allones[] = {-1, -1, -1}; +/* + * XXXRW: Locking needed here. + */ +u_short ipxpcb_lport_cache; +struct ipxpcbhead ipxpcb_list; +struct ipxpcbhead ipxrawpcb_list; -struct ipxpcb ipxpcb; -struct ipxpcb ipxrawpcb; - static int ipxqmaxlen = IFQ_MAXLEN; static struct ifqueue ipxintrq; +/* + * XXXRW: Locking needed here. + */ long ipx_pexseq; static int ipx_do_route(struct ipx_addr *src, struct route *ro); @@ -103,13 +119,14 @@ void ipx_init() { - ipx_broadnet = *(union ipx_net *)allones; - ipx_broadhost = *(union ipx_host *)allones; read_random(&ipx_pexseq, sizeof ipx_pexseq); - ipxpcb.ipxp_next = ipxpcb.ipxp_prev = &ipxpcb; - ipxrawpcb.ipxp_next = ipxrawpcb.ipxp_prev = &ipxrawpcb; - + LIST_INIT(&ipxpcb_list); + LIST_INIT(&ipxrawpcb_list); + + /* + * XXXRW: These should be const? + */ ipx_netmask.sipx_len = 6; ipx_netmask.sipx_addr.x_net = ipx_broadnet; @@ -133,6 +150,9 @@ struct ipx_ifaddr *ia; int len; + /* + * XXXRW: Would be nice to remove this. + */ GIANT_REQUIRED; /* @@ -153,8 +173,7 @@ /* * Give any raw listeners a crack at the packet */ - for (ipxp = ipxrawpcb.ipxp_next; ipxp != &ipxrawpcb; - ipxp = ipxp->ipxp_next) { + LIST_FOREACH(ipxp, &ipxrawpcb_list, ipxp_list) { struct mbuf *m1 = m_copy(m, 0, (int)M_COPYALL); if (m1 != NULL) ipx_input(m1, ipxp); @@ -467,8 +486,7 @@ /* * Give any raw listeners a crack at the packet */ - for (ipxp = ipxrawpcb.ipxp_next; ipxp != &ipxrawpcb; - ipxp = ipxp->ipxp_next) { + LIST_FOREACH(ipxp, &ipxrawpcb_list, ipxp_list) { struct mbuf *m0 = m_copy(m, 0, (int)M_COPYALL); if (m0 != NULL) { register struct ipx *ipx; --- //depot/vendor/freebsd/src/sys/netipx/ipx_pcb.c 2004/10/18 22:20:34 +++ //depot/user/rwatson/netperf/sys/netipx/ipx_pcb.c 2004/10/19 18:01:38 @@ -56,7 +56,7 @@ int ipx_pcballoc(so, head, td) struct socket *so; - struct ipxpcb *head; + struct ipxpcbhead *head; struct thread *td; { register struct ipxpcb *ipxp; @@ -107,13 +107,16 @@ } ipxp->ipxp_laddr = sipx->sipx_addr; noname: + /* + * XXXRW: I wonder what causes this loop to terminate... + */ if (lport == 0) do { - ipxpcb.ipxp_lport++; - if ((ipxpcb.ipxp_lport < IPXPORT_RESERVED) || - (ipxpcb.ipxp_lport >= IPXPORT_WELLKNOWN)) - ipxpcb.ipxp_lport = IPXPORT_RESERVED; - lport = htons(ipxpcb.ipxp_lport); + ipxpcb_lport_cache++; + if ((ipxpcb_lport_cache < IPXPORT_RESERVED) || + (ipxpcb_lport_cache >= IPXPORT_WELLKNOWN)) + ipxpcb_lport_cache = IPXPORT_RESERVED; + lport = htons(ipxpcb_lport_cache); } while (ipx_pcblookup(&zeroipx_addr, lport, 0)); ipxp->ipxp_lport = lport; return (0); @@ -325,18 +328,27 @@ register struct ipxpcb *ipxp, *oinp; int s = splimp(); - for (ipxp = (&ipxpcb)->ipxp_next; ipxp != (&ipxpcb);) { + for (ipxp = LIST_FIRST(&ipxpcb_list); ipxp != NULL;) { if (!ipx_hosteq(*dst,ipxp->ipxp_faddr)) { - next: - ipxp = ipxp->ipxp_next; +next: + ipxp = LIST_NEXT(ipxp, ipxp_list); + } + if (ipxp->ipxp_socket == 0) { + goto next; continue; } - if (ipxp->ipxp_socket == 0) - goto next; if (errno) ipxp->ipxp_socket->so_error = errno; + /* + * XXXRW: I can't find any consumers of this interface, and + * so don't know if calling the notify function could result + * in the ipxp list pointers changing. Before moving this to + * the queue(9) macros, there was some fancy footwork here + * that didn't seem to be useful. If the list can be changed + * by a notification, it will make locking very difficult. + */ oinp = ipxp; - ipxp = ipxp->ipxp_next; + ipxp = LIST_NEXT(ipxp, ipxp_list); oinp->ipxp_notify_param = param; (*notify)(oinp); } @@ -374,7 +386,7 @@ u_short fport; fport = faddr->x_port; - for (ipxp = (&ipxpcb)->ipxp_next; ipxp != (&ipxpcb); ipxp = ipxp->ipxp_next) { + LIST_FOREACH(ipxp, &ipxpcb_list, ipxp_list) { if (ipxp->ipxp_lport != lport) continue; wildcard = 0; --- //depot/vendor/freebsd/src/sys/netipx/ipx_pcb.h 2003/01/01 18:50:52 +++ //depot/user/rwatson/netperf/sys/netipx/ipx_pcb.h 2004/03/19 10:34:30 @@ -43,9 +43,7 @@ * IPX protocol interface control block. */ struct ipxpcb { - struct ipxpcb *ipxp_next; /* doubly linked list */ - struct ipxpcb *ipxp_prev; - struct ipxpcb *ipxp_head; + LIST_ENTRY(ipxpcb) ipxp_list; /* list of ipxpcbs */ struct socket *ipxp_socket; /* back pointer to socket */ struct ipx_addr ipxp_faddr; /* destination address */ struct ipx_addr ipxp_laddr; /* socket's address */ @@ -58,6 +56,11 @@ u_char ipxp_rpt; /* last received packet type by ipx_input() */ }; +LIST_HEAD(ipxpcbhead, ipxpcb); +extern struct ipxpcbhead ipxpcb_list; +extern struct ipxpcbhead ipxrawpcb_list; +extern u_short ipxpcb_lport_cache; + /* possible flags */ #define IPXP_IN_ABORT 0x1 /* calling abort through socket */ @@ -82,7 +85,7 @@ #ifdef _KERNEL extern struct ipxpcb ipxpcb; /* head of list */ -int ipx_pcballoc(struct socket *so, struct ipxpcb *head, +int ipx_pcballoc(struct socket *so, struct ipxpcbhead *head, struct thread *p); int ipx_pcbbind(struct ipxpcb *ipxp, struct sockaddr *nam, struct thread *p); --- //depot/vendor/freebsd/src/sys/netipx/ipx_usrreq.c 2004/11/08 14:45:39 +++ //depot/user/rwatson/netperf/sys/netipx/ipx_usrreq.c 2004/11/14 11:53:24 @@ -455,7 +455,7 @@ if (ipxp != NULL) return (EINVAL); s = splnet(); - error = ipx_pcballoc(so, &ipxpcb, td); + error = ipx_pcballoc(so, &ipxpcb_list, td); splx(s); if (error == 0) error = soreserve(so, ipxsendspace, ipxrecvspace); @@ -616,7 +616,7 @@ if (td != NULL && (error = suser(td)) != 0) return (error); s = splnet(); - error = ipx_pcballoc(so, &ipxrawpcb, td); + error = ipx_pcballoc(so, &ipxrawpcb_list, td); splx(s); if (error) return (error); --- //depot/vendor/freebsd/src/sys/netipx/ipx_var.h 2003/03/04 23:20:46 +++ //depot/user/rwatson/netperf/sys/netipx/ipx_var.h 2004/03/19 10:34:30 @@ -66,19 +66,19 @@ extern int ipxcksum; extern long ipx_pexseq; extern struct ipxstat ipxstat; -extern struct ipxpcb ipxrawpcb; extern struct pr_usrreqs ipx_usrreqs; extern struct pr_usrreqs ripx_usrreqs; extern struct sockaddr_ipx ipx_netmask; extern struct sockaddr_ipx ipx_hostmask; -extern union ipx_net ipx_zeronet; -extern union ipx_host ipx_zerohost; -extern union ipx_net ipx_broadnet; -extern union ipx_host ipx_broadhost; +extern const union ipx_net ipx_zeronet; +extern const union ipx_host ipx_zerohost; +extern const union ipx_net ipx_broadnet; +extern const union ipx_host ipx_broadhost; struct ifnet; struct ipx_addr; +struct ipxpcb; struct mbuf; struct thread; struct route; --- //depot/vendor/freebsd/src/sys/netipx/spx_usrreq.c 2004/11/08 14:45:39 +++ //depot/user/rwatson/netperf/sys/netipx/spx_usrreq.c 2004/11/14 11:53:24 @@ -1360,7 +1360,7 @@ if (ipxp != NULL) return (EISCONN); s = splnet(); - error = ipx_pcballoc(so, &ipxpcb, td); + error = ipx_pcballoc(so, &ipxpcb_list, td); if (error) goto spx_attach_end; if (so->so_snd.sb_hiwat == 0 || so->so_rcv.sb_hiwat == 0) { @@ -1571,6 +1571,7 @@ ipxp = sotoipxpcb(so); cb = ipxtospxpcb(ipxp); + /* XXXRW: sb_state locking? */ if ((cb->s_oobflags & SF_IOOB) || so->so_oobmark || (so->so_rcv.sb_state & SBS_RCVATMARK)) { m->m_len = 1; @@ -1781,9 +1782,7 @@ register struct spxpcb *cb; int s = splnet(); - ipxp = ipxpcb.ipxp_next; - if (ipxp != NULL) - for (; ipxp != &ipxpcb; ipxp = ipxp->ipxp_next) + LIST_FOREACH(ipxp, &ipxpcb_list, ipxp_list) { if ((cb = (struct spxpcb *)ipxp->ipxp_pcb) != NULL && (cb->s_flags & SF_DELACK)) { cb->s_flags &= ~SF_DELACK; @@ -1791,6 +1790,7 @@ spxstat.spxs_delack++; spx_output(cb, (struct mbuf *)NULL); } + } splx(s); } @@ -1809,15 +1809,15 @@ /* * Search through tcb's and update active timers. + * + * XXXRW: spx_timers() may remove an ipxpcb entry, so we have to be + * ready to continue despite that. The logic here is a bit + * obfuscated. */ - ip = ipxpcb.ipxp_next; - if (ip == NULL) { - splx(s); - return; - } - while (ip != &ipxpcb) { + ip = LIST_FIRST(&ipxpcb_list); + while (ip != NULL) { + ipnxt = LIST_NEXT(ip, ipxp_list); cb = ipxtospxpcb(ip); - ipnxt = ip->ipxp_next; if (cb == NULL) goto tpgone; for (i = 0; i < SPXT_NTIMERS; i++) { @@ -1831,7 +1831,7 @@ if (cb->s_rtt) cb->s_rtt++; tpgone: - ip = ipnxt; + ip = LIST_NEXT(ip, ipxp_list); } spx_iss += SPX_ISSINCR/PR_SLOWHZ; /* increment iss */ splx(s); --- //depot/vendor/freebsd/src/sys/netkey/keysock.c 2004/11/08 14:45:39 +++ //depot/user/rwatson/netperf/sys/netkey/keysock.c 2004/11/14 11:53:24 @@ -214,6 +214,11 @@ pfkeystat.in_msgtype[msg->sadb_msg_type]++; } + /* + * XXXRW: This will almost certainly cause lock order problems; + * we may need a netisr solution similar to that used in rtsock. + */ + mtx_lock(&rawcb_mtx); LIST_FOREACH(rp, &rawcb_list, list) { if (rp->rcb_proto.sp_family != PF_KEY) continue; @@ -261,6 +266,7 @@ continue; if ((n = m_copy(m, 0, (int)M_COPYALL)) == NULL) { + mtx_unlock(&rawcb_mtx); m_freem(m); pfkeystat.in_nomem++; return ENOBUFS; @@ -275,6 +281,7 @@ n = NULL; } + mtx_unlock(&rawcb_mtx); if (so) { error = key_sendup0(sotorawcb(so), m, 0); --- //depot/vendor/freebsd/src/sys/netsmb/smb_trantcp.c 2004/06/17 22:51:43 +++ //depot/user/rwatson/netperf/sys/netsmb/smb_trantcp.c 2004/06/18 00:24:39 @@ -184,6 +184,9 @@ return 0; } +/* + * XXXRW: nb_upcall() is called without Giant, which is probably safeish. + */ static void nb_upcall(struct socket *so, void *arg, int waitflag) { --- //depot/vendor/freebsd/src/sys/nfsclient/bootp_subr.c 2004/07/28 21:55:45 +++ //depot/user/rwatson/netperf/sys/nfsclient/bootp_subr.c 2004/08/02 01:36:03 @@ -983,7 +983,7 @@ struct ifaddr *ifa; struct sockaddr_dl *sdl; - GIANT_REQUIRED; /* XXX until socket locking done */ + NET_ASSERT_GIANT(); error = socreate(AF_INET, &ifctx->so, SOCK_DGRAM, 0, td->td_ucred, td); if (error != 0) --- //depot/vendor/freebsd/src/sys/nfsclient/nfs_socket.c 2004/08/25 01:25:22 +++ //depot/user/rwatson/netperf/sys/nfsclient/nfs_socket.c 2004/08/25 20:28:45 @@ -1197,6 +1197,7 @@ * Set r_rtt to -1 in case we fail to send it now. */ rep->r_rtt = -1; + /* XXXRW: Unlocked reads. */ if (sbspace(&so->so_snd) >= rep->r_mreq->m_pkthdr.len && ((nmp->nm_flag & NFSMNT_DUMBTIMR) || (rep->r_flags & R_SENT) || --- //depot/vendor/freebsd/src/sys/nfsclient/nfs_vfsops.c 2004/12/01 06:45:17 +++ //depot/user/rwatson/netperf/sys/nfsclient/nfs_vfsops.c 2004/12/04 17:43:45 @@ -394,7 +394,7 @@ u_long l; char buf[128]; - GIANT_REQUIRED; /* XXX until socket locking done */ + NET_ASSERT_GIANT(); #if defined(BOOTP_NFSROOT) && defined(BOOTP) bootpc_init(); /* use bootp to get nfs_diskless filled in */ --- //depot/vendor/freebsd/src/sys/nfsserver/nfs_syscalls.c 2004/06/17 22:51:43 +++ //depot/user/rwatson/netperf/sys/nfsserver/nfs_syscalls.c 2004/06/18 00:24:39 @@ -607,6 +607,10 @@ NFSD_UNLOCK(); slp->ns_fp = NULL; so = slp->ns_so; + /* + * XXXRW: More general locking issues here relating to + * upcalls. + */ SOCKBUF_LOCK(&so->so_rcv); so->so_rcv.sb_flags &= ~SB_UPCALL; SOCKBUF_UNLOCK(&so->so_rcv); --- //depot/vendor/freebsd/src/sys/pci/if_dc.c 2004/10/01 15:25:23 +++ //depot/user/rwatson/netperf/sys/pci/if_dc.c 2004/10/19 20:17:38 @@ -349,8 +349,6 @@ #define SIO_SET(x) DC_SETBIT(sc, DC_SIO, (x)) #define SIO_CLR(x) DC_CLRBIT(sc, DC_SIO, (x)) -#define IS_MPSAFE 0 - static void dc_delay(struct dc_softc *sc) { @@ -2192,8 +2190,6 @@ /* XXX: bleah, MTU gets overwritten in ether_ifattach() */ ifp->if_mtu = ETHERMTU; ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST; - if (!IS_MPSAFE) - ifp->if_flags |= IFF_NEEDSGIANT; ifp->if_ioctl = dc_ioctl; ifp->if_start = dc_start; ifp->if_watchdog = dc_watchdog; @@ -2273,7 +2269,7 @@ #endif ifp->if_capenable = ifp->if_capabilities; - callout_init(&sc->dc_stat_ch, IS_MPSAFE ? CALLOUT_MPSAFE : 0); + callout_init(&sc->dc_stat_ch, debug_mpsafenet ? CALLOUT_MPSAFE : 0); #ifdef SRM_MEDIA sc->dc_srm_media = 0; @@ -2307,8 +2303,7 @@ ether_ifattach(ifp, eaddr); /* Hook interrupt last to avoid having to lock softc */ - error = bus_setup_intr(dev, sc->dc_irq, INTR_TYPE_NET | - (IS_MPSAFE ? INTR_MPSAFE : 0), + error = bus_setup_intr(dev, sc->dc_irq, INTR_TYPE_NET | INTR_MPSAFE, dc_intr, sc, &sc->dc_intrhand); if (error) { --- //depot/vendor/freebsd/src/sys/pci/if_pcn.c 2004/12/03 18:35:57 +++ //depot/user/rwatson/netperf/sys/pci/if_pcn.c 2004/12/04 17:43:45 @@ -628,7 +628,7 @@ ether_ifattach(ifp, (u_int8_t *) eaddr); /* Hook interrupt last to avoid having to lock softc */ - error = bus_setup_intr(dev, sc->pcn_irq, INTR_TYPE_NET, + error = bus_setup_intr(dev, sc->pcn_irq, INTR_TYPE_NET | INTR_MPSAFE, pcn_intr, sc, &sc->pcn_intrhand); if (error) { --- //depot/vendor/freebsd/src/sys/rpc/rpcclnt.c 2004/07/15 22:25:35 +++ //depot/user/rwatson/netperf/sys/rpc/rpcclnt.c 2004/07/19 22:28:50 @@ -145,7 +145,7 @@ /* XXX ugly debug strings */ #define RPC_ERRSTR_ACCEPTED_SIZE 6 -char *rpc_errstr_accepted[RPC_ERRSTR_ACCEPTED_SIZE] = { +const char *rpc_errstr_accepted[RPC_ERRSTR_ACCEPTED_SIZE] = { "", /* no good message... */ "remote server hasn't exported program.", "remote server can't support version number.", @@ -154,13 +154,13 @@ "remote error. remote side memory allocation failure?" }; -char *rpc_errstr_denied[2] = { +const char *rpc_errstr_denied[2] = { "remote server doesnt support rpc version 2!", "remote server authentication error." }; #define RPC_ERRSTR_AUTH_SIZE 6 -char *rpc_errstr_auth[RPC_ERRSTR_AUTH_SIZE] = { +const char *rpc_errstr_auth[RPC_ERRSTR_AUTH_SIZE] = { "", "auth error: bad credential (seal broken).", "auth error: client must begin new session.", @@ -360,7 +360,7 @@ RPC_RETURN(EFAULT); } - GIANT_REQUIRED; /* XXX until socket locking done */ + NET_ASSERT_GIANT(); /* create the socket */ rpc->rc_so = NULL; @@ -618,7 +618,7 @@ { struct socket *so; - GIANT_REQUIRED; /* XXX until socket locking done */ + NET_ASSERT_GIANT(); if (rpc->rc_so) { so = rpc->rc_so; @@ -669,7 +669,7 @@ #endif int error, soflags, flags; - GIANT_REQUIRED; /* XXX until socket locking done */ + NET_ASSERT_GIANT(); if (rep) { if (rep->r_flags & R_SOFTTERM) { @@ -754,7 +754,7 @@ #endif int error, sotype, rcvflg; - GIANT_REQUIRED; /* XXX until socket locking done */ + NET_ASSERT_GIANT(); /* * Set up arguments for soreceive() @@ -1439,6 +1439,7 @@ * Set r_rtt to -1 in case we fail to send it now. */ rep->r_rtt = -1; + SOCKBUF_LOCK(&so->so_snd); if (sbspace(&so->so_snd) >= rep->r_mreq->m_pkthdr.len && ((rpc->rc_flag & RPCCLNT_DUMBTIMR) || (rep->r_flags & R_SENT) || @@ -1473,6 +1474,7 @@ rep->r_rtt = 0; } } + SOCKBUF_UNLOCK(&so->so_snd); } splx(s); --- //depot/vendor/freebsd/src/sys/security/mac/mac_net.c 2004/06/24 03:35:33 +++ //depot/user/rwatson/netperf/sys/security/mac/mac_net.c 2004/06/24 03:49:05 @@ -260,6 +260,11 @@ MAC_PERFORM(copy_ifnet_label, src, dest); } +/* + * XXXRW: Need to use a routine to copy the ifnet label while holding the + * ifnet mutex, similar to sockets, pipes, vnodes, et al, to avoid holding + * the mutex over the copyin/copyout. + */ static int mac_externalize_ifnet_label(struct label *label, char *elements, char *outbuf, size_t outbuflen) @@ -271,6 +276,9 @@ return (error); } +/* + * XXXRW: See comment for mac_externalize_ifnet_label(). + */ static int mac_internalize_ifnet_label(struct label *label, char *string) { --- //depot/vendor/freebsd/src/sys/sys/socketvar.h 2004/10/18 22:20:34 +++ //depot/user/rwatson/netperf/sys/sys/socketvar.h 2004/10/19 18:01:38 @@ -356,6 +356,7 @@ SOCK_UNLOCK(so); \ ACCEPT_UNLOCK(); \ } \ + so = NULL; \ } while (0) #define sotryfree(so) do { \ @@ -367,6 +368,7 @@ SOCK_UNLOCK(so); \ ACCEPT_UNLOCK(); \ } \ + so = NULL; \ } while(0) /*