Message-ID: <baafb22b-0dbc-bdcf-c692-c924d9d9671b@fb.com>
Date:   Thu, 26 Sep 2019 15:51:48 +0000
From:   Yonghong Song <yhs@...com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        Carlos Neira <cneirabustos@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "brouer@...hat.com" <brouer@...hat.com>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>
Subject: Re: [PATCH V11 0/4] BPF: New helper to obtain namespace data from
 current task



On 9/25/19 5:59 PM, Eric W. Biederman wrote:
> Carlos Neira <cneirabustos@...il.com> writes:
> 
>> Currently bpf_get_current_pid_tgid(), is used to do pid filtering in bcc's
>> scripts but this helper returns the pid as seen by the root namespace which is
>> fine when a bcc script is not executed inside a container.
>> When the process of interest is inside a container, pid filtering will not work
>> if bpf_get_current_pid_tgid() is used.
>> This helper addresses this limitation returning the pid as it's seen by the current
>> namespace where the script is executing.
>>
>> In the future different pid_ns files may belong to different devices, according to the
>> discussion between Eric Biederman and Yonghong in 2017 Linux plumbers conference.
>> To address that situation the helper requires inum and dev_t from /proc/self/ns/pid.
>> This helper has the same use cases as bpf_get_current_pid_tgid() as it can be
>> used to do pid filtering even inside a container.
> 
> I think I may have asked this before.  If I am repeating old gound
> please excuse me.
> 
> Am I correct in understanding these new helpers are designed to be used
> when programs running in ``conainers'' call it inside pid namespaces
> register bpf programs for tracing?

Right.

> 
> If so would it be possible to change how the existing bpf opcodes
> operate when they are used in the context of a pid namespace?


Today, typical bpf program getting pid like:
    uint64_t pid_tgid = bpf_get_current_pid_tgid();
    pid_t pid = pid_tgid >> 32;
    pid_t tid = pid_tgid;

    /* possible filtering ... */
    if (pid == <user_provided pid>) ....
    ...

    /* record pid in some places */
    map_val->pid = pid;
    ...

The bpf_get_current_pid_tgid() is a kernel helper
    BPF_CALL_0(bpf_get_current_pid_tgid)
    {
         struct task_struct *task = current;

         if (unlikely(!task))
                 return -EINVAL;

         return (u64) task->tgid << 32 | task->pid;
    }

So the bpf_get_current_pid_tgid() gets the tgid/pid outside any
pid namespaces.

To make the program work inside the container, just get namespace
pid/tgid not enough. You need to make sure the namespace you are
tracking is the one you are in. That is what the new proposed
helper to do.

Do you suggest we change
    bpf_get_current_pid_tgid()
to return namespaced tgid/pid?
First, this will break user API (kernel helper is an API) and second,
even if we do get pid/tgid, we still not sure whether
this is for my namespace or not.

Do you have something in mind to address this issue?

> 
> That later would seem to allow just moving an existing application into
> a pid namespace with no modifications.   If we can do this with trivial
> cost at bpf compile time and with no userspace changes that would seem
> a better approach.
> 
> If not can someone point me to why we can't do that?  What am I missing?
> 
> Eric
> 
>> Signed-off-by: Carlos Neira <cneirabustos@...il.com>
>>
>> Carlos Neira (4):
>>    fs/nsfs.c: added ns_match
>>    bpf: added new helper bpf_get_ns_current_pid_tgid
>>    tools: Added bpf_get_ns_current_pid_tgid helper
>>    tools/testing/selftests/bpf: Add self-tests for new helper. self tests
>>      added for new helper
>>
>>   fs/nsfs.c                                     |   8 +
>>   include/linux/bpf.h                           |   1 +
>>   include/linux/proc_ns.h                       |   2 +
>>   include/uapi/linux/bpf.h                      |  18 ++-
>>   kernel/bpf/core.c                             |   1 +
>>   kernel/bpf/helpers.c                          |  32 ++++
>>   kernel/trace/bpf_trace.c                      |   2 +
>>   tools/include/uapi/linux/bpf.h                |  18 ++-
>>   tools/testing/selftests/bpf/Makefile          |   2 +-
>>   tools/testing/selftests/bpf/bpf_helpers.h     |   3 +
>>   .../selftests/bpf/progs/test_pidns_kern.c     |  71 ++++++++
>>   tools/testing/selftests/bpf/test_pidns.c      | 152 ++++++++++++++++++
>>   12 files changed, 307 insertions(+), 3 deletions(-)
>>   create mode 100644 tools/testing/selftests/bpf/progs/test_pidns_kern.c
>>   create mode 100644 tools/testing/selftests/bpf/test_pidns.c

Powered by blists - more mailing lists