Array and string params #110

adamrk · 2021-03-15T21:27:02Z

Add support for array parameters and change the support for string parameters to use a Rust implementation instead of wrapping the C charp implementation.

The advantage of switching to a Rust implementation for string is that it allows us to store the length of the string and immediately return a slice when it is read (the charp wrapper requires scanning for the null terminator each time the parameter is read).

Resolves part of #11.

adamrk · 2021-03-15T21:31:51Z

Looks like CI isn't being triggered?

ojeda · 2021-03-15T23:09:50Z

Let me take a look.

ojeda · 2021-03-15T23:39:41Z

It triggers on my quick test (although there is a failure later in the CI). Can you please amend the latest commit (to make it touch the timestamp) and force-push to see if GitHub notices?

Ran into a use-after-free on the main io-wq struct, wq. It has a worker ref and completion event, but the manager itself isn't holding a reference. This can lead to a race where the manager thinks there are no workers and exits, but a worker is being added. That leads to the following trace: BUG: KASAN: use-after-free in io_wqe_worker+0x4c0/0x5e0 Read of size 8 at addr ffff888108baa8a0 by task iou-wrk-3080422/3080425 CPU: 5 PID: 3080425 Comm: iou-wrk-3080422 Not tainted 5.12.0-rc1+ #110 Hardware name: Micro-Star International Co., Ltd. MS-7C60/TRX40 PRO 10G (MS-7C60), BIOS 1.60 05/13/2020 Call Trace: dump_stack+0x90/0xbe print_address_description.constprop.0+0x67/0x28d ? io_wqe_worker+0x4c0/0x5e0 kasan_report.cold+0x7b/0xd4 ? io_wqe_worker+0x4c0/0x5e0 __asan_load8+0x6d/0xa0 io_wqe_worker+0x4c0/0x5e0 ? io_worker_handle_work+0xc00/0xc00 ? recalc_sigpending+0xe5/0x120 ? io_worker_handle_work+0xc00/0xc00 ? io_worker_handle_work+0xc00/0xc00 ret_from_fork+0x1f/0x30 Allocated by task 3080422: kasan_save_stack+0x23/0x60 __kasan_kmalloc+0x80/0xa0 kmem_cache_alloc_node_trace+0xa0/0x480 io_wq_create+0x3b5/0x600 io_uring_alloc_task_context+0x13c/0x380 io_uring_add_task_file+0x109/0x140 __x64_sys_io_uring_enter+0x45f/0x660 do_syscall_64+0x32/0x80 entry_SYSCALL_64_after_hwframe+0x44/0xae Freed by task 3080422: kasan_save_stack+0x23/0x60 kasan_set_track+0x20/0x40 kasan_set_free_info+0x24/0x40 __kasan_slab_free+0xe8/0x120 kfree+0xa8/0x400 io_wq_put+0x14a/0x220 io_wq_put_and_exit+0x9a/0xc0 io_uring_clean_tctx+0x101/0x140 __io_uring_files_cancel+0x36e/0x3c0 do_exit+0x169/0x1340 __x64_sys_exit+0x34/0x40 do_syscall_64+0x32/0x80 entry_SYSCALL_64_after_hwframe+0x44/0xae Have the manager itself hold a reference, and now both drop points drop and complete if we hit zero, and the manager can unconditionally do a wait_for_completion() instead of having a race between reading the ref count and waiting if it was non-zero. Fixes: fb3a1f6 ("io-wq: have manager wait for all workers to exit") Signed-off-by: Jens Axboe <axboe@kernel.dk>

adamrk · 2021-03-16T09:39:48Z

Yeah that triggered it. The failure was just #111.

ojeda

A few things I spotted.

rust/kernel/module_param.rs

rust/module.rs

rust/kernel/module_param.rs

rust/kernel/prelude.rs

rust/kernel/module_param.rs

rust/module.rs

ojeda · 2021-03-16T11:34:06Z

By the way, two thoughts:

Reviewing proc macros may be easier seeing a sample of how the generated code looks like.
I am feeling we should perhaps start thinking about holding new features into the module! macro until we are in mainline -- people may complain about the complexity of rust/module.rs in the RFC (in particular, proc macros aren't the prettiest thing in the world and they do stand up when people is skimming/scrolling over the code as a whole :-). Of course, this proc macro is intended to minimize complexity on the kernel modules themselves and make it as easy as possible to use, so we can answer those remarks with that, but nevertheless we should attempt to hit a sweet spot.

rust/kernel/module_param.rs

adamrk · 2021-03-16T17:51:10Z

Reviewing proc macros may be easier seeing a sample of how the generated code looks like.

I can put up a gist showing the diffs between the old and new generated code.

I am feeling we should perhaps start thinking about holding new features into the module! macro until we are in mainline -- people may complain about the complexity of rust/module.rs in the RFC (in particular, proc macros aren't the prettiest thing in the world and they do stand up when people is skimming/scrolling over the code as a whole :-). Of course, this proc macro is intended to minimize complexity on the kernel modules themselves and make it as easy as possible to use, so we can answer those remarks with that, but nevertheless we should attempt to hit a sweet spot.

No problem, happy to do whatever has the best chance of being accepted.

adamrk · 2021-03-16T20:21:22Z

Diff of the expanded macro for the example here: https://gist.github.com/adamrk/2eae5c24e485125f245513d63b69db00/revisions

ojeda · 2021-03-19T21:23:40Z

I'd like to have a bit more explanation on the overall timeline of how things happen since now we are playing with memory. Perhaps in the top of module_param.rs. For instance, "at boot time parameters are set, at which time we don't have an allocator; but the reference will remain valid; for the rest of the cases, we have allocator so we can keep a copy", etc. etc. So if you have time to add it later on in another PR, it would be great. :-)

With latest clang18, I hit test_progs failures for the following test: #13/2 bpf_cookie/multi_kprobe_link_api:FAIL #13/3 bpf_cookie/multi_kprobe_attach_api:FAIL #13 bpf_cookie:FAIL #75 fentry_fexit:FAIL #76/1 fentry_test/fentry:FAIL #76 fentry_test:FAIL #80/1 fexit_test/fexit:FAIL #80 fexit_test:FAIL #110/1 kprobe_multi_test/skel_api:FAIL #110/2 kprobe_multi_test/link_api_addrs:FAIL #110/3 kprobe_multi_test/link_api_syms:FAIL #110/4 kprobe_multi_test/attach_api_pattern:FAIL #110/5 kprobe_multi_test/attach_api_addrs:FAIL #110/6 kprobe_multi_test/attach_api_syms:FAIL #110 kprobe_multi_test:FAIL For example, for #13/2, the error messages are: [...] kprobe_multi_test_run:FAIL:kprobe_test7_result unexpected kprobe_test7_result: actual 0 != expected 1 [...] kprobe_multi_test_run:FAIL:kretprobe_test7_result unexpected kretprobe_test7_result: actual 0 != expected 1 clang17 does not have this issue. Further investigation shows that kernel func bpf_fentry_test7(), used in the above tests, is inlined by the compiler although it is marked as noinline. int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg) { return (long)arg; } It is known that for simple functions like the above (e.g. just returning a constant or an input argument), the clang compiler may still do inlining for a noinline function. Adding 'asm volatile ("")' in the beginning of the bpf_fentry_test7() can prevent inlining. Signed-off-by: Yonghong Song <yonghong.song@linux.dev> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Tested-by: Eduard Zingerman <eddyz87@gmail.com> Link: https://lore.kernel.org/bpf/20230826200843.2210074-1-yonghong.song@linux.dev

Billy Jheng Bing-Jhong reported a race between __unix_gc() and queue_oob(). __unix_gc() tries to garbage-collect close()d inflight sockets, and then if the socket has MSG_OOB in unix_sk(sk)->oob_skb, GC will drop the reference and set NULL to it locklessly. However, the peer socket still can send MSG_OOB message and queue_oob() can update unix_sk(sk)->oob_skb concurrently, leading NULL pointer dereference. [0] To fix the issue, let's update unix_sk(sk)->oob_skb under the sk_receive_queue's lock and take it everywhere we touch oob_skb. Note that we defer kfree_skb() in manage_oob() to silence lockdep false-positive (See [1]). [0]: BUG: kernel NULL pointer dereference, address: 0000000000000008 PF: supervisor write access in kernel mode PF: error_code(0x0002) - not-present page PGD 8000000009f5e067 P4D 8000000009f5e067 PUD 9f5d067 PMD 0 Oops: 0002 [#1] PREEMPT SMP PTI CPU: 3 PID: 50 Comm: kworker/3:1 Not tainted 6.9.0-rc5-00191-gd091e579b864 #110 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014 Workqueue: events delayed_fput RIP: 0010:skb_dequeue (./include/linux/skbuff.h:2386 ./include/linux/skbuff.h:2402 net/core/skbuff.c:3847) Code: 39 e3 74 3e 8b 43 10 48 89 ef 83 e8 01 89 43 10 49 8b 44 24 08 49 c7 44 24 08 00 00 00 00 49 8b 14 24 49 c7 04 24 00 00 00 00 <48> 89 42 08 48 89 10 e8 e7 c5 42 00 4c 89 e0 5b 5d 41 5c c3 cc cc RSP: 0018:ffffc900001bfd48 EFLAGS: 00000002 RAX: 0000000000000000 RBX: ffff8880088f5ae8 RCX: 00000000361289f9 RDX: 0000000000000000 RSI: 0000000000000206 RDI: ffff8880088f5b00 RBP: ffff8880088f5b00 R08: 0000000000080000 R09: 0000000000000001 R10: 0000000000000003 R11: 0000000000000001 R12: ffff8880056b6a00 R13: ffff8880088f5280 R14: 0000000000000001 R15: ffff8880088f5a80 FS: 0000000000000000(0000) GS:ffff88807dd80000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000000008 CR3: 0000000006314000 CR4: 00000000007506f0 PKRU: 55555554 Call Trace: <TASK> unix_release_sock (net/unix/af_unix.c:654) unix_release (net/unix/af_unix.c:1050) __sock_release (net/socket.c:660) sock_close (net/socket.c:1423) __fput (fs/file_table.c:423) delayed_fput (fs/file_table.c:444 (discriminator 3)) process_one_work (kernel/workqueue.c:3259) worker_thread (kernel/workqueue.c:3329 kernel/workqueue.c:3416) kthread (kernel/kthread.c:388) ret_from_fork (arch/x86/kernel/process.c:153) ret_from_fork_asm (arch/x86/entry/entry_64.S:257) </TASK> Modules linked in: CR2: 0000000000000008 Link: https://lore.kernel.org/netdev/a00d3993-c461-43f2-be6d-07259c98509a@rbox.co/ [1] Fixes: 1279f9d ("af_unix: Call kfree_skb() for dead unix_(sk)->oob_skb in GC.") Reported-by: Billy Jheng Bing-Jhong <billy@starlabs.sg> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> Link: https://lore.kernel.org/r/20240516134835.8332-1-kuniyu@amazon.com Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Zulip links to https://rust-for-linux.zulipchat.com can break in case of renaming the topic or channel if they are not a message links (which are permalinks). If a non-permanent zulip link is referenced emit a warning and directs to the zulip documentation. permanent links are of the format: https://.../#narrow/stream/x/topic/x/near/<numerical_id> Reported-by: ojeda@kernel.org Closes: Rust-for-Linux#110 Signed-off-by: Siddharth Menon <simeddon@gmail.com>

adamrk force-pushed the array-and-string-params branch 3 times, most recently from 8ab9760 to 1ec291a Compare March 16, 2021 09:23

ojeda requested changes Mar 16, 2021

View reviewed changes

adamrk commented Mar 16, 2021

View reviewed changes

rust/kernel/module_param.rs Show resolved Hide resolved

adamrk force-pushed the array-and-string-params branch from 1ec291a to 17ed16f Compare March 16, 2021 17:36

adamrk force-pushed the array-and-string-params branch 3 times, most recently from 6cc75cb to 126ed1a Compare March 17, 2021 08:16

Implement array and string parameters

d74e55e

adamrk force-pushed the array-and-string-params branch 3 times, most recently from 2466b9f to 42f6a6a Compare March 19, 2021 09:08

Update examples and ci

a1df67e

adamrk force-pushed the array-and-string-params branch from 42f6a6a to a1df67e Compare March 19, 2021 11:30

ojeda approved these changes Mar 19, 2021

View reviewed changes

ojeda merged commit d58e9d3 into Rust-for-Linux:rust Mar 20, 2021

ojeda mentioned this pull request Apr 29, 2021

Complete kernel params support #11

Open

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Array and string params #110

Array and string params #110

adamrk commented Mar 15, 2021 •

edited

Loading

adamrk commented Mar 15, 2021

ojeda commented Mar 15, 2021

ojeda commented Mar 15, 2021

adamrk commented Mar 16, 2021 •

edited

Loading

ojeda left a comment

ojeda commented Mar 16, 2021 •

edited

Loading

adamrk commented Mar 16, 2021

adamrk commented Mar 16, 2021

ojeda commented Mar 19, 2021 •

edited

Loading

Array and string params #110

Array and string params #110

Conversation

adamrk commented Mar 15, 2021 • edited Loading

adamrk commented Mar 15, 2021

ojeda commented Mar 15, 2021

ojeda commented Mar 15, 2021

adamrk commented Mar 16, 2021 • edited Loading

ojeda left a comment

Choose a reason for hiding this comment

ojeda commented Mar 16, 2021 • edited Loading

adamrk commented Mar 16, 2021

adamrk commented Mar 16, 2021

ojeda commented Mar 19, 2021 • edited Loading

adamrk commented Mar 15, 2021 •

edited

Loading

adamrk commented Mar 16, 2021 •

edited

Loading

ojeda commented Mar 16, 2021 •

edited

Loading

ojeda commented Mar 19, 2021 •

edited

Loading