Author: kib Date: Wed Nov 1 11:43:39 2017 New Revision: 325277 URL: https://svnweb.freebsd.org/changeset/base/325277
Log: Do not run pmclog_configure_log() without pmc_sx protection. The r195005 unlocked pmc_sx before calling into pmclog_configure_log() to avoid the LOR, but it allows flush or closelog to run in parallel with the configuration, causing many failure modes. Revert r195005. Pre-create the logging process, allowing it to run after the set up succeeded, otherwise the process terminates itself. Reported and tested by: pho Reviewed by: markj Sponsored by: The FreeBSD Foundation MFC after: 1 week Differential revision: https://reviews.freebsd.org/D12882 Modified: head/sys/dev/hwpmc/hwpmc_logging.c head/sys/dev/hwpmc/hwpmc_mod.c head/sys/sys/pmclog.h Modified: head/sys/dev/hwpmc/hwpmc_logging.c ============================================================================== --- head/sys/dev/hwpmc/hwpmc_logging.c Wed Nov 1 11:37:45 2017 (r325276) +++ head/sys/dev/hwpmc/hwpmc_logging.c Wed Nov 1 11:43:39 2017 (r325277) @@ -235,6 +235,54 @@ pmclog_get_buffer(struct pmc_owner *po) return (plb ? 0 : ENOMEM); } +struct pmclog_proc_init_args { + struct proc *kthr; + struct pmc_owner *po; + bool exit; + bool acted; +}; + +int +pmclog_proc_create(struct thread *td, void **handlep) +{ + struct pmclog_proc_init_args *ia; + int error; + + ia = malloc(sizeof(*ia), M_TEMP, M_WAITOK | M_ZERO); + error = kproc_create(pmclog_loop, ia, &ia->kthr, + RFHIGHPID, 0, "hwpmc: proc(%d)", td->td_proc->p_pid); + if (error == 0) + *handlep = ia; + return (error); +} + +void +pmclog_proc_ignite(void *handle, struct pmc_owner *po) +{ + struct pmclog_proc_init_args *ia; + + ia = handle; + mtx_lock(&pmc_kthread_mtx); + MPASS(!ia->acted); + MPASS(ia->po == NULL); + MPASS(!ia->exit); + MPASS(ia->kthr != NULL); + if (po == NULL) { + ia->exit = true; + } else { + ia->po = po; + KASSERT(po->po_kthread == NULL, + ("[pmclog,%d] po=%p kthread (%p) already present", + __LINE__, po, po->po_kthread)); + po->po_kthread = ia->kthr; + } + wakeup(ia); + while (!ia->acted) + msleep(ia, &pmc_kthread_mtx, PWAIT, "pmclogw", 0); + mtx_unlock(&pmc_kthread_mtx); + free(ia, M_TEMP); +} + /* * Log handler loop. * @@ -244,7 +292,7 @@ pmclog_get_buffer(struct pmc_owner *po) static void pmclog_loop(void *arg) { - int error; + struct pmclog_proc_init_args *ia; struct pmc_owner *po; struct pmclog_buffer *lb; struct proc *p; @@ -255,15 +303,34 @@ pmclog_loop(void *arg) struct uio auio; struct iovec aiov; size_t nbytes; + int error; - po = (struct pmc_owner *) arg; - p = po->po_owner; td = curthread; SIGEMPTYSET(unb); SIGADDSET(unb, SIGHUP); (void)kern_sigprocmask(td, SIG_UNBLOCK, &unb, NULL, 0); + ia = arg; + MPASS(ia->kthr == curproc); + MPASS(!ia->acted); + mtx_lock(&pmc_kthread_mtx); + while (ia->po == NULL && !ia->exit) + msleep(ia, &pmc_kthread_mtx, PWAIT, "pmclogi", 0); + if (ia->exit) { + ia->acted = true; + wakeup(ia); + mtx_unlock(&pmc_kthread_mtx); + kproc_exit(0); + } + MPASS(ia->po != NULL); + po = ia->po; + ia->acted = true; + wakeup(ia); + mtx_unlock(&pmc_kthread_mtx); + ia = NULL; + + p = po->po_owner; mycred = td->td_ucred; PROC_LOCK(p); @@ -568,15 +635,11 @@ pmclog_stop_kthread(struct pmc_owner *po) int pmclog_configure_log(struct pmc_mdep *md, struct pmc_owner *po, int logfd) { - int error; struct proc *p; cap_rights_t rights; - /* - * As long as it is possible to get a LOR between pmc_sx lock and - * proctree/allproc sx locks used for adding a new process, assure - * the former is not held here. - */ - sx_assert(&pmc_sx, SA_UNLOCKED); + int error; + + sx_assert(&pmc_sx, SA_XLOCKED); PMCDBG2(LOG,CFG,1, "config po=%p logfd=%d", po, logfd); p = po->po_owner; @@ -585,9 +648,6 @@ pmclog_configure_log(struct pmc_mdep *md, struct pmc_o if (po->po_flags & PMC_PO_OWNS_LOGFILE) return (EBUSY); - KASSERT(po->po_kthread == NULL, - ("[pmclog,%d] po=%p kthread (%p) already present", __LINE__, po, - po->po_kthread)); KASSERT(po->po_file == NULL, ("[pmclog,%d] po=%p file (%p) already present", __LINE__, po, po->po_file)); @@ -600,10 +660,6 @@ pmclog_configure_log(struct pmc_mdep *md, struct pmc_o /* mark process as owning a log file */ po->po_flags |= PMC_PO_OWNS_LOGFILE; - error = kproc_create(pmclog_loop, po, &po->po_kthread, - RFHIGHPID, 0, "hwpmc: proc(%d)", p->p_pid); - if (error) - goto error; /* mark process as using HWPMCs */ PROC_LOCK(p); @@ -620,10 +676,6 @@ pmclog_configure_log(struct pmc_mdep *md, struct pmc_o return (0); error: - /* shutdown the thread */ - if (po->po_kthread) - pmclog_stop_kthread(po); - KASSERT(po->po_kthread == NULL, ("[pmclog,%d] po=%p kthread not " "stopped", __LINE__, po)); Modified: head/sys/dev/hwpmc/hwpmc_mod.c ============================================================================== --- head/sys/dev/hwpmc/hwpmc_mod.c Wed Nov 1 11:37:45 2017 (r325276) +++ head/sys/dev/hwpmc/hwpmc_mod.c Wed Nov 1 11:43:39 2017 (r325277) @@ -2854,20 +2854,31 @@ static const char *pmc_op_to_name[] = { static int pmc_syscall_handler(struct thread *td, void *syscall_args) { - int error, is_sx_downgraded, is_sx_locked, op; + int error, is_sx_downgraded, op; struct pmc_syscall_args *c; + void *pmclog_proc_handle; void *arg; - PMC_GET_SX_XLOCK(ENOSYS); - - is_sx_downgraded = 0; - is_sx_locked = 1; - - c = (struct pmc_syscall_args *) syscall_args; - + c = (struct pmc_syscall_args *)syscall_args; op = c->pmop_code; arg = c->pmop_data; + if (op == PMC_OP_CONFIGURELOG) { + /* + * We cannot create the logging process inside + * pmclog_configure_log() because there is a LOR + * between pmc_sx and process structure locks. + * Instead, pre-create the process and ignite the loop + * if everything is fine, otherwise direct the process + * to exit. + */ + error = pmclog_proc_create(td, &pmclog_proc_handle); + if (error != 0) + goto done_syscall; + } + PMC_GET_SX_XLOCK(ENOSYS); + is_sx_downgraded = 0; + PMCDBG3(MOD,PMS,1, "syscall op=%d \"%s\" arg=%p", op, pmc_op_to_name[op], arg); @@ -2890,15 +2901,16 @@ pmc_syscall_handler(struct thread *td, void *syscall_a struct pmc_owner *po; struct pmc_op_configurelog cl; - sx_assert(&pmc_sx, SX_XLOCKED); - - if ((error = copyin(arg, &cl, sizeof(cl))) != 0) + if ((error = copyin(arg, &cl, sizeof(cl))) != 0) { + pmclog_proc_ignite(pmclog_proc_handle, NULL); break; + } /* mark this process as owning a log file */ p = td->td_proc; if ((po = pmc_find_owner_descriptor(p)) == NULL) if ((po = pmc_allocate_owner_descriptor(p)) == NULL) { + pmclog_proc_ignite(pmclog_proc_handle, NULL); error = ENOMEM; break; } @@ -2910,10 +2922,11 @@ pmc_syscall_handler(struct thread *td, void *syscall_a * de-configure it. */ if (cl.pm_logfd >= 0) { - sx_xunlock(&pmc_sx); - is_sx_locked = 0; error = pmclog_configure_log(md, po, cl.pm_logfd); + pmclog_proc_ignite(pmclog_proc_handle, error == 0 ? + po : NULL); } else if (po->po_flags & PMC_PO_OWNS_LOGFILE) { + pmclog_proc_ignite(pmclog_proc_handle, NULL); pmclog_process_closelog(po); error = pmclog_close(po); if (error == 0) { @@ -2923,11 +2936,10 @@ pmc_syscall_handler(struct thread *td, void *syscall_a pmc_stop(pm); error = pmclog_deconfigure_log(po); } - } else + } else { + pmclog_proc_ignite(pmclog_proc_handle, NULL); error = EINVAL; - - if (error) - break; + } } break; @@ -4022,13 +4034,11 @@ pmc_syscall_handler(struct thread *td, void *syscall_a break; } - if (is_sx_locked != 0) { - if (is_sx_downgraded) - sx_sunlock(&pmc_sx); - else - sx_xunlock(&pmc_sx); - } - + if (is_sx_downgraded) + sx_sunlock(&pmc_sx); + else + sx_xunlock(&pmc_sx); +done_syscall: if (error) atomic_add_int(&pmc_stats.pm_syscall_errors, 1); Modified: head/sys/sys/pmclog.h ============================================================================== --- head/sys/sys/pmclog.h Wed Nov 1 11:37:45 2017 (r325276) +++ head/sys/sys/pmclog.h Wed Nov 1 11:43:39 2017 (r325277) @@ -260,6 +260,8 @@ int pmclog_deconfigure_log(struct pmc_owner *_po); int pmclog_flush(struct pmc_owner *_po); int pmclog_close(struct pmc_owner *_po); void pmclog_initialize(void); +int pmclog_proc_create(struct thread *td, void **handlep); +void pmclog_proc_ignite(void *handle, struct pmc_owner *po); void pmclog_process_callchain(struct pmc *_pm, struct pmc_sample *_ps); void pmclog_process_closelog(struct pmc_owner *po); void pmclog_process_dropnotify(struct pmc_owner *po); _______________________________________________ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
