| Message ID | 20201119232244.2776720-2-andrii@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: add support for kernel module BTF CO-RE relocations | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 33 this patch: 33 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 12 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 33 this patch: 33 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, Nov 19, 2020 at 03:22:39PM -0800, Andrii Nakryiko wrote: > __module_address() needs to be called with preemption disabled or with > module_mutex taken. preempt_disable() is enough for read-only uses, which is > what this fix does. Acked-by: Martin KaFai Lau <kafai@fb.com>
On Thu, Nov 19, 2020 at 03:22:39PM -0800, Andrii Nakryiko wrote: > __module_address() needs to be called with preemption disabled or with > module_mutex taken. preempt_disable() is enough for read-only uses, which is > what this fix does. > > Fixes: a38d1107f937 ("bpf: support raw tracepoints in modules") > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- > kernel/trace/bpf_trace.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index d255bc9b2bfa..bb98a377050a 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -2060,7 +2060,11 @@ struct bpf_raw_event_map *bpf_get_raw_tracepoint(const char *name) > > void bpf_put_raw_tracepoint(struct bpf_raw_event_map *btp) > { > - struct module *mod = __module_address((unsigned long)btp); > + struct module *mod; > + > + preempt_disable(); > + mod = __module_address((unsigned long)btp); > + preempt_enable(); > > if (mod) > module_put(mod); I don't understand why 'mod' cannot become dangling pointer after preempt_enable(). Either it needs a comment explaining why it's ok or module_put() should be in preempt disabled section.
On Mon, Nov 23, 2020 at 9:49 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Nov 19, 2020 at 03:22:39PM -0800, Andrii Nakryiko wrote: > > __module_address() needs to be called with preemption disabled or with > > module_mutex taken. preempt_disable() is enough for read-only uses, which is > > what this fix does. > > > > Fixes: a38d1107f937 ("bpf: support raw tracepoints in modules") > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > --- > > kernel/trace/bpf_trace.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index d255bc9b2bfa..bb98a377050a 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -2060,7 +2060,11 @@ struct bpf_raw_event_map *bpf_get_raw_tracepoint(const char *name) > > > > void bpf_put_raw_tracepoint(struct bpf_raw_event_map *btp) > > { > > - struct module *mod = __module_address((unsigned long)btp); > > + struct module *mod; > > + > > + preempt_disable(); > > + mod = __module_address((unsigned long)btp); > > + preempt_enable(); > > > > if (mod) > > module_put(mod); > > I don't understand why 'mod' cannot become dangling pointer after preempt_enable(). > Either it needs a comment explaining why it's ok or module_put() should > be in preempt disabled section. Yeah, I think it can, assuming the kernel module can be unloaded despite non-zero refcnt (probably happens with force unload?). I'll drop the `if (mod)` part (module_put() checks that internally) and will move module_put(mod) inside the preempt disable/enable region.
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index d255bc9b2bfa..bb98a377050a 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -2060,7 +2060,11 @@ struct bpf_raw_event_map *bpf_get_raw_tracepoint(const char *name) void bpf_put_raw_tracepoint(struct bpf_raw_event_map *btp) { - struct module *mod = __module_address((unsigned long)btp); + struct module *mod; + + preempt_disable(); + mod = __module_address((unsigned long)btp); + preempt_enable(); if (mod) module_put(mod);
__module_address() needs to be called with preemption disabled or with module_mutex taken. preempt_disable() is enough for read-only uses, which is what this fix does. Fixes: a38d1107f937 ("bpf: support raw tracepoints in modules") Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- kernel/trace/bpf_trace.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)