bcc: Use bpf_probe_read_user in tools and provide backward compatibility

[sumanthkorikkar]

Hi @yonghong-song, @brendangregg, @4ast, all

[RFC 0/1] Support bpf_probe_read_user in bcc tools

bcc tracks the function arg pointers and then converts the pointer
deferences (both user and kernel pointer) into the bpf_probe_read().
However this is not valid for architecture which has overlapping address
space. To overcome this issue, the bpf_probe_read_{user/kernel} variant
was introduced.

Few Feasible solution for separation of user probe read and the kernel
probe read:

Approach 1 (temporary approach):

1. Explicitly use bpf_probe_read_user() for copying the user-space var
   into the ebpf stack. Like kernel/bpf/samples/.

• Make changes in the tools and examples.
• For backward compatibility, when kernel version < 5.5, convert
  bpf_probe_read_user call to bpf_probe_read(). Atleast the tools are
  valid for architecture which supports it.

Approach 2
2. Mark certain pointer as user and then track it while visiting AST
nodes in cases like return, callexp, binop, assignment etc.. I am yet
to try this and see how this can be performed

Let me know, your suggestions for approach 1, approach 2 or any other good approaches. Thank you

The first approach rfc is added below.

Best Regards
Sumanth

[yonghong-song]

Thanks! The patch looks good. I think approach 1 is practical for now. approach 2 is more involved. In kernel, __user is represented as address_space attribute and clang has pretty strict support for address_space attribute. I have a pending patch https://reviews.llvm.org/D69393 in clang to get better support for kernel with turning these __user marking etc. for compiler, no progress yet as it needs quite a lot of compiler work. Please comment if you have some thoughts.

[sumanthkorikkar]

Thanks! The patch looks good. I think approach 1 is practical for now. approach 2 is more involved. In kernel, __user is represented as address_space attribute and clang has pretty strict support for address_space attribute. I have a pending patch https://reviews.llvm.org/D69393 in clang to get better support for kernel with turning these __user marking etc. for compiler, no progress yet as it needs quite a lot of compiler work. Please comment if you have some thoughts.

Hi @yonghong-song ,

Thank you. For now, I will take the first approach and make necessary changes in the tools examples and send it.

Question - 2nd approach:

1. Could we do something like the following ?:

• Add the clang attribute __userptr, which is just a marker.
  • bcc program marks certain function arg pointer as __userptr.
    • bcc should then track this func arg ptr state in various assignments .
  • We dont really depend upon kernel __user attribute.
  • May be we dont need debug info in this case, which can be an advantage
  • However care should be taken by the bcc programmer in marking those appropriately, which is a disadvantage

[yonghong-song]

Thanks! The patch looks good. I think approach 1 is practical for now. approach 2 is more involved. In kernel, __user is represented as address_space attribute and clang has pretty strict support for address_space attribute. I have a pending patch https://reviews.llvm.org/D69393 in clang to get better support for kernel with turning these __user marking etc. for compiler, no progress yet as it needs quite a lot of compiler work. Please comment if you have some thoughts.

Hi @yonghong-song ,

Thank you. For now, I will take the first approach and make necessary changes in the tools examples and send it.

Question - 2nd approach:

1. Could we do something like the following ?:


* Add the clang attribute __userptr, which is just a marker.
  
  * bcc program marks certain function arg pointer as __userptr.
    
    * bcc should then track this func arg ptr state in various assignments .
  * We dont really depend upon kernel __user attribute.
  * May be we dont need debug info in this case, which can be an advantage
  * However care should be taken by the bcc programmer in marking those appropriately, which is a disadvantage

This should work. bcc could rewrite with proprer version of bpf_probe_read based on whether the attribute available or not. Not sure whether it is worthwhile or not since (1). we have a reasonable workaround (your apporach 1), (2). long term we still want clang can provide proper information with __user annotation, (3). we only limited cases (a few syscall's or a few calls close to syscalls). But thanks for suggestion, will continue to monitor situation.

[sumanthkorikkar]

Ok. Thank you. I made changes to the tools using approach 1. Currently testing it and I will update it here.

[yonghong-song]

Could you rebase on top of master? In the diff, there are a lot of libbpf submodule changes.

[sumanthkorikkar]

1. Two more changes needed in trace.py and argdist.py. I will do it another commit.

• tools/argdist.py -C 'p::do_sys_open(int dfd, const char __user filename, int flags, umode_t mode):char:filename'
  • This should be converted to bpf_probe_read_user() while reading the strings. Same applies for every uprobes
• tools/argdist.py -C 'p::someinternalkernelfunc(char filename):char:filename'
  • This should be converted to bpf_probe_read() while reading the strings.

2. @yonghong-song , Another thing, Do I also need to make bpf_probe_read_user changes in tools/old/ ?

[sumanthkorikkar]

Could you rebase on top of master? In the diff, there are a lot of libbpf submodule changes.

Ok. Thank you

[yonghong-song]

1. Two more changes needed in trace.py and argdist.py. I will do it another commit.


* tools/argdist.py -C 'p::do_sys_open(int dfd, const char __user _filename, int flags, umode_t mode):char_:filename'
  
  * This should be converted to bpf_probe_read_user() while reading the strings. Same applies  for every uprobes

* tools/argdist.py -C 'p::someinternalkernelfunc(char _filename):char_:filename'
  
  * This should be converted to bpf_probe_read() while reading the strings.


1. @yonghong-song , Another thing, Do I also need to make bpf_probe_read_user changes in tools/old/ ?

for tools/old/ change, no. They are for old kernels which do not have bpf_probe_read_user() anyway. Thanks for the patch!

[yonghong-song]

[buildbot, test this please]

[yonghong-song]

[buildbot, ok to test]

[sumanthkorikkar]

I have also modified trace.py and argdist.py to support bpf_probe_read_user. I will create a separate pull request for trace.py/argdist.py soon after ample testing.

[yonghong-song]

I have also modified trace.py and argdist.py to support bpf_probe_read_user. I will create a separate pull request for trace.py/argdist.py soon after ample testing.

Thanks!