Re: git: 7ae879b14a20 - main - kern_procctl(): convert the function to be table-driven
Date: Tue, 19 Oct 2021 20:35:13 UTC
In message <202110192004.19JK4jN3069844@gitrepo.freebsd.org>, Konstantin
Belous
ov writes:
> The branch main has been updated by kib:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=7ae879b14a2086df521c59c4a379d3a0
> 72e08bc6
>
> commit 7ae879b14a2086df521c59c4a379d3a072e08bc6
> Author: Konstantin Belousov <kib@FreeBSD.org>
> AuthorDate: 2021-10-15 18:57:17 +0000
> Commit: Konstantin Belousov <kib@FreeBSD.org>
> CommitDate: 2021-10-19 20:04:34 +0000
>
> kern_procctl(): convert the function to be table-driven
>
> Reviewed by: emaste, markj
> Sponsored by: The FreeBSD Foundation
> MFC after: 1 week
> Differential revision: https://reviews.freebsd.org/D32513
> ---
> sys/kern/kern_procctl.c | 123 +++++++++++++++++++++++++++-------------------
> --
> 1 file changed, 69 insertions(+), 54 deletions(-)
>
> diff --git a/sys/kern/kern_procctl.c b/sys/kern/kern_procctl.c
> index eb36f0822938..90c5e63c7219 100644
> --- a/sys/kern/kern_procctl.c
> +++ b/sys/kern/kern_procctl.c
> @@ -656,6 +656,57 @@ wxmap_status(struct thread *td, struct proc *p, int *dat
> a)
> return (0);
> }
>
> +struct procctl_cmd_info {
> + int lock_tree;
> + bool one_proc : 1;
> +};
> +static const struct procctl_cmd_info procctl_cmds_info[] = {
> + [PROC_SPROTECT] =
> + { .lock_tree = SA_SLOCKED, .one_proc = false, },
> + [PROC_REAP_ACQUIRE] =
> + { .lock_tree = SA_XLOCKED, .one_proc = true, },
> + [PROC_REAP_RELEASE] =
> + { .lock_tree = SA_XLOCKED, .one_proc = true, },
> + [PROC_REAP_STATUS] =
> + { .lock_tree = SA_SLOCKED, .one_proc = true, },
> + [PROC_REAP_GETPIDS] =
> + { .lock_tree = SA_SLOCKED, .one_proc = true, },
> + [PROC_REAP_KILL] =
> + { .lock_tree = SA_SLOCKED, .one_proc = true, },
> + [PROC_TRACE_CTL] =
> + { .lock_tree = SA_SLOCKED, .one_proc = false, },
> + [PROC_TRACE_STATUS] =
> + { .lock_tree = SA_UNLOCKED, .one_proc = true, },
> + [PROC_TRAPCAP_CTL] =
> + { .lock_tree = SA_SLOCKED, .one_proc = false, },
> + [PROC_TRAPCAP_STATUS] =
> + { .lock_tree = SA_UNLOCKED, .one_proc = true, },
> + [PROC_PDEATHSIG_CTL] =
> + { .lock_tree = SA_UNLOCKED, .one_proc = true, },
> + [PROC_PDEATHSIG_STATUS] =
> + { .lock_tree = SA_UNLOCKED, .one_proc = true, },
> + [PROC_ASLR_CTL] =
> + { .lock_tree = SA_UNLOCKED, .one_proc = true, },
> + [PROC_ASLR_STATUS] =
> + { .lock_tree = SA_UNLOCKED, .one_proc = true, },
> + [PROC_PROTMAX_CTL] =
> + { .lock_tree = SA_UNLOCKED, .one_proc = true, },
> + [PROC_PROTMAX_STATUS] =
> + { .lock_tree = SA_UNLOCKED, .one_proc = true, },
> + [PROC_STACKGAP_CTL] =
> + { .lock_tree = SA_UNLOCKED, .one_proc = true, },
> + [PROC_STACKGAP_STATUS] =
> + { .lock_tree = SA_UNLOCKED, .one_proc = true, },
> + [PROC_NO_NEW_PRIVS_CTL] =
> + { .lock_tree = SA_SLOCKED, .one_proc = true, },
> + [PROC_NO_NEW_PRIVS_STATUS] =
> + { .lock_tree = SA_UNLOCKED, .one_proc = true, },
> + [PROC_WXMAP_CTL] =
> + { .lock_tree = SA_UNLOCKED, .one_proc = true, },
> + [PROC_WXMAP_STATUS] =
> + { .lock_tree = SA_UNLOCKED, .one_proc = true, },
> +};
> +
> int
> sys_procctl(struct thread *td, struct procctl_args *uap)
> {
> @@ -812,33 +863,14 @@ kern_procctl(struct thread *td, idtype_t idtype, id_t i
> d, int com, void *data)
> {
> struct pgrp *pg;
> struct proc *p;
> + const struct procctl_cmd_info *cmd_info;
> int error, first_error, ok;
> int signum;
> - bool tree_locked;
>
> - switch (com) {
> - case PROC_ASLR_CTL:
> - case PROC_ASLR_STATUS:
> - case PROC_PROTMAX_CTL:
> - case PROC_PROTMAX_STATUS:
> - case PROC_REAP_ACQUIRE:
> - case PROC_REAP_RELEASE:
> - case PROC_REAP_STATUS:
> - case PROC_REAP_GETPIDS:
> - case PROC_REAP_KILL:
> - case PROC_STACKGAP_CTL:
> - case PROC_STACKGAP_STATUS:
> - case PROC_TRACE_STATUS:
> - case PROC_TRAPCAP_STATUS:
> - case PROC_PDEATHSIG_CTL:
> - case PROC_PDEATHSIG_STATUS:
> - case PROC_NO_NEW_PRIVS_CTL:
> - case PROC_NO_NEW_PRIVS_STATUS:
> - case PROC_WXMAP_CTL:
> - case PROC_WXMAP_STATUS:
> - if (idtype != P_PID)
> - return (EINVAL);
> - }
> + MPASS(com > 0 && com < nitems(procctl_cmds_info));
> + cmd_info = &procctl_cmds_info[com];
> + if (idtype != P_PID && cmd_info->one_proc)
> + return (EINVAL);
>
> switch (com) {
> case PROC_PDEATHSIG_CTL:
> @@ -861,37 +893,13 @@ kern_procctl(struct thread *td, idtype_t idtype, id_t i
> d, int com, void *data)
> return (0);
> }
>
> - switch (com) {
> - case PROC_SPROTECT:
> - case PROC_REAP_STATUS:
> - case PROC_REAP_GETPIDS:
> - case PROC_REAP_KILL:
> - case PROC_TRACE_CTL:
> - case PROC_TRAPCAP_CTL:
> - case PROC_NO_NEW_PRIVS_CTL:
> - sx_slock(&proctree_lock);
> - tree_locked = true;
> - break;
> - case PROC_REAP_ACQUIRE:
> - case PROC_REAP_RELEASE:
> + switch (cmd_info->lock_tree) {
> + case SA_XLOCKED:
> sx_xlock(&proctree_lock);
> - tree_locked = true;
> break;
> - case PROC_ASLR_CTL:
> - case PROC_ASLR_STATUS:
> - case PROC_PROTMAX_CTL:
> - case PROC_PROTMAX_STATUS:
> - case PROC_STACKGAP_CTL:
> - case PROC_STACKGAP_STATUS:
> - case PROC_TRACE_STATUS:
> - case PROC_TRAPCAP_STATUS:
> - case PROC_NO_NEW_PRIVS_STATUS:
> - case PROC_WXMAP_CTL:
> - case PROC_WXMAP_STATUS:
> - tree_locked = false;
> + case SA_SLOCKED:
> + sx_slock(&proctree_lock);
> break;
> - default:
> - return (EINVAL);
> }
>
> switch (idtype) {
> @@ -949,7 +957,14 @@ kern_procctl(struct thread *td, idtype_t idtype, id_t id
> , int com, void *data)
> error = EINVAL;
> break;
> }
> - if (tree_locked)
> - sx_unlock(&proctree_lock);
> +
> + switch (cmd_info->lock_tree) {
> + case SA_XLOCKED:
> + sx_xunlock(&proctree_lock);
> + break;
> + case SA_SLOCKED:
> + sx_sunlock(&proctree_lock);
> + break;
> + }
> return (error);
> }
>
Should SA_* in fact be LA_*? SA_* in sys/sx.h assumes INVARIANTS whereas
LA_* in sys/lock.h has no such requirement.
--
Cheers,
Cy Schubert <Cy.Schubert@cschubert.com>
FreeBSD UNIX: <cy@FreeBSD.org> Web: https://FreeBSD.org
NTP: <cy@nwtime.org> Web: https://nwtime.org
The need of the many outweighs the greed of the few.