bugtool: get bpffs mountpoint from /proc/mounts #13342

jibi · 2020-09-30T08:49:10Z

Rather then hardcoding the /sys/fs/bpf value in bugtool, parse
/proc/mounts to determine the correct mountpoint for the BPF filesystem.

Fixes: #13218

Signed-off-by: Gilberto Bertin gilberto@isovalent.com

qmonnet

Looks great, thank you!

This won't be “tested” by the CI, I suppose you validated the change locally to make sure we still dump the maps as intended?

bugtool/cmd/configuration.go

qmonnet · 2020-09-30T09:06:20Z

I'm just thinking: Although unlikely, we could have several bpffs mounted at different points. Would it be worth to try and list the maps under each of them in that case?

jibi · 2020-09-30T09:11:28Z

Looks great, thank you!

This won't be “tested” by the CI, I suppose you validated the change locally to make sure we still dump the maps as intended?

Yes, I tested it both manually in the dev VMs as well as by crashing some e2e tests and looking at its dump in /tmp

jibi · 2020-09-30T09:12:33Z

I'm just thinking: Although unlikely, we could have several bpffs mounted at different points. Would it be worth to try and list the maps under each of them in that case?

I wasn't aware of this possibility but yes, I agree that we should try list maps under each of the mounted bpffs 👍

qmonnet · 2020-09-30T09:22:25Z

I wasn't aware of this possibility but yes, I agree that we should try list maps under each of the mounted bpffs +1

You can mount multiple ones, but I don't think this is something Cilium would ever do. Just thinking that if for any reason users have mounted another bpffs, we would risk missing the correct one when dumping the maps.

vadorovsky

Thanks for the PR!

We already have a separate package for listing mounts (pkg/mountinfo), I think it would be better to use it here.

vadorovsky · 2020-09-30T10:13:16Z

bugtool/cmd/configuration.go

@@ -39,6 +41,27 @@ func setupDefaultConfig(path string, k8sPods []string, confDir, cmdDir string) (
 	return &c, save(&c, path)
 }

+func bpffsMountpoint() []string {


We already have a separate module for listing mounts from /proc/self/mountinfo (which contains more details than /proc/mounts):

https://github.com/cilium/cilium/blob/master/pkg/mountinfo/mountinfo.go#L125-L135

The example of using it, also for checking BPFFS mount, is here:

https://github.com/cilium/cilium/blob/master/pkg/bpf/bpffs_linux.go#L69-L81

Thanks @mrostecki, I'll take a look at that package now

vadorovsky · 2020-09-30T10:17:59Z

@qmonnet @jibi

You can mount multiple ones, but I don't think this is something Cilium would ever do. Just thinking that if for any reason users have mounted another bpffs, we would risk missing the correct one when dumping the maps.

In cilium-agent we even return an explicit error if there are multiple BPFFS mounts:

https://github.com/cilium/cilium/blob/master/pkg/bpf/bpffs_linux.go#L278-L284

jibi · 2020-09-30T10:26:55Z

@qmonnet @jibi

You can mount multiple ones, but I don't think this is something Cilium would ever do. Just thinking that if for any reason users have mounted another bpffs, we would risk missing the correct one when dumping the maps.

In cilium-agent we even return an explicit error if there are multiple BPFFS mounts:

https://github.com/cilium/cilium/blob/master/pkg/bpf/bpffs_linux.go#L278-L284

It looks like we have a case of multiple bpf fs even in the dev VM:

vagrant@k8s1:/sys/fs/bpf$ cat /proc/mounts | grep bpf
bpffs /sys/fs/bpf bpf rw,relatime 0 0
bpffs /sys/fs/bpf/xdp bpf rw,relatime 0 0
bpffs /sys/fs/bpf/ip bpf rw,relatime 0 0
bpffs /sys/fs/bpf/sk bpf rw,relatime 0 0
bpffs /sys/fs/bpf/sa bpf rw,relatime 0 0

and I'm slightly confused as tc (which is the only one we are interested in) is just a subdirectory of the main mountpoint (/sys/fs/bpf) while xdp, ip, sk and sa are all additional bpf filesystems 🤔

edit:
I'm getting multiple mountpoints also with the mountinfo module:

vagrant@k8s1:~/go/src/github.com/cilium/cilium/bugtool$ sudo ./cilium-bugtool
mountpoints: [/sys/fs/bpf /sys/fs/bpf/xdp /sys/fs/bpf/ip /sys/fs/bpf/sk /sys/fs/bpf/sa]
[..]

qmonnet · 2020-09-30T11:05:13Z

iproute2 considers tc as a reference subdirectory and attempts to do symlinks for the other ones (see related code), but seems to fall back on bind mounts when it fails with something different than EEXIST or EPERM. Not sure what's happening in the dev VMs.

Hmm I wonder why this does not seem to be picked up by the check in bpffs_linux.go 🤔

vadorovsky · 2020-09-30T11:39:06Z

@jibi /sys/fs/bpf/xdp, /sys/fs/bpf/ip, /sys/fs/bpf/sk and /sys/fs/bpf/sa are sub mounts of /sys/fs/bpf (it means /sys/fs/bpf is their parent mount). So it's fine.

We want to avoid multiple BPFFS mounts which have / as their root. As we do it here: https://github.com/cilium/cilium/blob/master/pkg/bpf/bpffs_linux.go#L162

vadorovsky · 2020-09-30T11:41:30Z

@qmonnet The last link I pasted is the reason why cilium-agent doesn't fail because of `/sys/fs/bpf´ subdirectories. ;)

jibi · 2020-09-30T12:07:49Z

Thank you both @qmonnet and @mrostecki, I just learnt a bunch of new things :)

In this case I think i will:

assume there's only a single bpffs mounted
consider it to be the first one returned by mountinfo.GetMountInfo() which has both the bpf type and and the / root

qmonnet

Looks good to me, please just add a comment so we can remember the logics here.

bugtool/cmd/configuration.go

Rather then hardcoding the /sys/fs/bpf value in bugtool, use the `mountinfo` package (which exposes the information in /proc/self/mounts) to determine the correct mountpoint for the BPF filesystem. Fixes: #13218 Signed-off-by: Gilberto Bertin <gilberto@isovalent.com>

qmonnet

Looks good to me, thanks!

vadorovsky

Looks good. Thanks!

jibi requested a review from a team as a code owner September 30, 2020 08:49

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 30, 2020

qmonnet self-requested a review September 30, 2020 08:51

qmonnet approved these changes Sep 30, 2020

View reviewed changes

bugtool/cmd/configuration.go Outdated Show resolved Hide resolved

qmonnet added area/bugtool Impacts gathering of data for debugging purposes. area/CI Continuous Integration testing issue or flake ci/flake This is a known failure that occurs in the tree. Please investigate me! release-note/ci This PR makes changes to the CI. labels Sep 30, 2020

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 30, 2020

qmonnet added the sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label Sep 30, 2020

qmonnet requested a review from a team September 30, 2020 09:06

tklauser approved these changes Sep 30, 2020

View reviewed changes

pchaigno approved these changes Sep 30, 2020

View reviewed changes

jibi marked this pull request as draft September 30, 2020 09:58

vadorovsky requested changes Sep 30, 2020

View reviewed changes

jibi marked this pull request as ready for review September 30, 2020 12:17

qmonnet requested changes Sep 30, 2020

View reviewed changes

bugtool/cmd/configuration.go Show resolved Hide resolved

qmonnet approved these changes Sep 30, 2020

View reviewed changes

qmonnet requested a review from vadorovsky September 30, 2020 13:56

qmonnet added needs-backport/1.7 labels Sep 30, 2020

maintainer-s-little-helper bot added this to Needs backport from master in 1.8.4 Sep 30, 2020

maintainer-s-little-helper bot added this to Needs backport from master in 1.7.10 Sep 30, 2020

vadorovsky approved these changes Sep 30, 2020

View reviewed changes

pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 30, 2020

joestringer removed this from Needs backport from master in 1.8.4 Sep 30, 2020

joestringer removed this from Needs backport from master in 1.7.10 Sep 30, 2020

joestringer added this to Needs backport from master in 1.7.11 Sep 30, 2020

joestringer added this to Needs backport from master in 1.8.5 Sep 30, 2020

borkmann merged commit 57d3473 into cilium:master Oct 1, 2020

jibi deleted the pr/bugtool-get-bpffs-mountpoint-from-proc-mounts branch October 1, 2020 11:21

errordeveloper mentioned this pull request Oct 2, 2020

v1.8 backports 2020-10-02 #13384

Merged

errordeveloper added backport-pending/1.8 and removed needs-backport/1.8 labels Oct 2, 2020

pchaigno mentioned this pull request Oct 5, 2020

v1.7 backports 2020-10-05 #13402

Merged

pchaigno added backport-pending/1.7 and removed needs-backport/1.7 labels Oct 5, 2020

errordeveloper added backport-done/1.8 and removed backport-pending/1.8 labels Oct 6, 2020

qmonnet added backport-done/1.7 and removed backport-pending/1.7 labels Oct 7, 2020

tklauser moved this from Needs backport from master to Backport done to v1.8 in 1.8.5 Oct 13, 2020

tklauser moved this from Needs backport from master to Backport done to v1.7 in 1.7.11 Oct 13, 2020

This was referenced Oct 28, 2020

Prepare for release v1.7.11 #13790

Merged

Prepare for release v1.8.5 #13803

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugtool: get bpffs mountpoint from /proc/mounts #13342

bugtool: get bpffs mountpoint from /proc/mounts #13342

jibi commented Sep 30, 2020

qmonnet left a comment

qmonnet commented Sep 30, 2020

jibi commented Sep 30, 2020

jibi commented Sep 30, 2020

qmonnet commented Sep 30, 2020

vadorovsky left a comment

vadorovsky Sep 30, 2020

jibi Sep 30, 2020

vadorovsky commented Sep 30, 2020

jibi commented Sep 30, 2020 •

edited

qmonnet commented Sep 30, 2020 •

edited

vadorovsky commented Sep 30, 2020 •

edited

vadorovsky commented Sep 30, 2020

jibi commented Sep 30, 2020

qmonnet left a comment

qmonnet left a comment

vadorovsky left a comment

bugtool: get bpffs mountpoint from /proc/mounts #13342

bugtool: get bpffs mountpoint from /proc/mounts #13342

Conversation

jibi commented Sep 30, 2020

qmonnet left a comment

Choose a reason for hiding this comment

qmonnet commented Sep 30, 2020

jibi commented Sep 30, 2020

jibi commented Sep 30, 2020

qmonnet commented Sep 30, 2020

vadorovsky left a comment

Choose a reason for hiding this comment

vadorovsky Sep 30, 2020

Choose a reason for hiding this comment

jibi Sep 30, 2020

Choose a reason for hiding this comment

vadorovsky commented Sep 30, 2020

jibi commented Sep 30, 2020 • edited

qmonnet commented Sep 30, 2020 • edited

vadorovsky commented Sep 30, 2020 • edited

vadorovsky commented Sep 30, 2020

jibi commented Sep 30, 2020

qmonnet left a comment

Choose a reason for hiding this comment

qmonnet left a comment

Choose a reason for hiding this comment

vadorovsky left a comment

Choose a reason for hiding this comment

jibi commented Sep 30, 2020 •

edited

qmonnet commented Sep 30, 2020 •

edited

vadorovsky commented Sep 30, 2020 •

edited