On 2/10/20 12:52 PM, Jakub Sitnicki wrote:
On Mon, Feb 10, 2020 at 03:55 AM GMT, John Fastabend wrote:
Jakub Sitnicki wrote:
On Sun, Feb 09, 2020 at 03:41 AM CET, Alexei Starovoitov wrote:
On Thu, Feb 6, 2020 at 3:28 AM Jakub Sitnicki <jakub@xxxxxxxxxxxxxx> wrote:
Commit 7e81a3530206 ("bpf: Sockmap, ensure sock lock held during tear
down") introduced sleeping issues inside RCU critical sections and while
holding a spinlock on sockmap/sockhash tear-down. There has to be at least
one socket in the map for the problem to surface.
This adds a test that triggers the warnings for broken locking rules. Not a
fix per se, but rather tooling to verify the accompanying fixes. Run on a
VM with 1 vCPU to reproduce the warnings.
Fixes: 7e81a3530206 ("bpf: Sockmap, ensure sock lock held during tear down")
Signed-off-by: Jakub Sitnicki <jakub@xxxxxxxxxxxxxx>
selftests/bpf no longer builds for me.
make
BINARY test_maps
TEST-OBJ [test_progs] sockmap_basic.test.o
/data/users/ast/net/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c:
In function ‘connected_socket_v4’:
/data/users/ast/net/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c:20:11:
error: ‘TCP_REPAIR_ON’ undeclared (first use in this function); did
you mean ‘TCP_REPAIR’?
20 | repair = TCP_REPAIR_ON;
| ^~~~~~~~~~~~~
| TCP_REPAIR
/data/users/ast/net/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c:20:11:
note: each undeclared identifier is reported only once for each
function it appears in
/data/users/ast/net/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c:29:11:
error: ‘TCP_REPAIR_OFF_NO_WP’ undeclared (first use in this function);
did you mean ‘TCP_REPAIR_OPTIONS’?
29 | repair = TCP_REPAIR_OFF_NO_WP;
| ^~~~~~~~~~~~~~~~~~~~
| TCP_REPAIR_OPTIONS
Clearly /usr/include/linux/tcp.h is too old.
Suggestions?
Sorry for the inconvenience. I see that tcp.h header is missing under
linux/tools/include/uapi/.
How about we just add the couple defines needed to sockmap_basic.c I don't
see a need to pull in all of tcp.h just for a couple defines that wont
change anyways.
Looking back at how this happened. test_progs.h pulls in netinet/tcp.h:
# 19 "/home/jkbs/src/linux/tools/testing/selftests/bpf/test_progs.h" 2
# 1 "/usr/include/netinet/tcp.h" 1 3 4
# 92 "/usr/include/netinet/tcp.h" 3 4
A glibc header, which gained TCP_REPAIR_* constants in 2.29 [0]:
$ git describe --contains 5cd7dbdea13eb302620491ef44837b17e9d39c5a
glibc-2.29~510
Pulling in linux/tcp.h would conflict with struct definitions in
netinet/tcp.h. So redefining the possibly missing constants, like John
suggests, is the right way out.
I'm not sure, though, how to protect against such mistakes in the
future. Any ideas?
We've usually been pulling in header copies into tools/include/. Last bigger one
was in commit 13a748ea6df1 ("bpf: Sync asm-generic/socket.h to tools/") to name
one example, but redefining the way John did for smaller things is also okay,
especially if a header has further dependencies which would then also be needed
under tools/ infra.
[0] https://sourceware.org/git/?p=glibc.git;a=commit;h=5cd7dbdea13eb302620491ef44837b17e9d39c5a
