Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220328175033.2437312-1-roberto.sassu@huawei.com>
 <20220328175033.2437312-6-roberto.sassu@huawei.com> <CAEf4BzY9d0pUP2TFkOY41dbjyYrsr5S+sNCpynPtg_9XZHFb-Q@mail.gmail.com>
 <4621def6171f4ca5948a59a7e714d25f@huawei.com>
In-Reply-To: <4621def6171f4ca5948a59a7e714d25f@huawei.com>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Sun, 3 Apr 2022 17:22:38 -0700
Message-ID: <CAEf4BzZB=aLA23xysFBm50AKZ72p0PBDeL5j1d0mmNCGW2VKwA@mail.gmail.com>
Subject: Re: [PATCH 05/18] bpf-preload: Generate static variables
To:     Roberto Sassu <roberto.sassu@huawei.com>
Cc:     Jonathan Corbet <corbet@lwn.net>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        KP Singh <kpsingh@kernel.org>, Shuah Khan <shuah@kernel.org>,
        "mcoquelin.stm32@gmail.com" <mcoquelin.stm32@gmail.com>,
        "alexandre.torgue@foss.st.com" <alexandre.torgue@foss.st.com>,
        Mimi Zohar <zohar@linux.ibm.com>,
        Linux Doc Mailing List <linux-doc@vger.kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        "linux-stm32@st-md-mailman.stormreply.com" 
        <linux-stm32@st-md-mailman.stormreply.com>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        "linux-integrity@vger.kernel.org" <linux-integrity@vger.kernel.org>,
        "linux-security-module@vger.kernel.org" 
        <linux-security-module@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Mar 30, 2022 at 12:44 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > From: Andrii Nakryiko [mailto:andrii.nakryiko@gmail.com]
> > Sent: Wednesday, March 30, 2022 1:52 AM
> > On Mon, Mar 28, 2022 at 10:52 AM Roberto Sassu
> > <roberto.sassu@huawei.com> wrote:
> > >
> > > The first part of the preload code generation consists in generating the
> > > static variables to be used by the code itself: the links and maps to be
> > > pinned, and the skeleton. Generation of the preload variables and
> > methods
> > > is enabled with the option -P added to 'bpftool gen skeleton'.
> > >
> > > The existing variables maps_link and progs_links in bpf_preload_kern.c
> > have
> > > been renamed respectively to dump_bpf_map_link and
> > dump_bpf_prog_link, to
> > > match the name of the variables in the main structure of the light
> > > skeleton.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >  kernel/bpf/preload/bpf_preload_kern.c         |  35 +-
> > >  kernel/bpf/preload/iterators/Makefile         |   2 +-
> > >  .../bpf/preload/iterators/iterators.lskel.h   | 378 +++++++++---------
> > >  .../bpf/bpftool/Documentation/bpftool-gen.rst |   5 +
> > >  tools/bpf/bpftool/bash-completion/bpftool     |   2 +-
> > >  tools/bpf/bpftool/gen.c                       |  27 ++
> > >  tools/bpf/bpftool/main.c                      |   7 +-
> > >  tools/bpf/bpftool/main.h                      |   1 +
> > >  8 files changed, 254 insertions(+), 203 deletions(-)
> > >
> >
> > [...]
> >
> > > +__attribute__((unused)) static void
> > > +iterators_bpf__assert(struct iterators_bpf *s)
> > > +{
> > > +#ifdef __cplusplus
> > > +#define _Static_assert static_assert
> > > +#endif
> > > +#ifdef __cplusplus
> > > +#undef _Static_assert
> > > +#endif
> > > +}
> > > +
> > > +static struct bpf_link *dump_bpf_map_link;
> > > +static struct bpf_link *dump_bpf_prog_link;
> > > +static struct iterators_bpf *skel;
> >
> > I don't understand what is this and what for? You are making an
> > assumption that light skeleton can be instantiated just once, why? And
> > adding extra bpftool option to light skeleton codegen just to save a
> > bit of typing at the place where light skeleton is actually
> > instantiated and used doesn't seems like a right approach.
>
> True, iterator_bpf is simple. Writing the preloading code
> for it is simple. But, what if you wanted to preload an LSM
> with 10 hooks or more?

I suppose you'd write a straightforward code to do pinning ten times
for ten different programs to ten different paths. But with this you
don't have to establish a random set of conventions that might not
apply in all the situations to anyone that would try to use this
feature.

Worst case, light skeleton can be extended to provide a way to iterate
all programs programmatically.

>
> Ok, regarding where the preloading code should be, I will
> try to move the generated code to the kernel module instead
> of the light skeleton.
>
> Thanks
>
> Roberto
>
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Zhong Ronghua
>
> > Further, even if this is the way to go, please split out bpftool
> > changes from kernel changes. There is nothing requiring them to be
> > coupled together.
> >
> > [...]