Message ID | 159467114191.370286.3577295271355257627.stgit@toke.dk |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | bpf: Support multi-attach for freplace programs | expand |
Hi "Toke,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
[also build test WARNING on vhost/linux-next ipvs/master v5.8-rc5 next-20200713]
[cannot apply to bpf-next/master bpf/master net/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Toke-H-iland-J-rgensen/bpf-Support-multi-attach-for-freplace-programs/20200714-041410
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 528ae84a34ffd40da5d3fbff740d28d6dc2c8f8a
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 02946de3802d3bc65bc9f2eb9b8d4969b5a7add8)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> kernel/bpf/verifier.c:10876:7: warning: variable 'addr' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (ret)
^~~
kernel/bpf/verifier.c:10923:14: note: uninitialized use occurs here
*tgt_addr = addr;
^~~~
kernel/bpf/verifier.c:10876:3: note: remove the 'if' if its condition is always true
if (ret)
^~~~~~~~
kernel/bpf/verifier.c:10861:7: warning: variable 'addr' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (!btf_type_is_func_proto(t))
^~~~~~~~~~~~~~~~~~~~~~~~~~
kernel/bpf/verifier.c:10923:14: note: uninitialized use occurs here
*tgt_addr = addr;
^~~~
kernel/bpf/verifier.c:10861:3: note: remove the 'if' if its condition is always true
if (!btf_type_is_func_proto(t))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
kernel/bpf/verifier.c:10750:11: note: initialize the variable 'addr' to silence this warning
long addr;
^
= 0
2 warnings generated.
vim +10876 kernel/bpf/verifier.c
10733
10734 int bpf_check_attach_target(struct bpf_verifier_log *log,
10735 const struct bpf_prog *prog,
10736 const struct bpf_prog *tgt_prog,
10737 u32 btf_id,
10738 struct btf_func_model *fmodel,
10739 long *tgt_addr,
10740 const char **tgt_name,
10741 const struct btf_type **tgt_type)
10742 {
10743 bool prog_extension = prog->type == BPF_PROG_TYPE_EXT;
10744 const char prefix[] = "btf_trace_";
10745 int ret = 0, subprog = -1, i;
10746 const struct btf_type *t;
10747 bool conservative = true;
10748 const char *tname;
10749 struct btf *btf;
10750 long addr;
10751
10752 if (!btf_id) {
10753 bpf_log(log, "Tracing programs must provide btf_id\n");
10754 return -EINVAL;
10755 }
10756 btf = bpf_prog_get_target_btf(prog);
10757 if (!btf) {
10758 bpf_log(log,
10759 "FENTRY/FEXIT program can only be attached to another program annotated with BTF\n");
10760 return -EINVAL;
10761 }
10762 t = btf_type_by_id(btf, btf_id);
10763 if (!t) {
10764 bpf_log(log, "attach_btf_id %u is invalid\n", btf_id);
10765 return -EINVAL;
10766 }
10767 tname = btf_name_by_offset(btf, t->name_off);
10768 if (!tname) {
10769 bpf_log(log, "attach_btf_id %u doesn't have a name\n", btf_id);
10770 return -EINVAL;
10771 }
10772 if (tgt_prog) {
10773 struct bpf_prog_aux *aux = tgt_prog->aux;
10774
10775 for (i = 0; i < aux->func_info_cnt; i++)
10776 if (aux->func_info[i].type_id == btf_id) {
10777 subprog = i;
10778 break;
10779 }
10780 if (subprog == -1) {
10781 bpf_log(log, "Subprog %s doesn't exist\n", tname);
10782 return -EINVAL;
10783 }
10784 conservative = aux->func_info_aux[subprog].unreliable;
10785 if (prog_extension) {
10786 if (conservative) {
10787 bpf_log(log,
10788 "Cannot replace static functions\n");
10789 return -EINVAL;
10790 }
10791 if (!prog->jit_requested) {
10792 bpf_log(log,
10793 "Extension programs should be JITed\n");
10794 return -EINVAL;
10795 }
10796 }
10797 if (!tgt_prog->jited) {
10798 bpf_log(log, "Can attach to only JITed progs\n");
10799 return -EINVAL;
10800 }
10801 if (tgt_prog->type == prog->type) {
10802 /* Cannot fentry/fexit another fentry/fexit program.
10803 * Cannot attach program extension to another extension.
10804 * It's ok to attach fentry/fexit to extension program.
10805 */
10806 bpf_log(log, "Cannot recursively attach\n");
10807 return -EINVAL;
10808 }
10809 if (tgt_prog->type == BPF_PROG_TYPE_TRACING &&
10810 prog_extension &&
10811 (tgt_prog->expected_attach_type == BPF_TRACE_FENTRY ||
10812 tgt_prog->expected_attach_type == BPF_TRACE_FEXIT)) {
10813 /* Program extensions can extend all program types
10814 * except fentry/fexit. The reason is the following.
10815 * The fentry/fexit programs are used for performance
10816 * analysis, stats and can be attached to any program
10817 * type except themselves. When extension program is
10818 * replacing XDP function it is necessary to allow
10819 * performance analysis of all functions. Both original
10820 * XDP program and its program extension. Hence
10821 * attaching fentry/fexit to BPF_PROG_TYPE_EXT is
10822 * allowed. If extending of fentry/fexit was allowed it
10823 * would be possible to create long call chain
10824 * fentry->extension->fentry->extension beyond
10825 * reasonable stack size. Hence extending fentry is not
10826 * allowed.
10827 */
10828 bpf_log(log, "Cannot extend fentry/fexit\n");
10829 return -EINVAL;
10830 }
10831 } else {
10832 if (prog_extension) {
10833 bpf_log(log, "Cannot replace kernel functions\n");
10834 return -EINVAL;
10835 }
10836 }
10837
10838 switch (prog->expected_attach_type) {
10839 case BPF_TRACE_RAW_TP:
10840 if (tgt_prog) {
10841 bpf_log(log,
10842 "Only FENTRY/FEXIT progs are attachable to another BPF prog\n");
10843 return -EINVAL;
10844 }
10845 if (!btf_type_is_typedef(t)) {
10846 bpf_log(log, "attach_btf_id %u is not a typedef\n",
10847 btf_id);
10848 return -EINVAL;
10849 }
10850 if (strncmp(prefix, tname, sizeof(prefix) - 1)) {
10851 bpf_log(log, "attach_btf_id %u points to wrong type name %s\n",
10852 btf_id, tname);
10853 return -EINVAL;
10854 }
10855 tname += sizeof(prefix) - 1;
10856 t = btf_type_by_id(btf, t->type);
10857 if (!btf_type_is_ptr(t))
10858 /* should never happen in valid vmlinux build */
10859 return -EINVAL;
10860 t = btf_type_by_id(btf, t->type);
10861 if (!btf_type_is_func_proto(t))
10862 /* should never happen in valid vmlinux build */
10863 return -EINVAL;
10864
10865 break;
10866 case BPF_TRACE_ITER:
10867 if (!btf_type_is_func(t)) {
10868 bpf_log(log, "attach_btf_id %u is not a function\n",
10869 btf_id);
10870 return -EINVAL;
10871 }
10872 t = btf_type_by_id(btf, t->type);
10873 if (!btf_type_is_func_proto(t))
10874 return -EINVAL;
10875 ret = btf_distill_func_proto(log, btf, t, tname, fmodel);
10876 if (ret)
10877 return ret;
10878 break;
10879 default:
10880 if (!prog_extension)
10881 return -EINVAL;
10882 /* fallthrough */
10883 case BPF_MODIFY_RETURN:
10884 case BPF_LSM_MAC:
10885 case BPF_TRACE_FENTRY:
10886 case BPF_TRACE_FEXIT:
10887 if (!btf_type_is_func(t)) {
10888 bpf_log(log, "attach_btf_id %u is not a function\n",
10889 btf_id);
10890 return -EINVAL;
10891 }
10892 if (prog_extension &&
10893 btf_check_type_match(log, prog, btf, t))
10894 return -EINVAL;
10895 t = btf_type_by_id(btf, t->type);
10896 if (!btf_type_is_func_proto(t))
10897 return -EINVAL;
10898
10899 if (tgt_prog && conservative)
10900 t = NULL;
10901
10902 ret = btf_distill_func_proto(log, btf, t, tname, fmodel);
10903 if (ret < 0)
10904 return ret;
10905
10906 if (tgt_prog) {
10907 if (subprog == 0)
10908 addr = (long) tgt_prog->bpf_func;
10909 else
10910 addr = (long) tgt_prog->aux->func[subprog]->bpf_func;
10911 } else {
10912 addr = kallsyms_lookup_name(tname);
10913 if (!addr) {
10914 bpf_log(log,
10915 "The address of function %s cannot be found\n",
10916 tname);
10917 return -ENOENT;
10918 }
10919 }
10920 break;
10921 }
10922
10923 *tgt_addr = addr;
10924 if (tgt_name)
10925 *tgt_name = tname;
10926 if (tgt_type)
10927 *tgt_type = t;
10928 return 0;
10929 }
10930
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index c547065387e4..67310595f720 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -570,6 +570,9 @@ static __always_inline unsigned int bpf_dispatcher_nop_func( struct bpf_trampoline *bpf_trampoline_lookup(u64 key); int bpf_trampoline_link_prog(struct bpf_prog *prog); int bpf_trampoline_unlink_prog(struct bpf_prog *prog); +int bpf_trampoline_get(u64 key, void *addr, + struct btf_func_model *fmodel, + struct bpf_trampoline **trampoline); void bpf_trampoline_put(struct bpf_trampoline *tr); #define BPF_DISPATCHER_INIT(_name) { \ .mutex = __MUTEX_INITIALIZER(_name.mutex), \ @@ -626,6 +629,12 @@ static inline int bpf_trampoline_unlink_prog(struct bpf_prog *prog) { return -ENOTSUPP; } +static inline int bpf_trampoline_get(u64 key, void *addr, + struct btf_func_model *fmodel, + struct bpf_trampoline **trampoline) +{ + return -EOPNOTSUPP; +} static inline void bpf_trampoline_put(struct bpf_trampoline *tr) {} #define DEFINE_BPF_DISPATCHER(name) #define DECLARE_BPF_DISPATCHER(name) diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 53c7bd568c5d..b882185257cc 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -446,4 +446,13 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt); int check_ctx_reg(struct bpf_verifier_env *env, const struct bpf_reg_state *reg, int regno); +int bpf_check_attach_target(struct bpf_verifier_log *log, + const struct bpf_prog *prog, + const struct bpf_prog *tgt_prog, + u32 btf_id, + struct btf_func_model *fmodel, + long *tgt_addr, + const char **tgt_name, + const struct btf_type **tgt_type); + #endif /* _LINUX_BPF_VERIFIER_H */ diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index 9be85aa4ec5f..fadfa330f728 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -331,6 +331,28 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog) return err; } +int bpf_trampoline_get(u64 key, void *addr, + struct btf_func_model *fmodel, + struct bpf_trampoline **trampoline) +{ + struct bpf_trampoline *tr; + + tr = bpf_trampoline_lookup(key); + if (!tr) + return -ENOMEM; + + mutex_lock(&tr->mutex); + if (tr->func.addr) + goto out; + + memcpy(&tr->func.model, fmodel, sizeof(*fmodel)); + tr->func.addr = addr; +out: + mutex_unlock(&tr->mutex); + *trampoline = tr; + return 0; +} + void bpf_trampoline_put(struct bpf_trampoline *tr) { if (!tr) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 0db338567cf9..ff3c5c53982c 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -10724,31 +10724,23 @@ static int check_attach_modify_return(struct bpf_prog *prog, unsigned long addr) return -EINVAL; } -static int check_attach_btf_id(struct bpf_verifier_env *env) +int bpf_check_attach_target(struct bpf_verifier_log *log, + const struct bpf_prog *prog, + const struct bpf_prog *tgt_prog, + u32 btf_id, + struct btf_func_model *fmodel, + long *tgt_addr, + const char **tgt_name, + const struct btf_type **tgt_type) { - struct bpf_prog *prog = env->prog; bool prog_extension = prog->type == BPF_PROG_TYPE_EXT; - struct bpf_prog *tgt_prog = prog->aux->linked_prog; - struct bpf_verifier_log *log = &env->log; - u32 btf_id = prog->aux->attach_btf_id; const char prefix[] = "btf_trace_"; - struct btf_func_model fmodel; int ret = 0, subprog = -1, i; - struct bpf_trampoline *tr; const struct btf_type *t; bool conservative = true; const char *tname; struct btf *btf; long addr; - u64 key; - - if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) - return check_struct_ops_btf_id(env); - - if (prog->type != BPF_PROG_TYPE_TRACING && - prog->type != BPF_PROG_TYPE_LSM && - !prog_extension) - return 0; if (!btf_id) { bpf_log(log, "Tracing programs must provide btf_id\n"); @@ -10794,8 +10786,6 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) "Extension programs should be JITed\n"); return -EINVAL; } - env->ops = bpf_verifier_ops[tgt_prog->type]; - prog->expected_attach_type = tgt_prog->expected_attach_type; } if (!tgt_prog->jited) { bpf_log(log, "Can attach to only JITed progs\n"); @@ -10831,13 +10821,11 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) bpf_log(log, "Cannot extend fentry/fexit\n"); return -EINVAL; } - key = ((u64)aux->id) << 32 | btf_id; } else { if (prog_extension) { bpf_log(log, "Cannot replace kernel functions\n"); return -EINVAL; } - key = btf_id; } switch (prog->expected_attach_type) { @@ -10867,13 +10855,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) /* should never happen in valid vmlinux build */ return -EINVAL; - /* remember two read only pointers that are valid for - * the life time of the kernel - */ - prog->aux->attach_func_name = tname; - prog->aux->attach_func_proto = t; - prog->aux->attach_btf_trace = true; - return 0; + break; case BPF_TRACE_ITER: if (!btf_type_is_func(t)) { bpf_log(log, "attach_btf_id %u is not a function\n", @@ -10883,12 +10865,10 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) t = btf_type_by_id(btf, t->type); if (!btf_type_is_func_proto(t)) return -EINVAL; - prog->aux->attach_func_name = tname; - prog->aux->attach_func_proto = t; - if (!bpf_iter_prog_supported(prog)) - return -EINVAL; - ret = btf_distill_func_proto(log, btf, t, tname, &fmodel); - return ret; + ret = btf_distill_func_proto(log, btf, t, tname, fmodel); + if (ret) + return ret; + break; default: if (!prog_extension) return -EINVAL; @@ -10897,13 +10877,6 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) case BPF_LSM_MAC: case BPF_TRACE_FENTRY: case BPF_TRACE_FEXIT: - prog->aux->attach_func_name = tname; - if (prog->type == BPF_PROG_TYPE_LSM) { - ret = bpf_lsm_verify_prog(log, prog); - if (ret < 0) - return ret; - } - if (!btf_type_is_func(t)) { bpf_log(log, "attach_btf_id %u is not a function\n", btf_id); @@ -10915,24 +10888,14 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) t = btf_type_by_id(btf, t->type); if (!btf_type_is_func_proto(t)) return -EINVAL; - tr = bpf_trampoline_lookup(key); - if (!tr) - return -ENOMEM; - /* t is either vmlinux type or another program's type */ - prog->aux->attach_func_proto = t; - mutex_lock(&tr->mutex); - if (tr->func.addr) { - prog->aux->trampoline = tr; - goto out; - } - if (tgt_prog && conservative) { - prog->aux->attach_func_proto = NULL; + + if (tgt_prog && conservative) t = NULL; - } - ret = btf_distill_func_proto(log, btf, t, - tname, &tr->func.model); + + ret = btf_distill_func_proto(log, btf, t, tname, fmodel); if (ret < 0) - goto out; + return ret; + if (tgt_prog) { if (subprog == 0) addr = (long) tgt_prog->bpf_func; @@ -10944,27 +10907,85 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) bpf_log(log, "The address of function %s cannot be found\n", tname); - ret = -ENOENT; - goto out; + return -ENOENT; } } + break; + } - if (prog->expected_attach_type == BPF_MODIFY_RETURN) { - ret = check_attach_modify_return(prog, addr); - if (ret) - bpf_log(log, "%s() is not modifiable\n", - prog->aux->attach_func_name); - } + *tgt_addr = addr; + if (tgt_name) + *tgt_name = tname; + if (tgt_type) + *tgt_type = t; + return 0; +} - if (ret) - goto out; - tr->func.addr = (void *)addr; - prog->aux->trampoline = tr; -out: - mutex_unlock(&tr->mutex); - if (ret) - bpf_trampoline_put(tr); +static int check_attach_btf_id(struct bpf_verifier_env *env) +{ + struct bpf_prog *prog = env->prog; + struct bpf_prog *tgt_prog = prog->aux->linked_prog; + u32 btf_id = prog->aux->attach_btf_id; + struct btf_func_model fmodel; + const struct btf_type *t; + const char *tname; + long addr; + int ret; + u64 key; + + if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) + return check_struct_ops_btf_id(env); + + if (prog->type != BPF_PROG_TYPE_TRACING && + prog->type != BPF_PROG_TYPE_LSM && + prog->type != BPF_PROG_TYPE_EXT) + return 0; + + ret = bpf_check_attach_target(&env->log, prog, tgt_prog, btf_id, + &fmodel, &addr, &tname, &t); + if (ret) return ret; + + if (tgt_prog) { + if (prog->type == BPF_PROG_TYPE_EXT) { + env->ops = bpf_verifier_ops[tgt_prog->type]; + prog->expected_attach_type = tgt_prog->expected_attach_type; + } + key = ((u64)tgt_prog->aux->id) << 32 | btf_id; + } else { + key = btf_id; + } + + prog->aux->attach_func_proto = t; + prog->aux->attach_func_name = tname; + + switch (prog->expected_attach_type) { + case BPF_TRACE_RAW_TP: + /* remember two read only pointers that are valid for + * the life time of the kernel + */ + prog->aux->attach_btf_trace = true; + return 0; + case BPF_TRACE_ITER: + if (!bpf_iter_prog_supported(prog)) + return -EINVAL; + return 0; + case BPF_MODIFY_RETURN: + ret = check_attach_modify_return(prog, addr); + if (ret) { + verbose(env, "%s() is not modifiable\n", + prog->aux->attach_func_name); + return ret; + } + fallthrough; + default: + if (prog->type == BPF_PROG_TYPE_LSM) { + ret = bpf_lsm_verify_prog(&env->log, prog); + if (ret < 0) + return ret; + } + return bpf_trampoline_get(key, (void *)addr, &fmodel, + &prog->aux->trampoline); } }