• Y
    bpf: Fix a bpf_timer initialization issue · 5eaed6ee
    Yonghong Song 提交于
    The patch in [1] intends to fix a bpf_timer related issue,
    but the fix caused existing 'timer' selftest to fail with
    hang or some random errors. After some debug, I found
    an issue with check_and_init_map_value() in the hashtab.c.
    More specifically, in hashtab.c, we have code
      l_new = bpf_map_kmalloc_node(&htab->map, ...)
      check_and_init_map_value(&htab->map, l_new...)
    Note that bpf_map_kmalloc_node() does not do initialization
    so l_new contains random value.
    
    The function check_and_init_map_value() intends to zero the
    bpf_spin_lock and bpf_timer if they exist in the map.
    But I found bpf_spin_lock is zero'ed but bpf_timer is not zero'ed.
    With [1], later copy_map_value() skips copying of
    bpf_spin_lock and bpf_timer. The non-zero bpf_timer caused
    random failures for 'timer' selftest.
    Without [1], for both bpf_spin_lock and bpf_timer case,
    bpf_timer will be zero'ed, so 'timer' self test is okay.
    
    For check_and_init_map_value(), why bpf_spin_lock is zero'ed
    properly while bpf_timer not. In bpf uapi header, we have
      struct bpf_spin_lock {
            __u32   val;
      };
      struct bpf_timer {
            __u64 :64;
            __u64 :64;
      } __attribute__((aligned(8)));
    
    The initialization code:
      *(struct bpf_spin_lock *)(dst + map->spin_lock_off) =
          (struct bpf_spin_lock){};
      *(struct bpf_timer *)(dst + map->timer_off) =
          (struct bpf_timer){};
    It appears the compiler has no obligation to initialize anonymous fields.
    For example, let us use clang with bpf target as below:
      $ cat t.c
      struct bpf_timer {
            unsigned long long :64;
      };
      struct bpf_timer2 {
            unsigned long long a;
      };
    
      void test(struct bpf_timer *t) {
        *t = (struct bpf_timer){};
      }
      void test2(struct bpf_timer2 *t) {
        *t = (struct bpf_timer2){};
      }
      $ clang -target bpf -O2 -c -g t.c
      $ llvm-objdump -d t.o
       ...
       0000000000000000 <test>:
           0:       95 00 00 00 00 00 00 00 exit
       0000000000000008 <test2>:
           1:       b7 02 00 00 00 00 00 00 r2 = 0
           2:       7b 21 00 00 00 00 00 00 *(u64 *)(r1 + 0) = r2
           3:       95 00 00 00 00 00 00 00 exit
    
    gcc11.2 does not have the above issue. But from
      INTERNATIONAL STANDARD ©ISO/IEC ISO/IEC 9899:201x
      Programming languages — C
      http://www.open-std.org/Jtc1/sc22/wg14/www/docs/n1547.pdf
      page 157:
      Except where explicitly stated otherwise, for the purposes of
      this subclause unnamed members of objects of structure and union
      type do not participate in initialization. Unnamed members of
      structure objects have indeterminate value even after initialization.
    
    To fix the problem, let use memset for bpf_timer case in
    check_and_init_map_value(). For consistency, memset is also
    used for bpf_spin_lock case.
    
      [1] https://lore.kernel.org/bpf/20220209070324.1093182-2-memxor@gmail.com/
    
    Fixes: 68134668 ("bpf: Add map side support for bpf timers.")
    Signed-off-by: NYonghong Song <yhs@fb.com>
    Signed-off-by: NAlexei Starovoitov <ast@kernel.org>
    Link: https://lore.kernel.org/bpf/20220211194953.3142152-1-yhs@fb.com
    5eaed6ee
bpf.h 73.4 KB