[PATCH 1/4] bpf: fix file descriptor leak

[PATCH 1/4] bpf: fix file descriptor leak

[Kris Van Hees]

When a BPF program has been attached to a perf event, it is safe to
close its file descriptor.

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

diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
index 0223764a..28eb890e 100644
--- a/libdtrace/dt_bpf.c
+++ b/libdtrace/dt_bpf.c
@@ -1385,8 +1385,8 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
 		if (prp->prov->impl->attach)
 			rc = prp->prov->impl->attach(dtp, prp, fd);
 
+		close(fd);
 		if (rc < 0 && !(prp->flags & DT_PROBE_FLAG_OPTIONAL)) {
-			close(fd);
 			dt_attach_error(dtp, rc,
 					prp->desc->prv, prp->desc->mod,
 					prp->desc->fun, prp->desc->prb);
-- 
2.43.5

Re: [DTrace-devel] [PATCH 1/4] bpf: fix file descriptor leak

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh@oracle.com>

But I'm curious about the "if (attach)" check.  It looks a little funny 
that we first set fd= and then we check "if (attach)" to see if we are 
going to use the fd value we just set.  I suppose it does not matter 
since we expect attach to be there, but normally one would first check 
if a value is needed before going to the trouble of determining the 
value.  In a way, this small, pre-existing issue is outside the scope of 
this patch, except that the patch makes more obvious the one, isolated 
place where this temporary value is used.

Up to you whether to clean that up.

On 11/10/25 10:27, Kris Van Hees via DTrace-devel wrote:
> When a BPF program has been attached to a perf event, it is safe to
> close its file descriptor.
>
> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> ---
>   libdtrace/dt_bpf.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index 0223764a..28eb890e 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -1385,8 +1385,8 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
>   		if (prp->prov->impl->attach)
>   			rc = prp->prov->impl->attach(dtp, prp, fd);
>   
> +		close(fd);
>   		if (rc < 0 && !(prp->flags & DT_PROBE_FLAG_OPTIONAL)) {
> -			close(fd);
>   			dt_attach_error(dtp, rc,
>   					prp->desc->prv, prp->desc->mod,
>   					prp->desc->fun, prp->desc->prb);

Re: [DTrace-devel] [PATCH 1/4] bpf: fix file descriptor leak

[Kris Van Hees]

On Mon, Nov 10, 2025 at 02:55:52PM -0500, Eugene Loh wrote:
> Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
> 
> But I'm curious about the "if (attach)" check.  It looks a little funny that
> we first set fd= and then we check "if (attach)" to see if we are going to
> use the fd value we just set.  I suppose it does not matter since we expect
> attach to be there, but normally one would first check if a value is needed
> before going to the trouble of determining the value.  In a way, this small,
> pre-existing issue is outside the scope of this patch, except that the patch
> makes more obvious the one, isolated place where this temporary value is
> used.
> 
> Up to you whether to clean that up.

There is a real reason for that.  As you can see, fentry-based FBT probes
have their own BPF program load function (the reason a hook was added for
that in providers).  That one performs the attach as part of the program
load, so it does not have an attach hook.

> On 11/10/25 10:27, Kris Van Hees via DTrace-devel wrote:
> > When a BPF program has been attached to a perf event, it is safe to
> > close its file descriptor.
> > 
> > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> > ---
> >   libdtrace/dt_bpf.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> > index 0223764a..28eb890e 100644
> > --- a/libdtrace/dt_bpf.c
> > +++ b/libdtrace/dt_bpf.c
> > @@ -1385,8 +1385,8 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
> >   		if (prp->prov->impl->attach)
> >   			rc = prp->prov->impl->attach(dtp, prp, fd);
> > +		close(fd);
> >   		if (rc < 0 && !(prp->flags & DT_PROBE_FLAG_OPTIONAL)) {
> > -			close(fd);
> >   			dt_attach_error(dtp, rc,
> >   					prp->desc->prv, prp->desc->mod,
> >   					prp->desc->fun, prp->desc->prb);

Re: [DTrace-devel] [PATCH 1/4] bpf: fix file descriptor leak

[Kris Van Hees]

On Mon, Nov 10, 2025 at 04:17:55PM -0500, Kris Van Hees wrote:
> On Mon, Nov 10, 2025 at 02:55:52PM -0500, Eugene Loh wrote:
> > Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
> > 
> > But I'm curious about the "if (attach)" check.  It looks a little funny that
> > we first set fd= and then we check "if (attach)" to see if we are going to
> > use the fd value we just set.  I suppose it does not matter since we expect
> > attach to be there, but normally one would first check if a value is needed
> > before going to the trouble of determining the value.  In a way, this small,
> > pre-existing issue is outside the scope of this patch, except that the patch
> > makes more obvious the one, isolated place where this temporary value is
> > used.
> > 
> > Up to you whether to clean that up.
> 
> There is a real reason for that.  As you can see, fentry-based FBT probes
> have their own BPF program load function (the reason a hook was added for
> that in providers).  That one performs the attach as part of the program
> load, so it does not have an attach hook.

Hm, I am mistaken.  Even fprobe-based FBT still provides an attach function.

However, the logic I describe is stil valid here...  the code supports the
possibility that the load already performs the attachment, and that therefore
no attach hook is provided.  Still, the expectation is that this code would
work under those conditions.

> > On 11/10/25 10:27, Kris Van Hees via DTrace-devel wrote:
> > > When a BPF program has been attached to a perf event, it is safe to
> > > close its file descriptor.
> > > 
> > > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> > > ---
> > >   libdtrace/dt_bpf.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> > > index 0223764a..28eb890e 100644
> > > --- a/libdtrace/dt_bpf.c
> > > +++ b/libdtrace/dt_bpf.c
> > > @@ -1385,8 +1385,8 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
> > >   		if (prp->prov->impl->attach)
> > >   			rc = prp->prov->impl->attach(dtp, prp, fd);
> > > +		close(fd);
> > >   		if (rc < 0 && !(prp->flags & DT_PROBE_FLAG_OPTIONAL)) {
> > > -			close(fd);
> > >   			dt_attach_error(dtp, rc,
> > >   					prp->desc->prv, prp->desc->mod,
> > >   					prp->desc->fun, prp->desc->prb);