| Message ID | 20241210142353.6457-3-yongxuan.wang@sifive.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: signal: fix the size of signal frame | expand |
Hi Yong-Xuan, Yong-Xuan Wang <yongxuan.wang@sifive.com> 於 2024年12月10日 週二 下午10:24寫道： > > The init_rt_signal_env() funciton is called before the alternative patch > is applied, so using the alternative-related API to check the availability > of an extension within this function doesn't have the intended effect. > Instead, the riscv_isa bitmap should be used to properly check whether > given extension is available. This patch also refactors the estimation of > signal_minsigstksz by extracting it from the get_rt_frame_size() funciton. > > Fixes: e92f469b0771 ("riscv: signal: Report signal frame size to userspace via auxv") > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com> > Reviewed-by: Zong Li <zong.li@sifive.com> > --- > arch/riscv/kernel/signal.c | 40 ++++++++++++++++++++++++-------------- > 1 file changed, 25 insertions(+), 15 deletions(-) > > diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c > index c3c517b9eee5..85eac84719e9 100644 > --- a/arch/riscv/kernel/signal.c > +++ b/arch/riscv/kernel/signal.c > @@ -203,22 +203,18 @@ static long restore_sigcontext(struct pt_regs *regs, > return err; > } > > -static size_t get_rt_frame_size(bool cal_all) > +static size_t get_rt_frame_size(struct pt_regs *regs) > { > struct rt_sigframe __user *frame; > - size_t frame_size; > + size_t frame_size = sizeof(*frame); > size_t total_context_size = 0; > > - frame_size = sizeof(*frame); > - > - if (has_vector()) { > - if (cal_all || riscv_v_vstate_query(task_pt_regs(current))) > - total_context_size += riscv_v_sc_size; > - } > + if (has_vector() && riscv_v_vstate_query(regs)) > + total_context_size += riscv_v_sc_size; > > frame_size += total_context_size; > - > frame_size = round_up(frame_size, 16); > + > return frame_size; > } > > @@ -228,7 +224,7 @@ SYSCALL_DEFINE0(rt_sigreturn) > struct rt_sigframe __user *frame; > struct task_struct *task; > sigset_t set; > - size_t frame_size = get_rt_frame_size(false); > + size_t frame_size = get_rt_frame_size(regs); > > /* Always make any pending restarted system calls return -EINTR */ > current->restart_block.fn = do_no_restart_syscall; > @@ -318,7 +314,7 @@ static int setup_rt_frame(struct ksignal *ksig, sigset_t *set, > struct rt_sigframe __user *frame; > long err = 0; > unsigned long __maybe_unused addr; > - size_t frame_size = get_rt_frame_size(false); > + size_t frame_size = get_rt_frame_size(regs); > > frame = get_sigframe(ksig, regs, frame_size); > if (!access_ok(frame, frame_size)) > @@ -465,19 +461,33 @@ void arch_do_signal_or_restart(struct pt_regs *regs) > void init_rt_signal_env(void); > void __init init_rt_signal_env(void) > { > - riscv_v_sc_size = sizeof(struct __riscv_ctx_hdr) + > - sizeof(struct __sc_riscv_v_state) + riscv_v_vsize; > + struct rt_sigframe __user *frame; > + size_t frame_size = sizeof(*frame); > + > + /* > + * init_rt_signal_env() is called before applying alternative patch. Do not use > + * __riscv_has_extension_likely()/__riscv_has_extension_unlikely() to check the > + * availiablity of an extension in this function. > + */ > + if (__riscv_isa_extension_available(NULL, RISCV_ISA_EXT_v)) { This should also check for IS_ENABLED(CONFIG_RISCV_ISA_V). Another way to do this is to call init_rt_signal_env() after apply_boot_alternative(). This can make assumptions on different piece of code more coherent. What do you think? > + riscv_v_sc_size = sizeof(struct __riscv_ctx_hdr) + > + sizeof(struct __sc_riscv_v_state) + riscv_v_vsize; > + frame_size += riscv_v_sc_size; > + } > + > + frame_size = round_up(frame_size, 16); > + > /* > * Determine the stack space required for guaranteed signal delivery. > * The signal_minsigstksz will be populated into the AT_MINSIGSTKSZ entry > * in the auxiliary array at process startup. > */ > - signal_minsigstksz = get_rt_frame_size(true); > + signal_minsigstksz = frame_size; > } > > #ifdef CONFIG_DYNAMIC_SIGFRAME > bool sigaltstack_size_valid(size_t ss_size) > { > - return ss_size > get_rt_frame_size(false); > + return ss_size > get_rt_frame_size(current_pt_regs()); > } > #endif /* CONFIG_DYNAMIC_SIGFRAME */ > -- > 2.17.1 > Thanks, Andy
Hi Andy, On Thu, Dec 12, 2024 at 12:17 AM Andy Chiu <andybnac@gmail.com> wrote: > > Hi Yong-Xuan, > > Yong-Xuan Wang <yongxuan.wang@sifive.com> 於 2024年12月10日 週二 下午10:24寫道： > > > > The init_rt_signal_env() funciton is called before the alternative patch > > is applied, so using the alternative-related API to check the availability > > of an extension within this function doesn't have the intended effect. > > Instead, the riscv_isa bitmap should be used to properly check whether > > given extension is available. This patch also refactors the estimation of > > signal_minsigstksz by extracting it from the get_rt_frame_size() funciton. > > > > Fixes: e92f469b0771 ("riscv: signal: Report signal frame size to userspace via auxv") > > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com> > > Reviewed-by: Zong Li <zong.li@sifive.com> > > --- > > arch/riscv/kernel/signal.c | 40 ++++++++++++++++++++++++-------------- > > 1 file changed, 25 insertions(+), 15 deletions(-) > > > > diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c > > index c3c517b9eee5..85eac84719e9 100644 > > --- a/arch/riscv/kernel/signal.c > > +++ b/arch/riscv/kernel/signal.c > > @@ -203,22 +203,18 @@ static long restore_sigcontext(struct pt_regs *regs, > > return err; > > } > > > > -static size_t get_rt_frame_size(bool cal_all) > > +static size_t get_rt_frame_size(struct pt_regs *regs) > > { > > struct rt_sigframe __user *frame; > > - size_t frame_size; > > + size_t frame_size = sizeof(*frame); > > size_t total_context_size = 0; > > > > - frame_size = sizeof(*frame); > > - > > - if (has_vector()) { > > - if (cal_all || riscv_v_vstate_query(task_pt_regs(current))) > > - total_context_size += riscv_v_sc_size; > > - } > > + if (has_vector() && riscv_v_vstate_query(regs)) > > + total_context_size += riscv_v_sc_size; > > > > frame_size += total_context_size; > > - > > frame_size = round_up(frame_size, 16); > > + > > return frame_size; > > } > > > > @@ -228,7 +224,7 @@ SYSCALL_DEFINE0(rt_sigreturn) > > struct rt_sigframe __user *frame; > > struct task_struct *task; > > sigset_t set; > > - size_t frame_size = get_rt_frame_size(false); > > + size_t frame_size = get_rt_frame_size(regs); > > > > /* Always make any pending restarted system calls return -EINTR */ > > current->restart_block.fn = do_no_restart_syscall; > > @@ -318,7 +314,7 @@ static int setup_rt_frame(struct ksignal *ksig, sigset_t *set, > > struct rt_sigframe __user *frame; > > long err = 0; > > unsigned long __maybe_unused addr; > > - size_t frame_size = get_rt_frame_size(false); > > + size_t frame_size = get_rt_frame_size(regs); > > > > frame = get_sigframe(ksig, regs, frame_size); > > if (!access_ok(frame, frame_size)) > > @@ -465,19 +461,33 @@ void arch_do_signal_or_restart(struct pt_regs *regs) > > void init_rt_signal_env(void); > > void __init init_rt_signal_env(void) > > { > > - riscv_v_sc_size = sizeof(struct __riscv_ctx_hdr) + > > - sizeof(struct __sc_riscv_v_state) + riscv_v_vsize; > > + struct rt_sigframe __user *frame; > > + size_t frame_size = sizeof(*frame); > > + > > + /* > > + * init_rt_signal_env() is called before applying alternative patch. Do not use > > + * __riscv_has_extension_likely()/__riscv_has_extension_unlikely() to check the > > + * availiablity of an extension in this function. > > + */ > > + if (__riscv_isa_extension_available(NULL, RISCV_ISA_EXT_v)) { > > This should also check for IS_ENABLED(CONFIG_RISCV_ISA_V). Another way > to do this is to call init_rt_signal_env() after > apply_boot_alternative(). This can make assumptions on different piece > of code more coherent. What do you think? > Reorder the init_rt_signal_env() and apply_boot_alternative() is better, I will fix it in the next version. Thank you! Regards, Yong-Xuan > > > + riscv_v_sc_size = sizeof(struct __riscv_ctx_hdr) + > > + sizeof(struct __sc_riscv_v_state) + riscv_v_vsize; > > + frame_size += riscv_v_sc_size; > > + } > > + > > + frame_size = round_up(frame_size, 16); > > + > > /* > > * Determine the stack space required for guaranteed signal delivery. > > * The signal_minsigstksz will be populated into the AT_MINSIGSTKSZ entry > > * in the auxiliary array at process startup. > > */ > > - signal_minsigstksz = get_rt_frame_size(true); > > + signal_minsigstksz = frame_size; > > } > > > > #ifdef CONFIG_DYNAMIC_SIGFRAME > > bool sigaltstack_size_valid(size_t ss_size) > > { > > - return ss_size > get_rt_frame_size(false); > > + return ss_size > get_rt_frame_size(current_pt_regs()); > > } > > #endif /* CONFIG_DYNAMIC_SIGFRAME */ > > -- > > 2.17.1 > > > > Thanks, > Andy
diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c index c3c517b9eee5..85eac84719e9 100644 --- a/arch/riscv/kernel/signal.c +++ b/arch/riscv/kernel/signal.c @@ -203,22 +203,18 @@ static long restore_sigcontext(struct pt_regs *regs, return err; } -static size_t get_rt_frame_size(bool cal_all) +static size_t get_rt_frame_size(struct pt_regs *regs) { struct rt_sigframe __user *frame; - size_t frame_size; + size_t frame_size = sizeof(*frame); size_t total_context_size = 0; - frame_size = sizeof(*frame); - - if (has_vector()) { - if (cal_all || riscv_v_vstate_query(task_pt_regs(current))) - total_context_size += riscv_v_sc_size; - } + if (has_vector() && riscv_v_vstate_query(regs)) + total_context_size += riscv_v_sc_size; frame_size += total_context_size; - frame_size = round_up(frame_size, 16); + return frame_size; } @@ -228,7 +224,7 @@ SYSCALL_DEFINE0(rt_sigreturn) struct rt_sigframe __user *frame; struct task_struct *task; sigset_t set; - size_t frame_size = get_rt_frame_size(false); + size_t frame_size = get_rt_frame_size(regs); /* Always make any pending restarted system calls return -EINTR */ current->restart_block.fn = do_no_restart_syscall; @@ -318,7 +314,7 @@ static int setup_rt_frame(struct ksignal *ksig, sigset_t *set, struct rt_sigframe __user *frame; long err = 0; unsigned long __maybe_unused addr; - size_t frame_size = get_rt_frame_size(false); + size_t frame_size = get_rt_frame_size(regs); frame = get_sigframe(ksig, regs, frame_size); if (!access_ok(frame, frame_size)) @@ -465,19 +461,33 @@ void arch_do_signal_or_restart(struct pt_regs *regs) void init_rt_signal_env(void); void __init init_rt_signal_env(void) { - riscv_v_sc_size = sizeof(struct __riscv_ctx_hdr) + - sizeof(struct __sc_riscv_v_state) + riscv_v_vsize; + struct rt_sigframe __user *frame; + size_t frame_size = sizeof(*frame); + + /* + * init_rt_signal_env() is called before applying alternative patch. Do not use + * __riscv_has_extension_likely()/__riscv_has_extension_unlikely() to check the + * availiablity of an extension in this function. + */ + if (__riscv_isa_extension_available(NULL, RISCV_ISA_EXT_v)) { + riscv_v_sc_size = sizeof(struct __riscv_ctx_hdr) + + sizeof(struct __sc_riscv_v_state) + riscv_v_vsize; + frame_size += riscv_v_sc_size; + } + + frame_size = round_up(frame_size, 16); + /* * Determine the stack space required for guaranteed signal delivery. * The signal_minsigstksz will be populated into the AT_MINSIGSTKSZ entry * in the auxiliary array at process startup. */ - signal_minsigstksz = get_rt_frame_size(true); + signal_minsigstksz = frame_size; } #ifdef CONFIG_DYNAMIC_SIGFRAME bool sigaltstack_size_valid(size_t ss_size) { - return ss_size > get_rt_frame_size(false); + return ss_size > get_rt_frame_size(current_pt_regs()); } #endif /* CONFIG_DYNAMIC_SIGFRAME */
The init_rt_signal_env() funciton is called before the alternative patch is applied, so using the alternative-related API to check the availability of an extension within this function doesn't have the intended effect. Instead, the riscv_isa bitmap should be used to properly check whether given extension is available. This patch also refactors the estimation of signal_minsigstksz by extracting it from the get_rt_frame_size() funciton. Fixes: e92f469b0771 ("riscv: signal: Report signal frame size to userspace via auxv") Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com> Reviewed-by: Zong Li <zong.li@sifive.com> --- arch/riscv/kernel/signal.c | 40 ++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 15 deletions(-)