Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 19 Oct 2021 13:35:13 -0700
From:      Cy Schubert <Cy.Schubert@cschubert.com>
To:        Konstantin Belousov <kib@FreeBSD.org>
Cc:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org
Subject:   Re: git: 7ae879b14a20 - main - kern_procctl(): convert the  function to be table-driven
Message-ID:  <202110192035.19JKZDqM026085@slippy.cwsent.com>
In-Reply-To: <202110192004.19JK4jN3069844@gitrepo.freebsd.org>
References:  <202110192004.19JK4jN3069844@gitrepo.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
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.






Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202110192035.19JKZDqM026085>