bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Toke Høiland-Jørgensen" <toke@redhat.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>,
	Alexei Starovoitov <ast@kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>,
	Jesper Dangaard Brouer <brouer@redhat.com>,
	David Miller <davem@davemloft.net>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf-next 3/3] libbpf: Add pin option to automount BPF filesystem before pinning
Date: Tue, 22 Oct 2019 21:04:15 +0200	[thread overview]
Message-ID: <8736fkob4g.fsf@toke.dk> (raw)
In-Reply-To: <CAEf4BzbfV5vrFnkNyG35Db2iPmM2ubtFh6OTvLiaetAx6eFHHw@mail.gmail.com>

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

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

  reply	other threads:[~2019-10-22 19:04 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-22 15:04 [PATCH bpf-next 0/3] libbpf: Support pinning of maps using 'pinning' BTF attribute Toke Høiland-Jørgensen
2019-10-22 15:04 ` [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map Toke Høiland-Jørgensen
2019-10-22 17:37   ` Andrii Nakryiko
2019-10-22 18:13     ` Toke Høiland-Jørgensen
2019-10-22 18:23       ` Andrii Nakryiko
2019-10-22 18:45         ` Toke Høiland-Jørgensen
2019-10-22 18:49           ` Andrii Nakryiko
2019-10-22 19:06             ` Toke Høiland-Jørgensen
2019-10-23  4:43               ` Andrii Nakryiko
2019-10-22 15:04 ` [PATCH bpf-next 2/3] libbpf: Support configurable pinning of maps from BTF annotations Toke Høiland-Jørgensen
2019-10-22 18:20   ` Andrii Nakryiko
2019-10-22 18:57     ` Toke Høiland-Jørgensen
2019-10-23  4:40       ` Andrii Nakryiko
2019-10-23  8:53         ` Toke Høiland-Jørgensen
2019-10-23 12:30           ` Daniel Borkmann
2019-10-23 13:05             ` Toke Høiland-Jørgensen
2019-10-22 15:04 ` [PATCH bpf-next 3/3] libbpf: Add pin option to automount BPF filesystem before pinning Toke Høiland-Jørgensen
2019-10-22 18:28   ` Andrii Nakryiko
2019-10-22 19:04     ` Toke Høiland-Jørgensen [this message]
2019-10-23  4:58       ` Andrii Nakryiko
2019-10-23  8:58         ` Toke Høiland-Jørgensen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8736fkob4g.fsf@toke.dk \
    --to=toke@redhat.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=brouer@redhat.com \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=kafai@fb.com \
    --cc=netdev@vger.kernel.org \
    --cc=songliubraving@fb.com \
    --cc=yhs@fb.com \
    --subject='Re: [PATCH bpf-next 3/3] libbpf: Add pin option to automount BPF filesystem before pinning' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).