Hi and thanks for your review and time and understand your stance.
On Fri, 21 Jan 2022 at 16:20, Sergey Kandaurov <pluknet@nginx.com> wrote:
>
>
> > On 21 Jan 2022, at 00:06, David CARLIER <devnexen@gmail.com> wrote:
> >
> > From 7f3194c36a9788d9b98630773ab907adb110cf6f Mon Sep 17 00:00:00 2001
> > From: David CARLIER <devnexen@gmail.com>
> > Date: Thu, 20 Jan 2022 20:56:49 +0000
> > Subject: [PATCH] process worker, enabling process tracing and core dumping on
> > FreeBSD 11.x and above using the procctl native API. Checking the version is
> > enough as the functions and the flag we re interested in are available right
> > off the bat.
> >
> > Signed-off-by: David CARLIER <devnexen@gmail.com>
>
> Can you please elaborate,
> what is the purpose of the proposed change?
>
> In FreeBSD, tracing is enabled by default by means of PROC_TRACE_CTL
> (yet, it could be denied by other means, see p_candebug() impl. in
> kernel sources). Thus, enabling tracing explicitly makes no sense
> unless it was previously disabled, such as with proccontrol(1).
> That is a P2_NOTRACE kernel process flag, it controls.
> (In particular, the kern.coredump sysctl used to enable/disable
> coredump systemwide has a weak connection to PROC_TRACE_CTL;
> it is a distinct way to specifically deny coredump).
>
> Note that while API indeed first appeared in FreeBSD 11,
> it was merged in some form to stable/10, back to FreeBSD 10.2.
> OTOH, the patch uses semantics not originally present in 11.
> P_PID:0 is a shortcut added rather recently in f833ab9dd187,
> MFCed to stable/13 post 13.0. So, it doesn't present in
> neither of the released versions.
> In particular, on 13.0 the proposed call results in [EPERM],
> since procctl() naturally searches for PID 0, which doesn't match.

ah good point I did the dev on freebsd 14-snapshot.

>
> With these thoughts, the patch doesn't look useful.
> See below for more.
>
> > ---
> > auto/os/freebsd | 11 +++++++++++
> > src/os/unix/ngx_freebsd_config.h | 4 ++++
> > src/os/unix/ngx_process_cycle.c | 10 ++++++++++
> > 3 files changed, 25 insertions(+)
> >
> > diff --git a/auto/os/freebsd b/auto/os/freebsd
> > index 870bac4b..8bb086f0 100644
> > --- a/auto/os/freebsd
> > +++ b/auto/os/freebsd
> > @@ -103,3 +103,14 @@ if [ $version -ge 701000 ]; then
> > echo " + cpuset_setaffinity() found"
> > have=NGX_HAVE_CPUSET_SETAFFINITY . auto/have
> > fi
> > +
> > +# procctl
> > +
> > +
> > +# the procctl api and its PROC_TRACE_CTL* flags exists from
> > +# FreeBSD 11.x
> > +
> > +if [ $version -ge 1100000 ]; then
> > + echo " + procctl() found"
> > + have=NGX_HAVE_PROCCTL . auto/have
> > +fi
> > diff --git a/src/os/unix/ngx_freebsd_config.h b/src/os/unix/ngx_freebsd_config.h
> > index c641108b..04ed19ca 100644
> > --- a/src/os/unix/ngx_freebsd_config.h
> > +++ b/src/os/unix/ngx_freebsd_config.h
> > @@ -87,6 +87,10 @@
> > #include <sys/event.h>
> > #endif
> >
> > +#if (NGX_HAVE_PROCCTL)
> > +#include <sys/procctl.h>
> > +#endif
> > +
> >
> > #if (NGX_HAVE_FILE_AIO)
> >
> > diff --git a/src/os/unix/ngx_process_cycle.c b/src/os/unix/ngx_process_cycle.c
> > index 07cd05e8..c0cf052f 100644
> > --- a/src/os/unix/ngx_process_cycle.c
> > +++ b/src/os/unix/ngx_process_cycle.c
> > @@ -869,6 +869,16 @@ ngx_worker_process_init(ngx_cycle_t *cycle,
> > ngx_int_t worker)
> >
> > #endif
> >
> > +#if (NGX_HAVE_PROCCTL)
> > + /* allow the process being traceable and producing a coredump in
> > FreeBSD 11.x */
> > + ngx_int_t ctl = PROC_TRACE_CTL_ENABLE;
> > +
> > + if (procctl(P_PID, 0, PROC_TRACE_CTL, &ctl) == -1) {
> > + ngx_log_error(NGX_LOG_ALERT, cycle->log, ngx_errno,
> > + "procctl(PROC_TRACE_CTL_ENABLE) failed");
> > + }
> > +#endif
> > +
> > if (ccf->working_directory.len) {
> > if (chdir((char *) ccf->working_directory.data) == -1) {
> > ngx_log_error(NGX_LOG_ALERT, cycle->log, ngx_errno,
>
> Given the place for patching, I can try to guess that the actual
> intention is to be on par with Linux'ish PR_SET_DUMPABLE,
> that is, to allow tracing/coredump with UID/GID set.
>
> Indeed, ability to trace a process is denied with UID/GID set.
> This is controlled with a kernel process flag P_SUGID.
> But, P_SUGID has a higher precedence over P2_NOTRACE.
>
> --
> Sergey Kandaurov
>
> _______________________________________________
> nginx-devel mailing list -- nginx-devel@nginx.org
> To unsubscribe send an email to nginx-devel-leave@nginx.org
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org