Message-ID: <6577a8ce777d9_1449c20858@john.notmuch>
Date: Mon, 11 Dec 2023 16:26:54 -0800
From: John Fastabend <john.fastabend@...il.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>, 
 John Fastabend <john.fastabend@...il.com>
Cc: Andrii Nakryiko <andrii@...nel.org>, 
 bpf@...r.kernel.org, 
 netdev@...r.kernel.org, 
 paul@...l-moore.com, 
 brauner@...nel.org, 
 linux-fsdevel@...r.kernel.org, 
 linux-security-module@...r.kernel.org, 
 keescook@...omium.org, 
 kernel-team@...a.com, 
 sargun@...gun.me
Subject: Re: [PATCH bpf-next 6/8] libbpf: wire up BPF token support at BPF
 object level

Andrii Nakryiko wrote:
> On Mon, Dec 11, 2023 at 2:56 PM John Fastabend <john.fastabend@...il.com> wrote:
> >
> > Andrii Nakryiko wrote:
> > > Add BPF token support to BPF object-level functionality.
> > >
> > > BPF token is supported by BPF object logic either as an explicitly
> > > provided BPF token from outside (through BPF FS path or explicit BPF
> > > token FD), or implicitly (unless prevented through
> > > bpf_object_open_opts).
> > >
> > > Implicit mode is assumed to be the most common one for user namespaced
> > > unprivileged workloads. The assumption is that privileged container
> > > manager sets up default BPF FS mount point at /sys/fs/bpf with BPF token
> > > delegation options (delegate_{cmds,maps,progs,attachs} mount options).
> > > BPF object during loading will attempt to create BPF token from
> > > /sys/fs/bpf location, and pass it for all relevant operations
> > > (currently, map creation, BTF load, and program load).
> > >
> > > In this implicit mode, if BPF token creation fails due to whatever
> > > reason (BPF FS is not mounted, or kernel doesn't support BPF token,
> > > etc), this is not considered an error. BPF object loading sequence will
> > > proceed with no BPF token.
> > >
> > > In explicit BPF token mode, user provides explicitly either custom BPF
> > > FS mount point path or creates BPF token on their own and just passes
> > > token FD directly. In such case, BPF object will either dup() token FD
> > > (to not require caller to hold onto it for entire duration of BPF object
> > > lifetime) or will attempt to create BPF token from provided BPF FS
> > > location. If BPF token creation fails, that is considered a critical
> > > error and BPF object load fails with an error.
> > >
> > > Libbpf provides a way to disable implicit BPF token creation, if it
> > > causes any troubles (BPF token is designed to be completely optional and
> > > shouldn't cause any problems even if provided, but in the world of BPF
> > > LSM, custom security logic can be installed that might change outcome
> > > dependin on the presence of BPF token). To disable libbpf's default BPF
> > > token creation behavior user should provide either invalid BPF token FD
> > > (negative), or empty bpf_token_path option.
> > >
> > > BPF token presence can influence libbpf's feature probing, so if BPF
> > > object has associated BPF token, feature probing is instructed to use
> > > BPF object-specific feature detection cache and token FD.
> > >
> > > Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
> > > ---
> > >  tools/lib/bpf/btf.c             |   7 +-
> > >  tools/lib/bpf/libbpf.c          | 120 ++++++++++++++++++++++++++++++--
> > >  tools/lib/bpf/libbpf.h          |  28 +++++++-
> > >  tools/lib/bpf/libbpf_internal.h |  17 ++++-
> > >  4 files changed, 160 insertions(+), 12 deletions(-)
> > >
> >
> > ...
> >
> > >
> > > +static int bpf_object_prepare_token(struct bpf_object *obj)
> > > +{
> > > +     const char *bpffs_path;
> > > +     int bpffs_fd = -1, token_fd, err;
> > > +     bool mandatory;
> > > +     enum libbpf_print_level level = LIBBPF_DEBUG;
> >
> > redundant set on level?
> >
> 
> yep, removed initialization
> 
> > > +
> > > +     /* token is already set up */
> > > +     if (obj->token_fd > 0)
> > > +             return 0;
> > > +     /* token is explicitly prevented */
> > > +     if (obj->token_fd < 0) {
> > > +             pr_debug("object '%s': token is prevented, skipping...\n", obj->name);
> > > +             /* reset to zero to avoid extra checks during map_create and prog_load steps */
> > > +             obj->token_fd = 0;
> > > +             return 0;
> > > +     }
> > > +
> > > +     mandatory = obj->token_path != NULL;
> > > +     level = mandatory ? LIBBPF_WARN : LIBBPF_DEBUG;
> > > +
> > > +     bpffs_path = obj->token_path ?: BPF_FS_DEFAULT_PATH;
> > > +     bpffs_fd = open(bpffs_path, O_DIRECTORY, O_RDWR);
> > > +     if (bpffs_fd < 0) {
> > > +             err = -errno;
> > > +             __pr(level, "object '%s': failed (%d) to open BPF FS mount at '%s'%s\n",
> > > +                  obj->name, err, bpffs_path,
> > > +                  mandatory ? "" : ", skipping optional step...");
> > > +             return mandatory ? err : 0;
> > > +     }
> > > +
> > > +     token_fd = bpf_token_create(bpffs_fd, 0);
> >
> > Did this get tested on older kernels? In that case TOKEN_CREATE will
> > fail with -EINVAL.
> 
> yep, I did actually test, it will generate expected *debug*-level
> "failed to create BPF token" message

Great.

> 
> >
> > > +     close(bpffs_fd);
> > > +     if (token_fd < 0) {
> > > +             if (!mandatory && token_fd == -ENOENT) {
> > > +                     pr_debug("object '%s': BPF FS at '%s' doesn't have BPF token delegation set up, skipping...\n",
> > > +                              obj->name, bpffs_path);
> > > +                     return 0;
> > > +             }
> >
> > Isn't there a case here we should give a warning about?  If BPF_TOKEN_CREATE
> > exists and !mandatory, but default BPFFS failed for enomem, or eperm reasons?
> > If the user reall/y doesn't want tokens here they should maybe override with
> > -1 token? My thought is if you have delegations set up then something on the
> > system is trying to configure this and an error might be ok? I'm asking just
> > because I paused on it for a bit not sure either way at the moment. I might
> > imagine a lazy program not specifying the default bpffs, but also really
> > thinking its going to get a valid token.
> 
> Interesting perspective! I actually came from the direction that BPF
> token is not really all that common and expected thing, and so in
> majority of cases (at least for some time) we won't be expecting to
> have BPF FS with delegation options. So emitting a warning that
> "something something BPF token failed" would be disconcerting to most
> users.
> 
> What's the worst that would happen if BPF token was expected but we
> failed to instantiate it? You'll get a BPF object load failure with
> -EPERM, so it will be a pretty clear signal that whatever delegation
> was supposed to happen didn't happen.
> 
> Also, if a user wants a BPF token for sure, they can explicitly set
> bpf_token_path = "/sys/fs/bpf" and then it becomes mandatory.
> 
> So tl;dr, my perspective is that most users won't know or care about
> BPF tokens. If sysadmin set up BPF FS correctly, it should just work
> without the BPF application being aware. But for those rare cases
> where a BPF token is expected and necessary, explicit bpf_token_path
> or bpf_token_fd is the way to fail early, if something is not set up
> the way it is expected.

Works for me. I don't have a strong opinion either way.

Powered by blists - more mailing lists