Skip to content

Add sync module with a mutex implementation. #102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 14, 2021
Merged

Add sync module with a mutex implementation. #102

merged 1 commit into from
Mar 14, 2021

Conversation

wedsonaf
Copy link

This introduces a sync module with a mutex implementation. In the next couple of days I'll send pull requests for spinlocks, condvars, and locked_by. These are all used in the driver: I'm planning to submit all rust/ changes before the driver.

@wedsonaf wedsonaf mentioned this pull request Mar 11, 2021
Copy link
Member

@ojeda ojeda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick usual doc review + some CI failures too.

Copy link
Member

@alex alex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a full example of using the mutex (particularly the init stuff) to the module comment.

@ojeda
Copy link
Member

ojeda commented Mar 11, 2021

Can you add a full example of using the mutex (particularly the init stuff) to the module comment.

+1, and please copy-paste it into rust_example.rs, since those are effectively our tests for the moment.

@ojeda
Copy link
Member

ojeda commented Mar 12, 2021

Why Mutex isn't PhantomPinned? Its init() requires a pinned reference, but that doesn't mean much if one can pin and unpin at will, no?

Related: it might be a good idea to document why we require a type to be pinned or not (e.g. x member from y C structure is self-referential), even if it only applies under some CONFIG option (e.g. debug) etc.

@wedsonaf
Copy link
Author

Why Mutex isn't PhantomPinned? Its init() requires a pinned reference, but that doesn't mean much if one can pin and unpin at will, no?

Yes, I'm adding PhantomPinned to all types that require pinning.

Related: it might be a good idea to document why we require a type to be pinned or not (e.g. x member from y C structure is self-referential), even if it only applies under some CONFIG option (e.g. debug) etc.

Sure, I'll add some text around this.

@wedsonaf
Copy link
Author

Can you add a full example of using the mutex (particularly the init stuff) to the module comment.

I added a trivial example.

+1, and please copy-paste it into rust_example.rs, since those are effectively our tests for the moment.

This is not very useful in rust_example.rs -- I'll eventually add something there, but at the moment I'm a bit busy with getting the existing code in shape for the merge window.

@ojeda
Copy link
Member

ojeda commented Mar 12, 2021

This is not very useful in rust_example.rs -- I'll eventually add something there, but at the moment I'm a bit busy with getting the existing code in shape for the merge window.

It is useful because it is the only way we have to at least compile-test the macros and run at least the happy path once in the CI.

If you are busy, that is fine, I can add it later myself :-)

@wedsonaf
Copy link
Author

It is useful because it is the only way we have to at least compile-test the macros and run at least the happy path once in the CI.

Fair enough. I've added some code there for this purpose, though it is not a useful sample of using mutexes.

(Later on I'd like to add something like a counter of the number of bytes written, then read will return this count. Not useful functionality but shows how a mutex could be used. I need to bring some more changes to rust/ before I can write this.)

@wedsonaf
Copy link
Author

Folks, thank you for the reviews. I believe I addressed all the comments.

I'd appreciate if you could have another look and let me know if you find anything else that could be improved.

Copy link
Member

@ojeda ojeda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good. None of the comments is really a blocker. I will merge it tomorrow to give time to others to take a second look and then we move into the other two.

//! This module contains the kernel APIs related to synchronisation that have been ported or
//! wrapped for usage by Rust code in the kernel and is shared by all of them.
//!
//! # Example
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move the example into mutex.rs, i.e. so that we have a different one for condvar, spinlock, etc.

Not sure if it is best to also move each of them to mutex_init!, rather than have them in the module doc, hmm... I will think about it, but we can move it later.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we move it to mutex.rs, would it be visible in the rendered docs? (Given that is gets 'exported' via pub use.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an attribute for pub use, to render the doc in place, so yes we can rerender it as it would be in this file.

//! # Example
//!
//!```
//! fn test() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for wrapping into a fn test() { in examples, i.e. the code can go directly in "file" scope.

@ojeda ojeda mentioned this pull request Mar 12, 2021
Copy link
Author

@wedsonaf wedsonaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good. None of the comments is really a blocker. I will merge it tomorrow to give time to others to take a second look and then we move into the other two.

Sounds good, thank you!

//! This module contains the kernel APIs related to synchronisation that have been ported or
//! wrapped for usage by Rust code in the kernel and is shared by all of them.
//!
//! # Example
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we move it to mutex.rs, would it be visible in the rendered docs? (Given that is gets 'exported' via pub use.)

@ojeda ojeda merged commit d320a36 into rust Mar 14, 2021
@ojeda ojeda deleted the sync branch March 14, 2021 13:51
ojeda pushed a commit that referenced this pull request Jul 11, 2022
Commit a7259df ("memblock: make memblock_find_in_range method
private") changed the API using which memory is reserved for the pKVM
hypervisor. However, memblock_phys_alloc() differs from the original API in
terms of kmemleak semantics -- the old one didn't report the reserved
regions to kmemleak while the new one does. Unfortunately, when protected
KVM is enabled, all kernel accesses to pKVM-private memory result in a
fatal exception, which can now happen because of kmemleak scans:

$ echo scan > /sys/kernel/debug/kmemleak
[   34.991354] kvm [304]: nVHE hyp BUG at: [<ffff800008fa3750>] __kvm_nvhe_handle_host_mem_abort+0x270/0x290!
[   34.991580] kvm [304]: Hyp Offset: 0xfffe8be807e00000
[   34.991813] Kernel panic - not syncing: HYP panic:
[   34.991813] PS:600003c9 PC:0000f418011a3750 ESR:00000000f2000800
[   34.991813] FAR:ffff000439200000 HPFAR:0000000004792000 PAR:0000000000000000
[   34.991813] VCPU:0000000000000000
[   34.993660] CPU: 0 PID: 304 Comm: bash Not tainted 5.19.0-rc2 #102
[   34.994059] Hardware name: linux,dummy-virt (DT)
[   34.994452] Call trace:
[   34.994641]  dump_backtrace.part.0+0xcc/0xe0
[   34.994932]  show_stack+0x18/0x6c
[   34.995094]  dump_stack_lvl+0x68/0x84
[   34.995276]  dump_stack+0x18/0x34
[   34.995484]  panic+0x16c/0x354
[   34.995673]  __hyp_pgtable_total_pages+0x0/0x60
[   34.995933]  scan_block+0x74/0x12c
[   34.996129]  scan_gray_list+0xd8/0x19c
[   34.996332]  kmemleak_scan+0x2c8/0x580
[   34.996535]  kmemleak_write+0x340/0x4a0
[   34.996744]  full_proxy_write+0x60/0xbc
[   34.996967]  vfs_write+0xc4/0x2b0
[   34.997136]  ksys_write+0x68/0xf4
[   34.997311]  __arm64_sys_write+0x20/0x2c
[   34.997532]  invoke_syscall+0x48/0x114
[   34.997779]  el0_svc_common.constprop.0+0x44/0xec
[   34.998029]  do_el0_svc+0x2c/0xc0
[   34.998205]  el0_svc+0x2c/0x84
[   34.998421]  el0t_64_sync_handler+0xf4/0x100
[   34.998653]  el0t_64_sync+0x18c/0x190
[   34.999252] SMP: stopping secondary CPUs
[   35.000034] Kernel Offset: disabled
[   35.000261] CPU features: 0x800,00007831,00001086
[   35.000642] Memory Limit: none
[   35.001329] ---[ end Kernel panic - not syncing: HYP panic:
[   35.001329] PS:600003c9 PC:0000f418011a3750 ESR:00000000f2000800
[   35.001329] FAR:ffff000439200000 HPFAR:0000000004792000 PAR:0000000000000000
[   35.001329] VCPU:0000000000000000 ]---

Fix this by explicitly excluding the hypervisor's memory pool from
kmemleak like we already do for the hyp BSS.

Cc: Mike Rapoport <[email protected]>
Fixes: a7259df ("memblock: make memblock_find_in_range method private")
Signed-off-by: Quentin Perret <[email protected]>
Acked-by: Catalin Marinas <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
senekor pushed a commit to senekor/linux that referenced this pull request Feb 24, 2025
After this patch:

 Rust-for-Linux#102/1   flow_dissector_classification/ipv4:OK
 Rust-for-Linux#102/2   flow_dissector_classification/ipv4_continue_dissect:OK
 Rust-for-Linux#102/3   flow_dissector_classification/ipip:OK
 Rust-for-Linux#102/4   flow_dissector_classification/gre:OK
 Rust-for-Linux#102/5   flow_dissector_classification/port_range:OK
 Rust-for-Linux#102/6   flow_dissector_classification/ipv6:OK
 Rust-for-Linux#102     flow_dissector_classification:OK
 Summary: 1/6 PASSED, 0 SKIPPED, 0 FAILED

Cc: Daniel Borkmann <[email protected]>
Cc: Andrii Nakryiko <[email protected]>
Signed-off-by: Cong Wang <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
ojeda pushed a commit that referenced this pull request Apr 3, 2025
When MPOA_cache_impos_rcvd() receives the msg, it can trigger
Null Pointer Dereference Vulnerability if both entry and
holding_time are NULL. Because there is only for the situation
where entry is NULL and holding_time exists, it can be passed
when both entry and holding_time are NULL. If these are NULL,
the entry will be passd to eg_cache_put() as parameter and
it is referenced by entry->use code in it.

kasan log:

[    3.316691] Oops: general protection fault, probably for non-canonical address 0xdffffc0000000006:I
[    3.317568] KASAN: null-ptr-deref in range [0x0000000000000030-0x0000000000000037]
[    3.318188] CPU: 3 UID: 0 PID: 79 Comm: ex Not tainted 6.14.0-rc2 #102
[    3.318601] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
[    3.319298] RIP: 0010:eg_cache_remove_entry+0xa5/0x470
[    3.319677] Code: c1 f7 6e fd 48 c7 c7 00 7e 38 b2 e8 95 64 54 fd 48 c7 c7 40 7e 38 b2 48 89 ee e80
[    3.321220] RSP: 0018:ffff88800583f8a8 EFLAGS: 00010006
[    3.321596] RAX: 0000000000000006 RBX: ffff888005989000 RCX: ffffffffaecc2d8e
[    3.322112] RDX: 0000000000000000 RSI: 0000000000000004 RDI: 0000000000000030
[    3.322643] RBP: 0000000000000000 R08: 0000000000000000 R09: fffffbfff6558b88
[    3.323181] R10: 0000000000000003 R11: 203a207972746e65 R12: 1ffff11000b07f15
[    3.323707] R13: dffffc0000000000 R14: ffff888005989000 R15: ffff888005989068
[    3.324185] FS:  000000001b6313c0(0000) GS:ffff88806d380000(0000) knlGS:0000000000000000
[    3.325042] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    3.325545] CR2: 00000000004b4b40 CR3: 000000000248e000 CR4: 00000000000006f0
[    3.326430] Call Trace:
[    3.326725]  <TASK>
[    3.326927]  ? die_addr+0x3c/0xa0
[    3.327330]  ? exc_general_protection+0x161/0x2a0
[    3.327662]  ? asm_exc_general_protection+0x26/0x30
[    3.328214]  ? vprintk_emit+0x15e/0x420
[    3.328543]  ? eg_cache_remove_entry+0xa5/0x470
[    3.328910]  ? eg_cache_remove_entry+0x9a/0x470
[    3.329294]  ? __pfx_eg_cache_remove_entry+0x10/0x10
[    3.329664]  ? console_unlock+0x107/0x1d0
[    3.329946]  ? __pfx_console_unlock+0x10/0x10
[    3.330283]  ? do_syscall_64+0xa6/0x1a0
[    3.330584]  ? entry_SYSCALL_64_after_hwframe+0x47/0x7f
[    3.331090]  ? __pfx_prb_read_valid+0x10/0x10
[    3.331395]  ? down_trylock+0x52/0x80
[    3.331703]  ? vprintk_emit+0x15e/0x420
[    3.331986]  ? __pfx_vprintk_emit+0x10/0x10
[    3.332279]  ? down_trylock+0x52/0x80
[    3.332527]  ? _printk+0xbf/0x100
[    3.332762]  ? __pfx__printk+0x10/0x10
[    3.333007]  ? _raw_write_lock_irq+0x81/0xe0
[    3.333284]  ? __pfx__raw_write_lock_irq+0x10/0x10
[    3.333614]  msg_from_mpoad+0x1185/0x2750
[    3.333893]  ? __build_skb_around+0x27b/0x3a0
[    3.334183]  ? __pfx_msg_from_mpoad+0x10/0x10
[    3.334501]  ? __alloc_skb+0x1c0/0x310
[    3.334809]  ? __pfx___alloc_skb+0x10/0x10
[    3.335283]  ? _raw_spin_lock+0xe0/0xe0
[    3.335632]  ? finish_wait+0x8d/0x1e0
[    3.335975]  vcc_sendmsg+0x684/0xba0
[    3.336250]  ? __pfx_vcc_sendmsg+0x10/0x10
[    3.336587]  ? __pfx_autoremove_wake_function+0x10/0x10
[    3.337056]  ? fdget+0x176/0x3e0
[    3.337348]  __sys_sendto+0x4a2/0x510
[    3.337663]  ? __pfx___sys_sendto+0x10/0x10
[    3.337969]  ? ioctl_has_perm.constprop.0.isra.0+0x284/0x400
[    3.338364]  ? sock_ioctl+0x1bb/0x5a0
[    3.338653]  ? __rseq_handle_notify_resume+0x825/0xd20
[    3.339017]  ? __pfx_sock_ioctl+0x10/0x10
[    3.339316]  ? __pfx___rseq_handle_notify_resume+0x10/0x10
[    3.339727]  ? selinux_file_ioctl+0xa4/0x260
[    3.340166]  __x64_sys_sendto+0xe0/0x1c0
[    3.340526]  ? syscall_exit_to_user_mode+0x123/0x140
[    3.340898]  do_syscall_64+0xa6/0x1a0
[    3.341170]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
[    3.341533] RIP: 0033:0x44a380
[    3.341757] Code: 0f 1f 84 00 00 00 00 00 66 90 f3 0f 1e fa 41 89 ca 64 8b 04 25 18 00 00 00 85 c00
[    3.343078] RSP: 002b:00007ffc1d404098 EFLAGS: 00000246 ORIG_RAX: 000000000000002c
[    3.343631] RAX: ffffffffffffffda RBX: 00007ffc1d404458 RCX: 000000000044a380
[    3.344306] RDX: 000000000000019c RSI: 00007ffc1d4040b0 RDI: 0000000000000003
[    3.344833] RBP: 00007ffc1d404260 R08: 0000000000000000 R09: 0000000000000000
[    3.345381] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
[    3.346015] R13: 00007ffc1d404448 R14: 00000000004c17d0 R15: 0000000000000001
[    3.346503]  </TASK>
[    3.346679] Modules linked in:
[    3.346956] ---[ end trace 0000000000000000 ]---
[    3.347315] RIP: 0010:eg_cache_remove_entry+0xa5/0x470
[    3.347737] Code: c1 f7 6e fd 48 c7 c7 00 7e 38 b2 e8 95 64 54 fd 48 c7 c7 40 7e 38 b2 48 89 ee e80
[    3.349157] RSP: 0018:ffff88800583f8a8 EFLAGS: 00010006
[    3.349517] RAX: 0000000000000006 RBX: ffff888005989000 RCX: ffffffffaecc2d8e
[    3.350103] RDX: 0000000000000000 RSI: 0000000000000004 RDI: 0000000000000030
[    3.350610] RBP: 0000000000000000 R08: 0000000000000000 R09: fffffbfff6558b88
[    3.351246] R10: 0000000000000003 R11: 203a207972746e65 R12: 1ffff11000b07f15
[    3.351785] R13: dffffc0000000000 R14: ffff888005989000 R15: ffff888005989068
[    3.352404] FS:  000000001b6313c0(0000) GS:ffff88806d380000(0000) knlGS:0000000000000000
[    3.353099] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    3.353544] CR2: 00000000004b4b40 CR3: 000000000248e000 CR4: 00000000000006f0
[    3.354072] note: ex[79] exited with irqs disabled
[    3.354458] note: ex[79] exited with preempt_count 1

Signed-off-by: Minjoong Kim <[email protected]>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reviewed-by: Simon Horman <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants