netdev - Re: [PATCH] tools/runqslower: use _

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 6 Jul 2021 21:03:29 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Song Liu <song@...nel.org>
Cc:     Yonghong Song <yhs@...com>, SanjayKumar J <vjsanjay@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, Martin Lau <kafai@...com>,
        Song Liu <songliubraving@...com>,
        john fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH] tools/runqslower: use __state instead of state

On Tue, Jul 6, 2021 at 4:11 PM Song Liu <song@...nel.org> wrote:
>
> On Tue, Jul 6, 2021 at 3:05 PM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> >
> > On Tue, Jul 6, 2021 at 11:26 AM Yonghong Song <yhs@...com> wrote:
> > >
> > >
> > >
> > > On 7/6/21 10:44 AM, SanjayKumar J wrote:
> > > >       task->state is renamed to task->__state in task_struct
> > >
> > > Could you add a reference to
> > >    2f064a59a11f ("sched: Change task_struct::state")
> > > which added this change?
> > >
> > > I think this should go to bpf tree as the change is in linus tree now.
> > > Could you annotate the tag as "[PATCH bpf]" ("[PATCH bpf v2]")?
> > >
> > > Please align comments to the left without margins.
> > >
> > > >
> > > >       Signed-off-by: SanjayKumar J <vjsanjay@...il.com>
> > >
> > > This Singed-off-by is not needed.
> > >
> > > You can add my Ack in the next revision:
> > > Acked-by: Yonghong Song <yhs@...com>
> > >
> > > >
> > > > Signed-off-by: SanjayKumar J <vjsanjay@...il.com>
> > > > ---
> > > >   tools/bpf/runqslower/runqslower.bpf.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/bpf/runqslower/runqslower.bpf.c b/tools/bpf/runqslower/runqslower.bpf.c
> > > > index 645530ca7e98..ab9353f2fd46 100644
> > > > --- a/tools/bpf/runqslower/runqslower.bpf.c
> > > > +++ b/tools/bpf/runqslower/runqslower.bpf.c
> > > > @@ -74,7 +74,7 @@ int handle__sched_switch(u64 *ctx)
> > > >       u32 pid;
> > > >
> > > >       /* ivcsw: treat like an enqueue event and store timestamp */
> > > > -     if (prev->state == TASK_RUNNING)
> > > > +     if (prev->__state == TASK_RUNNING)
> > >
> > > Currently, runqslower.bpf.c uses vmlinux.h.
> > > I am thinking to use bpf_core_field_exists(), but we need to
> > > single out task_struct structure from vmlinux.h
> > > with both state and __state fields, we could make it work
> > > by *changes* like
> > >
> > > #define task_struct task_struct_orig
> > > #include "vmlinux.h"
> > > #undef task_struct
> > >
> > > struct task_struct {
> > >     ... state;
> > >     ... __state;
> > > ...
> > > };
> >
> >
> > no need for such surgery, recommended way is to use ___suffix to
> > declare incompatible struct definition:
> >
> > struct task_struct___old {
> >     int state;
> > };
> >
> > Then do casting in BPF code. We don't have to do it in kernel tree's
> > runqslower, but we'll definitely have to do that for libbpf-tools'
> > runqslower and runqlat.
>
> Question on this topic: state and __state are of different sizes here. IIUC,
> bpf_core_types_are_compat() does allow size mismatch. But it may cause
> problems in some cases, no? For example, would some combination make
> task->state return 32 extra bits from another field and cause confusion?

In this case it's two different fields, long state vs int __state, so
there is no confusion, you'd be using either one or another. But even
if it was the same field and its type changed from long to int, libbpf
will still try to accommodate that. Worst case,
BPF_CORE_READ_BITFIELD() is able to read any bitfield or integer
field, regardless of its size.

>
> Thanks,
> Song