On 10/23/19 7:42 AM, Carlos Antonio Neira Bustos wrote:
> On Wed, Oct 23, 2019 at 03:02:51AM +0000, Yonghong Song wrote:
>>
>>
>> On 10/22/19 12:17 PM, Carlos Neira wrote:
>>> Self tests added for new helper
>>
>> Please mention the name of the new helper in the commit message.
>>
>>>
>>> Signed-off-by: Carlos Neira <cneirabustos@xxxxxxxxx>
>>
>> LGTM Ack with a few nits below.
>> Acked-by: Yonghong Song <yhs@xxxxxx>
>>
>>> ---
>>> .../bpf/prog_tests/ns_current_pid_tgid.c | 87 +++++++++++++++++++
>>> .../bpf/progs/test_ns_current_pid_tgid.c | 37 ++++++++
>>> 2 files changed, 124 insertions(+)
>>> create mode 100644 tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
>>> create mode 100644 tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c
>>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c b/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
>>> new file mode 100644
>>> index 000000000000..257f18999bb6
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
>>> @@ -0,0 +1,87 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/* Copyright (c) 2019 Carlos Neira cneirabustos@xxxxxxxxx */
>>> +#include <test_progs.h>
>>> +#include <sys/stat.h>
>>> +#include <sys/types.h>
>>> +#include <unistd.h>
>>> +#include <sys/syscall.h>
>>> +
>>> +struct bss {
>>> + __u64 dev;
>>> + __u64 ino;
>>> + __u64 pid_tgid;
>>> + __u64 user_pid_tgid;
>>> +};
>>> +
>>> +void test_ns_current_pid_tgid(void)
>>> +{
>>> + const char *probe_name = "raw_tracepoint/sys_enter";
>>> + const char *file = "test_ns_current_pid_tgid.o";
>>> + int err, key = 0, duration = 0;
>>> + struct bpf_link *link = NULL;
>>> + struct bpf_program *prog;
>>> + struct bpf_map *bss_map;
>>> + struct bpf_object *obj;
>>> + struct bss bss;
>>> + struct stat st;
>>> + __u64 id;
>>> +
>>> + obj = bpf_object__open_file(file, NULL);
>>> + if (CHECK(IS_ERR(obj), "obj_open", "err %ld\n", PTR_ERR(obj)))
>>> + return;
>>> +
>>> + err = bpf_object__load(obj);
>>> + if (CHECK(err, "obj_load", "err %d errno %d\n", err, errno))
>>> + goto cleanup;
>>> +
>>> + bss_map = bpf_object__find_map_by_name(obj, "test_ns_.bss");
>>> + if (CHECK(!bss_map, "find_bss_map", "failed\n"))
>>> + goto cleanup;
>>> +
>>> + prog = bpf_object__find_program_by_title(obj, probe_name);
>>> + if (CHECK(!prog, "find_prog", "prog '%s' not found\n",
>>> + probe_name))
>>> + goto cleanup;
>>> +
>>> + memset(&bss, 0, sizeof(bss));
>>> + pid_t tid = syscall(SYS_gettid);
>>> + pid_t pid = getpid();
>>> +
>>> + id = (__u64) tid << 32 | pid;
>>> + bss.user_pid_tgid = id;
>>> +
>>> + if (CHECK_FAIL(stat("/proc/self/ns/pid", &st))) {
>>> + perror("Failed to stat /proc/self/ns/pid");
>>> + goto cleanup;
>>> + }
>>> +
>>> + bss.dev = st.st_dev;
>>> + bss.ino = st.st_ino;
>>> +
>>> + err = bpf_map_update_elem(bpf_map__fd(bss_map), &key, &bss, 0);
>>> + if (CHECK(err, "setting_bss", "failed to set bss : %d\n", err))
>>> + goto cleanup;
>>> +
>>> + link = bpf_program__attach_raw_tracepoint(prog, "sys_enter");
>>> + if (CHECK(IS_ERR(link), "attach_raw_tp", "err %ld\n",
>>> + PTR_ERR(link)))
>>> + goto cleanup;
>>
>> You already have default link = NULL.
>> Here, I think you can do
>> link = NULL;
>> goto cleanup;
>>
>>> +
>>> + /* trigger some syscalls */
>>> + usleep(1);
>>> +
>>> + err = bpf_map_lookup_elem(bpf_map__fd(bss_map), &key, &bss);
>>> + if (CHECK(err, "set_bss", "failed to get bss : %d\n", err))
>>> + goto cleanup;
>>> +
>>> + if (CHECK(id != bss.pid_tgid, "Compare user pid/tgid vs. bpf pid/tgid",
>>> + "User pid/tgid %llu EBPF pid/tgid %llu\n", id, bss.pid_tgid))
>>
>> EBPF -> BPF?
>>
>>> + goto cleanup;
>>> +cleanup:
>>> +
>>
>> The above empty line can be removed.
>>
>>> + if (!IS_ERR_OR_NULL(link)) {
>>
>> With the above suggested change, you only need to check
>> if (!link)
>>
>>> + bpf_link__destroy(link);
>>> + link = NULL;
>>> + }
>>> + bpf_object__close(obj);
>>> +}
>>> diff --git a/tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c b/tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c
>>> new file mode 100644
>>> index 000000000000..cdb77eb1a4fb
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c
>>> @@ -0,0 +1,37 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/* Copyright (c) 2019 Carlos Neira cneirabustos@xxxxxxxxx */
>>> +
>>> +#include <linux/bpf.h>
>>> +#include <stdint.h>
>>> +#include "bpf_helpers.h"
>>> +
>>> +static volatile struct {
>>> + __u64 dev;
>>> + __u64 ino;
>>> + __u64 pid_tgid;
>>> + __u64 user_pid_tgid;
>>> +} res;
>>> +
>>> +SEC("raw_tracepoint/sys_enter")
>>> +int trace(void *ctx)
>>> +{
>>> + __u64 ns_pid_tgid, expected_pid;
>>> + struct bpf_pidns_info nsdata;
>>> + __u32 key = 0;
>>> +
>>> + if (bpf_get_ns_current_pid_tgid(res.dev, res.ino, &nsdata,
>>> + sizeof(struct bpf_pidns_info)))
>>> + return 0;
>>> +
>>> + ns_pid_tgid = (__u64)nsdata.tgid << 32 | nsdata.pid;
>>> + expected_pid = res.user_pid_tgid;
>>> +
>>> + if (expected_pid != ns_pid_tgid)
>>> + return 0;
>>> +
>>> + res.pid_tgid = ns_pid_tgid;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +char _license[] SEC("license") = "GPL";
>>
>> The new helper does not require GPL, could you double check this?
>> The above _license should not be necessary.
>
> Thanks, Yonghong.
>
> Do I need to re-send the series of patches as v16 ? or I could reply to this thread addressing your comments for patch 4/5.
You can wait for Eric's ACK and then resend a new version of the patch
set with all Ack's.
> Thanks again for your support.
>
> Bests
>
