On Wed, Jun 02, 2021 at 03:34:29PM -0700, Andrii Nakryiko wrote:
>
> > /* 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.
> > 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.
> > - 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.
