Re: [DTrace-devel] [PATCH 2/8] Reduce stack depth if kernel returns NULL frames

[Kris Van Hees <kris.van.hees@oracle.com>]

If bpf_get_stack() can give us stacks with NULL pointers at the top, wouldn't
we need code to remove those (if they need to be removed) from the actual
stack() data also?  If they are left there, then I would argue that we should
also include them in the stackdepth count.

On Tue, Jun 04, 2024 at 02:00:02PM -0400, eugene.loh--- via DTrace-devel wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> The return value from the BPF helper function bpf_get_stack()
> basically returns the size of the stack returned.  We use this
> value to report stack depth.
> 
> Some of the top frames can be NULL, however, leading to some
> inconsistencies between reported stacks and stack depths.
> 
> Add some code to reduce the stack depth if one or two top
> frames are NULL.
> 
> There is an existing test to check for this problem.  It will
> appear in a later patch since it has multiple problems.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  bpf/get_bvar.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/bpf/get_bvar.c b/bpf/get_bvar.c
> index ea5dc6b1..a0c04f3a 100644
> --- a/bpf/get_bvar.c
> +++ b/bpf/get_bvar.c
> @@ -67,7 +67,9 @@ noinline uint64_t dt_get_bvar(const dt_dctx_t *dctx, uint32_t id, uint32_t idx)
>  		uint32_t bufsiz = (uint32_t) (uint64_t) (&STKSIZ);
>  		uint64_t flags;
>  		char *buf = dctx->mem + (uint64_t)(&STACK_OFF);
> -		uint64_t stacksize;
> +		int64_t stacksize;
> +		int64_t topslot;
> +		uint64_t *pcs = (uint64_t *)buf;
>  
>  		if (id == DIF_VAR_USTACKDEPTH)
>  			flags = BPF_F_USER_STACK;
> @@ -87,8 +89,19 @@ noinline uint64_t dt_get_bvar(const dt_dctx_t *dctx, uint32_t id, uint32_t idx)
>  		 * 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.
> +		 *
> +		 * The helper function also sometimes returns some empty frames
> +		 * at the top.  Bump the depth down some so that the stack depth
> +		 * we report is consistent with the number of frames returned.
> +		 * Arguably, this should be fixed in the kernel, but we can
> +		 * work around the problem for now.
>  		 */
> -		return stacksize / sizeof(uint64_t);
> +		topslot = stacksize / sizeof(uint64_t) - 1;
> +		if (topslot >= 0 && topslot < (bufsiz / sizeof(uint64_t)) && pcs[topslot] == 0)
> +			topslot--;
> +		if (topslot >= 0 && topslot < (bufsiz / sizeof(uint64_t)) && pcs[topslot] == 0)
> +			topslot--;
> +		return topslot + 1;
>  	}
>  	case DIF_VAR_CALLER:
>  	case DIF_VAR_UCALLER: {
> -- 
> 2.18.4
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel