* [PATCH 1/4] bpf: fix file descriptor leak @ 2025-11-10 15:27 Kris Van Hees 2025-11-10 19:55 ` [DTrace-devel] " Eugene Loh 0 siblings, 1 reply; 4+ messages in thread From: Kris Van Hees @ 2025-11-10 15:27 UTC (permalink / raw) To: dtrace, dtrace-devel 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [DTrace-devel] [PATCH 1/4] bpf: fix file descriptor leak 2025-11-10 15:27 [PATCH 1/4] bpf: fix file descriptor leak Kris Van Hees @ 2025-11-10 19:55 ` Eugene Loh 2025-11-10 21:17 ` Kris Van Hees 0 siblings, 1 reply; 4+ messages in thread From: Eugene Loh @ 2025-11-10 19:55 UTC (permalink / raw) To: Kris Van Hees, dtrace, dtrace-devel 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); ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [DTrace-devel] [PATCH 1/4] bpf: fix file descriptor leak 2025-11-10 19:55 ` [DTrace-devel] " Eugene Loh @ 2025-11-10 21:17 ` Kris Van Hees 2025-11-10 21:50 ` Kris Van Hees 0 siblings, 1 reply; 4+ messages in thread From: Kris Van Hees @ 2025-11-10 21:17 UTC (permalink / raw) To: Eugene Loh; +Cc: Kris Van Hees, dtrace, dtrace-devel 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); ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [DTrace-devel] [PATCH 1/4] bpf: fix file descriptor leak 2025-11-10 21:17 ` Kris Van Hees @ 2025-11-10 21:50 ` Kris Van Hees 0 siblings, 0 replies; 4+ messages in thread From: Kris Van Hees @ 2025-11-10 21:50 UTC (permalink / raw) To: Kris Van Hees; +Cc: Eugene Loh, dtrace, dtrace-devel 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); ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-11-10 21:50 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-11-10 15:27 [PATCH 1/4] bpf: fix file descriptor leak Kris Van Hees 2025-11-10 19:55 ` [DTrace-devel] " Eugene Loh 2025-11-10 21:17 ` Kris Van Hees 2025-11-10 21:50 ` Kris Van Hees
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox