Re: [PATCH bpf-next 3/3] libbpf: Add pin option to automount BPF filesystem before pinning

On Tue, Oct 22, 2019 at 12:04 PM Toke Høiland-Jørgensen [off-list ref] wrote:

Andrii Nakryiko [off-list ref] writes:

quoted

On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

From: Toke Høiland-Jørgensen <redacted>

While the current map pinning functions will check whether the pin path is
contained on a BPF filesystem, it does not offer any options to mount the
file system if it doesn't exist. Since we now have pinning options, add a
new one to automount a BPF filesystem at the pinning path if that is not

Next thing we'll be adding extra options to mount BPF FS... Can we
leave the task of auto-mounting BPF FS to tools/applications?

Well, there was a reason I put this into a separate patch: I wasn't sure
it really fit here. My reasoning is the following: If we end up with a
default auto-pinning that works really well, people are going to just
use that. And end up really confused when bpffs is not mounted. And it
seems kinda silly to make every application re-implement the same mount
check and logic.

Or to put it another way: If we agree that the reasonable default thing
is to just pin things in /sys/fs/bpf, let's make it as easy as possible
for applications to do that right.

This reminds me the setrlimit() issue, though. And we decided that
library shouldn't be manipulating global resources on behalf of users.
I think this is a similar one.

quoted

quoted

already pointing at a bpffs.

The mounting logic itself is copied from the iproute2 BPF helper functions.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 tools/lib/bpf/libbpf.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 tools/lib/bpf/libbpf.h |    5 ++++-
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index aea3916de341..f527224bb211 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -37,6 +37,7 @@
 #include <sys/epoll.h>
 #include <sys/ioctl.h>
 #include <sys/mman.h>
+#include <sys/mount.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <sys/vfs.h>

@@ -4072,6 +4073,35 @@ int bpf_map__unpin(struct bpf_map *map, const char *path)
        return 0;
 }

+static int mount_bpf_fs(const char *target)
+{
+       bool bind_done = false;
+
+       while (mount("", target, "none", MS_PRIVATE | MS_REC, NULL)) {

what does this loop do? we need some comments explaining what's going
on here

Well, as it says in the commit message I stole this from iproute2. I
think the "--make-private, --bind" dance is there to make sure we don't
mess up some other mount points at this path. Which seems like a good
idea, and one of those things that most people probably won't think
about when just writing an application that wants to mount the fs; which
is another reason to put this into libbpf :)

I think this is exactly a reason to not do this and rely on
applications to know and set up their environment properly. All these
races, accidentally stomping on someone else's FS, etc, that sounds
like a really good excuse to not do this in libbpf. Definitely not
until we get a real experience, driven by production use cases, on how
to go about that correctly. It might be added as a helper, but I think
application has to call it explicitly.

quoted

quoted

+               if (errno != EINVAL || bind_done) {
+                       pr_warning("mount --make-private %s failed: %s\n",
+                                  target, strerror(errno));
+                       return -1;
+               }
+
+               if (mount(target, target, "none", MS_BIND, NULL)) {
+                       pr_warning("mount --bind %s %s failed: %s\n",
+                                  target, target, strerror(errno));
+                       return -1;
+               }
+
+               bind_done = true;
+       }
+
+       if (mount("bpf", target, "bpf", 0, "mode=0700")) {
+               fprintf(stderr, "mount -t bpf bpf %s failed: %s\n",
+                       target, strerror(errno));
+               return -1;
+       }
+
+       return 0;
+}
+
 static int get_pin_path(char *buf, size_t buf_len,
                        struct bpf_map *map, struct bpf_object_pin_opts *opts,
                        bool mkdir)

@@ -4102,6 +4132,23 @@ static int get_pin_path(char *buf, size_t buf_len,

Nothing in `get_pin_path` indicates that it's going to do an entire FS
mount, please split this out of get_pin_path.

Regardless of the arguments above, that is certainly a fair point ;)

-Toke