Add `Ref` to the `sync` module. #141

wedsonaf · 2021-03-23T16:34:05Z

No description provided.

ojeda

The usual quick review on the docs only.

As usual, it is a pleasure to review things when they are well-documented on the first go :-)

rust/kernel/sync/arc.rs

jabedude · 2021-03-23T20:21:44Z

@wedsonaf would it be a good idea to add sample usage of Ref to one of the example rust character devices?

rust/kernel/sync/arc.rs

alex · 2021-03-23T21:19:13Z

rust/kernel/sync/arc.rs

+        let ptr = NonNull::from(boxed.deref());
+        Box::into_raw(boxed);


Can this just be NonNull::new(boxed.into_raw())?

NonNull::new may fail (if its argument is null), in which case it returns None. NonNull::from OTOH never fails because its argument cannot be null -- by doing it the way I did I have one less failure path.

Ok. I think this maybe makes more sense as mem::forget(boxed) then? We're not actually using the into_raw() ptr, which momentarily threw me.

Now that I think about it, it is possible that some future version of Box::into_raw will return something else other than boxed.deref(), so I think it's better to use your original suggestion, even if it implies an extra failure path (as it should never happen, it's possible the optimiser remove it from the final binary).

Oh, look at this: rust-lang/rust#47336

So the way to go is NonNull::from(Box::leak(boxed)) -- no failure path and still compatible with Box::from_raw!

alex · 2021-03-23T21:21:33Z

rust/kernel/sync/arc.rs

+/// [`Ref::try_new`].
+pub unsafe trait RefCounted: Sized {
+    /// Returns a pointer to the object field holds the reference count.
+    fn get_count(&self) -> &RefCount;


Since everything just accesses .count, would it make more sense to have this ->&AtomicUsize?

I think an opaque RefCount is better for a couple of reasons:

We can easily add more fields in the future if we choose to. For example, we could have a debug mode where we link all the references so that we know at any given time what all the references are. We could also add some instrumentation/perf numbers like a high water mark for the number of references, etc.

In an earlier version of the code, RefCount didn't have any constructors: the only way to get one [outside the module] was through Ref::try_new, which took a closure as argument that had a RefCount as argument and returned T (which would then be wrapped in Ref). This was my attempt at enforcing that all instances of T be wrapped in Ref. It didn't really guarantee it because one could just 'exfiltrate' the RefCount instance and use it to build an unsafe T (it could never return because it wouldn't be able to construct the T that it needs to return, but one could exfiltrate T to another thread and sleep forever). This is a convoluted way to build an unsafe program, but it is a way nonetheless, so I had to make RefCounted an unsafe trait. Anyway, I still hope we'll find a way to implement this safely and make RefCounted safe; not allowing RefCount to be arbitrarily constructed may be part of the solution.

wedsonaf

@wedsonaf would it be a good idea to add sample usage of Ref to one of the example rust character devices?

Yes, it would. I will add one in my next PR as it allows file instances to share some state through their device registration. I'll have implementations of read and write there that use this.

(Everything I'm pushing is used in a driver I'm writing. I'm actually teasing apart the driver and generic pieces and trying to commit them here before I push the initial version of the driver. Anyway, the reason I mention this is that people will be able to see real usage then as well.)

wedsonaf · 2021-03-23T21:28:35Z

rust/kernel/sync/arc.rs

+        let ptr = NonNull::from(boxed.deref());
+        Box::into_raw(boxed);


NonNull::new may fail (if its argument is null), in which case it returns None. NonNull::from OTOH never fails because its argument cannot be null -- by doing it the way I did I have one less failure path.

wedsonaf · 2021-03-23T21:38:41Z

rust/kernel/sync/arc.rs

+/// [`Ref::try_new`].
+pub unsafe trait RefCounted: Sized {
+    /// Returns a pointer to the object field holds the reference count.
+    fn get_count(&self) -> &RefCount;


I think an opaque RefCount is better for a couple of reasons:

We can easily add more fields in the future if we choose to. For example, we could have a debug mode where we link all the references so that we know at any given time what all the references are. We could also add some instrumentation/perf numbers like a high water mark for the number of references, etc.

In an earlier version of the code, RefCount didn't have any constructors: the only way to get one [outside the module] was through Ref::try_new, which took a closure as argument that had a RefCount as argument and returned T (which would then be wrapped in Ref). This was my attempt at enforcing that all instances of T be wrapped in Ref. It didn't really guarantee it because one could just 'exfiltrate' the RefCount instance and use it to build an unsafe T (it could never return because it wouldn't be able to construct the T that it needs to return, but one could exfiltrate T to another thread and sleep forever). This is a convoluted way to build an unsafe program, but it is a way nonetheless, so I had to make RefCounted an unsafe trait. Anyway, I still hope we'll find a way to implement this safely and make RefCounted safe; not allowing RefCount to be arbitrarily constructed may be part of the solution.

rust/kernel/sync/arc.rs

fbq

I think we need to add comments for every Ordering usage.

rust/kernel/sync/arc.rs

alex

One small comment, otherwise this LGTM.

alex · 2021-03-30T22:13:14Z

rust/kernel/sync/arc.rs

+        let ptr = NonNull::from(boxed.deref());
+        Box::into_raw(boxed);


Ok. I think this maybe makes more sense as mem::forget(boxed) then? We're not actually using the into_raw() ptr, which momentarily threw me.

Inject fault while probing kunit-example-test.ko, if kzalloc fails in kunit_parse_glob_filter(), strcpy() or strncpy() to NULL will cause below null-ptr-deref bug. So check NULL for kzalloc() and return int instead of void for kunit_parse_glob_filter(). Unable to handle kernel paging request at virtual address dfff800000000000 KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007] Mem abort info: ESR = 0x0000000096000005 EC = 0x25: DABT (current EL), IL = 32 bits SET = 0, FnV = 0 EA = 0, S1PTW = 0 FSC = 0x05: level 1 translation fault Data abort info: ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000 CM = 0, WnR = 0, TnD = 0, TagAccess = 0 GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [dfff800000000000] address between user and kernel address ranges Internal error: Oops: 0000000096000005 [#1] PREEMPT SMP Modules linked in: kunit_example_test cfg80211 rfkill 8021q garp mrp stp llc ipv6 [last unloaded: kunit_example_test] CPU: 4 PID: 6047 Comm: modprobe Tainted: G W N 6.5.0-next-20230829+ #141 Hardware name: linux,dummy-virt (DT) pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : strncpy+0x58/0xc0 lr : kunit_filter_suites+0x15c/0xa84 sp : ffff800082a17420 x29: ffff800082a17420 x28: 0000000000000000 x27: 0000000000000004 x26: 0000000000000000 x25: ffffa847e40a5320 x24: 0000000000000001 x23: 0000000000000000 x22: 0000000000000001 x21: dfff800000000000 x20: 000000000000002a x19: 0000000000000000 x18: 00000000750b3b54 x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 x14: 0000000000000000 x13: 34393178302f3039 x12: ffff7508fcea4ec1 x11: 1ffff508fcea4ec0 x10: ffff7508fcea4ec0 x9 : dfff800000000000 x8 : ffff6051b1a7f86a x7 : ffff800082a17270 x6 : 0000000000000002 x5 : 0000000000000098 x4 : ffff028d9817b250 x3 : 0000000000000000 x2 : 0000000000000000 x1 : ffffa847e40a5320 x0 : 0000000000000000 Call trace: strncpy+0x58/0xc0 kunit_filter_suites+0x15c/0xa84 kunit_module_notify+0x1b0/0x3ac blocking_notifier_call_chain+0xc4/0x128 do_init_module+0x250/0x594 load_module+0x37b0/0x44b4 init_module_from_file+0xd4/0x128 idempotent_init_module+0x2c8/0x524 __arm64_sys_finit_module+0xac/0x100 invoke_syscall+0x6c/0x258 el0_svc_common.constprop.0+0x160/0x22c do_el0_svc+0x44/0x5c el0_svc+0x38/0x78 el0t_64_sync_handler+0x13c/0x158 el0t_64_sync+0x190/0x194 Code: 5400028a d343fe63 12000a62 39400034 (38f56863) ---[ end trace 0000000000000000 ]--- Kernel panic - not syncing: Oops: Fatal exception SMP: stopping secondary CPUs Kernel Offset: 0x284761400000 from 0xffff800080000000 PHYS_OFFSET: 0xfffffd7380000000 CPU features: 0x88000203,3c020000,1000421b Memory Limit: none Rebooting in 1 seconds.. Fixes: a127b15 ("kunit: tool: allow filtering test cases via glob") Signed-off-by: Jinjie Ruan <[email protected]> Reviewed-by: Rae Moar <[email protected]> Reviewed-by: David Gow <[email protected]> Signed-off-by: Shuah Khan <[email protected]>

ojeda reviewed Mar 23, 2021

View reviewed changes

jabedude reviewed Mar 23, 2021

View reviewed changes

rust/kernel/sync/arc.rs Show resolved Hide resolved

wedsonaf force-pushed the sync-arc branch 2 times, most recently from 8e008b9 to b9d8c65 Compare March 23, 2021 17:48

alex reviewed Mar 23, 2021

View reviewed changes

wedsonaf commented Mar 23, 2021

View reviewed changes

fbq reviewed Mar 23, 2021

View reviewed changes

rust/kernel/sync/arc.rs Outdated Show resolved Hide resolved

rust/kernel/sync/arc.rs Show resolved Hide resolved

fbq reviewed Mar 24, 2021

View reviewed changes

rust/kernel/sync/arc.rs Show resolved Hide resolved

Add Ref to the sync module.

c73b81e

wedsonaf force-pushed the sync-arc branch from b9d8c65 to c73b81e Compare March 27, 2021 00:30

alex reviewed Mar 30, 2021

View reviewed changes

Use Box::leak to get pointer to data.

9b8feeb

alex approved these changes Mar 30, 2021

View reviewed changes

alex merged commit adddec4 into Rust-for-Linux:rust Mar 30, 2021

wedsonaf deleted the sync-arc branch March 30, 2021 23:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Ref` to the `sync` module. #141

Add `Ref` to the `sync` module. #141

wedsonaf commented Mar 23, 2021

ojeda left a comment

jabedude commented Mar 23, 2021

alex Mar 23, 2021

wedsonaf Mar 23, 2021

alex Mar 30, 2021

wedsonaf Mar 30, 2021

wedsonaf Mar 30, 2021

alex Mar 23, 2021

wedsonaf Mar 23, 2021

wedsonaf left a comment

wedsonaf Mar 23, 2021

wedsonaf Mar 23, 2021

fbq left a comment

alex left a comment

alex Mar 30, 2021

		let ptr = NonNull::from(boxed.deref());
		Box::into_raw(boxed);

Add Ref to the sync module. #141

Add Ref to the sync module. #141

Conversation

wedsonaf commented Mar 23, 2021

ojeda left a comment

Choose a reason for hiding this comment

jabedude commented Mar 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wedsonaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbq left a comment

Choose a reason for hiding this comment

alex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `Ref` to the `sync` module. #141

Add `Ref` to the `sync` module. #141