On Fri, Sep 24, 2021 at 04:12:11PM -0700, Andrii Nakryiko wrote:

That's not what I proposed. So let's say somewhere in the kernel we
have this variable:

static int bpf_bloom_exists = 1;

Now, for bpf_map_lookup_elem() helper we pass data as key pointer. If
all its hashed bits are set in Bloom filter (it "exists"), we return
&bpf_bloom_exists. So it's not a NULL pointer.

imo that's too much of a hack.

Now, despite returning a valid pointer, it would be good to prevent
reading/writing from/to it in a valid BPF program. I'm hoping it is as
easy as just seetting map->value_size = 0 during map creation. But
worst case, we can let BPF program just overwrite that 1 with whatever
they want. It doesn't matter because the contract is that
bpf_map_lookup_elem() returns non-NULL for "exists" and NULL
otherwise.

Now, for MAP_LOOKUP_ELEM command on user-space side. I'd say the
contract should be 0 return code on "exists" (and nothing is written
to value pointer, perhaps even disallow to specify the non-NULL value
pointer altogether); -ENOENT, otherwise. Again, worst-case we can
specify that "value_size" is 1 or 4 bytes and write 1 to it.

Does it make sense?


BTW, for STACK/QUEUE maps, I'm not clear why we had to add
map_peek_elem/map_pop_elem/map_push_elem operations, to be honest.
They could have been pretty reasonably mapped to map_lookup_elem (peek
is non-destructive lookup), map_lookup_elem + some flag (pop is
lookup, but destructive, so flag allows "destruction"), and
map_update_elem (for push). map_delete_elem would be pop for when
value can be ignored.

That's to say that I don't consider STACK/QUEUE maps as good examples,
it's rather a counter-example of maps that barely anyone is using, yet
it just adds clutter to BPF internals.

Repurposing lookup/update ops for stack/queue and forcing bpf progs
to pass dummy key would have looked quite ugly.
peek/pop/push have one pointer. That pointer points to value.
For bitset we have single pointer as well.
So it makes perfect sense to reuse push/pop/peek and keep bitset
as a value from the verifier side.
bpf_helper_defs.h could have static inline functions:
bool bpf_bitset_clear(map, key);
bool bpf_bitset_set(map, key);
bool bpf_bitset_test(map, key);

But they will map to FUNC_map_pop_elem/push/peek as func ids
and will be seen as values from the verifier pov.

The bpf progs might think of them as keys, but they will be value_size.
The bitset creation could use key_size as an input, but internally
set it as value_size.
Not sure whether such internal vs uapi remaping will not lead
to confusion and bugs though.
I agree that key as an input to bitset_clear/set/test make more sense,
but the verifier needs value_size to plug into peek/pop infra.

I don't think it's worth to extend ops with yet another 3 callbacks
and have clean ARG_PTR_TO_UNINIT_MAP_KEY there.
That is probably too much clutter.

I think 
bool bpf_bitset_clear(map, value);
bool bpf_bitset_set(map, value);
bool bpf_bitset_test(map, value);
doesn't look too bad either.
At least this way the bit set map_create op will use value_size
and keep it as value_size. The bpf prog won't be confusing.
That's my preference, so far.