elf: use per-instruction metadata for .kconfig references #994
Conversation
lmb
force-pushed
the
kconfig-metadata
branch
2 times, most recently
from
b97c239
to
ed6b67b
Compare
lmb
changed the title
| return nil, fmt.Errorf("instruction %d: reference to multiple .kconfig maps is not allowed", iter.Index) | ||
| } | ||
|
|
||
| if err := iter.Ins.AssociateMap(kconfig); err != nil { |
There was a problem hiding this comment.
Only half joking: we could implement this without going via a map in the first place and just patch instructions directly. Something for the future maybe.
There was a problem hiding this comment.
I am not sure to understand what you mean here.
Do you mean we can compute a "whole offset" instead of using the offset inside the map?
There was a problem hiding this comment.
Instead of putting LINUX_VERSION_CODE into the map at an offset, and referencing the map from this instruction, we could rewrite the instruction to be a LdImm64 of LINUX_VERSION_CODE. This avoids the indirection via the map and works on older kernels. It has subtleties though when a single value is accessed multiple times, etc.
There was a problem hiding this comment.
Oh definitely! This seems a good idea!
Nonetheless, maybe we should not do it in case we add more generic __kconfig value?
Indeed, it will add an exception for LINUX_KERNEL_VERSION as others will need to be map load?
There was a problem hiding this comment.
I'm not going to do it now, since I don't have a use case yet. It might happen that we need something like it in cilium though. Then we can probably extend this to anything that fits in 64 bit.
There was a problem hiding this comment.
It has subtleties though when a single value is accessed multiple times, etc.
Indeed, but it's also a subtlety that cannot realistically be worked around by the library. It would require userspace to track all accesses to the register the map pointer was loaded into, accounting for branches and jumps, as well as the possibility of aliasing/moving, etc. This is best left to the verifier. 🙂
|
cc @eiffel-fl |
lmb
force-pushed
the
kconfig-metadata
branch
2 times, most recently
from
84db3ff
to
615381b
Compare
| @@ -1134,7 +1132,7 @@ func (ec *elfCode) loadKconfigSection() error { | |||
| } | |||
|
|
|||
| var ds *btf.Datasec | |||
| err := ec.btf.TypeByName(kconfigMap, &ds) | |||
| err := ec.btf.TypeByName(".kconfig", &ds) | |||
There was a problem hiding this comment.
Should you keep this const and use it here:
const kconfigMap = ".kconfig"
?
There was a problem hiding this comment.
It's only used in three places now: here in the elf reader and when fixing up datasec in btf. The one use for MapSpec.Name is purely informational, so if we get it wrong it doesn't matter much.
In general I'm not a big fan of turning string literals into a constant, since it adds a layer of indirection. When reading code it's not clear that it's a constant (could be a var) and also what it's value is. If a string literal is repeated over an over as a key I try to reduce the number of lookups instead of introducing a constant.
| return nil, fmt.Errorf("instruction %d: reference to multiple .kconfig maps is not allowed", iter.Index) | ||
| } | ||
|
|
||
| if err := iter.Ins.AssociateMap(kconfig); err != nil { |
There was a problem hiding this comment.
I am not sure to understand what you mean here.
Do you mean we can compute a "whole offset" instead of using the offset inside the map?
Here you go: e3e0e9e |
.kconfig support is currently implemented in the ELF loader, which does a fix up of the relevant instructions to point at a ".kconfig" map. This map is available in CollectionSpec.Maps. This is basically how constants (aka references to .rodata) work today. Now that we have per-instruction metadata this isn't necessary anymore. Instead of using the generic WithReference, introduce a new kconfig metadata and tag instructions with that. Right now the metadata just contains the MapSpec for .kconfig as generated by the compiler. In the future we can create the MapSpec on the fly, which would allow referencing kconfig via ins.WithKconfig or similar. Prevent ".kconfig" from being accessible via CollectionSpec.Maps. This preserves our ability to change how kconfig references work without having to retain the original MapSpec. Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
.kconfig support is currently implemented in the ELF loader, which does
a fix up of the relevant instructions to point at a ".kconfig" map. This
map is available in CollectionSpec.Maps. This is basically how constants
(aka references to .rodata) work today. Now that we have per-instruction
metadata this isn't necessary anymore.
Instead of using the generic WithReference, introduce a new kconfig
metadata and tag instructions with that. Right now the metadata just
contains the MapSpec for .kconfig as generated by the compiler. In
the future we can create the MapSpec on the fly, which would allow
referencing kconfig via ins.WithKconfig or similar.
Prevent ".kconfig" from being accessible via CollectionSpec.Maps.
This preserves our ability to change how kconfig references work
without having to retain the original MapSpec.