Re: [PATCH 01/14] Fix stack-skip counts for caller and stackdepth

[Nick Alcock <nick.alcock@oracle.com>]

On 22 May 2025, eugene loh told this:

> From: Eugene Loh <eugene.loh@oracle.com>
>
> Apparently, when we call the BPF get_stack() helper function,
> it generally knows how many frames to skip to get the real kernel
> stack.  For fentry/fexit, however, this is apparently not the case,
> and commit bc65cb44d
> ("cg: allow providers to specify a skip count for stack retrieval")
> added the ability to skip frames for fentry/fexit probes.
>
> When this "skip" is needed, however, it must must be even deeper
> when we descend further frames, such as when we call dt_bpf_*()
> precompiled functions.

Ack.

> Add this support for dt_bpf_caller() and dt_bpf_stackdepth().
> That is, if there are stack-skip frames, skip yet one more frame
> when inside a bpf/get_bvar.c function.

The general approach here seems sound, but this confuses me. Is the
assumption that if the skip is not needed, get_stack() will itself know
how many frames to skip, and will skip dt_dt_bvar_caller() for you?

(So a zero skip is actually a "the system knows", and a nonzero skip
causes the system to not skip anything, so you have to do all the
skipping yourself?

This behaviour is not documented in bpf_get_stack()'s documentation,
which absolutely does not mean it doesn't do it...)

> Note that we declare the skip count volatile.  The compiler might
> optimize code that uses the STACK_SKIP value, but we will subsequently
> perform relocations that adjust this value.

... why doesn't this apply to every other extern global variable in
get_bvar()? They're all similarly relocated...

> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  bpf/get_bvar.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/bpf/get_bvar.c b/bpf/get_bvar.c
> index 9625e764e..99a6503d5 100644
> --- a/bpf/get_bvar.c
> +++ b/bpf/get_bvar.c
> @@ -53,12 +53,17 @@ noinline uint64_t dt_bvar_args(const dt_dctx_t *dctx, uint32_t idx)
>  
>  noinline uint64_t dt_bvar_caller(const dt_dctx_t *dctx)
>  {
> -	uint64_t	buf[2] = { 0, };
> +	uint64_t	buf[3] = { 0, };
> +	volatile uint64_t
> +			skip = (uint64_t)(&STACK_SKIP);
>  
>  	if (bpf_get_stack(dctx->ctx, buf, sizeof(buf),
> -			  (uint64_t)(&STACK_SKIP) & BPF_F_SKIP_FIELD_MASK) < 0)
> +			  skip & BPF_F_SKIP_FIELD_MASK) < 0)
>  		return 0;
>  
> +	/* If we had to skip any frames, account for the dt_bvar_caller() frame. */
> +	if (skip)
> +		return buf[2];
>  	return buf[1];
>  }
>  
> @@ -203,9 +208,11 @@ noinline uint64_t dt_bvar_stackdepth(const dt_dctx_t *dctx)
>  	uint32_t	bufsiz = (uint32_t) (uint64_t) (&STKSIZ);
>  	char		*buf = dctx->mem + (uint64_t)(&STACK_OFF);
>  	uint64_t	retv;
> +	volatile uint64_t
> +			skip = (uint64_t)(&STACK_SKIP);
>  
>  	retv = bpf_get_stack(dctx->ctx, buf, bufsiz,
> -			     (uint64_t)(&STACK_SKIP) & BPF_F_SKIP_FIELD_MASK);
> +			     skip & BPF_F_SKIP_FIELD_MASK);
>  	if (retv < 0)
>  		return error(dctx, DTRACEFLT_BADSTACK, 0 /* FIXME */);
>  
> @@ -217,7 +224,11 @@ noinline uint64_t dt_bvar_stackdepth(const dt_dctx_t *dctx)
>  	 * If retv==bufsiz, presumably the stack is larger than what we
>  	 * can retrieve.  But it's also possible that the buffer was exactly
>  	 * large enough.  So, leave it to the user to interpret the result.
> +	 *
> +	 * If we had to skip any frames, account for the dt_bvar_stackdepth() frame.
>  	 */
> +	if (skip)
> +		return retv / sizeof(uint64_t) - 1;
>  	return retv / sizeof(uint64_t);
>  }

-- 
NULL && (void)