tests: socketpair: fix userspace thread permissions

[cfriedt]

Kernel objects were being directly accessed without previously calling k_thread_access_grant().

This change allows each test that requires an asynchronous event to send it to a common work queue with correct permissions given to the userspace thread.

Also, clarified some conditions in subsys/net/lib/sockets/socketpair.c and added / reordered some calls to respect whether semaphores were held / object were valid.

Fixes #25270

[zephyrbot]

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

[cfriedt]

Realized yesterday that I was reinventing the work queue. Should probably use one of those instead of manually doing it with a thread, stack, k_thread_suspend() / k_thread_resume(). Made a huge difference. I think only one of the tests is hanging atm, so will need to debug that a bit.

[cfriedt]

99.88% of tests passing but a page fault on qemu_x86. Any pointers on how to get rid of that?

(gdb) info sym 0x136004
test_socketpair_work_q + 4 in section bss

[cfriedt]

Paging Dr. @andrewboie

[cfriedt]

I think my assumption was that a thread's stack was fair game from zephyr kernel space was wrong - at least on x86. I guess I need to use a struct k_mem_slab?

[andrewboie]

Were you going to post the contents of the error message? A page fault means you accessed memory improperly in some way.

What is going on at the PC value (EIP) when the fault happens? Have you attached a debugger and gotten a stack trace?
What memory location is it faulting on, what is located there?
What other steps have you taken to debug this?

[cfriedt]

error message:

SeaBIOS (version rel-1.12.1-0-ga5cab58-dirty-20200214_052440-f7294c49af13-zephyr
)
Booting from ROM..Optimal CONFIG_X86_MMU_PAGE_POOL_PAGES 11
*** Booting Zephyr OS build v2.3.0-rc1-98-g21dc7a957ddf  ***
Running test suite socketpair
===================================================================
starting test - test_socketpair_read_block
E: Page fault at address 0x15e004 (error code 0x4)
E: Linear address not present in page tables
E:   PDE: 0x000000000017d827 Writable, User, Execute Enabled
E:   PTE: Non-present
E: EAX: 0x0015e000, EBX: 0x00180033, ECX: 0x00000001, EDX: 0x00182f64
E: ESI: 0x00000000, EDI: 0x00182fe8, EBP: 0x00182ef0, ESP: 0x00182ef0
E: EFLAGS: 0x00000216 CS: 0x002b CR3: 0x0015e1e0
E: call trace:
E: EIP: 0x00132897
E:      0x0013313b (0x15e000)
E:      0x00100c95 (0x15e000)
E:      0x001013c8 (0x15e000)
E:      0x0012d8a0 (0x0)
E:      0x0012d911 (0x14d000)
E:      0x00103281 (0x14d000)
E: >>> ZEPHYR FATAL ERROR 0: CPU exception on CPU 0
E: Current thread: 0x0015ee00 (unknown)
E: Halting system

backtrace:

#0  sys_sflist_peek_tail (list=0x15e000 <test_socketpair_work_q>) at ../include/sys/sflist.h:251
#1  0x0013313b in k_queue_append (queue=0x15e000 <test_socketpair_work_q>, data=0x182f5c <ztest_thread_stack+28508>) at /home/cfriedt/workspace/zephyrproject/zephyr/kernel/queue.c:193
#2  0x00100c95 in k_work_submit_to_queue (work_q=0x15e000 <test_socketpair_work_q>, work=0x182f5c <ztest_thread_stack+28508>) at /home/cfriedt/workspace/zephyrproject/zephyr/include/kernel.h:3128
#3  0x001013c8 in test_socketpair_read_block () at /home/cfriedt/workspace/zephyrproject/zephyr/tests/net/socket/socketpair/src/test_socketpair_block.c:150
#4  0x0012d8a0 in run_test_functions (test=0x14d000 <_socketpair.5261>) at /home/cfriedt/workspace/zephyrproject/zephyr/subsys/testsuite/ztest/src/ztest.c:177
#5  0x0012d911 in test_cb (a=0x14d000 <_socketpair.5261>, dummy2=0x0, dummy=0x0) at /home/cfriedt/workspace/zephyrproject/zephyr/subsys/testsuite/ztest/src/ztest.c:307
#6  0x00103281 in z_thread_entry (entry=0x12d8f0 <test_cb>, p1=0x14d000 <_socketpair.5261>, p2=0x0, p3=0x0) at /home/cfriedt/workspace/zephyrproject/zephyr/lib/os/thread_entry.c:29
#7  0x00000000 in ?? ()

(gdb) info sym 0x15e004
test_socketpair_work_q + 4 in section bss
(gdb) info line *((void *)0x00132897)
Line 251 of "../include/sys/sflist.h" starts at address 0x132894 <sys_sflist_peek_tail+3> and ends at 0x13289a <sys_sflist_peek_tail+9>.

Originally I thought maybe the issue was the user thread accessing the static "struct ctx" that I have in tests/net/socket/socketpair/src/test_socketpair_block.c, so then I tried to move the struct ctx to the thread stack, but it makes no difference. Also tried making it static with the ZTEST_BMEM attribute.

Trying to follow along with tests/kernel/workq/work_queue_api/src/main.c ..

[cfriedt]

Aha - k_work_submit_to_user_queue() was the missing link. Sorry for the spam @andrewboie !

[andrewboie]

some minor comments otherwise LGTM

concurrency issue

[cfriedt]

Testing seems to be kind of slow. Here is the output of sanitycheck on my machine locally.

$ ./scripts/sanitycheck --enable-slow --inline-logs -j8 --testcase-root tests/net/socket/socketpair/
Renaming output directory to /home/cfriedt/workspace/zephyrproject/zephyr/sanity-out.1
INFO    - JOBS: 8
INFO    - Selecting default platforms per test case
INFO    - Building initial testcase list...
INFO    - 6 test configurations selected, 242 configurations discarded due to filters.
INFO    - Adding tasks to the queue...
INFO    - Total complete:    6/   6  100%  skipped:    0, failed:    0
INFO    - 6 of 6 tests passed (100.00%), 0 failed, 0 skipped with 0 warnings in 101.16 seconds
INFO    - In total 1 test cases were executed on 6 out of total 249 platforms (2.41%)
INFO    - 5 tests executed on platforms, 1 tests were only built.

[cfriedt]

@pfalcon @jukkar @andrewboie - anyone able to approve?

[pfalcon]

Commit message:

Also, clarified some conditions in subsys/net/lib/sockets/socketpair.c and added / reordered some calls to respect whether semaphores were held / object were valid.

My usual comment would be that the changes to subsys/net/lib/sockets/socketpair.c are substantial enough to go in a separate commit and not piggy-back on a commit, which patches tests, with a fineprint "Also, ..." at the end.

[carlescufi]

@pfalcon and @andrewboie would very much appreciate a review for this one

[andrewboie]

I really think your changes for is_nonblock synchronization need to be in a separate commit. You should not keep adding unrelated stuff to the same commit.

Ultimately the networking team needs to approve this. The changes to support user mode look fine.

[cfriedt]

Split into 2 commits. Ok to merge, @pfalcon @jukkar @andrewboie?

[pfalcon]

Thanks.

[andrewboie]

user mode changes look OK.
no opinion on the race condition change.