On Wed, Jun 02, 2021 at 03:34:29PM -0700, Andrii Nakryiko wrote:

quoted

 /* copy everything but bpf_spin_lock */
 static inline void copy_map_value(struct bpf_map *map, void *dst, void *src)
 {
+       u32 off = 0, size = 0;
+
        if (unlikely(map_value_has_spin_lock(map))) {
-               u32 off = map->spin_lock_off;
+               off = map->spin_lock_off;
+               size = sizeof(struct bpf_spin_lock);
+       } else if (unlikely(map_value_has_timer(map))) {
+               off = map->timer_off;
+               size = sizeof(struct bpf_timer);
+       }

so the need to handle 0, 1, or 2 gaps seems to be the only reason to
disallow both bpf_spinlock and bpf_timer in one map element, right?

exactly.

Isn't it worth addressing it from the very beginning to lift the
artificial restriction? E.g., for speed, you'd do:

if (likely(neither spinlock nor timer)) {
 /* fastest pass */
} else if (only one of spinlock or timer) {
  /* do what you do here */
} else {
  int off1, off2, sz1, sz2;

  if (spinlock_off < timer_off) {
    off1 = spinlock_off;
    sz1 = spinlock_sz;
    off2 = timer_off;
    sz2 = timer_sz;
  } else {
    ... you get the idea

Not really :)
Are you suggesting to support one bpf_spin_lock and one
bpf_timer inside single map element, but not two spin_locks
and/or not two bpf_timers?
Two me it's either one or support any.
Anything in-between doesn't seem worth extra code.

quoted

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f386f85aee5c..0a828dc4968e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -3241,6 +3241,15 @@ static int check_map_access(struct bpf_verifier_env *env, u32 regno,
                        return -EACCES;
                }
        }
+       if (map_value_has_timer(map)) {
+               u32 t = map->timer_off;
+
+               if (reg->smin_value + off < t + sizeof(struct bpf_timer) &&

<= ? Otherwise we allow accessing the first byte, unless I'm mistaken.

I don't think so. See the comment above in if (map_value_has_spin_lock(map))
I didn't copy-paste it, because it's the same logic.

quoted

-       if (val) {
-               /* todo: relax this requirement */
-               verbose(env, "bpf_timer field can only be first in the map value element\n");

ok, this was confusing, but now I see why you did that...

I'll clarify the comment to say that the next patch fixes it.