Re: [PATCH bpf-next] RFC: libbpf: resolve rodata lookups

[sdf@google.com]

On 07/19, Alexei Starovoitov wrote:
> On Tue, Jul 19, 2022 at 2:41 PM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > On Tue, Jul 19, 2022 at 1:33 PM Stanislav Fomichev <sdf@google.com>  
> wrote:
> > >
> > > On Tue, Jul 19, 2022 at 1:21 PM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Mon, Jul 18, 2022 at 12:07 PM Stanislav Fomichev  
> <sdf@google.com> wrote:
> > > > >
> > > > > Motivation:
> > > > >
> > > > > Our bpf programs have a bunch of options which are set at the  
> loading
> > > > > time. After loading, they don't change. We currently use array map
> > > > > to store them and bpf program does the following:
> > > > >
> > > > > val = bpf_map_lookup_elem(&config_map, &key);
> > > > > if (likely(val && *val)) {
> > > > >   // do some optional feature
> > > > > }
> > > > >
> > > > > Since the configuration is static and we have a lot of those  
> features,
> > > > > I feel like we're wasting precious cycles doing dynamic lookups
> > > > > (and stalling on memory loads).
> > > > >
> > > > > I was assuming that converting those to some fake kconfig options
> > > > > would solve it, but it still seems like kconfig is stored in the
> > > > > global map and kconfig entries are resolved dynamically.
> > > > >
> > > > > Proposal:
> > > > >
> > > > > Resolve kconfig options statically upon loading. Basically rewrite
> > > > > ld+ldx to two nops and 'mov val, x'.
> > > > >
> > > > > I'm also trying to rewrite conditional jump when the condition is
> > > > > !imm. This seems to be catching all the cases in my program, but
> > > > > it's probably too hacky.
> > > > >
> > > > > I've attached very raw RFC patch to demonstrate the idea. Anything
> > > > > I'm missing? Any potential problems with this approach?
> > > >
> > > > Have you considered using global variables for that?
> > > > With skeleton the user space has a natural way to set
> > > > all of these knobs after doing skel_open and before skel_load.
> > > > Then the verifier sees them as readonly vars and
> > > > automatically converts LDX into fixed constants and if the code
> > > > looks like if (my_config_var) then the verifier will remove
> > > > all the dead code too.
> > >
> > > Hm, that's a good alternative, let me try it out. Thanks!
> >
> > Turns out we already freeze kconfig map in libbpf:
> > if (map_type == LIBBPF_MAP_RODATA || map_type == LIBBPF_MAP_KCONFIG) {
> >         err = bpf_map_freeze(map->fd);
> >
> > And I've verified that I do hit bpf_map_direct_read in the verifier.
> >
> > But the code still stays the same (bpftool dump xlated):
> >   72: (18) r1 = map[id:24][0]+20
> >   74: (61) r1 = *(u32 *)(r1 +0)
> >   75: (bf) r2 = r9
> >   76: (b7) r0 = 0
> >   77: (15) if r1 == 0x0 goto pc+9
> >
> > I guess there is nothing for sanitize_dead_code to do because my
> > conditional is "if (likely(some_condition)) { do something }" and the
> > branch instruction itself is '.seen' by the verifier.

> I bet your variable is not 'const'.
> Please see any of the progs in selftests that do:
> const volatile int var = 123;
> to express configs.

Yeah, I was testing against the following:

	extern int CONFIG_XYZ __kconfig __weak;

But ended up writing this small reproducer:

	struct __sk_buff;

	const volatile int CONFIG_DROP = 1; // volatile so it's not
					    // clang-optimized

	__attribute__((section("tc"), used))
	int my_config(struct __sk_buff *skb)
	{
		int ret = 0; /*TC_ACT_OK*/

		if (CONFIG_DROP)
			ret = 2 /*TC_ACT_SHOT*/;

		return ret;
	}

$ bpftool map dump name my_confi.rodata

[{
         "value": {
             ".rodata": [{
                     "CONFIG_DROP": 1
                 }
             ]
         }
     }
]

$ bpftool prog dump xlated name my_config

int my_config(struct __sk_buff * skb):
; if (CONFIG_DROP)
    0: (18) r1 = map[id:3][0]+0
    2: (61) r1 = *(u32 *)(r1 +0)
    3: (b4) w0 = 1
; if (CONFIG_DROP)
    4: (64) w0 <<= 1
; return ret;
    5: (95) exit

The branch is gone, but the map lookup is still there :-(