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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8736fkob4g.fsf@toke.dk>
Date:   Tue, 22 Oct 2019 21:04:15 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        David Miller <davem@...emloft.net>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next 3/3] libbpf: Add pin option to automount BPF filesystem before pinning

Andrii Nakryiko <andrii.nakryiko@...il.com> writes:

> On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>>
>> From: Toke Høiland-Jørgensen <toke@...hat.com>
>>
>> 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.

>> 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 <toke@...hat.com>
>> ---
>>  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 :)

>> +               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