Re: [RFC PATCH] bpf: Add helpers to read useful task_struct members

[Atish Patra]

On 11/07/2017 04:42 PM, Y Song wrote:
> On Tue, Nov 7, 2017 at 2:04 PM, Alexei Starovoitov <ast@xxxxxx> wrote:
> > On 11/8/17 6:47 AM, Y Song wrote:
> > > On Tue, Nov 7, 2017 at 1:39 PM, Alexei Starovoitov <ast@xxxxxx> wrote:
> > > > On 11/8/17 6:14 AM, Y Song wrote:
> > > > > On Tue, Nov 7, 2017 at 12:37 AM, Naveen N. Rao
> > > > > <naveen.n.rao@xxxxxxxxxxxxxxxxxx> wrote:
> > > > > > Alexei Starovoitov wrote:
> > > > > > > On 11/7/17 12:55 AM, Naveen N. Rao wrote:
> > > > > > > > > I thought such struct shouldn't change layout.
> > > > > > > > > If it is we need to fix include/linux/compiler-clang.h to do that
> > > > > > > > > anon struct as well.
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > We considered that, but it looked to be very dependent on the version
> > > > > > > > of
> > > > > > > > gcc used to build the kernel. But, this may be a simpler approach for
> > > > > > > > the shorter term.
> > > > > > >
> > > > > > > why it would depend on version of gcc?
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > From what I can see, randomized_struct_fields_start is defined only for
> > > > > > gcc
> > > > > > > = 4.6. For older versions, it does not get mapped to an anonymous
> > > > > >
> > > > > >
> > > > > > structure. We may not care for older gcc versions, but..
> > > > > >
> > > > > > The other issue was that __randomize_layout maps to __designated_init
> > > > > > when
> > > > > > randstruct plugin is not enabled, which is in turn an attribute on gcc
> > > > > > > =
> > > > > > v5.1, but not otherwise.
> > > > > >
> > > > > > > We just need this, no?
> > > > > > >
> > > > > > > diff --git a/include/linux/compiler-clang.h
> > > > > > > b/include/linux/compiler-clang.h
> > > > > > > index de179993e039..4e29ab6187cb 100644
> > > > > > > --- a/include/linux/compiler-clang.h
> > > > > > > +++ b/include/linux/compiler-clang.h
> > > > > > > @@ -15,3 +15,6 @@
> > > > > > >    * with any version that can compile the kernel
> > > > > > >    */
> > > > > > >   #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix),
> > > > > > > __COUNTER__)
> > > > > > > +
> > > > > > > +#define randomized_struct_fields_start struct {
> > > > > > > +#define randomized_struct_fields_end   };
> > > > > > >
> > > > > > > since offsets are mandated by C standard.
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > Yes, this is what we're testing with and is probably sufficient for our
> > > > > > purposes.
> > > > >
> > > > >
> > > > >
> > > > > Just tested this with bcc. bcc actually complains. the rewriter
> > > > > is not able to rewrite prev->pid where prev is "struct task_struct
> > > > > *prev".
> > > > > I will change bcc rewriter to see whether the field value is correct or
> > > > > not.
> > > > >
> > > > > Not sure my understanding is correct or not, but I am afraid that
> > > > > the above approach for clang compiler change may not work.
> > > > > If clang calculates the field offset based on header file, the offset
> > > > > may not be the same as kernel one....
> > > >
> > > >
> > > >
> > > > why is that?
> > > > When randomization is off both gcc and clang must generate the same
> > > > offsets, since it's C standard.
> > >
> > >
> > > The patch changed compiler-clang.h, so gcc still do randomization.
> >
> >
> > gcc_plugins are off by default and randomization will not be
> > turned on for any sane distro or datacenter that cares about
> > performance and stability.
> > So imo above compiler-clang.h patch together with bcc fix would
> > be enough.
>
> Agree that short time the suggested fix should be enough.
> Long time, disto could become "insane" someday :-)

Yup it works with clang compiler & bcc hack. Thanks :).
I verified it with task_switch.py & runqlat.py.

Are you going to push the bcc hacks to github repo ?

Regards,
Atish