From mboxrd@z Thu Jan 1 00:00:00 1970 From: Martin Lau Subject: Re: [PATCH bpf-next v2] bpf: support raw tracepoints in modules Date: Thu, 13 Dec 2018 19:22:50 +0000 Message-ID: <20181213192248.ljc6i5unafdlgryf@kafai-mbp> References: <20181213004237.3888568-1-mmullins@fb.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "ast@kernel.org" , "daniel@iogearbox.net" , "netdev@vger.kernel.org" , Kernel Team , Jessica Yu , "Steven Rostedt" , Ingo Molnar , "linux-kernel@vger.kernel.org" To: Matt Mullins Return-path: In-Reply-To: <20181213004237.3888568-1-mmullins@fb.com> Content-Language: en-US Content-ID: <49F6FCE59E48214F85F4BDD59F7DF716@namprd15.prod.outlook.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On Wed, Dec 12, 2018 at 04:42:37PM -0800, Matt Mullins wrote: > Distributions build drivers as modules, including network and filesystem > drivers which export numerous tracepoints. This enables > bpf(BPF_RAW_TRACEPOINT_OPEN) to attach to those tracepoints. >=20 > Signed-off-by: Matt Mullins > --- > v1->v2: > * avoid taking the mutex in bpf_event_notify when op is neither COMING = nor > GOING. > * check that kzalloc actually succeeded >=20 > I didn't try to check list_empty before taking the mutex since I want to = avoid > races between bpf_event_notify and bpf_get_raw_tracepoint. Additionally, > list_for_each_entry_safe is not strictly necessary upon MODULE_STATE_GOIN= G, but > Alexei suggested I use it to protect against fragility if the subsequent = break; > eventually disappears. >=20 > include/linux/module.h | 4 ++ > include/linux/trace_events.h | 8 ++- > kernel/bpf/syscall.c | 11 ++-- > kernel/module.c | 5 ++ > kernel/trace/bpf_trace.c | 99 +++++++++++++++++++++++++++++++++++- > 5 files changed, 120 insertions(+), 7 deletions(-) >=20 > diff --git a/include/linux/module.h b/include/linux/module.h > index fce6b4335e36..5f147dd5e709 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -432,6 +432,10 @@ struct module { > unsigned int num_tracepoints; > tracepoint_ptr_t *tracepoints_ptrs; > #endif > +#ifdef CONFIG_BPF_EVENTS > + unsigned int num_bpf_raw_events; > + struct bpf_raw_event_map *bpf_raw_events; > +#endif > #ifdef HAVE_JUMP_LABEL > struct jump_entry *jump_entries; > unsigned int num_jump_entries; > diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h > index 4130a5497d40..8a62731673f7 100644 > --- a/include/linux/trace_events.h > +++ b/include/linux/trace_events.h > @@ -471,7 +471,8 @@ void perf_event_detach_bpf_prog(struct perf_event *ev= ent); > int perf_event_query_prog_array(struct perf_event *event, void __user *i= nfo); > int bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_prog *p= rog); > int bpf_probe_unregister(struct bpf_raw_event_map *btp, struct bpf_prog = *prog); > -struct bpf_raw_event_map *bpf_find_raw_tracepoint(const char *name); > +struct bpf_raw_event_map *bpf_get_raw_tracepoint(const char *name); > +void bpf_put_raw_tracepoint(struct bpf_raw_event_map *btp); > int bpf_get_perf_event_info(const struct perf_event *event, u32 *prog_id= , > u32 *fd_type, const char **buf, > u64 *probe_offset, u64 *probe_addr); > @@ -502,10 +503,13 @@ static inline int bpf_probe_unregister(struct bpf_r= aw_event_map *btp, struct bpf > { > return -EOPNOTSUPP; > } > -static inline struct bpf_raw_event_map *bpf_find_raw_tracepoint(const ch= ar *name) > +static inline struct bpf_raw_event_map *bpf_get_raw_tracepoint(const cha= r *name) > { > return NULL; > } > +static inline void bpf_put_raw_tracepoint(struct bpf_raw_event_map *btp) > +{ > +} > static inline int bpf_get_perf_event_info(const struct perf_event *event= , > u32 *prog_id, u32 *fd_type, > const char **buf, u64 *probe_offset, > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 70fb11106fc2..754370e3155e 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -1609,6 +1609,7 @@ static int bpf_raw_tracepoint_release(struct inode = *inode, struct file *filp) > bpf_probe_unregister(raw_tp->btp, raw_tp->prog); > bpf_prog_put(raw_tp->prog); > } > + bpf_put_raw_tracepoint(raw_tp->btp); > kfree(raw_tp); > return 0; > } > @@ -1634,13 +1635,15 @@ static int bpf_raw_tracepoint_open(const union bp= f_attr *attr) > return -EFAULT; > tp_name[sizeof(tp_name) - 1] =3D 0; > =20 > - btp =3D bpf_find_raw_tracepoint(tp_name); > + btp =3D bpf_get_raw_tracepoint(tp_name); > if (!btp) > return -ENOENT; > =20 > raw_tp =3D kzalloc(sizeof(*raw_tp), GFP_USER); > - if (!raw_tp) > - return -ENOMEM; > + if (!raw_tp) { > + err =3D -ENOMEM; > + goto out_put_btp; > + } > raw_tp->btp =3D btp; > =20 > prog =3D bpf_prog_get_type(attr->raw_tracepoint.prog_fd, > @@ -1668,6 +1671,8 @@ static int bpf_raw_tracepoint_open(const union bpf_= attr *attr) > bpf_prog_put(prog); > out_free_tp: > kfree(raw_tp); > +out_put_btp: > + bpf_put_raw_tracepoint(btp); > return err; > } > =20 > diff --git a/kernel/module.c b/kernel/module.c > index 49a405891587..06ec68f08387 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -3093,6 +3093,11 @@ static int find_module_sections(struct module *mod= , struct load_info *info) > sizeof(*mod->tracepoints_ptrs), > &mod->num_tracepoints); > #endif > +#ifdef CONFIG_BPF_EVENTS > + mod->bpf_raw_events =3D section_objs(info, "__bpf_raw_tp_map", > + sizeof(*mod->bpf_raw_events), > + &mod->num_bpf_raw_events); > +#endif > #ifdef HAVE_JUMP_LABEL > mod->jump_entries =3D section_objs(info, "__jump_table", > sizeof(*mod->jump_entries), > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 9864a35c8bb5..9ddb6fddb4e0 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -17,6 +17,43 @@ > #include "trace_probe.h" > #include "trace.h" > =20 > +#ifdef CONFIG_MODULES > +struct bpf_trace_module { > + struct module *module; > + struct list_head list; > +}; > + > +static LIST_HEAD(bpf_trace_modules); > +static DEFINE_MUTEX(bpf_module_mutex); > + > +static struct bpf_raw_event_map *bpf_get_raw_tracepoint_module(const cha= r *name) > +{ > + struct bpf_raw_event_map *btp, *ret =3D NULL; > + struct bpf_trace_module *btm; > + unsigned int i; > + > + mutex_lock(&bpf_module_mutex); > + list_for_each_entry(btm, &bpf_trace_modules, list) { > + for (i =3D 0; i < btm->module->num_bpf_raw_events; ++i) { > + btp =3D &btm->module->bpf_raw_events[i]; > + if (!strcmp(btp->tp->name, name)) { > + if (try_module_get(btm->module)) > + ret =3D btp; > + goto out; > + } > + } > + } > +out: > + mutex_unlock(&bpf_module_mutex); > + return ret; > +} > +#else > +static struct bpf_raw_event_map *bpf_get_raw_tracepoint_module(const cha= r *name) > +{ > + return NULL; > +} > +#endif /* CONFIG_MODULES */ > + > u64 bpf_get_stackid(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5); > u64 bpf_get_stack(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5); > =20 > @@ -1076,7 +1113,7 @@ int perf_event_query_prog_array(struct perf_event *= event, void __user *info) > extern struct bpf_raw_event_map __start__bpf_raw_tp[]; > extern struct bpf_raw_event_map __stop__bpf_raw_tp[]; > =20 > -struct bpf_raw_event_map *bpf_find_raw_tracepoint(const char *name) > +struct bpf_raw_event_map *bpf_get_raw_tracepoint(const char *name) > { > struct bpf_raw_event_map *btp =3D __start__bpf_raw_tp; > =20 > @@ -1084,7 +1121,16 @@ struct bpf_raw_event_map *bpf_find_raw_tracepoint(= const char *name) > if (!strcmp(btp->tp->name, name)) > return btp; > } > - return NULL; > + > + return bpf_get_raw_tracepoint_module(name); > +} > + > +void bpf_put_raw_tracepoint(struct bpf_raw_event_map *btp) > +{ > + struct module *mod =3D __module_address((unsigned long)btp); > + > + if (mod) > + module_put(mod); > } > =20 > static __always_inline > @@ -1222,3 +1268,52 @@ int bpf_get_perf_event_info(const struct perf_even= t *event, u32 *prog_id, > =20 > return err; > } > + > +#ifdef CONFIG_MODULES > +int bpf_event_notify(struct notifier_block *nb, unsigned long op, void *= module) > +{ > + struct bpf_trace_module *btm, *tmp; > + struct module *mod =3D module; > + > + if (mod->num_bpf_raw_events =3D=3D 0 || > + (op !=3D MODULE_STATE_COMING && op !=3D MODULE_STATE_GOING)) > + return 0; > + > + mutex_lock(&bpf_module_mutex); > + > + switch (op) { > + case MODULE_STATE_COMING: > + btm =3D kzalloc(sizeof(*btm), GFP_KERNEL); > + if (btm) { > + btm->module =3D module; > + list_add(&btm->list, &bpf_trace_modules); > + } Is it fine to return 0 on !btm case? Other looks good. > + break; > + case MODULE_STATE_GOING: > + list_for_each_entry_safe(btm, tmp, &bpf_trace_modules, list) { > + if (btm->module =3D=3D module) { > + list_del(&btm->list); > + kfree(btm); > + break; > + } > + } > + break; > + } > + > + mutex_unlock(&bpf_module_mutex); > + > + return 0; > +} > + > +static struct notifier_block bpf_module_nb =3D { > + .notifier_call =3D bpf_event_notify, > +}; > + > +int __init bpf_event_init(void) > +{ > + register_module_notifier(&bpf_module_nb); > + return 0; > +} > + > +fs_initcall(bpf_event_init); > +#endif /* CONFIG_MODULES */ > --=20 > 2.17.1 >=20