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)) {
+		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

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