[DTrace-devel] [PATCH] options: check *frames option values against system limit

[Kris Van Hees]

On Tue, Sep 30, 2025 at 05:31:42PM -0400, Eugene Loh wrote:
> I'm confused.
> 
> Is this amending an earlier patch, which introduced dt_opt_maxframes() -- a
> patch that has not yet been applied to the dev branch?  If so, why not
> rework the earlier patch for a cleaner commit history?

I think it is cleaner to do it in two patches mainly because this one is
adding tests (see below) and therefore is really about this specific change
rather than the introduction of dt_opt_maxframes() that was necessary for
other things to work.

> What observable change in behavior does this patch produce?  Why isn't there
> a test?

Oops, I forgot to include the tests in the patch.  They will be there in the
v2.

> Also...
> 
> On 9/30/25 12:47, Kris Van Hees wrote:
> > Commit 81e99397d ("Add support for the stack() action") added code to
> > retrieve the system limit for stack traces and used that value to set
> > the 'maxframes' option.
> > 
> > However, a user can set the 'maxframes' option from the command line,
> > which would allow setting the option to value beyond the system limit.
> > 
> > We now record the system limit in dtrace_hdl_t as well as setting the
> 
> In general, "now" in a commit message strikes me as ambiguous.  Does it mean
> before or after the patch?  Not a huge deal here since the patch quickly
> indicates that this is new behavior, but I'd prefer avoiding "now."
> 
> > 'maxframes' option to this value as a default.  Options processing now
> > ensures that *frames options cannot be set beyond the system limit.
> > 
> > Note: 'stackframes', 'ustackframes', and 'jstackframes' can be set to
> > a value that is higher than 'maxframes'.  The existing immplementation
> > in the code generator already lowers their value to 'maxframes' when
> > needed.
> 
> This seems to contradict the new comment in dt_vopen(), which says you
> cannot set to a higher value, and the behavior in the new dt_opt_frames().
> 
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> >   libdtrace/dt_impl.h    |  1 +
> >   libdtrace/dt_open.c    |  7 +++++--
> >   libdtrace/dt_options.c | 38 +++++++++-----------------------------
> >   3 files changed, 15 insertions(+), 31 deletions(-)
> > 
> > diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> > index 4dc4c9a7..723aac8b 100644
> > --- a/libdtrace/dt_impl.h
> > +++ b/libdtrace/dt_impl.h
> > @@ -310,6 +310,7 @@ struct dtrace_hdl {
> >   	uint_t dt_maxtuplesize;	/* largest tuple across programs */
> >   	uint_t dt_maxlvaralloc;	/* largest lvar alloc across pcbs */
> >   	uint_t dt_maxaggdsize;	/* largest aggregation data size */
> > +	uint64_t dt_maxframes;	/* system limit on stack traces */
> >   	dt_tstring_t *dt_tstrings; /* temporary string slots */
> >   	dt_list_t dt_modlist;	/* linked list of dt_module_t's */
> >   	dt_htab_t *dt_mods;	/* hash table of dt_module_t's */
> > diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> > index ed0b14a1..ba5463a0 100644
> > --- a/libdtrace/dt_open.c
> > +++ b/libdtrace/dt_open.c
> > @@ -764,13 +764,16 @@ dt_vopen(int version, int flags, int *errp,
> >   	dtp->dt_options[DTRACEOPT_SCRATCHSIZE] = sizeof(uint64_t) + _dtrace_scratchsize;
> >   	/*
> > -	 * Set the default value of maxframes.
> > +	 * Set the default value of maxframes.  This is the maximum of stack
> > +	 * frames that can be requested from the kernel.  The *frames options
> > +	 * cannot be set to a value that exceeds this limit.
> >   	 */
> >   	fd = fopen("/proc/sys/kernel/perf_event_max_stack", "r");
> >   	assert(fd);
> > -	if (fscanf(fd, "%lu", &dtp->dt_options[DTRACEOPT_MAXFRAMES]) != 1)
> > +	if (fscanf(fd, "%lu", &dtp->dt_maxframes) != 1)
> >   		return set_open_errno(dtp, errp, EDT_READMAXSTACK);
> >   	fclose(fd);
> > +	dtp->dt_options[DTRACEOPT_MAXFRAMES] = dtp->dt_maxframes;
> >   	/*
> >   	 * Set the default pcap capture size.
> > diff --git a/libdtrace/dt_options.c b/libdtrace/dt_options.c
> > index 7508f044..97516517 100644
> > --- a/libdtrace/dt_options.c
> > +++ b/libdtrace/dt_options.c
> > @@ -913,40 +913,20 @@ dt_opt_rate(dtrace_hdl_t *dtp, const char *arg, uintptr_t option)
> >   }
> >   /*
> > - * When setting the maxframes option, set the option in the dt_options array
> > - * using dt_opt_runtime() as usual, and then update the definition of the CTF
> > - * type for "_stack" to be an array of the corresponding size.
> > - * If any errors occur, reset dt_options[option] to its previous value.
> > + * Set a *frames option, ensuring that its value is within the system
> > + * constaints.
> >    */
> >   static int
> > -dt_opt_maxframes(dtrace_hdl_t *dtp, const char *arg, uintptr_t option)
> > +dt_opt_frames(dtrace_hdl_t *dtp, const char *arg, uintptr_t option)
> >   {
> >   	dtrace_optval_t val = dtp->dt_options[option];
> > -	ctf_file_t *fp = DT_STACK_CTFP(dtp);
> > -	ctf_id_t type = ctf_type_resolve(fp, DT_STACK_TYPE(dtp));
> > -	ctf_arinfo_t r;
> >   	if (dt_opt_runtime(dtp, arg, option) != 0)
> >   		return -1; /* dt_errno is set for us */
> > -	if (dtp->dt_options[option] > UINT_MAX) {
> > -		dtp->dt_options[option] = val;
> > -		return dt_set_errno(dtp, EOVERFLOW);
> > -	}
> > -
> > -	if (ctf_array_info(fp, type, &r) == CTF_ERR) {
> > -		dtp->dt_options[option] = val;
> > -		dtp->dt_ctferr = ctf_errno(fp);
> > -		return dt_set_errno(dtp, EDT_CTF);
> > -	}
> > -
> > -	r.ctr_nelems = (uint_t)dtp->dt_options[option];
> > -
> > -	if (ctf_set_array(fp, type, &r) == CTF_ERR ||
> > -	    ctf_update(fp) == CTF_ERR) {
> > +	if (dtp->dt_options[option] > dtp->dt_maxframes) {
> >   		dtp->dt_options[option] = val;
> > -		dtp->dt_ctferr = ctf_errno(fp);
> > -		return dt_set_errno(dtp, EDT_CTF);
> > +		return dt_set_errno(dtp, EDT_BADOPTVAL);
> >   	}
> >   	return 0;
> > @@ -1194,19 +1174,19 @@ static const dt_option_t _dtrace_rtoptions[] = {
> >   	{ "destructive", dt_opt_runtime, DTRACEOPT_DESTRUCTIVE },
> >   	{ "dynvarsize", dt_opt_size, DTRACEOPT_DYNVARSIZE },
> >   	{ "grabanon", dt_opt_runtime, DTRACEOPT_GRABANON },
> > -	{ "jstackframes", dt_opt_runtime, DTRACEOPT_JSTACKFRAMES },
> > +	{ "jstackframes", dt_opt_frames, DTRACEOPT_JSTACKFRAMES },
> >   	{ "jstackstrsize", dt_opt_size, DTRACEOPT_JSTACKSTRSIZE },
> >   	{ "lockmem", dt_opt_lockmem, DTRACEOPT_LOCKMEM },
> > -	{ "maxframes", dt_opt_maxframes, DTRACEOPT_MAXFRAMES },
> > +	{ "maxframes", dt_opt_frames, DTRACEOPT_MAXFRAMES },
> >   	{ "nusdtprobes", dt_opt_runtime, DTRACEOPT_NUSDTPROBES },
> >   	{ "nspec", dt_opt_runtime, DTRACEOPT_NSPEC },
> >   	{ "pcapsize", dt_opt_pcapsize, DTRACEOPT_PCAPSIZE },
> >   	{ "scratchsize", dt_opt_scratchsize, DTRACEOPT_SCRATCHSIZE },
> >   	{ "specsize", dt_opt_size, DTRACEOPT_SPECSIZE },
> > -	{ "stackframes", dt_opt_runtime, DTRACEOPT_STACKFRAMES },
> > +	{ "stackframes", dt_opt_frames, DTRACEOPT_STACKFRAMES },
> >   	{ "statusrate", dt_opt_rate, DTRACEOPT_STATUSRATE },
> >   	{ "strsize", dt_opt_strsize, DTRACEOPT_STRSIZE },
> > -	{ "ustackframes", dt_opt_runtime, DTRACEOPT_USTACKFRAMES },
> > +	{ "ustackframes", dt_opt_frames, DTRACEOPT_USTACKFRAMES },
> >   	{ "noresolve", dt_opt_runtime, DTRACEOPT_NORESOLVE },
> >   	{ NULL }
> >   };