Skip to content

Improve safety with fallible version of pointer::pin() - #292

Merged
ojeda merged 2 commits into
Rust-for-Linux:rustfrom
TheSven73:rust-for-linux-pin-wrapper
May 26, 2021
Merged

Improve safety with fallible version of pointer::pin()#292
ojeda merged 2 commits into
Rust-for-Linux:rustfrom
TheSven73:rust-for-linux-pin-wrapper

Conversation

@TheSven73

@TheSven73TheSven73 commented May 25, 2021

Copy link
Copy Markdown
Collaborator

Introduce a fallible version of pointer::pin(), called try_pin(), which should improve safety by moving the unsafe block from the point where the pinned pointer is created, to a library function.

I am not sure I understand pinning properly yet, please review carefully.

@ksquirrel

This comment has been minimized.

Sign in to view
@alex

alex commented May 25, 2021

Copy link
Copy Markdown
Member

This looks correct to me, but it's really just a workaround for the absence of rust-lang/rust#85579, right? If that methods existed, would we want our own trait?

@TheSven73

TheSven73 commented May 25, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Probably also related to #290. I hadn't even spotted that Issue yet. Maybe @nbdd0121 also looked at #289 and said "hey"... ?

@TheSven73

TheSven73 commented May 25, 2021

Copy link
Copy Markdown
CollaboratorAuthor

This looks correct to me, but it's really just a workaround for the absence of rust-lang/rust#85579, right? If that methods existed, would we want our own trait?

It really depends on @ojeda 's plans. And how long it'll take to get try_pin() via Rust. Something good now is better than something perfect later? Except if it has little value or muddies the water.

Independently of that, do the changes to // SAFETY have merit? Should these be moved into their own PR?

@alex

alex commented May 25, 2021 via email

Copy link
Copy Markdown
Member

@TheSven73

TheSven73 commented May 25, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Ah, I see what you mean... I created this as a trait, because it's the only way to add methods to foreign types (that I know of). I didn't intend to create an abstraction. Not sure if that'd be useful in itself.

@alex

alex commented May 26, 2021

Copy link
Copy Markdown
Member

Unfortunately this now has merge conflicts.

nbdd0121
nbdd0121 approved these changes May 26, 2021
View reviewed changes

@nbdd0121nbdd0121 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment threadsamples/rust/rust_miscdev.rs Outdated
io_buffer::{IoBufferReader, IoBufferWriter},
miscdev,
sync::{CondVar, Mutex},
traits::TryPin,

@nbdd0121nbdd0121May 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To avoid having all users importing this explicitly, you can put it in the prelude:

pubusecrate::traits::TryPin;// Or alternative use `as _` so they see `Arc::try_pin` but not the trait itself.pubusecrate::traits::TryPinas _;

ojeda
ojeda reviewed May 26, 2021
View reviewed changes
Comment threadrust/kernel/traits.rs Outdated
Comment threadrust/kernel/traits.rs Outdated
Comment threadrust/kernel/traits.rs Outdated
Comment threadrust/kernel/traits.rs Outdated
Comment threadrust/kernel/traits.rs Outdated
@ksquirrel

This comment has been minimized.

Sign in to view
Sven Van Asbroeck added 2 commits May 26, 2021 07:55
Sven Van Asbroeck
rust/kernel: define a fallible implementation of Arc::pin
02138fd
This is convenient to create pinned pointers inside the kernel,
without having to use/prove an `unsafe` block each time.
Defined as a trait, so it can be implemented for `core` pointers
such as `Arc`, `Rc`, or `Box`.
Includes a sample implementation for `Arc`.
Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
Sven Van Asbroeck
rust/samples: miscdev: eliminate unsafe block
5a67315
When creating a pinned `Arc`, eliminate an `unsafe` block by using
the fallible version of `Arc::pin()`.
While we're here, update the `// SAFETY` proofs, which have
become stale.
Tested using QEMU.
Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
@TheSven73
TheSven73force-pushed the rust-for-linux-pin-wrapper branch from c2811a6 to 5a67315CompareMay 26, 2021 12:27
@ksquirrel

ksquirrel commented May 26, 2021

Copy link
Copy Markdown
Member

Review of 5a67315fc779:

@TheSven73

TheSven73 commented May 26, 2021

Copy link
Copy Markdown
CollaboratorAuthor

v1 -> v2

ojeda: improve comments/docs
nbdd0121: add to kernel prelude

Rebased to eliminate merge conflicts.

@ojeda
ojeda merged commit 50f2249 into Rust-for-Linux:rustMay 26, 2021
@TheSven73
TheSven73 deleted the rust-for-linux-pin-wrapper branch May 27, 2021 14:37
TheSven73 pushed a commit to TheSven73/linux that referenced this pull request May 28, 2021
Sven Van Asbroeck
rust/kernel/traits: fix rustdoc warnings
0a91321
A previous commit (Rust-for-Linux#292) introduced rustdoc warnings.
Fix rustdoc links to eliminate these warnings.
Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
@TheSven73TheSven73 mentioned this pull request May 28, 2021
Merged
TheSven73 pushed a commit to TheSven73/linux that referenced this pull request May 28, 2021
Sven Van Asbroeck
rust/kernel/traits: fix rustdoc warnings
f4f353b
A previous commit (Rust-for-Linux#292) introduced rustdoc warnings.
Fix rustdoc links to eliminate these warnings.
Remove mention of `Rc`, as it's not currently used anywhere in the
Rust kernel code.
Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
ojeda pushed a commit that referenced this pull request Dec 20, 2021
@dhowells
netfs: Fix lockdep warning from taking sb_writers whilst holding mmap…
598ad0b
…_lock
Taking sb_writers whilst holding mmap_lock isn't allowed and will result in
a lockdep warning like that below. The problem comes from cachefiles
needing to take the sb_writers lock in order to do a write to the cache,
but being asked to do this by netfslib called from readpage, readahead or
write_begin[1].
Fix this by always offloading the write to the cache off to a worker
thread. The main thread doesn't need to wait for it, so deadlock can be
avoided.
This can be tested by running the quick xfstests on something like afs or
ceph with lockdep enabled.
WARNING: possible circular locking dependency detected
5.15.0-rc1-build2+ #292 Not tainted
------------------------------------------------------
holetest/65517 is trying to acquire lock:
ffff88810c81d730 (mapping.invalidate_lock#3){.+.+}-{3:3}, at: filemap_fault+0x276/0x7a5
but task is already holding lock:
ffff8881595b53e8 (&mm->mmap_lock#2){++++}-{3:3}, at: do_user_addr_fault+0x28d/0x59c
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #2 (&mm->mmap_lock#2){++++}-{3:3}:
validate_chain+0x3c4/0x4a8
__lock_acquire+0x89d/0x949
lock_acquire+0x2dc/0x34b
__might_fault+0x87/0xb1
strncpy_from_user+0x25/0x18c
removexattr+0x7c/0xe5
__do_sys_fremovexattr+0x73/0x96
do_syscall_64+0x67/0x7a
entry_SYSCALL_64_after_hwframe+0x44/0xae
-> #1 (sb_writers#10){.+.+}-{0:0}:
validate_chain+0x3c4/0x4a8
__lock_acquire+0x89d/0x949
lock_acquire+0x2dc/0x34b
cachefiles_write+0x2b3/0x4bb
netfs_rreq_do_write_to_cache+0x3b5/0x432
netfs_readpage+0x2de/0x39d
filemap_read_page+0x51/0x94
filemap_get_pages+0x26f/0x413
filemap_read+0x182/0x427
new_sync_read+0xf0/0x161
vfs_read+0x118/0x16e
ksys_read+0xb8/0x12e
do_syscall_64+0x67/0x7a
entry_SYSCALL_64_after_hwframe+0x44/0xae
-> #0 (mapping.invalidate_lock#3){.+.+}-{3:3}:
check_noncircular+0xe4/0x129
check_prev_add+0x16b/0x3a4
validate_chain+0x3c4/0x4a8
__lock_acquire+0x89d/0x949
lock_acquire+0x2dc/0x34b
down_read+0x40/0x4a
filemap_fault+0x276/0x7a5
__do_fault+0x96/0xbf
do_fault+0x262/0x35a
__handle_mm_fault+0x171/0x1b5
handle_mm_fault+0x12a/0x233
do_user_addr_fault+0x3d2/0x59c
exc_page_fault+0x85/0xa5
asm_exc_page_fault+0x1e/0x30
other info that might help us debug this:
Chain exists of:
mapping.invalidate_lock#3 --> sb_writers#10 --> &mm->mmap_lock#2
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&mm->mmap_lock#2);
lock(sb_writers#10);
lock(&mm->mmap_lock#2);
lock(mapping.invalidate_lock#3);
*** DEADLOCK ***
1 lock held by holetest/65517:
#0: ffff8881595b53e8 (&mm->mmap_lock#2){++++}-{3:3}, at: do_user_addr_fault+0x28d/0x59c
stack backtrace:
CPU: 0 PID: 65517 Comm: holetest Not tainted 5.15.0-rc1-build2+ #292
Hardware name: ASUS All Series/H97-PLUS, BIOS 2306 10/09/2014
Call Trace:
dump_stack_lvl+0x45/0x59
check_noncircular+0xe4/0x129
? print_circular_bug+0x207/0x207
? validate_chain+0x461/0x4a8
? add_chain_block+0x88/0xd9
? hlist_add_head_rcu+0x49/0x53
check_prev_add+0x16b/0x3a4
validate_chain+0x3c4/0x4a8
? check_prev_add+0x3a4/0x3a4
? mark_lock+0xa5/0x1c6
__lock_acquire+0x89d/0x949
lock_acquire+0x2dc/0x34b
? filemap_fault+0x276/0x7a5
? rcu_read_unlock+0x59/0x59
? add_to_page_cache_lru+0x13c/0x13c
? lock_is_held_type+0x7b/0xd3
down_read+0x40/0x4a
? filemap_fault+0x276/0x7a5
filemap_fault+0x276/0x7a5
? pagecache_get_page+0x2dd/0x2dd
? __lock_acquire+0x8bc/0x949
? pte_offset_kernel.isra.0+0x6d/0xc3
__do_fault+0x96/0xbf
? do_fault+0x124/0x35a
do_fault+0x262/0x35a
? handle_pte_fault+0x1c1/0x20d
__handle_mm_fault+0x171/0x1b5
? handle_pte_fault+0x20d/0x20d
? __lock_release+0x151/0x254
? mark_held_locks+0x1f/0x78
? rcu_read_unlock+0x3a/0x59
handle_mm_fault+0x12a/0x233
do_user_addr_fault+0x3d2/0x59c
? pgtable_bad+0x70/0x70
? rcu_read_lock_bh_held+0xab/0xab
exc_page_fault+0x85/0xa5
? asm_exc_page_fault+0x8/0x30
asm_exc_page_fault+0x1e/0x30
RIP: 0033:0x40192f
Code: ff 48 89 c3 48 8b 05 50 28 00 00 48 85 ed 7e 23 31 d2 4b 8d 0c 2f eb 0a 0f 1f 00 48 8b 05 39 28 00 00 48 0f af c2 48 83 c2 01 <48> 89 1c 01 48 39 d5 7f e8 8b 0d f2 27 00 00 31 c0 85 c9 74 0e 8b
RSP: 002b:00007f9931867eb0 EFLAGS: 00010202
RAX: 0000000000000000 RBX: 00007f9931868700 RCX: 00007f993206ac00
RDX: 0000000000000001 RSI: 0000000000000000 RDI: 00007ffc13e06ee0
RBP: 0000000000000100 R08: 0000000000000000 R09: 00007f9931868700
R10: 00007f99318689d0 R11: 0000000000000202 R12: 00007ffc13e06ee0
R13: 0000000000000c00 R14: 00007ffc13e06e00 R15: 00007f993206a000
Fixes: 726218f ("netfs: Define an interface to talk to a cache")
Signed-off-by: David Howells <dhowells@redhat.com>
Tested-by: Jeff Layton <jlayton@kernel.org>
cc: Jan Kara <jack@suse.cz>
cc: linux-cachefs@redhat.com
cc: linux-fsdevel@vger.kernel.org
Link: https://lore.kernel.org/r/20210922110420.GA21576@quack2.suse.cz/ [1]
Link: https://lore.kernel.org/r/163887597541.1596626.2668163316598972956.stgit@warthog.procyon.org.uk/ # v1
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ojedaojedaojeda left review comments
@nbdd0121nbdd0121nbdd0121 approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@TheSven73@ksquirrel@alex@ojeda@nbdd0121