mount_bpffs_for_pin(): if passing a directory, mount bpffs on this dir, not on parent dir

[qmonnet]

Context: https://lore.kernel.org/bpf/[email protected]/T/#t

Function mount_bpffs_for_pin() was initially written for getting files as arguments, so it tries to mount (when relevant) the bpffs on the parent directory of the file it gets.

When receiving a directory name instead, it still uses the parent directory, when it should instead:

1. If the dir exists and is bpffs, then: do nothing
2. If the dir exists but is not bpffs, then: mount the bpffs, but on the provided directory only, not its parent
3. If the dir does not exist, but its parent dir is bpffs, then: do nothing
4. If the dir does not exist and its parent is not bpffs, then: mount the bpffs on the provided directory, but this requires to create this dir first.

[valdaarhun]

Hi. I am quite new to bpf/ebpf. I only recently started studying about it. Having spent some time tinkering with ebpf and bpftool, I have got some familiarity with it.

I am interested in the bpf subsystem of the linux kernel as well, and I think this would be a good first issue to get some exposure to bpftool's internals and linux bpf in general.

I haven't verified firsthand if this issue still exists. I plan on doing that and if it exists, I would like to work on resolving it as well.

[qmonnet]

I've not seen any fix being posted or merged, so I'm pretty sure the issue is still present. Thanks for picking this up! Let me know if you need help.

[qmonnet]

I'm pretty sure the issue is still present

And this should be easy to check by running an action that may try to load the bpffs on a given directory. If I remember correctly, bpftool prog loadall <elf_file> <some_dir> should do that and end up mounting the bpffs on the parent dir of <some_dir>.

[valdaarhun]

I've not seen any fix being posted or merged, so I'm pretty sure the issue is still present. Thanks for picking this up! Let me know if you need help.

Thank you for the reply. Sure, I'll let you know if I get stuck and need help.

And this should be easy to check by running an action that may try to load the bpffs on a given directory. If I remember correctly, bpftool prog loadall <elf_file> <some_dir> should do that and end up mounting the bpffs on the parent dir of <some_dir>.

Got it. I'll start with this.

[valdaarhun]

Hi. I have gone through the current implementation and tinkered with bpftool prog loadall. I think I have understood what the current behavior is and what its expected behavior should be. Correct me if I am wrong.

Reproducing the issue

• Create new bpffs mountpoint

  mount bpffs /var/run/example/map -t bpf

• Create a normal directory

  mkdir -p /var/run/example/test/pins

• Load and pin bpf program

  bpftool prog loadall ELF_OBJ /var/run/example/map/bpf
  Running `bpftool --bpffs prog show name BPF_OBJ_NAME`

  This simply pins the bpf program as /var/run/example/map/bpf/BPF_OBJ_NAME. Here, /var/run/example/map is the bpffs.

  But when pinning to /var/run/example/test/pins,

  bpftool prog loadall ELF_OBJ /var/run/example/test/pins

  the bpf program is pinned as /var/run/example/test/pins/BPF_OBJ_NAME. However, cat /proc/mounts shows that a new bpffs is mounted at /var/run/example/test.

Resolving this issue would mount the bpffs at /var/run/example/test/pins, right?

[qmonnet]

100% correct :) There's no logical reason why we should mount the bpffs on .../test rather than simply .../test/pins in that case, and mounting it this way could even hide other files that are present under .../test/, so we would like to fix that. Thanks!

[valdaarhun]

Thank you for the reply.

mounting it this way could even hide other files that are present under .../test/

Right, I noticed this and for a moment I thought I had corrupted my system. I'll start working on a fix.

[valdaarhun]

Hi. I have got a patch ready and have tested it. I have never sent a patch to a linux subsystem's mailing list before. I would like to clarify one thing before sending this patch.

Apart from sending the patch to [email protected], I will also need to cc the mail ids suggested by the get_maintainers.pl script in the linux repo, right?

[qmonnet]

Apart from sending the patch to [email protected], I will also need to cc the mail ids suggested by the get_maintainers.pl script in the linux repo, right?

Correct - You need at least myself (for bpftool), the BPF maintainers (Alexei, Daniel, Andrii), ideally the BPF reviewers; and just add the other people as well if the script mentions them. You don't need linux-kernel@ in copy, though.

For your information, there's some doc on the submitting process:

• Here for the submissions related to BPF
• Here for the generic documentation on submitting patches (it's dense).

Let me know if there's anything else I can help with. If you feel more comfortable sending me a preliminary version to test your setup, that's entirely doable, too.

[valdaarhun]

Thank you for the reply.

Correct - You need at least myself (for bpftool), the BPF maintainers (Alexei, Daniel, Andrii), ideally the BPF reviewers; and just add the other people as well if the script mentions them. You don't need linux-kernel@ in copy, though.

Understood, I'll do that.

For your information, there's some doc on the submitting process:

* [Here for the submissions related to BPF](https://docs.kernel.org/bpf/bpf_devel_QA.html#submitting-patches)

* [Here for the generic documentation on submitting patches](https://docs.kernel.org/process/submitting-patches.html) (it's dense).

I had read through the second resource. I almost missed the first one. I'll go through that too.

Let me know if there's anything else I can help with.

I am confused about one thing. I prepared my patch with the bpf-next prefix since that's the authoritative source code for development and this repo is synced with that tree. After reading the answer to this question and netdev's doc, I am slightly confused about which tree it should target. Since this issue has been tagged as a bug, should I rebase my patch against the bpf tree?

If you feel more comfortable sending me a preliminary version to test your setup, that's entirely doable, too.

Thank you, that will be very helpful. I'll attach the file here.

0001-bpftool-Mount-bpffs-on-provided-dir-instead-of-paren.zip

Also, I haven't written any test yet. That will be a learning curve as well for me. I'll work on writing a test if that's required and relevant for this patch.

[qmonnet]

I am confused about one thing. I prepared my patch with the bpf-next prefix since that's the authoritative source code for development and this repo is synced with that tree. After reading the answer to this question and netdev's doc, I am slightly confused about which tree it should target. Since this issue has been tagged as a bug, should I rebase my patch against the bpf tree?

Please target bpf-next. This is not a kernel bug, simply a bug in bpftool, and we're usually routing them through bpf-next - they land in the GitHub repo just as fast.

If you feel more comfortable sending me a preliminary version to test your setup, that's entirely doable, too.

Thank you, that will be very helpful. I'll attach the file here.

0001-bpftool-Mount-bpffs-on-provided-dir-instead-of-paren.zip

Thanks, I will try to take a look tomorrow.

Also, I haven't written any test yet. That will be a learning curve as well for me. I'll work on writing a test if that's required and relevant for this patch.

Relevant, yes, required, no. We have a few tests for bpftool but overall we're behind in terms of CI, so there's no requirement on that side currently. See #64

[valdaarhun]

Please target bpf-next. This is not a kernel bug, simply a bug in bpftool, and we're usually routing them through bpf-next - they land in the GitHub repo just as fast.

Got it.

Thanks, I will try to take a look tomorrow.

Sure thing! Thank you.

Relevant, yes, required, no. We have a few tests for bpftool but overall we're behind in terms of CI, so there's no requirement on that side currently. See #64

In that case, I think I'll let this be until later, unless the changes in this patch necessitates a test as soon as possible.

[valdaarhun]

Hi. I have gone through this page on submitting bpf patches. I also opened a PR here to run the CI against my changes.

I decided to send the patch to the mailing list since there were no problems with the CI run. Please let me know if I have missed something.

Patch thread here.

[qmonnet]

The submission itself is all good, thanks! I commented on the code directly on the mailing list.

[valdaarhun]

Thank you for the review. I'll make the changes and will send a new patch.

[qmonnet]

This is the way :)

[qmonnet]

Note: Looking at the code around mounting the bpffs in bpftool's prog.c, I see that we don't call mount_bpffs_for_pin() for the directory where the user wants to pin maps, if pinmaps is passed:

	err = mount_bpffs_for_pin(pinfile, !first_prog_only);
	if (err)
		goto err_close_obj;

	if (first_prog_only) {
		prog = bpf_object__next_program(obj, NULL);
		if (!prog) {
			p_err("object file doesn't contain any bpf program");
			goto err_close_obj;
		}

		if (auto_attach)
			err = auto_attach_program(prog, pinfile);
		else
			err = bpf_obj_pin(bpf_program__fd(prog), pinfile);
		if (err) {
			p_err("failed to pin program %s",
			      bpf_program__section_name(prog));
			goto err_close_obj;
		}
	} else {
		if (auto_attach)
			err = auto_attach_programs(obj, pinfile);
		else
			err = bpf_object__pin_programs(obj, pinfile);
		if (err) {
			p_err("failed to pin all programs");
			goto err_close_obj;
		}
	}

	if (pinmaps) {

		// XXX No guarantee that the bpffs is mounted for directory pinmaps, here

		err = bpf_object__pin_maps(obj, pinmaps);
		if (err) {
			p_err("failed to pin all maps");
			goto err_unpin;
		}
	}

I haven't tested it though, but I expect the map pinning to fail if we pass a pinmaps directory that is not under the bpffs.

This is something you could address by adding a second patch to your series, if you wanted - obviously you don't have to. I'll open a dedicated issue to track this otherwise.

[valdaarhun]

I haven't tested it though, but I expect the map pinning to fail if we pass a pinmaps directory that is not under the bpffs.

This is something you could address by adding a second patch to your series, if you wanted - obviously you don't have to. I'll open a dedicated issue to track this otherwise.

Hi. I would definitely like to take this up too. I am almost ready with v2 of the first patch. I'll test this out as well. If I can test this out and get a fix ready for this, then I'll send this patch as well as part of v2. Otherwise - if it's all right - I'll work on this while concurrently sending v2 of the first patch for review.