linux-kernel - Re: [PATCH bpf-next v2 1/4] selftests/bpf: Replaces the usage of CHECK calls for ASSERTs in bpf_tcp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 20 Nov 2023 22:45:27 +0530
From:   Yuran Pereira <yuran.pereira@...mail.com>
To:     Yonghong Song <yonghong.song@...ux.dev>
Cc:     bpf@...r.kernel.org, andrii@...nel.org, andrii.nakryiko@...il.com,
        mykolal@...com, ast@...nel.org, martin.lau@...ux.dev,
        song@...nel.org, john.fastabend@...il.com, kpsingh@...nel.org,
        sdf@...gle.com, haoluo@...gle.com, jolsa@...nel.org,
        shuah@...nel.org, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH bpf-next v2 1/4] selftests/bpf: Replaces the usage of
 CHECK calls for ASSERTs in bpf_tcp_ca

Hello Yonghong,
On Mon, Nov 20, 2023 at 07:22:59AM -0800, Yonghong Song wrote:
> > -		if (CHECK(!err || errno != ENOENT,
> > -			  "bpf_map_lookup_elem(sk_stg_map)",
> > -			  "err:%d errno:%d\n", err, errno))
> > +		if (!ASSERT_NEQ(err, 0, "bpf_map_lookup_elem(sk_stg_map)") ||
> 
> !ASSERT_ERR(err, "bpf_map_lookup_elem(sk_stg_map)")
> might be simpler than !ASSERT_NEQ(..).
> 
Sure, that makes sense. I'll change it in v3.
> > -	pthread_join(srv_thread, &thread_ret);
> > -	CHECK(IS_ERR(thread_ret), "pthread_join", "thread_ret:%ld",
> > -	      PTR_ERR(thread_ret));
> > +	err = pthread_join(srv_thread, &thread_ret);
> > +	ASSERT_OK(err, "pthread_join");
> 
> The above is not equivalent to the original code.
> The original didn't check pthread_join() return as it
> is very very unlikely to fail. And check 'thread_ret'
> is still needed.
> 
Yes that is true, but the v1 [1] broke the tests because the
ASSERT_OK_PTR(thread_ret, "pthread_join") kept failing, even
though all the asserts within the `server()` function itself
passed.

Also, isn't asserting `thread_ret` technically checking the
`server()` function instead of `pthread_join`? So should we
have two asserts here? One for `server` and one for `pthread_join`
or is it not necessary?
i.e:
```
ASSERT_OK_PTR(thread_ret, "server");
ASSERT_OK(err, "pthread_join");
```

Upon taking a second look, I now think that the reason why
`ASSERT_OK_PTR(thread_ret, "pthread_join");` failed in v1 might
have been because it calls `libbpf_get_error` which returns
`-errno` when the pointer is `NULL`.

Since `server`'s return value is not a bpf address, which
`ASSERT_OK_PTR` expects it to be, do you that think we should
explicitly set `errno = 0` prior to returning NULL on server?
That way that assert would pass even when the pointer is NULL
(which is the case when `server` returns successfuly).

[1] - https://lore.kernel.org/lkml/GV1PR10MB6563A0BE91080E6E8EC2651DE8B0A@GV1PR10MB6563.EURPRD10.PROD.OUTLOOK.COM/

As always, thank you for your feedback.

Yuran Pereira