On Mon, Jul 14, 2025 at 5:55 AM KP Singh <kpsingh@xxxxxxxxxx> wrote:
>
> On Mon, Jul 14, 2025 at 2:29 PM KP Singh <kpsingh@xxxxxxxxxx> wrote:
> >
> > On Fri, Jun 13, 2025 at 12:56 AM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> > >
> > > On Fri, Jun 6, 2025 at 4:29 PM KP Singh <kpsingh@xxxxxxxxxx> wrote:
> > > >
> > > > Implement a convenient method i.e. bpf_map__make_exclusive which
> > > > calculates the hash for the program and registers it with the map for
> > > > creation as an exclusive map when the objects are loaded.
> > > >
> > > > The hash of the program must be computed after all the relocations are
> > > > done.
> > > >
> > > > Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx>
> > > > ---
> > > > tools/lib/bpf/bpf.c | 4 +-
> > > > tools/lib/bpf/bpf.h | 4 +-
> > > > tools/lib/bpf/libbpf.c | 68 +++++++++++++++++++++++++++++++++-
> > > > tools/lib/bpf/libbpf.h | 13 +++++++
> > > > tools/lib/bpf/libbpf.map | 5 +++
> > > > tools/lib/bpf/libbpf_version.h | 2 +-
> > > > 6 files changed, 92 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > > > index a9c3e33d0f8a..11fa2d64ccca 100644
> > > > --- a/tools/lib/bpf/bpf.c
> > > > +++ b/tools/lib/bpf/bpf.c
> > > > @@ -172,7 +172,7 @@ int bpf_map_create(enum bpf_map_type map_type,
> > > > __u32 max_entries,
> > > > const struct bpf_map_create_opts *opts)
> > > > {
> > > > - const size_t attr_sz = offsetofend(union bpf_attr, map_token_fd);
> > > > + const size_t attr_sz = offsetofend(union bpf_attr, excl_prog_hash);
> > > > union bpf_attr attr;
> > > > int fd;
> > > >
> > > > @@ -203,6 +203,8 @@ int bpf_map_create(enum bpf_map_type map_type,
> > > > attr.map_ifindex = OPTS_GET(opts, map_ifindex, 0);
> > > >
> > > > attr.map_token_fd = OPTS_GET(opts, token_fd, 0);
> > > > + attr.excl_prog_hash = ptr_to_u64(OPTS_GET(opts, excl_prog_hash, NULL));
> > > > + attr.excl_prog_hash_size = OPTS_GET(opts, excl_prog_hash_size, 0);
> > > >
> > > > fd = sys_bpf_fd(BPF_MAP_CREATE, &attr, attr_sz);
> > > > return libbpf_err_errno(fd);
> > > > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > > > index 777627d33d25..a82b79c0c349 100644
> > > > --- a/tools/lib/bpf/bpf.h
> > > > +++ b/tools/lib/bpf/bpf.h
> > > > @@ -54,9 +54,11 @@ struct bpf_map_create_opts {
> > > > __s32 value_type_btf_obj_fd;
> > > >
> > > > __u32 token_fd;
> > > > + __u32 excl_prog_hash_size;
> > > > + const void *excl_prog_hash;
> > > > size_t :0;
> > > > };
> > > > -#define bpf_map_create_opts__last_field token_fd
> > > > +#define bpf_map_create_opts__last_field excl_prog_hash
> > > >
> > > > LIBBPF_API int bpf_map_create(enum bpf_map_type map_type,
> > > > const char *map_name,
> > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > > index 475038d04cb4..17de756973f4 100644
> > > > --- a/tools/lib/bpf/libbpf.c
> > > > +++ b/tools/lib/bpf/libbpf.c
> > > > @@ -499,6 +499,7 @@ struct bpf_program {
> > > > __u32 line_info_rec_size;
> > > > __u32 line_info_cnt;
> > > > __u32 prog_flags;
> > > > + __u8 hash[SHA256_DIGEST_LENGTH];
> > > > };
> > > >
> > > > struct bpf_struct_ops {
> > > > @@ -578,6 +579,8 @@ struct bpf_map {
> > > > bool autocreate;
> > > > bool autoattach;
> > > > __u64 map_extra;
> > > > + const void *excl_prog_sha;
> > > > + __u32 excl_prog_sha_size;
> > > > };
> > > >
> > > > enum extern_type {
> > > > @@ -4485,6 +4488,43 @@ bpf_object__section_to_libbpf_map_type(const struct bpf_object *obj, int shndx)
> > > > }
> > > > }
> > > >
> > > > +static int bpf_program__compute_hash(struct bpf_program *prog)
> > > > +{
> > > > + struct bpf_insn *purged;
> > > > + bool was_ld_map;
> > > > + int i, err;
> > > > +
> > > > + purged = calloc(1, BPF_INSN_SZ * prog->insns_cnt);
> > > > + if (!purged)
> > > > + return -ENOMEM;
> > > > +
> > > > + /* If relocations have been done, the map_fd needs to be
> > > > + * discarded for the digest calculation.
> > > > + */
> > >
> > > all this looks sketchy, let's think about some more robust approach
> > > here rather than randomly clearing some fields of some instructions...
> > >
> > > > + for (i = 0, was_ld_map = false; i < prog->insns_cnt; i++) {
> > > > + purged[i] = prog->insns[i];
> > > > + if (!was_ld_map &&
> > > > + purged[i].code == (BPF_LD | BPF_IMM | BPF_DW) &&
> > > > + (purged[i].src_reg == BPF_PSEUDO_MAP_FD ||
> > > > + purged[i].src_reg == BPF_PSEUDO_MAP_VALUE)) {
> > > > + was_ld_map = true;
> > > > + purged[i].imm = 0;
> > > > + } else if (was_ld_map && purged[i].code == 0 &&
> > > > + purged[i].dst_reg == 0 && purged[i].src_reg == 0 &&
> > > > + purged[i].off == 0) {
> > > > + was_ld_map = false;
> > > > + purged[i].imm = 0;
> > > > + } else {
> > > > + was_ld_map = false;
> > > > + }
> > > > + }
> > >
> > > this was_ld_map business is... unnecessary? Just access purged[i + 1]
> > > (checking i + 1 < prog->insns_cnt, of course), and i += 1. This
> > > stateful approach is an unnecessary complication, IMO
> >
> > Does this look better to you, the next instruction has to be the
> > second half of the double word right?
> >
> > for (int i = 0; i < prog->insns_cnt; i++) {
> > purged[i] = prog->insns[i];
> > if (purged[i].code == (BPF_LD | BPF_IMM | BPF_DW) &&
> > (purged[i].src_reg == BPF_PSEUDO_MAP_FD ||
> > purged[i].src_reg == BPF_PSEUDO_MAP_VALUE)) {
> > purged[i].imm = 0;
> > i++;
> > if (i >= prog->insns_cnt ||
> > prog->insns[i].code != 0 ||
> > prog->insns[i].dst_reg != 0 ||
> > prog->insns[i].src_reg != 0 ||
> > prog->insns[i].off != 0) {
> > return -EINVAL;
> > }
>
> I mean ofcourse
>
> err = -EINVAL;
> goto out;
>
> to free the buffer.
Yes, but I'd probably modify it a bit for conciseness:
struct bpf_insn *purged, *insn;
int i;
purged = calloc(..);
memcpy(purged, prog->insns, ...);
for (i = 0; i < prog->insns_cnt; i++) {
insn = &purged[i];
if (insn[0].code == (BPF_LD | BPF_IMM | BPF_DW) &&
(insn[0].src_reg == BPF_PSEUDO_MAP_FD || ...) {
insn[0].imm = 0;
if (i >= prog_insns_cnt) {
err = -EINVAL;
goto err;
}
insn[1].imm = 0;
i++;
}
(I'm not sure libbpf needs to check code,dst_reg,src_reg,off for
ldimm64, verifier will do it anyways, so I'd protect against
out-of-bounds access only)
I'd even consider just doing:
if (i + 1 < prog->insns_cnt && insn[0].code == (BPF_LD | BPF_IMM |
BPF_DW) ...) {
insn[0].imm = 0;
insn[1].imm = 0;
i++;
}
i.e., don't even error out, verifier will do that anyway later because
program is malformed, and hash won't even matter at that point
[...]
