Date:   Tue, 7 Nov 2017 13:45:14 -0800
From:   Y Song <ys114321@...il.com>
To:     atish.patra@...cle.com
Cc:     "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>,
        Alexei Starovoitov <ast@...com>,
        netdev <netdev@...r.kernel.org>,
        Sandipan Das <sandipan@...ux.vnet.ibm.com>,
        Brendan Gregg <brendan.d.gregg@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Kees Cook <keescook@...omium.org>, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] bpf: Add helpers to read useful task_struct members

On Tue, Nov 7, 2017 at 1:31 PM, Atish Patra <atish.patra@...cle.com> wrote:
> On 11/07/2017 03:14 PM, Y Song wrote:
>>
>> On Tue, Nov 7, 2017 at 12:37 AM, Naveen N. Rao
>> <naveen.n.rao@...ux.vnet.ibm.com> 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.

Just verified in bcc with the following hack:
--- a/examples/tracing/task_switch.py
+++ b/examples/tracing/task_switch.py
@@ -20,7 +20,8 @@ int count_sched(struct pt_regs *ctx, struct
task_struct *prev) {
   u64 zero = 0, *val;

   key.curr_pid = bpf_get_current_pid_tgid();
-  key.prev_pid = prev->pid;
+  bpf_probe_read(&key.prev_pid, 4, &prev->pid);
+  // key.prev_pid = prev->pid;

   val = stats.lookup_or_init(&key, &zero);
   (*val)++;
diff --git a/src/cc/frontends/clang/b_frontend_action.cc
b/src/cc/frontends/clang/b_frontend_action.cc
index c11d89e..df48115 100644
--- a/src/cc/frontends/clang/b_frontend_action.cc
+++ b/src/cc/frontends/clang/b_frontend_action.cc
@@ -827,6 +827,7 @@ void
BTypeConsumer::HandleTranslationUnit(ASTContext &Context) {
    */
   for (it = DC->decls_begin(); it != DC->decls_end(); it++) {
     Decl *D = *it;
+#if 0
     if (FunctionDecl *F = dyn_cast<FunctionDecl>(D)) {
       if (fe_.is_rewritable_ext_func(F)) {
         for (auto arg : F->parameters()) {
@@ -836,6 +837,7 @@ void
BTypeConsumer::HandleTranslationUnit(ASTContext &Context) {
         probe_visitor_.TraverseDecl(D);
       }
     }
+#endif

Basically, explicitly using bpf_probe_read instead of letting rewriter
to change it.
Also disable probe rewriting part in the frontend.

I confirmed that value of prev->pid is always 0 (wrong).

The linux patch I have is:
diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index 54dfef70a072..5d9609ea8e30 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -16,3 +16,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   };

Therefore, getting bpf program to access randomized structure
through either BTF or fixed dwarfinfo may be the best option.
I know dwarfinfo is too big so smaller BTF is more desirable.

>>
>> 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....
>>
>> I verified that the drawf info with randomized structure config does not
>> match randomized structure member offset. Specifically, I tried
>> linux/proc_ns.h struct proc_ns_operations,
>> dwarf says:
>>   field name: offset 0
>>   field real_ns_name: offset 8
>> But if you print out the real offset at runtime, you get 40 and 16
>> respectively.
>>
> I am also trying to get it work with bcc as any python scripts that access
> task_struct gives wrong output (task_switch.py, runqlen.py). I recompiled my
> kernel (4.14-rc7 & bcc) with the patch.
>
> I am seeing following error.
> # ./task_switch.py
> /virtual/main.c:17:18: warning: implicit declaration of function
> 'bpf_get_task_pid_tgid' is invalid in C99
>       [-Wimplicit-function-declaration]
>   key.prev_pid = bpf_get_task_pid_tgid(prev);
>                  ^
> 1 warning generated.
> LLVM ERROR: Program used external function 'bpf_get_task_pid_tgid' which
> could not be resolved!
>
> Are you also seeing the same issue or something else ?

I did not apply the patch so I did not use bpf_get_task_pid_tgid helper.
When applying the above compiler-clang.h patch, I got:
[yhs@...alhost tracing]$ sudo ./task_switch.py
/virtual/main.c:16:18: error: internal error: opLoc is invalid while
preparing probe rewrite
  key.prev_pid = prev->pid;
                 ^
1 error generated.
Traceback (most recent call last):
  File "./task_switch.py", line 29, in <module>
    """)
  File "/usr/lib/python2.7/site-packages/bcc/__init__.py", line 296, in __init__
    raise Exception("Failed to compile BPF module %s" % src_file)
Exception: Failed to compile BPF module

I just hacked bcc with the above patch and the issue is going away.

>
> Regards,
> Atish
>>>
>>>
>>> - Naveen
>>>
>>>
>>
>

Powered by blists - more mailing lists