Is lock required for accessing map/program objects in hash maps?

[shankarseal]

For example, look at this function in ebpf_maps.c:

static _Ret_maybenull_ ebpf_object_t*
_get_object_from_hash_map_entry(_In_ ebpf_core_map_t* map, _In_ const uint8_t* key)
{
    // We need to take a lock here to make sure we can
    // safely reference the object when another thread
    // might be trying to delete the entry we find.
    ebpf_lock_state_t lock_state = ebpf_lock_lock(&map->lock);

    ebpf_object_t* object = NULL;
    uint8_t* value = NULL;
    if (_find_hash_map_entry(map, key, false, &value) == EBPF_SUCCESS) {
        ebpf_id_t id = *(ebpf_id_t*)value;
        (void)ebpf_object_reference_by_id(id, EBPF_OBJECT_MAP, &object);
    }

    ebpf_lock_unlock(&map->lock, lock_state);

    return object;
}

The hashtable implementation is lock free. And there is a separate lock _ebpf_object_tracking_list_lock for object tracking. Then why is the lock acquired before accessing the underlying object? Similarly a lock is acquired in _update_hash_map_entry_with_handle. But no such lock is acquired in _delete_hash_map_entry_with_reference function. Why is that?

[dthaler]

A lock is acquired in _delete_array_map_entry_with_reference() so I think the lack of acquiring it in _delete_hash_map_entry_with_reference() is probably a bug. If I remember right, the lock is trying to provide a guarantee against things like ID reuse where ebpf_object_reference_by_id() ends up referencing an object other than the one originally stored in the map. Though I think the ID generation algorithm uses a 2-byte counter per index so you can't get ID reuse unless you could do 2^16 deletion/additions in between snapshotting the entry value (ID) and taking the tracking list lock inside ebpf_object_reference_by_id(). If we think that is sufficient protection then a separate lock is not needed.

[shankarseal]

I suppose its not only the object ID reuse, whose chance is really small as you pointed out, but I think the insertion deletion of the map values and the ref-count operations on the underlying eBPF object has to be atomic. Do you agree? If so, I will mark the discussion as answered.

[dthaler]

Done. @shankarseal If you agree that "the lack of acquiring it in _delete_hash_map_entry_with_reference()" is a bug, can you file that as issue?

[shankarseal]

#719 addresses the locking issue in delete_hash_map_entry_with_reference.