On Tue, Jul 22, 2025 at 5:51 PM Quentin Monnet <qmo@xxxxxxxxxx> wrote:
>
> 2025-07-21 23:19 UTC+0200 ~ KP Singh <kpsingh@xxxxxxxxxx>
> > Two modes of operation being added:
> >
> > Add two modes of operation:
> >
> > * For prog load, allow signing a program immediately before loading. This
> > is essential for command-line testing and administration.
> >
> > bpftool prog load -S -k <private_key> -i <identity_cert> fentry_test.bpf.o
> >
> > * For gen skeleton, embed a pre-generated signature into the C skeleton
> > file. This supports the use of signed programs in compiled applications.
> >
> > bpftool gen skeleton -S -k <private_key> -i <identity_cert> fentry_test.bpf.o
> >
> > Generation of the loader program and its metadata map is implemented in
> > libbpf (bpf_obj__gen_loader). bpftool generates a skeleton that loads
> > the program and automates the required steps: freezing the map, creating
> > an exclusive map, loading, and running. Users can use standard libbpf
> > APIs directly or integrate loader program generation into their own
> > toolchains.
>
>
> Thanks KP! Some bpftool-related comments below. Looks good overall, I
> mostly have minor comments.
>
> One concern might be the license for the new file, GPL-2.0 in your
> patch, whereas bpftool is dual-licensed. I hope this is simply an oversight?
An oversight, fixed.
>
>
> >
> > Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx>
> > ---
> > .../bpf/bpftool/Documentation/bpftool-gen.rst | 12 +
> > .../bpftool/Documentation/bpftool-prog.rst | 12 +
> > tools/bpf/bpftool/Makefile | 6 +-
> > tools/bpf/bpftool/cgroup.c | 5 +-
> > tools/bpf/bpftool/gen.c | 58 ++++-
> > tools/bpf/bpftool/main.c | 21 +-
> > tools/bpf/bpftool/main.h | 11 +
> > tools/bpf/bpftool/prog.c | 25 +++
> > tools/bpf/bpftool/sign.c | 210 ++++++++++++++++++
> > 9 files changed, 352 insertions(+), 8 deletions(-)
> > create mode 100644 tools/bpf/bpftool/sign.c
> >
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-gen.rst b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
> > index ca860fd97d8d..2997313003b1 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-gen.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
> > @@ -185,6 +185,18 @@ OPTIONS
> > For skeletons, generate a "light" skeleton (also known as "loader"
> > skeleton). A light skeleton contains a loader eBPF program. It does not use
> > the majority of the libbpf infrastructure, and does not need libelf.
>
>
> Blank line separator, please
done
>
>
> > +-S, --sign
> > + For skeletons, generate a signed skeleton. This option must be used with
> > + **-k** and **-i**. Using this flag implicitly enables **--use-loader**.
> > + See the "Signed Skeletons" section in the description of the
> > + **gen skeleton** command for more details.
> > +
> > +-k <private_key.pem>
> > + Path to the private key file in PEM format, required for signing.
> > +
> > +-i <certificate.x509>
> > + Path to the X.509 certificate file in PEM or DER format, required for
> > + signing.
>
>
> Please also update the options list in the SYNOPSIS section at the top
> of the page; and the option list at the bottom of gen.c (just like for
> "--use-loader").
done also, isn't this the right formatting for the SYNOPSIS given that
some of these are optional?
**bpftool** [*OPTIONS*] **prog** *COMMAND*
*OPTIONS* := { |COMMON_OPTIONS| [ { **-f** | **--bpffs** } ] [ {
**-m** | **--mapcompat** } ]
[ { **-n** | **--nomount** } ] [ { **-L** | **--use-loader** } ]
[ { { **-S** | **--sign** } **-k** <private_key.pem> **-i**
<certificate.x509> } ] }
not an expert here but I vaguely remember.
Also do you think we need to:
{ "use-loader", no_argument, NULL, 'L' },
- { "sign", required_argument, NULL, 'S'},
+ { "sign", no_argument, NULL, 'S' },
Now that we don't use an argument blob for --sign?
>
> Can you also please take a look at the bash completion update? It
> shouldn't be too hard if you look at how it deals with other options, in
> particular --base-btf that also takes one argument - and I can help if
> necessary.
I will give it a go.
>
>
> >
> > EXAMPLES
> > ========
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > index f69fd92df8d8..dc2ca196137e 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > @@ -248,6 +248,18 @@ OPTIONS
> > creating the maps, and loading the programs (see **bpftool prog tracelog**
> > as a way to dump those messages).
> >
> > +-S, --sign
> > + Enable signing of the BPF program before loading. This option must be
> > + used with **-k** and **-i**. Using this flag implicitly enables
> > + **--use-loader**.
> > +
> > +-k <private_key.pem>
> > + Path to the private key file in PEM format, required when signing.
> > +
> > +-i <certificate.x509>
> > + Path to the X.509 certificate file in PEM or DER format, required when
> > + signing.
>
>
> Same as for skeletons: please update the list of options in the synopsis
> and at the bottom of prog.c (bash completion for skeletons' options
> should also cover this case, so no additional work required here).
>
>
> > +
> > EXAMPLES
> > ========
> > **# bpftool prog show**
> > diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> > index 9e9a5f006cd2..586d1b2595d1 100644
> > --- a/tools/bpf/bpftool/Makefile
> > +++ b/tools/bpf/bpftool/Makefile
> > @@ -130,8 +130,8 @@ include $(FEATURES_DUMP)
> > endif
> > endif
> >
> > -LIBS = $(LIBBPF) -lelf -lz
> > -LIBS_BOOTSTRAP = $(LIBBPF_BOOTSTRAP) -lelf -lz
> > +LIBS = $(LIBBPF) -lelf -lz -lcrypto
> > +LIBS_BOOTSTRAP = $(LIBBPF_BOOTSTRAP) -lelf -lz -lcrypto
> >
> > ifeq ($(feature-libelf-zstd),1)
> > LIBS += -lzstd
> > @@ -194,7 +194,7 @@ endif
> >
> > BPFTOOL_BOOTSTRAP := $(BOOTSTRAP_OUTPUT)bpftool
> >
> > -BOOTSTRAP_OBJS = $(addprefix $(BOOTSTRAP_OUTPUT),main.o common.o json_writer.o gen.o btf.o)
> > +BOOTSTRAP_OBJS = $(addprefix $(BOOTSTRAP_OUTPUT),main.o common.o json_writer.o gen.o btf.o sign.o)
> > $(BOOTSTRAP_OBJS): $(LIBBPF_BOOTSTRAP)
> >
> > OBJS = $(patsubst %.c,$(OUTPUT)%.o,$(SRCS)) $(OUTPUT)disasm.o
> > diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
> > index 944ebe21a216..90c9aa297806 100644
> > --- a/tools/bpf/bpftool/cgroup.c
> > +++ b/tools/bpf/bpftool/cgroup.c
> > @@ -1,7 +1,10 @@
> > // SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > // Copyright (C) 2017 Facebook
> > // Author: Roman Gushchin <guro@xxxxxx>
> > -
>
>
> Let's keep the blank line
Done
>
>
> > +#undef GCC_VERSION
> > +#ifndef _GNU_SOURCE
> > +#define _GNU_SOURCE
> > +#endif
>
>
> What are these for?
kpsingh@kpsingh-genoa:~/projects/linux/tools/bpf/bpftool$ vmk
Auto-detecting system features:
... clang-bpf-co-re: [ on ]
... llvm: [ on ]
... libcap: [ on ]
... libbfd: [ OFF ]
In file included from cgroup.c:19:
In file included from ./main.h:16:
/home/kpsingh/projects/linux/tools/bpf/bpftool/libbpf/include/bpf/skel_internal.h:87:9:
error: call to undeclared function 'syscall'; ISO C99 and later do not
support implicit function declarations
[-Wimplicit-function-declaration]
87 | return syscall(__NR_bpf, cmd, attr, size);
| ^
1 error generated.
>
>
> > #define _XOPEN_SOURCE 500
> > #include <errno.h>
> > #include <fcntl.h>
>
> [...]
>
> > diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> > index 2b7f2bd3a7db..fc25bb390ec7 100644
> > --- a/tools/bpf/bpftool/main.c
> > +++ b/tools/bpf/bpftool/main.c
> > @@ -33,6 +33,9 @@ bool relaxed_maps;
> > bool use_loader;
> > struct btf *base_btf;
> > struct hashmap *refs_table;
> > +bool sign_progs;
> > +const char *private_key_path;
> > +const char *cert_path;
> >
> > static void __noreturn clean_and_exit(int i)
> > {
> > @@ -447,6 +450,7 @@ int main(int argc, char **argv)
> > { "nomount", no_argument, NULL, 'n' },
> > { "debug", no_argument, NULL, 'd' },
> > { "use-loader", no_argument, NULL, 'L' },
> > + { "sign", required_argument, NULL, 'S'},
> > { "base-btf", required_argument, NULL, 'B' },
> > { 0 }
> > };
> > @@ -473,7 +477,7 @@ int main(int argc, char **argv)
> > bin_name = "bpftool";
> >
> > opterr = 0;
> > - while ((opt = getopt_long(argc, argv, "VhpjfLmndB:l",
> > + while ((opt = getopt_long(argc, argv, "VhpjfLmndSi:k:B:l",
> > options, NULL)) >= 0) {
> > switch (opt) {
> > case 'V':
> > @@ -519,6 +523,16 @@ int main(int argc, char **argv)
> > case 'L':
> > use_loader = true;
> > break;
> > + case 'S':
> > + sign_progs = true;
> > + use_loader = true;
> > + break;
> > + case 'k':
> > + private_key_path = optarg;
> > + break;
> > + case 'i':
> > + cert_path = optarg;
> > + break;
> > default:
> > p_err("unrecognized option '%s'", argv[optind - 1]);
> > if (json_output)
> > @@ -533,6 +547,11 @@ int main(int argc, char **argv)
> > if (argc < 0)
> > usage();
> >
> > + if (sign_progs && (private_key_path == NULL || cert_path == NULL)) {
> > + p_err("-i <identity_x509_cert> and -k <private> key must be supplied with -S for signing");
> > + return -EINVAL;
> > + }
>
>
> What if -i and/or -k are passed without -S?
We can either print a warning or error out
A) User does not want to sign removes --sign and forgets to remove -i
-k (better with warning)
B) User wants to sign but forgets to --sign (better with error)
I'd say we print an error so that we don't accidentally not sign, WDYT?
The reason why I think we should keep an explicit --sign is because we
can also extend this to have e.g. --verify.
- KP
>
>
> > +
> > if (version_requested)
> > ret = do_version(argc, argv);
> > else
> > diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> > index 6db704fda5c0..f921af3cda87 100644
> > --- a/tools/bpf/bpftool/main.h
> > +++ b/tools/bpf/bpftool/main.h
> > @@ -6,9 +6,14 @@
> >
> > /* BFD and kernel.h both define GCC_VERSION, differently */
> > #undef GCC_VERSION
> > +#ifndef _GNU_SOURCE
> > +#define _GNU_SOURCE
> > +#endif
> > #include <stdbool.h>
> > #include <stdio.h>
> > +#include <errno.h>
> > #include <stdlib.h>
> > +#include <bpf/skel_internal.h>
>
>
> Wnat do you need these includes (and _GNU_SOURCE) in main.h for?
Explained above, let me know if you have better ideas on where to place these.
>
>
> > #include <linux/bpf.h>
> > #include <linux/compiler.h>
> > #include <linux/kernel.h>
>
> [...]
>
> > diff --git a/tools/bpf/bpftool/sign.c b/tools/bpf/bpftool/sign.c
> > new file mode 100644
> > index 000000000000..f0b5dd10a46b
> > --- /dev/null
> > +++ b/tools/bpf/bpftool/sign.c
> > @@ -0,0 +1,210 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
>
> Please consider making this file dual-licensed like the rest of
> bpftool's source code, "(GPL-2.0-only OR BSD-2-Clause)".
Done.
>
>
> > +
> > +/*
> > + * Copyright (C) 2022 Google LLC.
>
>
> 2025?
Let's keep it 2022, nah just kidding :) Thanks.
>
>
> > + */
> > +#define _GNU_SOURCE
>
>
> Please guard this:
>
> #ifndef _GNU_SOURCE
> #define _GNU_SOURCE
> #endif
>
> This is because "llvm-config --cflags" passes -D_GNU_SOURCE and we may
> end up with a duplicate definition, otherwise.
ack, done.
>
>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <stdint.h>
> > +#include <stdbool.h>
> > +#include <string.h>
> > +#include <string.h>
> > +#include <getopt.h>
> > +#include <err.h>
> > +#include <openssl/opensslv.h>
> > +#include <openssl/bio.h>
> > +#include <openssl/evp.h>
> > +#include <openssl/pem.h>
> > +#include <openssl/err.h>
> > +#include <openssl/cms.h>
> > +#include <linux/keyctl.h>
> > +#include <errno.h>
> > +
> > +#include <bpf/skel_internal.h>
> > +
> > +#include "main.h"
> > +
> > +#define OPEN_SSL_ERR_BUF_LEN 256
> > +
> > +static void display_openssl_errors(int l)
> > +{
> > + char buf[OPEN_SSL_ERR_BUF_LEN];
> > + const char *file;
> > + const char *data;
> > + unsigned long e;
> > + int flags;
> > + int line;
> > +
> > + while ((e = ERR_get_error_all(&file, &line, NULL, &data, &flags))) {
> > + ERR_error_string_n(e, buf, sizeof(buf));
> > + if (data && (flags & ERR_TXT_STRING)) {
> > + p_err("OpenSSL %s: %s:%d: %s\n", buf, file, line, data);
>
>
> Please remove the trailing '\n', p_err() handles it already.
>
>
> > + } else {
> > + p_err("OpenSSL %s: %s:%d\n", buf, file, line);
>
>
> Same here.
done.
- KP
>
> [...]
