[PATCH] fbt: implement return value support for fexit-based FBT return probes

[PATCH] fbt: implement return value support for fexit-based FBT return probes

[Kris Van Hees]

Commit fe2101e5 ("fbt: implement based on fentry/fexit probes") failed
to provide a proper implementation to pass the function return value as
arg1 for FBT return probes based on fexit probes.

Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
---
 libdtrace/dt_prov_fbt.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
index fa888ed8..99b42586 100644
--- a/libdtrace/dt_prov_fbt.c
+++ b/libdtrace/dt_prov_fbt.c
@@ -172,6 +172,7 @@ static int populate(dtrace_hdl_t *dtp)
  */
 static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
 {
+	dtrace_hdl_t	*dtp = pcb->pcb_hdl;
 	dt_irlist_t	*dlp = &pcb->pcb_ir;
 	dt_probe_t	*prp = pcb->pcb_probe;
 
@@ -185,6 +186,8 @@ static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
 			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
 		}
 	} else {
+		dt_module_t	*dmp;
+
 		/*
 		 * fbt:::return arg0 should be the function offset for the
 		 * return instruction.  The fexit prpbe fires at a point where
@@ -194,6 +197,21 @@ static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
 		 */
 		dt_cg_xsetx(dlp, NULL, DT_LBL_NONE, BPF_REG_0, -1);
 		emit(dlp,  BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(0), BPF_REG_0));
+
+		/*
+		 * The return value is provided by the fexit probe as an
+		 * argument slot past the last function argument.  We can get
+		 * the number of function arguments using the BTF id that has
+		 * been stored as the tracepoint event id.
+		 */
+		dmp = dt_module_lookup_by_name(dtp, prp->desc->mod);
+		if (dmp != NULL) {
+			int32_t	btf_id = dt_tp_get_event_id(prp);
+			int	i = dt_btf_func_argc(dtp, dmp->dm_btf, btf_id);
+
+			emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_8, i * 8));
+			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(1), BPF_REG_0));
+		}
 	}
 
 	dt_cg_tramp_epilogue(pcb);
@@ -274,7 +292,7 @@ static int fprobe_prog_load(dtrace_hdl_t *dtp, const dt_probe_t *prp,
 	dt_module_t			*dmp;
 
 	atype = strcmp(desc->prb, "entry") == 0 ? BPF_TRACE_FENTRY
-						     : BPF_TRACE_FEXIT;
+						: BPF_TRACE_FEXIT;
 
 	dmp = dt_module_lookup_by_name(dtp, desc->mod);
 	if (dmp == NULL)
-- 
2.45.2

Re: [PATCH] fbt: implement return value support for fexit-based FBT return probes

[Eugene Loh]

Is there a test?

On 8/1/24 01:19, Kris Van Hees wrote:
> Commit fe2101e5 ("fbt: implement based on fentry/fexit probes") failed
> to provide a proper implementation to pass the function return value as
> arg1 for FBT return probes based on fexit probes.
>
> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> ---
>   libdtrace/dt_prov_fbt.c | 20 +++++++++++++++++++-
>   1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> index fa888ed8..99b42586 100644
> --- a/libdtrace/dt_prov_fbt.c
> +++ b/libdtrace/dt_prov_fbt.c
> @@ -172,6 +172,7 @@ static int populate(dtrace_hdl_t *dtp)
>    */
>   static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>   {
> +	dtrace_hdl_t	*dtp = pcb->pcb_hdl;
>   	dt_irlist_t	*dlp = &pcb->pcb_ir;
>   	dt_probe_t	*prp = pcb->pcb_probe;
>   
> @@ -185,6 +186,8 @@ static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>   			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
>   		}
>   	} else {
> +		dt_module_t	*dmp;
> +
>   		/*
>   		 * fbt:::return arg0 should be the function offset for the
>   		 * return instruction.  The fexit prpbe fires at a point where
> @@ -194,6 +197,21 @@ static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>   		 */
>   		dt_cg_xsetx(dlp, NULL, DT_LBL_NONE, BPF_REG_0, -1);
>   		emit(dlp,  BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(0), BPF_REG_0));
> +
> +		/*
> +		 * The return value is provided by the fexit probe as an
> +		 * argument slot past the last function argument.  We can get
> +		 * the number of function arguments using the BTF id that has
> +		 * been stored as the tracepoint event id.
> +		 */
> +		dmp = dt_module_lookup_by_name(dtp, prp->desc->mod);
> +		if (dmp != NULL) {
> +			int32_t	btf_id = dt_tp_get_event_id(prp);
> +			int	i = dt_btf_func_argc(dtp, dmp->dm_btf, btf_id);
> +
> +			emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_8, i * 8));
> +			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(1), BPF_REG_0));
> +		}
>   	}
>   
>   	dt_cg_tramp_epilogue(pcb);
> @@ -274,7 +292,7 @@ static int fprobe_prog_load(dtrace_hdl_t *dtp, const dt_probe_t *prp,
>   	dt_module_t			*dmp;
>   
>   	atype = strcmp(desc->prb, "entry") == 0 ? BPF_TRACE_FENTRY
> -						     : BPF_TRACE_FEXIT;
> +						: BPF_TRACE_FEXIT;
>   
>   	dmp = dt_module_lookup_by_name(dtp, desc->mod);
>   	if (dmp == NULL)

Re: [PATCH] fbt: implement return value support for fexit-based FBT return probes

[Kris Van Hees]

On Thu, Aug 01, 2024 at 01:49:35AM -0400, Eugene Loh wrote:
> Is there a test?

Yes, the testsuite already contains a test (tst.return1.d) whose failure
prompted the need for this patch.  And with the patch, it passes.

> 
> On 8/1/24 01:19, Kris Van Hees wrote:
> > Commit fe2101e5 ("fbt: implement based on fentry/fexit probes") failed
> > to provide a proper implementation to pass the function return value as
> > arg1 for FBT return probes based on fexit probes.
> > 
> > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> > ---
> >   libdtrace/dt_prov_fbt.c | 20 +++++++++++++++++++-
> >   1 file changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> > index fa888ed8..99b42586 100644
> > --- a/libdtrace/dt_prov_fbt.c
> > +++ b/libdtrace/dt_prov_fbt.c
> > @@ -172,6 +172,7 @@ static int populate(dtrace_hdl_t *dtp)
> >    */
> >   static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> >   {
> > +	dtrace_hdl_t	*dtp = pcb->pcb_hdl;
> >   	dt_irlist_t	*dlp = &pcb->pcb_ir;
> >   	dt_probe_t	*prp = pcb->pcb_probe;
> > @@ -185,6 +186,8 @@ static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> >   			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
> >   		}
> >   	} else {
> > +		dt_module_t	*dmp;
> > +
> >   		/*
> >   		 * fbt:::return arg0 should be the function offset for the
> >   		 * return instruction.  The fexit prpbe fires at a point where
> > @@ -194,6 +197,21 @@ static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> >   		 */
> >   		dt_cg_xsetx(dlp, NULL, DT_LBL_NONE, BPF_REG_0, -1);
> >   		emit(dlp,  BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(0), BPF_REG_0));
> > +
> > +		/*
> > +		 * The return value is provided by the fexit probe as an
> > +		 * argument slot past the last function argument.  We can get
> > +		 * the number of function arguments using the BTF id that has
> > +		 * been stored as the tracepoint event id.
> > +		 */
> > +		dmp = dt_module_lookup_by_name(dtp, prp->desc->mod);
> > +		if (dmp != NULL) {
> > +			int32_t	btf_id = dt_tp_get_event_id(prp);
> > +			int	i = dt_btf_func_argc(dtp, dmp->dm_btf, btf_id);
> > +
> > +			emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_8, i * 8));
> > +			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(1), BPF_REG_0));
> > +		}
> >   	}
> >   	dt_cg_tramp_epilogue(pcb);
> > @@ -274,7 +292,7 @@ static int fprobe_prog_load(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> >   	dt_module_t			*dmp;
> >   	atype = strcmp(desc->prb, "entry") == 0 ? BPF_TRACE_FENTRY
> > -						     : BPF_TRACE_FEXIT;
> > +						: BPF_TRACE_FEXIT;
> >   	dmp = dt_module_lookup_by_name(dtp, desc->mod);
> >   	if (dmp == NULL)

Re: [PATCH] fbt: implement return value support for fexit-based FBT return probes

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
with a few nits...

On 8/1/24 10:24, Kris Van Hees wrote:
> On Thu, Aug 01, 2024 at 01:49:35AM -0400, Eugene Loh wrote:
>> Is there a test?
> Yes, the testsuite already contains a test (tst.return1.d) whose failure
> prompted the need for this patch.  And with the patch, it passes.

Thanks.  So the earlier culprit/insufficient patch (fe2101e5) went out 
in the last release with this test... failing?  Not tested (on fentry 
systems, where it makes a difference)?  A known failure in the last 
release?  (I forget.)

>> On 8/1/24 01:19, Kris Van Hees wrote:
>>> Commit fe2101e5 ("fbt: implement based on fentry/fexit probes") failed
>>> to provide a proper implementation to pass the function return value as
>>> arg1 for FBT return probes based on fexit probes.
>>>
>>> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
>>> ---
>>>    libdtrace/dt_prov_fbt.c | 20 +++++++++++++++++++-
>>>    1 file changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
>>> index fa888ed8..99b42586 100644
>>> --- a/libdtrace/dt_prov_fbt.c
>>> +++ b/libdtrace/dt_prov_fbt.c
>>> @@ -172,6 +172,7 @@ static int populate(dtrace_hdl_t *dtp)
>>>     */
>>>    static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>>>    {
>>> +	dtrace_hdl_t	*dtp = pcb->pcb_hdl;
>>>    	dt_irlist_t	*dlp = &pcb->pcb_ir;
>>>    	dt_probe_t	*prp = pcb->pcb_probe;
>>> @@ -185,6 +186,8 @@ static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>>>    			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
>>>    		}
>>>    	} else {
>>> +		dt_module_t	*dmp;
>>> +
>>>    		/*
>>>    		 * fbt:::return arg0 should be the function offset for the
>>>    		 * return instruction.  The fexit prpbe fires at a point where

Might as well s/prpbe/probe/ while we're here.

>>> @@ -194,6 +197,21 @@ static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>>>    		 */
>>>    		dt_cg_xsetx(dlp, NULL, DT_LBL_NONE, BPF_REG_0, -1);
>>>    		emit(dlp,  BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(0), BPF_REG_0));
>>> +
>>> +		/*
>>> +		 * The return value is provided by the fexit probe as an
>>> +		 * argument slot past the last function argument.  We can get
>>> +		 * the number of function arguments using the BTF id that has
>>> +		 * been stored as the tracepoint event id.
>>> +		 */
>>> +		dmp = dt_module_lookup_by_name(dtp, prp->desc->mod);
>>> +		if (dmp != NULL) {
>>> +			int32_t	btf_id = dt_tp_get_event_id(prp);
>>> +			int	i = dt_btf_func_argc(dtp, dmp->dm_btf, btf_id);
>>> +
>>> +			emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_8, i * 8));
>>> +			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(1), BPF_REG_0));
>>> +		}

I assume we don't worry about the handling of dmp==NULL because it's 
"unlikely" (already been checked) and there's nothing more sensible to 
do anyhow?  Or load arg1 with 0xdeadbeef or something?

>>>    	}
>>>    	dt_cg_tramp_epilogue(pcb);
>>> @@ -274,7 +292,7 @@ static int fprobe_prog_load(dtrace_hdl_t *dtp, const dt_probe_t *prp,
>>>    	dt_module_t			*dmp;
>>>    	atype = strcmp(desc->prb, "entry") == 0 ? BPF_TRACE_FENTRY
>>> -						     : BPF_TRACE_FEXIT;
>>> +						: BPF_TRACE_FEXIT;
>>>    	dmp = dt_module_lookup_by_name(dtp, desc->mod);
>>>    	if (dmp == NULL)

Re: [PATCH] fbt: implement return value support for fexit-based FBT return probes

[Kris Van Hees]

On Thu, Aug 01, 2024 at 02:15:17PM -0400, Eugene Loh wrote:
> Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
> with a few nits...
> 
> On 8/1/24 10:24, Kris Van Hees wrote:
> > On Thu, Aug 01, 2024 at 01:49:35AM -0400, Eugene Loh wrote:
> > > Is there a test?
> > Yes, the testsuite already contains a test (tst.return1.d) whose failure
> > prompted the need for this patch.  And with the patch, it passes.
> 
> Thanks.  So the earlier culprit/insufficient patch (fe2101e5) went out in
> the last release with this test... failing?  Not tested (on fentry systems,
> where it makes a difference)?  A known failure in the last release?  (I
> forget.)

Well, against all odds it actually "works" on older kernels with the quite
insufficient patch because by total coincidence the right value happens to be
present in the correct arg slot.

> > > On 8/1/24 01:19, Kris Van Hees wrote:
> > > > Commit fe2101e5 ("fbt: implement based on fentry/fexit probes") failed
> > > > to provide a proper implementation to pass the function return value as
> > > > arg1 for FBT return probes based on fexit probes.
> > > > 
> > > > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> > > > ---
> > > >    libdtrace/dt_prov_fbt.c | 20 +++++++++++++++++++-
> > > >    1 file changed, 19 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> > > > index fa888ed8..99b42586 100644
> > > > --- a/libdtrace/dt_prov_fbt.c
> > > > +++ b/libdtrace/dt_prov_fbt.c
> > > > @@ -172,6 +172,7 @@ static int populate(dtrace_hdl_t *dtp)
> > > >     */
> > > >    static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> > > >    {
> > > > +	dtrace_hdl_t	*dtp = pcb->pcb_hdl;
> > > >    	dt_irlist_t	*dlp = &pcb->pcb_ir;
> > > >    	dt_probe_t	*prp = pcb->pcb_probe;
> > > > @@ -185,6 +186,8 @@ static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> > > >    			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
> > > >    		}
> > > >    	} else {
> > > > +		dt_module_t	*dmp;
> > > > +
> > > >    		/*
> > > >    		 * fbt:::return arg0 should be the function offset for the
> > > >    		 * return instruction.  The fexit prpbe fires at a point where
> 
> Might as well s/prpbe/probe/ while we're here.

Good catch.

> > > > @@ -194,6 +197,21 @@ static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> > > >    		 */
> > > >    		dt_cg_xsetx(dlp, NULL, DT_LBL_NONE, BPF_REG_0, -1);
> > > >    		emit(dlp,  BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(0), BPF_REG_0));
> > > > +
> > > > +		/*
> > > > +		 * The return value is provided by the fexit probe as an
> > > > +		 * argument slot past the last function argument.  We can get
> > > > +		 * the number of function arguments using the BTF id that has
> > > > +		 * been stored as the tracepoint event id.
> > > > +		 */
> > > > +		dmp = dt_module_lookup_by_name(dtp, prp->desc->mod);
> > > > +		if (dmp != NULL) {
> > > > +			int32_t	btf_id = dt_tp_get_event_id(prp);
> > > > +			int	i = dt_btf_func_argc(dtp, dmp->dm_btf, btf_id);
> > > > +
> > > > +			emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_8, i * 8));
> > > > +			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(1), BPF_REG_0));
> > > > +		}
> 
> I assume we don't worry about the handling of dmp==NULL because it's
> "unlikely" (already been checked) and there's nothing more sensible to do
> anyhow?  Or load arg1 with 0xdeadbeef or something?

Unpredictable result is probably the best we can do right now.  Any deliberatea
value isn't really any better.
 
We need to look into a future solution to provide proper error reporting
during the trampoline creation.

> > > >    	}
> > > >    	dt_cg_tramp_epilogue(pcb);
> > > > @@ -274,7 +292,7 @@ static int fprobe_prog_load(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > > >    	dt_module_t			*dmp;
> > > >    	atype = strcmp(desc->prb, "entry") == 0 ? BPF_TRACE_FENTRY
> > > > -						     : BPF_TRACE_FEXIT;
> > > > +						: BPF_TRACE_FEXIT;
> > > >    	dmp = dt_module_lookup_by_name(dtp, desc->mod);
> > > >    	if (dmp == NULL)