[PATCH bpf 0/2] bpf: fix a bpf_timer initialization issue

[PATCH bpf 0/2] bpf: fix a bpf_timer initialization issue

[Yonghong Song]

The patch [1] exposed a bpf_timer initialization bug in function
check_and_init_map_value(). With bug fix here, the patch [1]
can be applied with all selftests passed. Please see individual
patches for fix details.

  [1] https://lore.kernel.org/bpf/20220209070324.1093182-2-memxor@gmail.com/

Yonghong Song (2):
  bpf: fix a bpf_timer initialization issue
  bpf: emit bpf_timer in vmlinux BTF

 include/linux/bpf.h  | 6 ++----
 kernel/bpf/helpers.c | 2 ++
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.30.2

[PATCH bpf 1/2] bpf: fix a bpf_timer initialization issue

[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

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/

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 include/linux/bpf.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c1c554249698..f19abc59b6cd 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -220,11 +220,9 @@ static inline bool map_value_has_timer(const struct bpf_map *map)
 static inline void check_and_init_map_value(struct bpf_map *map, void *dst)
 {
 	if (unlikely(map_value_has_spin_lock(map)))
-		*(struct bpf_spin_lock *)(dst + map->spin_lock_off) =
-			(struct bpf_spin_lock){};
+		memset(dst + map->spin_lock_off, 0, sizeof(struct bpf_spin_lock));
 	if (unlikely(map_value_has_timer(map)))
-		*(struct bpf_timer *)(dst + map->timer_off) =
-			(struct bpf_timer){};
+		memset(dst + map->timer_off, 0, sizeof(struct bpf_timer));
 }
 
 /* copy everything but bpf_spin_lock and bpf_timer. There could be one of each. */
-- 
2.30.2

[PATCH bpf 2/2] bpf: emit bpf_timer in vmlinux BTF

[Yonghong Song]

Previously, the following code in check_and_init_map_value()
  *(struct bpf_timer *)(dst + map->timer_off) =
      (struct bpf_timer){};
can help generate bpf_timer definition in vmlinuxBTF.
But previous patch replaced the above code with memset
so bpf_timer definition disappears from vmlinuxBTF.
Let us emit the type explicitly so bpf program can continue
to use it from vmlinux.h.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 kernel/bpf/helpers.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 4e5969fde0b3..d7fc3c270a4c 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -17,6 +17,7 @@
 #include <linux/proc_ns.h>
 #include <linux/security.h>
 #include <linux/btf_ids.h>
+#include <linux/btf.h>
 
 #include "../../lib/kstrtox.h"
 
@@ -1109,6 +1110,7 @@ static enum hrtimer_restart bpf_timer_cb(struct hrtimer *hrtimer)
 	void *key;
 	u32 idx;
 
+	BTF_TYPE_EMIT(struct bpf_timer);
 	callback_fn = rcu_dereference_check(t->callback_fn, rcu_read_lock_bh_held());
 	if (!callback_fn)
 		goto out;
-- 
2.30.2

Re: [PATCH bpf 1/2] bpf: fix a bpf_timer initialization issue

[Yonghong Song]

On 2/10/22 11:39 PM, Yonghong Song wrote:
> 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
> 
> 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/
> 
> Signed-off-by: Yonghong Song <yhs@fb.com>

Sorry, missed Fixes tag:
   Fixes: 68134668c17f3 ("bpf: Add map side support for bpf timers.")

> ---
>   include/linux/bpf.h | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index c1c554249698..f19abc59b6cd 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -220,11 +220,9 @@ static inline bool map_value_has_timer(const struct bpf_map *map)
>   static inline void check_and_init_map_value(struct bpf_map *map, void *dst)
>   {
>   	if (unlikely(map_value_has_spin_lock(map)))
> -		*(struct bpf_spin_lock *)(dst + map->spin_lock_off) =
> -			(struct bpf_spin_lock){};
> +		memset(dst + map->spin_lock_off, 0, sizeof(struct bpf_spin_lock));
>   	if (unlikely(map_value_has_timer(map)))
> -		*(struct bpf_timer *)(dst + map->timer_off) =
> -			(struct bpf_timer){};
> +		memset(dst + map->timer_off, 0, sizeof(struct bpf_timer));
>   }
>   
>   /* copy everything but bpf_spin_lock and bpf_timer. There could be one of each. */

Re: [PATCH bpf 2/2] bpf: emit bpf_timer in vmlinux BTF

[Daniel Borkmann]

On 2/11/22 8:39 AM, Yonghong Song wrote:
> Previously, the following code in check_and_init_map_value()
>    *(struct bpf_timer *)(dst + map->timer_off) =
>        (struct bpf_timer){};
> can help generate bpf_timer definition in vmlinuxBTF.
> But previous patch replaced the above code with memset
> so bpf_timer definition disappears from vmlinuxBTF.
> Let us emit the type explicitly so bpf program can continue
> to use it from vmlinux.h.
> 
> Signed-off-by: Yonghong Song <yhs@fb.com>

Needs at minimum rebase for the bpf tree as target, see also:

   https://github.com/kernel-patches/bpf/pull/2549

Thanks,
Daniel

Re: [PATCH bpf 2/2] bpf: emit bpf_timer in vmlinux BTF

[Daniel Borkmann]

On 2/11/22 3:48 PM, Daniel Borkmann wrote:
> On 2/11/22 8:39 AM, Yonghong Song wrote:
>> Previously, the following code in check_and_init_map_value()
>>    *(struct bpf_timer *)(dst + map->timer_off) =
>>        (struct bpf_timer){};
>> can help generate bpf_timer definition in vmlinuxBTF.
>> But previous patch replaced the above code with memset
>> so bpf_timer definition disappears from vmlinuxBTF.
>> Let us emit the type explicitly so bpf program can continue
>> to use it from vmlinux.h.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
> 
> Needs at minimum rebase for the bpf tree as target, see also:
> 
>    https://github.com/kernel-patches/bpf/pull/2549

(Nevermind, noticed the dependency too late, sry for noise.)

Re: [PATCH bpf 2/2] bpf: emit bpf_timer in vmlinux BTF

[Yonghong Song]

On 2/11/22 6:52 AM, Daniel Borkmann wrote:
> On 2/11/22 3:48 PM, Daniel Borkmann wrote:
>> On 2/11/22 8:39 AM, Yonghong Song wrote:
>>> Previously, the following code in check_and_init_map_value()
>>>    *(struct bpf_timer *)(dst + map->timer_off) =
>>>        (struct bpf_timer){};
>>> can help generate bpf_timer definition in vmlinuxBTF.
>>> But previous patch replaced the above code with memset
>>> so bpf_timer definition disappears from vmlinuxBTF.
>>> Let us emit the type explicitly so bpf program can continue
>>> to use it from vmlinux.h.
>>>
>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>
>> Needs at minimum rebase for the bpf tree as target, see also:
>>
>>    https://github.com/kernel-patches/bpf/pull/2549
> 
> (Nevermind, noticed the dependency too late, sry for noise.)

Thanks! The patch is prepared based on bpf-next.
Just rebased locally and tested against bpf tree.
Will submit version 2 soon.