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
| ||
|
Date: Thu, 28 Apr 2022 11:02:35 +0100 From: Qais Yousef <qais.yousef@....com> To: Andrii Nakryiko <andrii.nakryiko@...il.com> Cc: Peter Zijlstra <peterz@...radead.org>, Alexei Starovoitov <alexei.starovoitov@...il.com>, Delyan Kratunov <delyank@...com>, Namhyung Kim <namhyung@...nel.org>, Arnaldo Carvalho de Melo <acme@...nel.org>, "bigeasy@...utronix.de" <bigeasy@...utronix.de>, "dietmar.eggemann@....com" <dietmar.eggemann@....com>, "keescook@...omium.org" <keescook@...omium.org>, "x86@...nel.org" <x86@...nel.org>, "andrii@...nel.org" <andrii@...nel.org>, "u.kleine-koenig@...gutronix.de" <u.kleine-koenig@...gutronix.de>, "vincent.guittot@...aro.org" <vincent.guittot@...aro.org>, "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>, "mingo@...nel.org" <mingo@...nel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "rdunlap@...radead.org" <rdunlap@...radead.org>, "rostedt@...dmis.org" <rostedt@...dmis.org>, "Kenta.Tada@...y.com" <Kenta.Tada@...y.com>, "tglx@...utronix.de" <tglx@...utronix.de>, "bristot@...hat.com" <bristot@...hat.com>, "ebiederm@...ssion.com" <ebiederm@...ssion.com>, "ast@...nel.org" <ast@...nel.org>, "legion@...nel.org" <legion@...nel.org>, "adharmap@...cinc.com" <adharmap@...cinc.com>, "valentin.schneider@....com" <valentin.schneider@....com>, "ed.tsai@...iatek.com" <ed.tsai@...iatek.com>, "juri.lelli@...hat.com" <juri.lelli@...hat.com> Subject: Re: [PATCH] sched/tracing: append prev_state to tp args instead On 04/27/22 11:17, Andrii Nakryiko wrote: > On Wed, Apr 27, 2022 at 3:35 AM Qais Yousef <qais.yousef@....com> wrote: > > > > On 04/26/22 08:54, Andrii Nakryiko wrote: > > > On Tue, Apr 26, 2022 at 7:10 AM Qais Yousef <qais.yousef@....com> wrote: > > > > > > > > On 04/26/22 14:28, Peter Zijlstra wrote: > > > > > On Fri, Apr 22, 2022 at 11:30:12AM -0700, Alexei Starovoitov wrote: > > > > > > On Fri, Apr 22, 2022 at 10:22 AM Delyan Kratunov <delyank@...com> wrote: > > > > > > > > > > > > > > On Fri, 2022-04-22 at 13:09 +0200, Peter Zijlstra wrote: > > > > > > > > And on the other hand; those users need to be fixed anyway, right? > > > > > > > > Accessing prev->__state is equally broken. > > > > > > > > > > > > > > The users that access prev->__state would most likely have to be fixed, for sure. > > > > > > > > > > > > > > However, not all users access prev->__state. `offcputime` for example just takes a > > > > > > > stack trace and associates it with the switched out task. This kind of user > > > > > > > would continue working with the proposed patch. > > > > > > > > > > > > > > > If bpf wants to ride on them, it needs to suffer the pain of doing so. > > > > > > > > > > > > > > Sure, I'm just advocating for a fairly trivial patch to avoid some of the suffering, > > > > > > > hopefully without being a burden to development. If that's not the case, then it's a > > > > > > > clear no-go. > > > > > > > > > > > > > > > > > > Namhyung just sent this patch set: > > > > > > https://patchwork.kernel.org/project/netdevbpf/patch/20220422053401.208207-3-namhyung@kernel.org/ > > > > > > > > > > That has: > > > > > > > > > > + * recently task_struct->state renamed to __state so it made an incompatible > > > > > + * change. > > > > > > > > > > git tells me: > > > > > > > > > > 2f064a59a11f ("sched: Change task_struct::state") > > > > > > > > > > is almost a year old by now. That don't qualify as recently in my book. > > > > > That says that 'old kernels used to call this...'. > > > > > > > > > > > to add off-cpu profiling to perf. > > > > > > It also hooks into sched_switch tracepoint. > > > > > > Notice it deals with state->__state rename just fine. > > > > > > > > > > So I don't speak BPF much; it always takes me more time to make bpf work > > > > > than to just hack up the kernel, which makes it hard to get motivated. > > > > > > > > > > However, it was not just a rename, state changed type too, which is why I > > > > > did the rename, to make sure all users would get a compile fail and > > > > > could adjust. > > > > > > > > > > If you're silently making it work by frobbing the name, you loose that. > > > > > > > > > > Specifically, task_struct::state used to be 'volatile long', while > > > > > task_struct::__state is 'unsigned int'. As such, any user must now be > > > > > very careful to use READ_ONCE(). I don't see that happening with just > > > > > frobbing the name. > > > > > > > > > > Additinoally, by shrinking the field, I suppose BE systems get to keep > > > > > the pieces? > > > > > > > > > > > But it will have a hard time without this patch > > > > > > until we add all the extra CO-RE features to detect > > > > > > and automatically adjust bpf progs when tracepoint > > > > > > arguments order changed. > > > > > > > > > > Could be me, but silently making it work sounds like fail :/ There's a > > > > > reason code changes, users need to adapt, not silently pretend stuff is > > > > > as before. > > > > > > > > > > How will you know you need to fix your tool? > > > > > > > > If libbpf doesn't fail, then yeah it's a big problem. I wonder how users of > > > > kprobe who I suppose are more prone to this kind of problems have been coping. > > > > > > See my reply to Peter. libbpf can't know user's intent to fail this > > > automatically, in general. In some cases when it can it does > > > accommodate this automatically. In other cases it provides instruments > > > for user to handle this (bpf_core_field_size(), > > > BPF_CORE_READ_BITFIELD(), etc). > > > > My naiive thinking is that the function signature has changed (there's 1 extra > > arg not just a subtle swap of args of the same type) - so I thought that can be > > detected. But maybe it is harder said than done. > > It is. We don't have number of arguments either: > > struct bpf_raw_tracepoint_args { > __u64 args[0]; > }; > > What BPF program is getting is just an array of u64s. > > > > > I am trying to remember as I've used this before; I think you get the arg list > > as part of ctx when you attach to a function? > > > > I wonder if it'd be hard to provide a macro for the user to provide the > > signature of the function they expect; this macro can try then to verify/assert > > the number, type and order is the same. Not bullet proof and requires opt-in, > > but could be useful? > > > > > > // dummy pseudo-code > > > > BPF_CORE_ASSERT_SIG(sched_switch, NR_ARGS, ARG0, ARG1, ...) > > if (ctx->nr_args != NR_ARGS) > > assert() > > if (type_of(ctx->args[0]) != type_of(ARG0)) > > assert() > > ... > > > > I'm not sure if you have any info about the type though.. > > What we have now under discussion is more generic way for user to > check signature of function prototype, struct/union, etc. But all that > will take some time to implement and finalize. So this patch is a way > to stop/prevent the bleeding until we have that available to users. Okay good to know. Alexei mentioned a plan, but I didn't get that it included signature verification. Cheers -- Qais Yousef
Powered by blists - more mailing lists