Introduce GetFd() for BPFLink

[geyslan]

GetFd() returns the link file descriptor.

[rafaeldtinoco]

Have you tested this ?

func main() {

	var err error

	var bpfModule *bpf.Module
	var bpfMapEvents *bpf.BPFMap
	var bpfProgKsysSync *bpf.BPFProg
	var bpfLinkKsysSync *bpf.BPFLink
	var perfBuffer *bpf.PerfBuffer

	var eventsChannel chan []byte
	var lostChannel chan uint64

	// create BPF module using BPF object file
	bpfModule, err = bpf.NewModuleFromFile("mine.bpf.o")
	if err != nil {
		errExit(err)
	}
	defer bpfModule.Close()

	// BPF map "events": resize it before object is loaded
	bpfMapEvents, err = bpfModule.GetMap("events")
	err = bpfMapEvents.Resize(8192)
	if err != nil {
		errExit(err)
	}

	// load BPF object from BPF module
	if err = bpfModule.BPFLoadObject(); err != nil {
		errExit(err)
	}

	// get BPF program from BPF object
	bpfProgKsysSync, err = bpfModule.GetProgram("ksys_sync")
	if err != nil {
		errExit(err)
	}

	// attach to BPF program to kprobe
	bpfLinkKsysSync, err = bpfProgKsysSync.AttachKprobe("ksys_sync")
	if err != nil {
		errExit(err)
	}

	err = bpfLinkKsysSync.Detach()
	if err != nil {
		errExit(err)
	}

2021/09/13 06:42:47 error: /home/rafaeldtinoco/work/sources/ebpf/mine-portablebpf/mine.go:96 invalid argument

While if I do:

	err = bpfLinkKsysSync.Destroy()
	if err != nil {
		errExit(err)
	}

I get no errors.

This is exactly what I was calling attention in the comments.

If you check libbpf.c (bpf_link__disconnect and bpf_link__destroy):

/* Release "ownership" of underlying BPF resource (typically, BPF program
 * attached to some BPF hook, e.g., tracepoint, kprobe, etc). Disconnected
 * link, when destructed through bpf_link__destroy() call won't attempt to
 * detach/unregisted that BPF resource. This is useful in situations where,
 * say, attached BPF program has to outlive userspace program that attached it
 * in the system. Depending on type of BPF program, though, there might be
 * additional steps (like pinning BPF program in BPF FS) necessary to ensure
 * exit of userspace program doesn't trigger automatic detachment and clean up
 * inside the kernel.
 */

And the destroy might not have errors because link->disconnected is not true, so it does not call detach. It looks like simply calling detach might not be enough, we might need to do some cleanup before calling it. Libbpf does not seem to export that logic, but, if you read "bpf_link_perf_detach()" you will understand the logic I'm taking about (ioctl PERF_EVENT_IOC_DISABLE).

[geyslan]

Have you tested this ?

This was untested, I would do this today. Thanks for already doing it.

And the destroy might not have errors because link->disconnected is not true, so it does not call detach. It looks like simply calling detach might not be enough, we might need to do some cleanup before calling it. Libbpf does not seem to export that logic, but, if you read "bpf_link_perf_detach()" you will understand the logic I'm taking about (ioctl PERF_EVENT_IOC_DISABLE).

Interesting. I was thinking about implementing Disconnect() as a next step. We will probably have to do this along with some cleaning suggested by you. I'll dig into the _detach() and _destroy() logic.

[rafaeldtinoco]

And the destroy might not have errors because link->disconnected is not true, so it does not call detach. It looks like simply calling detach might not be enough, we might need to do some cleanup before calling it. Libbpf does not seem to export that logic, but, if you read "bpf_link_perf_detach()" you will understand the logic I'm taking about (ioctl PERF_EVENT_IOC_DISABLE).

Interesting. I was thinking about implementing Disconnect() as a next step. We will probably have to do this along with some cleaning suggested by you. I'll dig into the _detach() and _destroy() logic.

Note that even libbpf is not doing things right (as it appears). The comment simply states that 'there might be stuff going on'. If you check the patch I suggested, I had to add file descriptor closure in order to have link detached correctly. You might end up checking libbpf execution logic as well (as a heads up). Let me know how it goes!

[rafaeldtinoco]

I converted it to draft. Convert it back once you're done. Thanks.

[CLAassistant]

All committers have signed the CLA.

[rafaeldtinoco]

Alright, this is the situation... we CANNOT call bpf_link__detach() - see bellow - and expect it to have cleared the probe resources being used by the link.

Check

int bpf_link__detach(struct bpf_link *link)
{
	return bpf_link_detach(link->fd) ? -errno : 0;
}

which is basically:

int bpf_link_detach(int link_fd)
{
	union bpf_attr attr;
	int ret;

	memset(&attr, 0, sizeof(attr));
	attr.link_detach.link_fd = link_fd;

	ret = sys_bpf(BPF_LINK_DETACH, &attr, sizeof(attr));
	return libbpf_err_errno(ret);
}

And realize that we're getting the error from the sys_bpf(BPF_LINK_DETACH) call because it still has a perf event file descriptor attached.

We first need to call bpf_link__destroy() without having called bpf_link__disconnect() first. This will make link->detach() function to be executed, freeing probe resources. One example might be the kprobe perf event detach function:

bpf_program__attach_kprobe_opts()

...
	link = bpf_program__attach_perf_event_opts(prog, pfd, &pe_opts);
...

and then the bpf_program__attach_perf_event_opts() function:

	link->link.detach = &bpf_link_perf_detach;
	link->link.dealloc = &bpf_link_perf_dealloc;
	link->perf_event_fd = pfd;

So basically when calling bpf_link__destroy():

int bpf_link__destroy(struct bpf_link *link)
{
	int err = 0;

	if (IS_ERR_OR_NULL(link))
		return 0;

	if (!link->disconnected && link->detach)
		err = link->detach(link);
	if (link->pin_path)
		free(link->pin_path);
	if (link->dealloc)
		link->dealloc(link);
	else
		free(link);

	return libbpf_err(err);
}

you're actually calling the link->detach logic (which is what you want).

[geyslan]

@rafaeldtinoco I got it. bpf_link__detach() is more likely an internal function. It's just curious why it's part of the public API. However, thanks so much for the review and feedback.

Is it okay if I force push this PR changing things accordingly? I mean, implementing the bpf_link__destroy() wrapper - BPFLink.Destroy()?

[rafaeldtinoco]

@rafaeldtinoco I got it. bpf_link__detach() is more likely an internal function. It's just curious why it's part of the public API. However, thanks so much for the review and feedback.

Is it okay if I force push this PR changing things accordingly? I mean, implementing the bpf_link__destroy() wrapper - BPFLink.Destroy()?

Yes, I think we should have Destroy and Disconnect. If you call Destroy it will call Detach. If you call Disconnect and Destroy, the destruction won't call Detach. This is how libbpf is doing things currently.

Note: we can try to leverage this discussion upstream IF there is another use case. this code has recently changed and I dont feel its solid rock before libbpf 1.0.

[yanivagman]

Interesting discussion!

If I understand correctly, the purpose of detach is to "prevent execution of a previously attached program from any future events" (https://facebookmicrosites.github.io/bpf/blog/2018/08/31/object-lifetime.html), which seems like something that I would want to do if I just wanted to temporarily stop an event from triggering the program. But then I ask myself - what is the meaning of detaching a link (and not destroying it) if there is no way to attach it back (without re-creating the link object)? I don't see any function named bpf_link__attach() that would do such a thing, or any other function in libbpf API that can do something similar, am I right? If that's the case, I don't see a reason (for now) to use this API, and not bpf_link__destroy() instead.

I also think there is a bug here in libbpf. If we already called link->detach(link) then it shouldn't return an error in bpf_link__destroy() as it does now:

	if (!link->disconnected && link->detach)
		err = link->detach(link);

This can be resolved by adding an extra boolean 'detached' to the bpf_link struct, and setting it when detach() is called. I guess that this is the error that @rafaeldtinoco sees, and not because:

And realize that we're getting the error from the sys_bpf(BPF_LINK_DETACH) call because it still has a perf event file descriptor attached.

[yanivagman]

After reading some more of the libbpf sources, I now understand that calling bpf_link__detach() is not the same as calling link->detach(link) as the first just calls sys_bpf(BPF_LINK_DETACH) while the second may release other resources, such as the perf event fd like @rafaeldtinoco mentioned. This is confusing to me, as the first 'detach' (bpf_link__detach()) doesn't seem to close any fd, just set some internal kernel state of the fd to be 'detached' while the second 'detach' (link->detach(link)) also closes the fd, in addition to some link type specific operations (such as PERF_EVENT_IOC_DISABLE).

So, I do think that there are still some things here that need to be considered upstream:

1. When should one use detach and not destroy considered that there is no way to re-attach an already detached link?
2. It seems that using bpf_link__detach() does not fit to all link types. For example, when attaching a (non legacy) kprobe, detaching it should probably happen using PERF_EVENT_IOC_DISABLE and not through sys_bpf(BPF_LINK_DETACH)

[rafaeldtinoco]

If I understand correctly, the purpose of detach is to "prevent execution of a previously attached program from any future events" (https://facebookmicrosites.github.io/bpf/blog/2018/08/31/object-lifetime.html), which seems like something that I would want to do if I just wanted to temporarily stop an event from triggering the program. But then I ask myself - what is the meaning of detaching a link (and not destroying it) if there is no way to attach it back (without re-creating the link object)? I don't see any function named bpf_link__attach() that would do such a thing, or any other function in libbpf API that can do something similar, am I right? If that's the case, I don't see a reason (for now) to use this API, and not bpf_link__destroy() instead.

After reading some more of the libbpf sources, I now understand that calling bpf_link__detach() is not the same as calling link->detach(link) as the first just calls sys_bpf(BPF_LINK_DETACH) while the second may release other resources, such as the perf event fd like @rafaeldtinoco mentioned. This is confusing to me, as the first 'detach' (bpf_link__detach()) doesn't seem to close any fd, just set some internal kernel state of the fd to be 'detached' while the second 'detach' (link->detach(link)) also closes the fd, in addition to some link type specific operations (such as PERF_EVENT_IOC_DISABLE).

Yep, that is exactly my understanding as well. I'm not sure that I like upstream approach here (and the plumbers presentation had a slide for this because of that).

So, I do think that there are still some things here that need to be considered upstream:

1. When should one use detach and not destroy considered that there is no way to re-attach an already detached link?

After the discussion in this issue, based on what we've seen.. never! Unless upstream changes its approach to detach. If we allow libbpfgo to have Detach() we might have issues because, like you said, and like we discussed, there won't be a good way to re-attach the links (specially considering that, for the new introduced legacy kprobes, for example, we might have bugs since the attachment did not clean up used resources).

2. It seems that using bpf_link__detach() does not fit to all link types. For example, when attaching a (non legacy) kprobe, detaching it should probably happen using PERF_EVENT_IOC_DISABLE and not through sys_bpf(BPF_LINK_DETACH)

The real purpose for bpf_link__detach(), as I could understand, was to simply consider the link 'detached' from the userland logic but to keep all resources still 'attached' for the kernel portions (or for another userland code that will use the file descriptors from pinned bpffs, for example).

With that said, there would not be a need for a specific link type detachment logic within bpf_link__detach() in the way it is now because it is meant to be 'generic'. (not saying I agree or find this useful/beautiful).

[geyslan]

When should one use detach and not destroy considered that there is no way to re-attach an already detached link?

I can't visualize any use cases at the moment, so I would implement the bpf_link__destroy() wrapper instead.

If we allow libbpfgo to have Detach() we might have issues because, like you said, and like we discussed, there won't be a good way to re-attach the links [...]

Agree.

[rafaeldtinoco]

When should one use detach and not destroy considered that there is no way to re-attach an already detached link?

I can't visualize any use cases at the moment, so I would implement the bpf_link__destroy() wrapper instead.

So we already have it:

func (l *BPFLink) Destroy() error {
	ret := C.bpf_link__destroy(l.link)
	if ret < 0 {
		return syscall.Errno(-ret)
	}
	return nil
}

and there is no need for anything else now, correct ? Would you mind force pushing your branch without libbpfgo: introduce Detach() for BPFLink then ? Assuming you still want to keep libbpfgo: introduce GetFd() for BPFLink.

Thanks.

[geyslan]

So we already have it:

Oh nice indeed. It's a recent change. =)

Would you mind force pushing your branch without libbpfgo: introduce Detach() for BPFLink then ? Assuming you still want to keep libbpfgo: introduce GetFd() for BPFLink.

Sure thing.

[rafaeldtinoco]

LGTM