On Thu, May 27, 2021 at 07:53:23PM +0000, Mark Johnston wrote:
> The branch main has been updated by markj:
>
> URL:
> https://cgit.FreeBSD.org/src/commit/?id=f3851b235b23d9220ace31bbc89b1fe0a78fc75c
>
> commit f3851b235b23d9220ace31bbc89b1fe0a78fc75c
> Author: Mark Johnston <[email protected]>
> AuthorDate: 2021-05-27 19:49:59 +0000
> Commit: Mark Johnston <[email protected]>
> CommitDate: 2021-05-27 19:52:20 +0000
>
> ktrace: Fix a race with fork()
>
> ktrace(2) may toggle trace points in any of
> 1. a single process
> 2. all members of a process group
> 3. all descendents of the processes in 1 or 2
>
> In the first two cases, we do not permit the operation if the process is
> being forked or not visible. However, in case 3 we did not enforce this
> restriction for descendents. As a result, the assertions about the child
> in ktrprocfork() may be violated.
>
> Move these checks into ktrops() so that they are applied consistently.
>
> Allow KTROP_CLEAR for nascent processes. Otherwise, there is a window
> where we cannot clear trace points for a nascent child if they are
> inherited from the parent.
>
> Reported by: [email protected]
> Reported by: [email protected]
> Reviewed by: kib
> MFC after: 1 week
> Sponsored by: The FreeBSD Foundation
> Differential Revision: https://reviews.freebsd.org/D30481
> ---
> sys/kern/kern_ktrace.c | 43 ++++++++++++++++++++++---------------------
> 1 file changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/sys/kern/kern_ktrace.c b/sys/kern/kern_ktrace.c
> index dc064d9ebd67..4a2dad20b035 100644
> --- a/sys/kern/kern_ktrace.c
> +++ b/sys/kern/kern_ktrace.c
> @@ -1006,7 +1006,7 @@ sys_ktrace(struct thread *td, struct ktrace_args *uap)
> int facs = uap->facs & ~KTRFAC_ROOT;
> int ops = KTROP(uap->ops);
> int descend = uap->ops & KTRFLAG_DESCEND;
> - int nfound, ret = 0;
> + int ret = 0;
> int flags, error = 0;
> struct nameidata nd;
> struct ktr_io_params *kiop, *old_kiop;
> @@ -1080,42 +1080,31 @@ restart:
> error = ESRCH;
> goto done;
> }
> +
> /*
> * ktrops() may call vrele(). Lock pg_members
> * by the proctree_lock rather than pg_mtx.
> */
> PGRP_UNLOCK(pg);
> - nfound = 0;
> + if (LIST_EMPTY(&pg->pg_members)) {
> + sx_sunlock(&proctree_lock);
> + error = ESRCH;
> + goto done;
> + }
> LIST_FOREACH(p, &pg->pg_members, p_pglist) {
> PROC_LOCK(p);
> - if (p->p_state == PRS_NEW ||
> - p_cansee(td, p) != 0) {
> - PROC_UNLOCK(p);
> - continue;
> - }
> - nfound++;
> if (descend)
> ret |= ktrsetchildren(td, p, ops, facs, kiop);
> else
> ret |= ktrops(td, p, ops, facs, kiop);
> }
> - if (nfound == 0) {
> - sx_sunlock(&proctree_lock);
> - error = ESRCH;
> - goto done;
> - }
> } else {
> /*
> * by pid
> */
> p = pfind(uap->pid);
> - if (p == NULL)
> + if (p == NULL) {
> error = ESRCH;
> - else
> - error = p_cansee(td, p);
> - if (error) {
> - if (p != NULL)
> - PROC_UNLOCK(p);
> sx_sunlock(&proctree_lock);
> goto done;
> }
> @@ -1187,8 +1176,20 @@ ktrops(struct thread *td, struct proc *p, int ops, int
> facs,
> PROC_UNLOCK(p);
> return (0);
> }
> - if (p->p_flag & P_WEXIT) {
> - /* If the process is exiting, just ignore it. */
> + if ((ops == KTROP_SET && p->p_state == PRS_NEW) || !p_cansee(td, p)) {
^^^^^^^^^^^^^^ seems that it broke ktrace:
root@mordor:~ # ktrace ls
ktrace: ktrace.out: Operation not permitted
> + /*
> + * Disallow setting trace points if the process is being born.
> + * This avoids races with trace point inheritance in
> + * ktrprocfork().
> + */
> + PROC_UNLOCK(p);
> + return (0);
> + }
> + if ((p->p_flag & P_WEXIT) != 0) {
> + /*
> + * There's nothing to do if the process is exiting, but avoid
> + * signaling an error.
> + */
> PROC_UNLOCK(p);
> return (1);
> }
_______________________________________________
[email protected] mailing list
https://lists.freebsd.org/mailman/listinfo/dev-commits-src-all
To unsubscribe, send any mail to "[email protected]"
