lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <0BCC6E9C-90E1-4B56-8829-12D180520D71@fb.com> Date: Wed, 30 Mar 2022 21:11:13 +0000 From: Song Liu <songliubraving@...com> To: Andrii Nakryiko <andrii.nakryiko@...il.com> CC: Valentin Schneider <valentin.schneider@....com>, Steven Rostedt <rostedt@...dmis.org>, Song Liu <song@...nel.org>, bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, Kernel Team <Kernel-team@...com> Subject: Re: [PATCH bpf] tools/runqslower: fix handle__sched_switch for updated tp sched_switch > On Mar 29, 2022, at 11:43 PM, Song Liu <songliubraving@...com> wrote: > > > >> On Mar 29, 2022, at 7:47 PM, Andrii Nakryiko <andrii.nakryiko@...il.com> wrote: >> >> On Tue, Mar 29, 2022 at 5:39 PM Song Liu <songliubraving@...com> wrote: >>> >>> >>> >>>> On Mar 29, 2022, at 5:00 PM, Andrii Nakryiko <andrii.nakryiko@...il.com> wrote: >>>> >>>> On Tue, Mar 29, 2022 at 4:19 PM Song Liu <song@...nel.org> wrote: >>>>> >>>>> TP_PROTO of sched_switch is updated with a new arg prev_state, which >>>>> causes runqslower load failure: >>>>> >>>>> libbpf: prog 'handle__sched_switch': BPF program load failed: Permission denied >>>>> libbpf: prog 'handle__sched_switch': -- BEGIN PROG LOAD LOG -- >>>>> R1 type=ctx expected=fp >>>>> 0: R1=ctx(off=0,imm=0) R10=fp0 >>>>> ; int handle__sched_switch(u64 *ctx) >>>>> 0: (bf) r7 = r1 ; R1=ctx(off=0,imm=0) R7_w=ctx(off=0,imm=0) >>>>> ; struct task_struct *next = (struct task_struct *)ctx[2]; >>>>> 1: (79) r6 = *(u64 *)(r7 +16) >>>>> func 'sched_switch' arg2 has btf_id 186 type STRUCT 'task_struct' >>>>> 2: R6_w=ptr_task_struct(off=0,imm=0) R7_w=ctx(off=0,imm=0) >>>>> ; struct task_struct *prev = (struct task_struct *)ctx[1]; >>>>> 2: (79) r2 = *(u64 *)(r7 +8) ; R2_w=scalar() R7_w=ctx(off=0,imm=0) >>>>> 3: (b7) r1 = 0 ; R1_w=0 >>>>> ; struct runq_event event = {}; >>>>> 4: (7b) *(u64 *)(r10 -8) = r1 ; R1_w=P0 R10=fp0 fp-8_w=00000000 >>>>> 5: (7b) *(u64 *)(r10 -16) = r1 ; R1_w=P0 R10=fp0 fp-16_w=00000000 >>>>> 6: (7b) *(u64 *)(r10 -24) = r1 ; R1_w=P0 R10=fp0 fp-24_w=00000000 >>>>> 7: (7b) *(u64 *)(r10 -32) = r1 ; R1_w=P0 R10=fp0 fp-32_w=00000000 >>>>> ; if (prev->__state == TASK_RUNNING) >>>>> 8: (61) r1 = *(u32 *)(r2 +24) >>>>> R2 invalid mem access 'scalar' >>>>> processed 9 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0 >>>>> -- END PROG LOAD LOG -- >>>>> libbpf: failed to load program 'handle__sched_switch' >>>>> libbpf: failed to load object 'runqslower_bpf' >>>>> libbpf: failed to load BPF skeleton 'runqslower_bpf': -13 >>>>> failed to load BPF object: -13 >>>>> >>>>> Update runqslower to fix this issue. Also, as we are on this, use BPF_PROG >>>>> in runqslower for cleaner code. >>>>> >>>>> Fixes: fa2c3254d7cf ("sched/tracing: Don't re-read p->state when emitting sched_switch event") >>>>> Signed-off-by: Song Liu <song@...nel.org> >>>>> --- >>>>> tools/bpf/runqslower/runqslower.bpf.c | 19 +++++-------------- >>>>> 1 file changed, 5 insertions(+), 14 deletions(-) >>>>> >>>> >>>> It would be much less disruptive if that prev_state was added after >>>> "next", but oh well... >>> >>> Maybe we should change that. >>> >>> +Valentin and Steven, how about we change the order with the attached >>> diff (not the original patch in this thread, but the one at the end of >>> this email)? >>> >>>> >>>> But anyways, let's handle this in a way that can handle both old >>>> kernels and new ones and do the same change in libbpf-tool's >>>> runqslower ([0]). Can you please follow up there as well? >>> >>> Yeah, I will also fix that one. >> >> Thanks! >> >>> >>>> >>>> >>>> We can use BPF CO-RE to detect which order of arguments running kernel >>>> has by checking prev_state field existence in struct >>>> trace_event_raw_sched_switch. Can you please try that? Use >>>> bpf_core_field_exists() for that. >>> >>> Do you mean something like >>> >>> if (bpf_core_field_exists(ctx->prev_state)) >>> /* use ctx[2] and ctx[3] */ >>> else >>> /* use ctx[1] and ctx[2] */ >> >> yep, that's what I meant, except you don't have ctx->prev_state, you have to do: >> >> if (bpf_core_field_exists(((struct trace_event_raw_sched_switch >> *)0)->prev_state)) Actually, struct trace_event_raw_sched_switch is not the arguments we have for tp_btf. For both older and newer kernel, it is the same: struct trace_event_raw_sched_switch { struct trace_entry ent; char prev_comm[16]; pid_t prev_pid; int prev_prio; long int prev_state; char next_comm[16]; pid_t next_pid; int next_prio; char __data[0]; }; So I guess this check won't work? Thanks, Song
Powered by blists - more mailing lists