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

On Tue, Nov 7, 2017 at 1:31 PM, Atish Patra [off-list ref] wrote:

On 11/07/2017 03:14 PM, Y Song wrote:

quoted

On Tue, Nov 7, 2017 at 12:37 AM, Naveen N. Rao
[off-list ref] wrote:

quoted

Alexei Starovoitov wrote:

quoted

On 11/7/17 12:55 AM, Naveen N. Rao wrote:

quoted

quoted

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

quoted

= 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.

quoted

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.

quoted

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

quoted

quoted

- Naveen