Re: [PATCH v5 bpf-next 03/13] bpf: introduce BPF token object

[Christian Brauner <brauner@kernel.org>]

> > > +#define BPF_TOKEN_INODE_NAME "bpf-token"
> > > +
> > > +/* Alloc anon_inode and FD for prepared token.
> > > + * Returns fd >= 0 on success; negative error, otherwise.
> > > + */
> > > +int bpf_token_new_fd(struct bpf_token *token)
> > > +{
> > > +     return anon_inode_getfd(BPF_TOKEN_INODE_NAME, &bpf_token_fops, token, O_CLOEXEC);
> >
> > It's unnecessary to use the anonymous inode infrastructure for bpf
> > tokens. It adds even more moving parts and makes reasoning about it even
> > harder. Just keep it all in bpffs. IIRC, something like the following
> > (broken, non-compiling draft) should work:
> >
> > /* bpf_token_file - get an unlinked file living in bpffs */
> > struct file *bpf_token_file(...)
> > {
> >         inode = bpf_get_inode(bpffs_mnt->mnt_sb, dir, mode);
> >         inode->i_op = &bpf_token_iop;
> >         inode->i_fop = &bpf_token_fops;
> >
> >         // some other stuff you might want or need
> >
> >         res = alloc_file_pseudo(inode, bpffs_mnt, "bpf-token", O_RDWR, &bpf_token_fops);
> > }
> >
> > Now set your private data that you might need, reserve an fd, install
> > the file into the fdtable and return the fd. You should have an unlinked
> > bpffs file that serves as your bpf token.
> 
> Just to make sure I understand. You are saying that instead of having
> `struct bpf_token *` and passing that into internal APIs
> (bpf_token_capable() and bpf_token_allow_xxx()), I should just pass
> around `struct super_block *` representing BPF FS instance? Or `struct
> bpf_mount_opts *` maybe? Or 'struct vfsmount *'? (Any preferences
> here?). Is that right?

No, that's not what I meant.

So, what you're doing right now to create a bpf token file descriptor is:

return anon_inode_getfd(BPF_TOKEN_INODE_NAME, &bpf_token_fops, token, O_CLOEXEC);

which is using the anonymous inode infrastructure. That is an entirely
different filesystems (glossing over details) that is best leveraged for
stuff like kvm fds and other stuff that doesn't need or have its own
filesytem implementation.

But you do have your own filesystem implementation so why abuse another
one to create bpf token fds when they can just be created directly from
the bpffs instance.

IOW, everything stays the same apart from the fact that bpf token fds
are actually file descriptors referring to a detached bpffs file instead
of an anonymous inode file. IOW, bpf tokens are actual bpffs objects
tied to a bpffs instance.

**BROKEN BROKEN BROKEN AND UGLY**

int bpf_token_create(union bpf_attr *attr)
{
        struct inode *inode;
        struct path path;
        struct bpf_mount_opts *mnt_opts;
        struct bpf_token *token;
        struct fd fd;
        int fd, ret;
        struct file *file;

        fd = fdget(attr->token_create.bpffs_path_fd);
        if (!fd.file)
                goto cleanup;

        if (fd.file->f_path->dentry != fd.file->f_path->dentry->d_sb->s_root)
                goto cleanup;

        inode = bpf_get_inode(fd.file->f_path->mnt->mnt_sb, NULL, 1234123412341234);
        if (!inode)
                goto cleanup;

        fd = get_unused_fd_flags(O_RDWR | O_CLOEXEC);
        if (fd < 0)
                goto cleanup;

        clear_nlink(inode); /* make sure it is unlinked */

        file = alloc_file_pseudo(inode, fd.file->f_path->mnt, "bpf-token", O_RDWR, &&bpf_token_fops);
        if (IS_ERR(file))
                goto cleanup;

        token = bpf_token_alloc();
        if (!token)
                goto cleanup;

        /* remember bpffs owning userns for future ns_capable() checks */
        token->userns = get_user_ns(path.dentry->d_sb->s_user_ns);

        mnt_opts = path.dentry->d_sb->s_fs_info;
        token->allowed_cmds = mnt_opts->delegate_cmds;
        token->allowed_maps = mnt_opts->delegate_maps;
        token->allowed_progs = mnt_opts->delegate_progs;
        token->allowed_attachs = mnt_opts->delegate_attachs;

        file->private_data = token;
        fd_install(fd, file);
        return fd;

cleanup:
        // cleanup stuff here
        return -SOME_ERROR;
}