netdev - Re: [PATCH 1/4] bpf: Add pin_name into struct bpf_prog

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: <CALOAHbAeuE+zc4z4P4t=boDk25_CZ9fgMosVn5eDt6uVYofyfw@mail.gmail.com>
Date:   Sun, 13 Feb 2022 19:27:01 +0800
From:   Yafang Shao <laoar.shao@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>, Martin Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        john fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>, netdev <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH 1/4] bpf: Add pin_name into struct bpf_prog_aux

On Fri, Feb 11, 2022 at 8:43 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 2/11/22 1:11 PM, Yafang Shao wrote:
> > A new member pin_name is added into struct bpf_prog_aux, which will be
> > set when the prog is set and cleared when the pinned file is removed.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@...il.com>
> > ---
> >   include/linux/bpf.h      |  2 ++
> >   include/uapi/linux/bpf.h |  1 +
> >   kernel/bpf/inode.c       | 20 +++++++++++++++++++-
> >   3 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 0ceb25b..9cf8055 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -933,6 +933,8 @@ struct bpf_prog_aux {
> >               struct work_struct work;
> >               struct rcu_head rcu;
> >       };
> > +
> > +     char pin_name[BPF_PIN_NAME_LEN];
> >   };
>
> I'm afraid this is not possible. You are assuming a 1:1 relationship between prog
> and pin location, but it's really a 1:n (prog can be pinned in multiple locations
> and also across multiple mount instances).

Thanks for the explanation!
I didn't notice the 1:n relationship before. I will read the code more
to try to find if there's a good way to implement it.

> Also, you can create hard links of pins
> which are not handled via bpf_obj_do_pin().
>

Yes, it seems we should introduce bpf_{link, unlink, rename} first
instead of the simple_* one.

> >   struct bpf_array_aux {
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index c14fed8..bada5cc 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -1217,6 +1217,7 @@ struct bpf_stack_build_id {
> >   };
> >
> >   #define BPF_OBJ_NAME_LEN 16U
> > +#define BPF_PIN_NAME_LEN 64U
> >
> >   union bpf_attr {
> >       struct { /* anonymous struct used by BPF_MAP_CREATE command */
> > diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
> > index 4477ca8..f1a8811 100644
> > --- a/kernel/bpf/inode.c
> > +++ b/kernel/bpf/inode.c
> > @@ -437,6 +437,8 @@ static int bpf_iter_link_pin_kernel(struct dentry *parent,
> >   static int bpf_obj_do_pin(const char __user *pathname, void *raw,
> >                         enum bpf_type type)
> >   {
> > +     struct bpf_prog_aux *aux;
> > +     struct bpf_prog *prog;
> >       struct dentry *dentry;
> >       struct inode *dir;
> >       struct path path;
> > @@ -461,6 +463,10 @@ static int bpf_obj_do_pin(const char __user *pathname, void *raw,
> >
> >       switch (type) {
> >       case BPF_TYPE_PROG:
> > +             prog = raw;
> > +             aux = prog->aux;
> > +             (void) strncpy_from_user(aux->pin_name, pathname, BPF_PIN_NAME_LEN);
> > +             aux->pin_name[BPF_PIN_NAME_LEN - 1] = '\0';
> >               ret = vfs_mkobj(dentry, mode, bpf_mkprog, raw);
> >               break;
> >       case BPF_TYPE_MAP:
> > @@ -611,12 +617,24 @@ static int bpf_show_options(struct seq_file *m, struct dentry *root)
> >
> >   static void bpf_free_inode(struct inode *inode)
> >   {
> > +     struct bpf_prog_aux *aux;
> > +     struct bpf_prog *prog;
> >       enum bpf_type type;
> >
> >       if (S_ISLNK(inode->i_mode))
> >               kfree(inode->i_link);
> > -     if (!bpf_inode_type(inode, &type))
> > +     if (!bpf_inode_type(inode, &type)) {
> > +             switch (type) {
> > +             case BPF_TYPE_PROG:
> > +                     prog = inode->i_private;
> > +                     aux = prog->aux;
> > +                     aux->pin_name[0] = '\0';
> > +                     break;
> > +             default:
> > +                     break;
> > +             }
> >               bpf_any_put(inode->i_private, type);
> > +     }
> >       free_inode_nonrcu(inode);
> >   }
> >
> >
>


-- 
Thanks
Yafang