Re: [PATCH bpf-next 1/4] bpf: enable task local storage for tracing programs

On Mon, Jan 11, 2021 at 7:27 AM Yonghong Song [off-list ref] wrote:

On 1/8/21 3:19 PM, Song Liu wrote:

quoted

To access per-task data, BPF program typically creates a hash table with
pid as the key. This is not ideal because:
  1. The use need to estimate requires size of the hash table, with may be
     inaccurate;
  2. Big hash tables are slow;
  3. To clean up the data properly during task terminations, the user need
     to write code.

Task local storage overcomes these issues and becomes a better option for
these per-task data. Task local storage is only available to BPF_LSM. Now
enable it for tracing programs.

Reported-by: kernel test robot <redacted>
Signed-off-by: Song Liu <redacted>
---

[...]

quoted

  struct cfs_rq;
  struct fs_struct;

@@ -1348,6 +1349,10 @@ struct task_struct {
      /* Used by LSM modules for access restriction: */
      void                            *security;
  #endif
+#ifdef CONFIG_BPF_SYSCALL
+     /* Used by BPF task local storage */
+     struct bpf_local_storage        *bpf_storage;
+#endif

I remembered there is a discussion where KP initially wanted to put
bpf_local_storage in task_struct, but later on changed to
use in lsm as his use case mostly for lsm. Did anybody
remember the details of the discussion? Just want to be
sure what is the concern people has with putting bpf_local_storage
in task_struct and whether the use case presented by
Song will justify it.

If I recall correctly, the discussion was about inode local storage and
it was decided to use the security blob since the use-case was only LSM
programs. Since we now plan to use it in tracing,
detangling the dependency from CONFIG_BPF_LSM
sounds logical to me.

quoted

  #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
      unsigned long                   lowest_stack;

diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
index d1249340fd6ba..ca995fdfa45e7 100644
--- a/kernel/bpf/Makefile
+++ b/kernel/bpf/Makefile

@@ -8,9 +8,8 @@ CFLAGS_core.o += $(call cc-disable-warning, override-init) $(cflags-nogcse-yy)

  obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o inode.o helpers.o tnum.o bpf_iter.o map_iter.o task_iter.o prog_iter.o
  obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list.o lpm_trie.o map_in_map.o
-obj-$(CONFIG_BPF_SYSCALL) += local_storage.o queue_stack_maps.o ringbuf.o
+obj-$(CONFIG_BPF_SYSCALL) += local_storage.o queue_stack_maps.o ringbuf.o bpf_task_storage.o
  obj-${CONFIG_BPF_LSM}         += bpf_inode_storage.o
-obj-${CONFIG_BPF_LSM}          += bpf_task_storage.o
  obj-$(CONFIG_BPF_SYSCALL) += disasm.o
  obj-$(CONFIG_BPF_JIT) += trampoline.o
  obj-$(CONFIG_BPF_SYSCALL) += btf.o

[...]