Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpf2c should not allow static initialization for BPF_MAP_TYPE_HASH_OF_MAPS map value, except for key_size is integer #2882

Closed
shpalani opened this issue Sep 22, 2023 · 4 comments · Fixed by #3211
Assignees
Labels
bug Something isn't working triaged Discussed in a triage meeting
Milestone

Comments

@shpalani
Copy link
Collaborator

shpalani commented Sep 22, 2023

Describe the bug

The request is to bpf2c should flag error that it cannot statically initialize the value for Hash-of-Maps.

Only BPF_MAP_TYPE_ARRAY_OF_MAPS and BPF_MAP_TYPE_PROG_ARRAY type maps can be statically initialized as they have an implicit key value.
BPF_MAP_TYPE_HASH_OF_MAPS style maps have no implicit key value and can't be statically initialized.

Incorrect Map with static initialization:

struct {
		__uint(type, **BPF_MAP_TYPE_ARRAY)**;
		__uint(key_size, sizeof(__u32));
		__uint(value_size, sizeof(__u32) * LB_MAGLEV_LUT_SIZE);
		__uint(max_entries, 1);
} LB6_MAGLEV_MAP_INNER  __section_maps_btf;

struct {
	__uint(type, **BPF_MAP_TYPE_HASH_OF_MAPS**);
	__type(key, __u16);
	__type(value, __u32);
	__uint(pinning, LIBBPF_PIN_BY_NAME);
	__uint(max_entries, CILIUM_LB_MAGLEV_MAP_MAX_ENTRIES);
	__uint(map_flags, CONDITIONAL_PREALLOC);
	/* Maglev inner map definition */
	__array(values, LB6_MAGLEV_MAP_INNER);
} LB6_MAGLEV_MAP_OUTER __section_maps_btf = {
    .values = {&LB6_MAGLEV_MAP_INNER},
};

Trace:
[39]1624.193C::‎2023‎-‎09‎-‎21 15:06:26.974 [EbpfForWindowsProvider]_ebpf_native_set_initial_map_values
[39]1624.193C::‎2023‎-‎09‎-‎21 15:06:26.974 [EbpfForWindowsProvider]_ebpf_native_set_initial_map_values returned error,6
[39]1624.193C::‎2023‎-‎09‎-‎21 15:06:26.974 [EbpfForWindowsProvider]ebpf_native_load_programs: set initial map values failed,{eb7ad2f3-6a4b-465b-8fb7-93ad58c5c456}

Reference: #2854

OS information

Windows 10 and above

Steps taken to reproduce bug

Use the above map.

Expected behavior

bpf2c should flag the error that BPF_MAP_TYPE_HASH_OF_MAPS cannot be statically initialized.

Actual outcome

Trace:
[39]1624.193C::‎2023‎-‎09‎-‎21 15:06:26.974 [EbpfForWindowsProvider]_ebpf_native_set_initial_map_values
[39]1624.193C::‎2023‎-‎09‎-‎21 15:06:26.974 [EbpfForWindowsProvider]_ebpf_native_set_initial_map_values returned error,6
[39]1624.193C::‎2023‎-‎09‎-‎21 15:06:26.974 [EbpfForWindowsProvider]ebpf_native_load_programs: set initial map values failed,{eb7ad2f3-6a4b-465b-8fb7-93ad58c5c456}

Additional details

@shpalani shpalani added the bug Something isn't working label Sep 22, 2023
@shpalani shpalani self-assigned this Sep 22, 2023
@shpalani
Copy link
Collaborator Author

shpalani commented Sep 22, 2023

From code analysis,
BPF_MAP_TYPE_ARRAY_OF_MAPS and BPF_MAP_TYPE_PROG_ARRAY are supported, but not BPF_MAP_TYPE_HASH_OF_MAPS.

BPF_MAP_TYPE_ARRAY_OF_MAPS and BPF_MAP_TYPE_HASH_OF_MAPS provide general-purpose support for map in map storage.

 static ebpf_result_t
_ebpf_native_set_initial_map_values(_Inout_ ebpf_native_module_t* module)
{
    EBPF_LOG_ENTRY();
    ebpf_result_t result = EBPF_SUCCESS;
    map_initial_values_t* map_initial_values = NULL;
    size_t map_initial_values_count = 0;

    // Get initial value for maps.
    module->table.map_initial_values(&map_initial_values, &map_initial_values_count);

    // For each map, update the initial values.
    for (size_t i = 0; i < map_initial_values_count; i++) {
        ebpf_native_map_t* native_map_to_update = _ebpf_native_find_map_by_name(module, map_initial_values[i].name);
        if (native_map_to_update == NULL) {
            result = EBPF_INVALID_ARGUMENT;
            break;
        }

        if (native_map_to_update->reused) {
            // Map is reused. Skip updating initial values.
            continue;
        }

        // For each value in the map, find the map or program to insert.
        for (size_t j = 0; j < map_initial_values[i].count; j++) {
            // Skip empty initial values.
            if (!map_initial_values[i].values[j]) {
                continue;
            }

            ebpf_handle_t handle_to_insert = ebpf_handle_invalid;

            if (native_map_to_update->entry->definition.type == BPF_MAP_TYPE_ARRAY_OF_MAPS) {
                ebpf_native_map_t* native_map_to_insert =
                    _ebpf_native_find_map_by_name(module, map_initial_values[i].values[j]);
                if (native_map_to_update == NULL) {
                    result = EBPF_INVALID_ARGUMENT;
                    break;
                }
                handle_to_insert = native_map_to_insert->handle;
            } else if (native_map_to_update->entry->definition.type == BPF_MAP_TYPE_PROG_ARRAY) {
                ebpf_native_program_t* program_to_insert =
                    _ebpf_native_find_program_by_name(module, map_initial_values[i].values[j]);
                if (program_to_insert == NULL) {
                    result = EBPF_INVALID_ARGUMENT;
                    break;
                }
                handle_to_insert = program_to_insert->handle;
            } else {
                result = EBPF_INVALID_ARGUMENT;    <<<<<< return invalid
                break;
            }
			
	...

}
38: kd> dx -r1 ((ebpfcore!_ebpf_native_map *)0xffffc3073ae8aa40)((ebpfcore!_ebpf_native_map *)0xffffc3073ae8aa40)                 : 0xffffc3073ae8aa40 [Type: _ebpf_native_map *]

    [+0x000] entry            : 0xfffff8061571b090 [Type: _map_entry *]

    [+0x008] inner_map        : 0xffffc3073ae8aa00 [Type: _ebpf_native_map *]

    [+0x010] handle           : 1184 [Type: __int64]

    [+0x018] inner_map_handle : 0 [Type: __int64]

    [+0x020] original_id      : 4 [Type: int]

    [+0x024] inner_map_original_id : 3 [Type: int]

    [+0x028] pin_path         [Type: _cxplat_utf8_string]

    [+0x038] reused           : false [Type: bool]

    [+0x039] pinned           : true [Type: bool]

38: kd> dx -r1 ((ebpfcore!_map_entry *)0xfffff8061571b090)((ebpfcore!_map_entry *)0xfffff8061571b090)                 : 0xfffff8061571b090 [Type: _map_entry *]

    [+0x000] address          : 0x0 [Type: void *]

    [+0x008] definition       [Type: _ebpf_map_definition_in_file]

    [+0x028] name             : 0xfffff80615716128 : "cilium_lb4_maglev_outer" [Type: char *]

38: kd> dx -r1 (*((ebpfcore!_ebpf_map_definition_in_file *)0xfffff8061571b098))(*((ebpfcore!_ebpf_map_definition_in_file *)0xfffff8061571b098))                 [Type: _ebpf_map_definition_in_file]

    [+0x000] type             : BPF_MAP_TYPE_HASH_OF_MAPS (6) [Type: bpf_map_type]

    [+0x004] key_size         : 0x2 [Type: unsigned int]

    [+0x008] value_size       : 0x4 [Type: unsigned int]

    [+0x00c] max_entries      : 0x10000 [Type: unsigned int]

    [+0x010] inner_map_idx    : 0x0 [Type: unsigned int]

    [+0x014] pinning          : PIN_GLOBAL_NS (2) [Type: ebpf_pin_type]

    [+0x018] id               : 0x1f [Type: unsigned int]

    [+0x01c] inner_id         : 0x1b [Type: unsigned int]

38: kd>

@shpalani shpalani removed their assignment Sep 23, 2023
@shpalani shpalani changed the title BPF_MAP_TYPE_HASH_OF_MAPS is not supported to initialize map value bpf2c should not allow static initialization for BPF_MAP_TYPE_HASH_OF_MAPS map value. Sep 23, 2023
@shpalani
Copy link
Collaborator Author

Trace

xdp_trace_2 1.txt

@shpalani
Copy link
Collaborator Author

Reading this note from https://docs.kernel.org/bpf/map_of_maps.html
"For BPF_MAP_TYPE_HASH_OF_MAPS the key type can be chosen when defining the map. "

If the key and value (fd of the inner map) are integers, then static initialization should be allowed. I think. that is the reason, the maps provided in the description worked and were loaded.

@dahavey dahavey added the triaged Discussed in a triage meeting label Sep 25, 2023
@dahavey dahavey modified the milestones: 2310, Backlog Sep 25, 2023
@Alan-Jowett
Copy link
Member

Updating the title based on what Linux does.
https://github.com/libbpf/libbpf/blob/e26b84dc330c9644c07428c271ab491b0f01f4e1/src/libbpf.c#L6779C1-L6784C5

It appears as if libbpf on Linux supports statically initializing hash of maps, but only if the key size == sizeof(int) and then assigning offset as the key.

@shpalani shpalani changed the title bpf2c should not allow static initialization for BPF_MAP_TYPE_HASH_OF_MAPS map value. bpf2c should not allow static initialization for BPF_MAP_TYPE_HASH_OF_MAPS map value, except for key_size is integer Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triaged Discussed in a triage meeting
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants