Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add CO-RE bitfield support to bindsnoop #529

Merged
merged 3 commits into from Apr 27, 2022
Merged

Add CO-RE bitfield support to bindsnoop #529

merged 3 commits into from Apr 27, 2022

Conversation

eiffel-fl
Copy link
Member

@eiffel-fl eiffel-fl commented Feb 22, 2022 •
edited

Hi.

This PR uses a new feature from cilium/ebpf to be able to print port and options of a bind().

Best regards.

@eiffel-fl eiffel-fl force-pushed the francis/bindsnoop-golang-core-bitfield branch from b52dc69 to a4efbb2 Compare February 22, 2022 17:26
@eiffel-fl eiffel-fl force-pushed the francis/bindsnoop-golang-core-bitfield branch from a4efbb2 to 5dc520b Compare February 22, 2022 17:34
@eiffel-fl eiffel-fl force-pushed the francis/bindsnoop-golang branch 4 times, most recently from 26347a2 to f846a09 Compare March 1, 2022 12:29
Base automatically changed from francis/bindsnoop-golang to main March 4, 2022 12:06
@eiffel-fl eiffel-fl force-pushed the francis/bindsnoop-golang-core-bitfield branch from 5dc520b to 414806a Compare March 15, 2022 19:10
@eiffel-fl eiffel-fl marked this pull request as ready for review March 15, 2022 19:11
@mauriciovasquezbernal mauriciovasquezbernal added this to the v0.6.0 milestone Mar 30, 2022
@eiffel-fl eiffel-fl force-pushed the francis/bindsnoop-golang-core-bitfield branch 3 times, most recently from a6cd587 to 7fdde77 Compare April 1, 2022 14:00
@eiffel-fl
Copy link
Member Author

eiffel-fl commented Apr 1, 2022 •
edited

I reworked this PR to use cilium/ebpf master on a specific commit since cilium/ebpf#573 was merged.
But I am not forcefully satisfied with how I handled it, so your reviews are welcomed.
Particularly, I do not know if we should wait for a cilium/ebpf release to use and I do not like how I had to deal with CI cache.

mauriciovasquezbernal
mauriciovasquezbernal reviewed Apr 1, 2022
View reviewed changes
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lest some comments. Please organize the commits a bit, commit messages are not complete and the don't follow a logic sequence.

pkg/gadgettracermanager/gadgettracermanager.go Outdated Show resolved Hide resolved
.github/actions/install-debian-packages/action.yml Outdated
Comment on lines 22 to 24
# There is no way to clear the cache and llvm-strip is needed by bpf2go.
# For now, we will add it here.
sudo apt install llvm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about updating the key used in actions/cache@v2?

Btw, now that I look with more detail at the logic to install the deb packages, isn't it possible to move all usages of actions/cache@v2 from workflows/inspektor-gadget.yml to install-debian-packages/action.yml to avoid repeating that so many times?

@eiffel-fl
Copy link
Member Author

Please organize the commits a bit, commit messages are not complete and the don't follow a logic sequence.

I am also not satified with them and I do not really know how to organize them...
The only good way to do it would be:

  1. One commit to bump cilium/ebpf version which also adapt the code to upstream changes.
  2. Another which does the revert.
    Do you see a better option?

@eiffel-fl eiffel-fl force-pushed the francis/bindsnoop-golang-core-bitfield branch from 7fdde77 to 0dffcd6 Compare April 1, 2022 15:41
@mauriciovasquezbernal
Copy link
Member 

1. One commit to bump cilium/ebpf version which also adapt the code to upstream changes.

2. Another which does the revert.
   Do you see a better option?

Yes, that should work fine.

@eiffel-fl eiffel-fl force-pushed the francis/bindsnoop-golang-core-bitfield branch 2 times, most recently from 1a31213 to ced6dc7 Compare April 1, 2022 16:57
@eiffel-fl eiffel-fl force-pushed the francis/bindsnoop-golang-core-bitfield branch from 6de1edb to bef78b3 Compare April 5, 2022 13:20
mauriciovasquezbernal
mauriciovasquezbernal reviewed Apr 6, 2022
View reviewed changes
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested and it worked fine. Just one last question about debs cache mechanism in github actions.

.github/actions/install-debian-packages/action.yml
with:
path: "~/cache-debs"
# Update cache key if you add or update a package.
key: v3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One additional question here. What is the procedure to update a dependency? Or better, how avoid this logic to always use the check instead of trying to install new versions of the libraries?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as we do not change the cache key, we will be stuck with the version of the software which were installed for this given cache key.
This should not be a problem for libbpf and libbseccomp as we install specific version of this libraries, but for llvm it can be more troublesome...
We will need to think to a better way to handle this case while not losing the speed gain of the cache.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Let's think about a better way to handle it in another PR.

mauriciovasquezbernal
mauriciovasquezbernal approved these changes Apr 7, 2022
View reviewed changes
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Please notice we should wait the release of v0.5.0 before merging it.

@eiffel-fl
Copy link
Member Author

Thank you for the review!
I will hold it until.

@eiffel-fl
go.mod: Bump cilium/ebpf to 4d5607ee1690.
632e5ba 
We need to bump cilium/ebpf to be able to use BPF_CORE_READ_BITFIELD_PROBED().
To do so, we had to modify the following due to upstream changes:
1. Replace ebpf.RemoveMemlockRlimit() by rlimit.RemoveMemlock().
The former was removed in:
8ea11c59b057 ("rlimit: create package, only bump memlock to infinite if necessary")
2. Add -no-global-types to bpf2go running due to compile errors.
3. Add nil as last argument to link.K(ret)?probe() and link.Tracepoint() because
they take now an option argument.
4. Add llvm to container software and CI because bfp2go now needs llvm-strip.

Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
@eiffel-fl
Revert "pkg/gadgets: Comment out BPF_CORE_READ_BITFIELD_PROBED in bin…
12c4340 
…dsnoop CO-RE."

This reverts commit ff78729.
@eiffel-fl
docs: Update OPTS output for trace bind guide.
873557c 
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
@eiffel-fl eiffel-fl force-pushed the francis/bindsnoop-golang-core-bitfield branch from bef78b3 to 873557c Compare April 26, 2022 15:59
@margamanterola
Copy link
Contributor

0.5.0 has been released, do you want to merge this now?

@margamanterola margamanterola modified the milestones: v0.6.0, v0.5.1 Apr 27, 2022
@eiffel-fl
Copy link
Member Author

eiffel-fl commented Apr 27, 2022 via email

@eiffel-fl eiffel-fl merged commit 79af7fc into main Apr 27, 2022
@eiffel-fl
Copy link
Member Author

Thank you for the reviews!

@eiffel-fl eiffel-fl deleted the francis/bindsnoop-golang-core-bitfield branch April 27, 2022 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mauriciovasquezbernal mauriciovasquezbernal mauriciovasquezbernal approved these changes

Assignees
No one assigned
Labels
state/hold
Projects
None yet
Milestone
v0.5.1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@eiffel-fl @mauriciovasquezbernal @margamanterola