Skip to content
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.

Sign up for GitHub

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

Jump to bottom

rust: helpers: export more functions #637

Merged
merged 1 commit into from
Jan 20, 2022
Merged

rust: helpers: export more functions #637

merged 1 commit into from
Jan 20, 2022

Conversation

liuw
Copy link

@liuw liuw commented Jan 20, 2022

Export them so that they can be used by modules.

Signed-off-by: Wei Liu wei.liu@kernel.org

@liuw
rust: helpers: export more functions
5569058 
Export them so that they can be used by modules.

Signed-off-by: Wei Liu <wei.liu@kernel.org>
@ojeda
Copy link
Member

ojeda commented Jan 20, 2022

Note that the helpers should not be used by modules. So far we export only those that are used from generic items since the reference will appear in the module.

@bjorn3
Copy link
Member

bjorn3 commented Jan 20, 2022

Strictly speaking rustc may choose to delay codegen of the functions in the kernel crate using these helpers until the compilation of the module itself. While it doesn't do so currently, it may in the future for example automatically mark all functions that only call a single function without doing anything else as #[inline] (as it would likely be beneficial in almost all cases to do so), which causes codegen to be delayed just like with generics. Pretty much only #[no_mangle] functions are guaranteed to be codegened in the same crate as the definition I think as they have to be available even if there is no dependent crate using them.

@ojeda
Copy link
Member

ojeda commented Jan 20, 2022

Of course, we could export all of them for simplicity (and in that case automate it), but it is not clear to me we should do that.

What we should do, however, if we keep things as they are, is add a small comment about this in the top of the header.

@ojeda
Copy link
Member

ojeda commented Jan 20, 2022

Strictly speaking rustc may choose to delay codegen of the functions in the kernel crate using these helpers until the compilation of the module itself. While it doesn't do so currently, it may in the future for example automatically mark all functions that only call a single function without doing anything else as #[inline] (as it would likely be beneficial in almost all cases to do so), which causes codegen to be delayed just like with generics. Pretty much only #[no_mangle] functions are guaranteed to be codegened in the same crate as the definition I think as they have to be available even if there is no dependent crate using them.

Hmm... it would be a bit surprising, but if it starts doing that, then yeah, we would need to export everything. Do you happen to know if there a discussion to follow on this to be aware?

@bjorn3
Copy link
Member

bjorn3 commented Jan 20, 2022

Hmm... it would be a bit surprising, but if it starts doing that, then yeah, we would need to export everything. Do you happen to know if there a discussion to follow on this to be aware?

The closest I can think of if mir-only rlibs which has been discussed a couple of years ago and there has been an implementation for benchmarking that was never merged. I'm not aware of any more discussion in this area. It may or may not get supported in the future, but probably won't happen in the short term. As for my example of automatically adding #[inline] to certain functions I just came up with the idea on the spot, but it is something that could realistically be implemented in the short to mid term and may prove to be beneficial without as much of a tradeoff as mir-only rlibs.

@liuw
Copy link
Author

liuw commented Jan 20, 2022

Note that the helpers should not be used by modules. So far we export only those that are used from generic items since the reference will appear in the module.

Feel free to close this PR if you think exporting them is inappropriate.

@ojeda
Copy link
Member

ojeda commented Jan 20, 2022

The closest I can think of if mir-only rlibs which has been discussed a couple of years ago and there has been an implementation for benchmarking that was never merged. I'm not aware of any more discussion in this area. It may or may not get supported in the future, but probably won't happen in the short term. As for my example of automatically adding #[inline] to certain functions I just came up with the idea on the spot, but it is something that could realistically be implemented in the short to mid term and may prove to be beneficial without as much of a tradeoff as mir-only rlibs.

Thanks! Yeah, I think it would be reasonable if rustc started doing that. After all, most Rust code does not need/care about whether codegen is performed in the spot or not, and may be thought as an implementation detail for most use cases. As long as it is well-known and there is a escape hatch of some kind, it could be a nice optimization.

@ojeda
Copy link
Member

ojeda commented Jan 20, 2022

Feel free to close this PR if you think exporting them is inappropriate.

I think either way is fine. On the one hand, this way is simpler (not just for the list of symbols, but also in case we change who calls it from kernel) and allows to automate it, plus rustc does not guarantee it will always work. On the other hand, exporting things that are not needed always feels wrong and we are still on the experimental phase (so changing this if rustc behavior changes is not a big deal yet).

Let's merge it to avoid surprises since it is not that many helpers; and then later on we can add a future-proof way to do this. I will send a quick PR to add the comment at the top, which seems to me like the important bit to keep track of regardless of what we end up with!

Thanks a lot for the PR!

@ojeda ojeda merged commit a50107e into Rust-for-Linux:rust Jan 20, 2022
@liuw liuw deleted the export-functions branch January 20, 2022 16:32
@ojeda ojeda mentioned this pull request Jan 20, 2022
Merged
ojeda added a commit to ojeda/linux that referenced this pull request Jan 20, 2022
@ojeda
rust: helpers: explain file with a top-level comment
5991c70 
Also copy the same `EXPORT_SYMBOL_GPL` explanation to `exports.c`.

This also sums up the discussion at Rust-for-Linux#637 about exporting or not the helpers that are not technically required at the moment (thanks @bjorn3 for the note about `rustc` not guaranteeing codegen for non-inline functions).

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ojeda pushed a commit that referenced this pull request May 27, 2024
@dcaratti @kuba-moo
netlabel: fix RCU annotation for IPv4 options on socket creation
8ec9897 
Xiumei reports the following splat when netlabel and TCP socket are used:

 =============================
 WARNING: suspicious RCU usage
 6.9.0-rc2+ #637 Not tainted
 -----------------------------
 net/ipv4/cipso_ipv4.c:1880 suspicious rcu_dereference_protected() usage!

 other info that might help us debug this:

 rcu_scheduler_active = 2, debug_locks = 1
 1 lock held by ncat/23333:
  #0: ffffffff906030c0 (rcu_read_lock){....}-{1:2}, at: netlbl_sock_setattr+0x25/0x1b0

 stack backtrace:
 CPU: 11 PID: 23333 Comm: ncat Kdump: loaded Not tainted 6.9.0-rc2+ #637
 Hardware name: Supermicro SYS-6027R-72RF/X9DRH-7TF/7F/iTF/iF, BIOS 3.0  07/26/2013
 Call Trace:
  <TASK>
  dump_stack_lvl+0xa9/0xc0
  lockdep_rcu_suspicious+0x117/0x190
  cipso_v4_sock_setattr+0x1ab/0x1b0
  netlbl_sock_setattr+0x13e/0x1b0
  selinux_netlbl_socket_post_create+0x3f/0x80
  selinux_socket_post_create+0x1a0/0x460
  security_socket_post_create+0x42/0x60
  __sock_create+0x342/0x3a0
  __sys_socket_create.part.22+0x42/0x70
  __sys_socket+0x37/0xb0
  __x64_sys_socket+0x16/0x20
  do_syscall_64+0x96/0x180
  ? do_user_addr_fault+0x68d/0xa30
  ? exc_page_fault+0x171/0x280
  ? asm_exc_page_fault+0x22/0x30
  entry_SYSCALL_64_after_hwframe+0x71/0x79
 RIP: 0033:0x7fbc0ca3fc1b
 Code: 73 01 c3 48 8b 0d 05 f2 1b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 29 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d d5 f1 1b 00 f7 d8 64 89 01 48
 RSP: 002b:00007fff18635208 EFLAGS: 00000246 ORIG_RAX: 0000000000000029
 RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007fbc0ca3fc1b
 RDX: 0000000000000006 RSI: 0000000000000001 RDI: 0000000000000002
 RBP: 000055d24f80f8a0 R08: 0000000000000003 R09: 0000000000000001

R10: 0000000000020000 R11: 0000000000000246 R12: 000055d24f80f8a0
 R13: 0000000000000000 R14: 000055d24f80fb88 R15: 0000000000000000
  </TASK>

The current implementation of cipso_v4_sock_setattr() replaces IP options
under the assumption that the caller holds the socket lock; however, such
assumption is not true, nor needed, in selinux_socket_post_create() hook.

Let all callers of cipso_v4_sock_setattr() specify the "socket lock held"
condition, except selinux_socket_post_create() _ where such condition can
safely be set as true even without holding the socket lock.

Fixes: f6d8bd0 ("inet: add RCU protection to inet->opt")
Reported-by: Xiumei Mu <xmu@redhat.com>
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
Acked-by: Paul Moore <paul@paul-moore.com>
Link: https://lore.kernel.org/r/f4260d000a3a55b9e8b6a3b4e3fffc7da9f82d41.1715359817.git.dcaratti@redhat.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex alex alex approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@liuw @ojeda @bjorn3 @alex