elf_reader: permit multiple .data* sections

[mejedi]

Mirror libbpf behavior. Quoting the respective commit message[1]:

Having the ability to use multiple .data sections and putting
dedicated global variables into separate .data.custom section also
opens up some new and interesting possibilities (like dynamically
sizing global arrays, without using BPF_MAP_TYPE_ARRAY map).

[1] libbpf/libbpf#274

Another motivation for dedicated data sections is a more granular access control, such as making a subset of variables BPF_F_RDONLY_PROG (userspace can alter but BPF can't). Also note that verifier doesn't prevent overflowing .data buffers into adjacent variables as long as the write is within the backing array map bounds.

[ti-mo]

@mejedi Thanks, this has been in the back of my mind for years, but no one asked. Did you make this in response to #1545? If so, it would be useful to call out when one PR supersedes another in the future.

[ti-mo]

Thanks for adding tests! Could we make the patch shorter, though? There's no real need to gate this on 5.2 as far as I know, and we have a similar test for e.g. .rodata.test in loader.c.

Not sure if using LoadAndAssign and using prog.Test() add much value, either. If a .data.test map is present in the CollectionSpec, we're good.

I'd like to limit the amount of objects we have in testdata/, as each additional object slows down make a bit. I think many of them could be written in a single .c file, or at least fewer than we have today.

Thanks again!

[mejedi]

That's the downside of overloading loader.c, I guess. To be honest, I do not understand where 26 is coming from, but it (kind of) makes sense that the value grew by 2 after two lines were added. @ti-mo

[ti-mo]

Good question, that's quite straightforward to investigate!

		if src := ins.Source(); src != nil {
			lineInfoCount++
+			fmt.Println(src)
+			fmt.Println(ins)
		}

Results in:

...
LoadMapValue dst: r1, fd: 40 off: 0
	return static_fn(arg) + global_fn(arg) + arg2;
AddReg dst: r0 src: r1
	return static_fn(arg) + global_fn(arg) + arg2;
Exit

int __attribute__((noinline)) global_fn(uint32_t arg) {
...
	return static_fn(arg) + global_fn2(arg) + global_fn3(arg);
AddReg dst: r8 src: r0
	return static_fn(arg) + global_fn2(arg) + global_fn3(arg);
MovReg dst: r0 src: r8

One instruction for the variable access and one for accumulating into r0. (this is on main) Adding another variable to the return statement would result in 2 extra insns/lineinfo.

[mejedi]

@ti-mo Thank you for having a look. I reduced the amount of changes by piggy-backing on the loader.c and existing test in elf_reader_test.

I suspect that all data sections share the same machinery for resolving relocations. Therefore a test ensuring that a program referencing variables in .data.custom actually works as expected is not strictly necessary.

[ti-mo]

Thanks!

[ti-mo]

Pushed a small fixup to add the volatile qualifier to arg3 for consistency with the others.

[mejedi]

Pushed a small fixup to add the volatile qualifier to arg3 for consistency with the others.

Up to you but volatile is used with const to prevent compiler from eliding load, otherwise https://github.com/torvalds/linux/blob/master/Documentation/process/volatile-considered-harmful.rst

[ti-mo]

Pushed a small fixup to add the volatile qualifier to arg3 for consistency with the others.

Up to you but volatile is used with const to prevent compiler from eliding load, otherwise https://github.com/torvalds/linux/blob/master/Documentation/process/volatile-considered-harmful.rst

It's not only useful for const, in my experience it makes compiler output around global vars more predictable since the compiler can't make any assumptions about the contents of the variable. I know on this particular var it doesn't really matter since it's never written from user space, but historically this has been a pain point.

The readme you linked applies to kernel code proper and mainly cites concurrency concerns, which doesn't apply to bpf as much. There are some mailing list threads about the use of static and volatile (bpf skeleton no longer exposes statics, for example), but usually it's hard to figure out the conclusion(s). If we should really avoid certain qualifiers, we should make a change across the code base so things remain consistent, and also document specifically why they should be avoided.