On Mon, Oct 21, 2024 at 12:06:07PM +0200, Kevin Brodsky wrote:
On 17/10/2024 17:53, Dave Martin wrote:
[...]
+/*
- Save the unpriv access state into ua_state and reset it to disable any
- restrictions.
- */
+static void save_reset_unpriv_access_state(struct unpriv_access_state *ua_state)
Would _user_ be more consistent naming than _unpriv_ ?
I did ponder on the naming. I considered user_access/uaccess instead of unpriv_access, but my concern is that it might imply that only uaccess is concerned, while in reality loads/stores that userspace itself executes are impacted too. I thought using the "unpriv" terminology from the Arm ARM (used for stage 1 permissions) might avoid such misunderstanding. I'm interested to hear opinions on this, maybe accuracy sacrifices readability.
"user_access" seemed natural to me: it parses equally as "[user access]" (i.e., uaccess) and "[user] access" (i.e., access by, to, or on behalf of user(space)).
Introducing an architectural term when there is already a generic OS and Linux kernel term that means the right thing seemed not to improve readability, but I guess it's a matter of opinion.
Anyway, it doesn't really matter.
Same elsewhere.
+{
- if (system_supports_poe()) {
/** Enable all permissions in all 8 keys* (inspired by REPEAT_BYTE())*/u64 por_enable_all = (~0u / POE_MASK) * POE_RXW;Yikes!
Seriously though, why are we granting permissions that the signal handler isn't itself going to have over its own stack?
I think the logical thing to do is to think of the write/read of the signal frame as being done on behalf of the signal handler, so the permissions should be those we're going to give the signal handler: not less, and (so far as we can approximate) not more.
Will continue that discussion on the cover letter.
ua_state->por_el0 = read_sysreg_s(SYS_POR_EL0);write_sysreg_s(por_enable_all, SYS_POR_EL0);/* Ensure that any subsequent uaccess observes the updated value */isb();- }
+}
+/*
- Set the unpriv access state for invoking the signal handler.
- No uaccess should be done after that function is called.
- */
+static void set_handler_unpriv_access_state(void) +{
- if (system_supports_poe())
write_sysreg_s(POR_EL0_INIT, SYS_POR_EL0);Spurious blank line?
Thanks!
+}
[...]
@@ -1252,9 +1310,11 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set, { struct rt_sigframe_user_layout user; struct rt_sigframe __user *frame;
- struct unpriv_access_state ua_state; int err = 0;
fpsimd_signal_preserve_current_state();
- save_reset_unpriv_access_state(&ua_state);
(Trivial nit: maybe put the blank line before this rather than after? This has nothing to do with "settling" the kernel's internal context switch state, and a lot to do with generaing the signal frame...)
In fact considering the concern Catalin brought up with POR_EL0 being reset even when we fail to deliver the signal [1], I'm realising this call should be moved after get_sigframe(), since the latter doesn't use uaccess and can fail.
[1] https://lore.kernel.org/linux-arm-kernel/Zw6D2waVyIwYE7wd@arm.com/
if (get_sigframe(&user, ksig, regs)) return 1;
[...]
^
Ah, good point. The save_reset_unpriv_access_state(&ua_state) call probably belong just before the first __put_user() then.
@@ -1273,6 +1333,7 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set, regs->regs[1] = (unsigned long)&frame->info; regs->regs[2] = (unsigned long)&frame->uc; }
set_handler_unpriv_access_state();This bit feels prematurely factored? We don't have separate functions for the other low-level preparation done here...
I preferred to have a consistent API for all manipulations of POR_EL0, the idea being that if more registers are added to struct unpriv_access_state, only the *unpriv_access* helpers need to be amended.
Certainly if that struct grows more state, then the factoring will help in future. I wasn't clear on how we expect this all to evolve.
Either way, this is basically a non-issue, and keeping the symmetry is probably a good idea.
Cheers ---Dave