On Tue, Nov 7, 2017 at 1:31 PM, Atish Patra <atish.pa...@oracle.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....@linux.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@localhost 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
>>>
>>>
>>
>
