All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] riscv: prevent null-pointer dereference with sbi_remote_fence_i
@ 2021-12-13 11:20 ` Heiko Stuebner
  0 siblings, 0 replies; 12+ messages in thread
From: Heiko Stuebner @ 2021-12-13 11:20 UTC (permalink / raw)
  To: paul.walmsley, palmer, aou
  Cc: atishp, anup, jszhang, christoph.muellner, philipp.tomsich, mick,
	linux-riscv, linux-kernel, Heiko Stuebner

The callback used inside sbi_remote_fence_i is set at sbi probe time
to the needed variant. Before that it is a NULL pointer.

The selection between using sbi_remote_fence_i or ipi_remote_fence_i
right now is solely done on the presence of the RISCV_SBI config option.

On a multiplatform kernel, SBI will probably always be built in, but
in the future not all machines using that kernel may have SBI
on them, so in some cases this setup can lead to NULL pointer dereferences.

Also if in future one of the flush_icache_all / flush_icache_mm functions
gets called earlier in the boot process - before sbi_init - this would
trigger the issue.

To prevent this, add a default __sbi_rfence_none returning an error code
and adapt the callers to check for an error from the remote fence
to then fall back to the other method.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/riscv/kernel/sbi.c    | 10 +++++++++-
 arch/riscv/mm/cacheflush.c | 25 +++++++++++++++++--------
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
index 7402a417f38e..69d0a96b97d0 100644
--- a/arch/riscv/kernel/sbi.c
+++ b/arch/riscv/kernel/sbi.c
@@ -14,11 +14,19 @@
 unsigned long sbi_spec_version __ro_after_init = SBI_SPEC_VERSION_DEFAULT;
 EXPORT_SYMBOL(sbi_spec_version);
 
+static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
+			     unsigned long start, unsigned long size,
+			     unsigned long arg4, unsigned long arg5)
+{
+	return -EOPNOTSUPP;
+}
+
 static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init;
 static int (*__sbi_send_ipi)(const unsigned long *hart_mask) __ro_after_init;
 static int (*__sbi_rfence)(int fid, const unsigned long *hart_mask,
 			   unsigned long start, unsigned long size,
-			   unsigned long arg4, unsigned long arg5) __ro_after_init;
+			   unsigned long arg4, unsigned long arg5)
+			   __ro_after_init = __sbi_rfence_none;
 
 struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
 			unsigned long arg1, unsigned long arg2,
diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
index 89f81067e09e..128e23c094ea 100644
--- a/arch/riscv/mm/cacheflush.c
+++ b/arch/riscv/mm/cacheflush.c
@@ -16,11 +16,15 @@ static void ipi_remote_fence_i(void *info)
 
 void flush_icache_all(void)
 {
+	int ret = -EINVAL;
+
 	local_flush_icache_all();
 
 	if (IS_ENABLED(CONFIG_RISCV_SBI))
-		sbi_remote_fence_i(NULL);
-	else
+		ret = sbi_remote_fence_i(NULL);
+
+	/* fall back to ipi_remote_fence_i if sbi failed or not available */
+	if (ret)
 		on_each_cpu(ipi_remote_fence_i, NULL, 1);
 }
 EXPORT_SYMBOL(flush_icache_all);
@@ -66,13 +70,18 @@ void flush_icache_mm(struct mm_struct *mm, bool local)
 		 * with flush_icache_deferred().
 		 */
 		smp_mb();
-	} else if (IS_ENABLED(CONFIG_RISCV_SBI)) {
-		cpumask_t hartid_mask;
-
-		riscv_cpuid_to_hartid_mask(&others, &hartid_mask);
-		sbi_remote_fence_i(cpumask_bits(&hartid_mask));
 	} else {
-		on_each_cpu_mask(&others, ipi_remote_fence_i, NULL, 1);
+		int ret = -EINVAL;
+
+		if (IS_ENABLED(CONFIG_RISCV_SBI)) {
+			cpumask_t hartid_mask;
+
+			riscv_cpuid_to_hartid_mask(&others, &hartid_mask);
+			ret = sbi_remote_fence_i(cpumask_bits(&hartid_mask));
+		}
+
+		if (ret)
+			on_each_cpu_mask(&others, ipi_remote_fence_i, NULL, 1);
 	}
 
 	preempt_enable();
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 1/2] riscv: prevent null-pointer dereference with sbi_remote_fence_i
@ 2021-12-13 11:20 ` Heiko Stuebner
  0 siblings, 0 replies; 12+ messages in thread
From: Heiko Stuebner @ 2021-12-13 11:20 UTC (permalink / raw)
  To: paul.walmsley, palmer, aou
  Cc: atishp, anup, jszhang, christoph.muellner, philipp.tomsich, mick,
	linux-riscv, linux-kernel, Heiko Stuebner

The callback used inside sbi_remote_fence_i is set at sbi probe time
to the needed variant. Before that it is a NULL pointer.

The selection between using sbi_remote_fence_i or ipi_remote_fence_i
right now is solely done on the presence of the RISCV_SBI config option.

On a multiplatform kernel, SBI will probably always be built in, but
in the future not all machines using that kernel may have SBI
on them, so in some cases this setup can lead to NULL pointer dereferences.

Also if in future one of the flush_icache_all / flush_icache_mm functions
gets called earlier in the boot process - before sbi_init - this would
trigger the issue.

To prevent this, add a default __sbi_rfence_none returning an error code
and adapt the callers to check for an error from the remote fence
to then fall back to the other method.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/riscv/kernel/sbi.c    | 10 +++++++++-
 arch/riscv/mm/cacheflush.c | 25 +++++++++++++++++--------
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
index 7402a417f38e..69d0a96b97d0 100644
--- a/arch/riscv/kernel/sbi.c
+++ b/arch/riscv/kernel/sbi.c
@@ -14,11 +14,19 @@
 unsigned long sbi_spec_version __ro_after_init = SBI_SPEC_VERSION_DEFAULT;
 EXPORT_SYMBOL(sbi_spec_version);
 
+static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
+			     unsigned long start, unsigned long size,
+			     unsigned long arg4, unsigned long arg5)
+{
+	return -EOPNOTSUPP;
+}
+
 static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init;
 static int (*__sbi_send_ipi)(const unsigned long *hart_mask) __ro_after_init;
 static int (*__sbi_rfence)(int fid, const unsigned long *hart_mask,
 			   unsigned long start, unsigned long size,
-			   unsigned long arg4, unsigned long arg5) __ro_after_init;
+			   unsigned long arg4, unsigned long arg5)
+			   __ro_after_init = __sbi_rfence_none;
 
 struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
 			unsigned long arg1, unsigned long arg2,
diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
index 89f81067e09e..128e23c094ea 100644
--- a/arch/riscv/mm/cacheflush.c
+++ b/arch/riscv/mm/cacheflush.c
@@ -16,11 +16,15 @@ static void ipi_remote_fence_i(void *info)
 
 void flush_icache_all(void)
 {
+	int ret = -EINVAL;
+
 	local_flush_icache_all();
 
 	if (IS_ENABLED(CONFIG_RISCV_SBI))
-		sbi_remote_fence_i(NULL);
-	else
+		ret = sbi_remote_fence_i(NULL);
+
+	/* fall back to ipi_remote_fence_i if sbi failed or not available */
+	if (ret)
 		on_each_cpu(ipi_remote_fence_i, NULL, 1);
 }
 EXPORT_SYMBOL(flush_icache_all);
@@ -66,13 +70,18 @@ void flush_icache_mm(struct mm_struct *mm, bool local)
 		 * with flush_icache_deferred().
 		 */
 		smp_mb();
-	} else if (IS_ENABLED(CONFIG_RISCV_SBI)) {
-		cpumask_t hartid_mask;
-
-		riscv_cpuid_to_hartid_mask(&others, &hartid_mask);
-		sbi_remote_fence_i(cpumask_bits(&hartid_mask));
 	} else {
-		on_each_cpu_mask(&others, ipi_remote_fence_i, NULL, 1);
+		int ret = -EINVAL;
+
+		if (IS_ENABLED(CONFIG_RISCV_SBI)) {
+			cpumask_t hartid_mask;
+
+			riscv_cpuid_to_hartid_mask(&others, &hartid_mask);
+			ret = sbi_remote_fence_i(cpumask_bits(&hartid_mask));
+		}
+
+		if (ret)
+			on_each_cpu_mask(&others, ipi_remote_fence_i, NULL, 1);
 	}
 
 	preempt_enable();
-- 
2.30.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] riscv: provide default implementations for __sbi_set_timer and __sbi_send_ipi
  2021-12-13 11:20 ` Heiko Stuebner
@ 2021-12-13 11:20   ` Heiko Stuebner
  -1 siblings, 0 replies; 12+ messages in thread
From: Heiko Stuebner @ 2021-12-13 11:20 UTC (permalink / raw)
  To: paul.walmsley, palmer, aou
  Cc: atishp, anup, jszhang, christoph.muellner, philipp.tomsich, mick,
	linux-riscv, linux-kernel, Heiko Stuebner

The mentioned function pointers get called from different sbi functions
which may get called from other areas of the kernel without fully
checking if the sbi initialization was done.

So similarly to sbi_remote_fence_i, provide empty functions for them
to prevent any null-pointer dereferences in the future.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/riscv/kernel/sbi.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
index 69d0a96b97d0..6a21345c6712 100644
--- a/arch/riscv/kernel/sbi.c
+++ b/arch/riscv/kernel/sbi.c
@@ -14,6 +14,13 @@
 unsigned long sbi_spec_version __ro_after_init = SBI_SPEC_VERSION_DEFAULT;
 EXPORT_SYMBOL(sbi_spec_version);
 
+static void __sbi_set_timer_none(uint64_t stime_value) {}
+
+static int __sbi_send_ipi_none(const unsigned long *hart_mask)
+{
+	return -EOPNOTSUPP;
+}
+
 static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
 			     unsigned long start, unsigned long size,
 			     unsigned long arg4, unsigned long arg5)
@@ -21,8 +28,9 @@ static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
 	return -EOPNOTSUPP;
 }
 
-static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init;
-static int (*__sbi_send_ipi)(const unsigned long *hart_mask) __ro_after_init;
+static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init = __sbi_set_timer_none;
+static int (*__sbi_send_ipi)(const unsigned long *hart_mask)
+			    __ro_after_init = __sbi_send_ipi_none;
 static int (*__sbi_rfence)(int fid, const unsigned long *hart_mask,
 			   unsigned long start, unsigned long size,
 			   unsigned long arg4, unsigned long arg5)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] riscv: provide default implementations for __sbi_set_timer and __sbi_send_ipi
@ 2021-12-13 11:20   ` Heiko Stuebner
  0 siblings, 0 replies; 12+ messages in thread
From: Heiko Stuebner @ 2021-12-13 11:20 UTC (permalink / raw)
  To: paul.walmsley, palmer, aou
  Cc: atishp, anup, jszhang, christoph.muellner, philipp.tomsich, mick,
	linux-riscv, linux-kernel, Heiko Stuebner

The mentioned function pointers get called from different sbi functions
which may get called from other areas of the kernel without fully
checking if the sbi initialization was done.

So similarly to sbi_remote_fence_i, provide empty functions for them
to prevent any null-pointer dereferences in the future.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/riscv/kernel/sbi.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
index 69d0a96b97d0..6a21345c6712 100644
--- a/arch/riscv/kernel/sbi.c
+++ b/arch/riscv/kernel/sbi.c
@@ -14,6 +14,13 @@
 unsigned long sbi_spec_version __ro_after_init = SBI_SPEC_VERSION_DEFAULT;
 EXPORT_SYMBOL(sbi_spec_version);
 
+static void __sbi_set_timer_none(uint64_t stime_value) {}
+
+static int __sbi_send_ipi_none(const unsigned long *hart_mask)
+{
+	return -EOPNOTSUPP;
+}
+
 static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
 			     unsigned long start, unsigned long size,
 			     unsigned long arg4, unsigned long arg5)
@@ -21,8 +28,9 @@ static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
 	return -EOPNOTSUPP;
 }
 
-static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init;
-static int (*__sbi_send_ipi)(const unsigned long *hart_mask) __ro_after_init;
+static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init = __sbi_set_timer_none;
+static int (*__sbi_send_ipi)(const unsigned long *hart_mask)
+			    __ro_after_init = __sbi_send_ipi_none;
 static int (*__sbi_rfence)(int fid, const unsigned long *hart_mask,
 			   unsigned long start, unsigned long size,
 			   unsigned long arg4, unsigned long arg5)
-- 
2.30.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] riscv: prevent null-pointer dereference with sbi_remote_fence_i
  2021-12-13 11:20 ` Heiko Stuebner
@ 2021-12-14  1:51   ` Atish Patra
  -1 siblings, 0 replies; 12+ messages in thread
From: Atish Patra @ 2021-12-14  1:51 UTC (permalink / raw)
  To: Heiko Stuebner
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Anup Patel,
	Jisheng Zhang, Christoph Müllner, Philipp Tomsich,
	Nick Kossifidis, linux-riscv, linux-kernel@vger.kernel.org List

On Mon, Dec 13, 2021 at 3:21 AM Heiko Stuebner <heiko@sntech.de> wrote:
>
> The callback used inside sbi_remote_fence_i is set at sbi probe time
> to the needed variant. Before that it is a NULL pointer.
>
> The selection between using sbi_remote_fence_i or ipi_remote_fence_i
> right now is solely done on the presence of the RISCV_SBI config option.
>
> On a multiplatform kernel, SBI will probably always be built in, but
> in the future not all machines using that kernel may have SBI
> on them, so in some cases this setup can lead to NULL pointer dereferences.
>

I didn't quite understand this. The only platforms without SBI are
M-mode Linux based platforms.
All other platforms will require SBI if the kernel is running on
supervisor mode.

They may not need SBI IPI or SBI RFENCE extensions if an alternate
mechanism(ACLINT SSWI or AIA IMSIC)
is already available in hardware. IIRC, Anup's aclint & aia series
already takes care of that.

> Also if in future one of the flush_icache_all / flush_icache_mm functions
> gets called earlier in the boot process - before sbi_init - this would
> trigger the issue.
>

sbi_init is called before setup_smp. Do you think there is a use case where
flush_icache_xxx can be called before smp initialization ?

> To prevent this, add a default __sbi_rfence_none returning an error code
> and adapt the callers to check for an error from the remote fence
> to then fall back to the other method.
>

Having said that, it is always good to have guards to avoid NULL
pointer dereferences.
However, the commit text may be updated based on the above questions.

> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  arch/riscv/kernel/sbi.c    | 10 +++++++++-
>  arch/riscv/mm/cacheflush.c | 25 +++++++++++++++++--------
>  2 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index 7402a417f38e..69d0a96b97d0 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -14,11 +14,19 @@
>  unsigned long sbi_spec_version __ro_after_init = SBI_SPEC_VERSION_DEFAULT;
>  EXPORT_SYMBOL(sbi_spec_version);
>
> +static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
> +                            unsigned long start, unsigned long size,
> +                            unsigned long arg4, unsigned long arg5)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
>  static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init;
>  static int (*__sbi_send_ipi)(const unsigned long *hart_mask) __ro_after_init;
>  static int (*__sbi_rfence)(int fid, const unsigned long *hart_mask,
>                            unsigned long start, unsigned long size,
> -                          unsigned long arg4, unsigned long arg5) __ro_after_init;
> +                          unsigned long arg4, unsigned long arg5)
> +                          __ro_after_init = __sbi_rfence_none;
>
>  struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
>                         unsigned long arg1, unsigned long arg2,
> diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> index 89f81067e09e..128e23c094ea 100644
> --- a/arch/riscv/mm/cacheflush.c
> +++ b/arch/riscv/mm/cacheflush.c
> @@ -16,11 +16,15 @@ static void ipi_remote_fence_i(void *info)
>
>  void flush_icache_all(void)
>  {
> +       int ret = -EINVAL;
> +
>         local_flush_icache_all();
>
>         if (IS_ENABLED(CONFIG_RISCV_SBI))
> -               sbi_remote_fence_i(NULL);
> -       else
> +               ret = sbi_remote_fence_i(NULL);
> +
> +       /* fall back to ipi_remote_fence_i if sbi failed or not available */
> +       if (ret)
>                 on_each_cpu(ipi_remote_fence_i, NULL, 1);
>  }
>  EXPORT_SYMBOL(flush_icache_all);
> @@ -66,13 +70,18 @@ void flush_icache_mm(struct mm_struct *mm, bool local)
>                  * with flush_icache_deferred().
>                  */
>                 smp_mb();
> -       } else if (IS_ENABLED(CONFIG_RISCV_SBI)) {
> -               cpumask_t hartid_mask;
> -
> -               riscv_cpuid_to_hartid_mask(&others, &hartid_mask);
> -               sbi_remote_fence_i(cpumask_bits(&hartid_mask));
>         } else {
> -               on_each_cpu_mask(&others, ipi_remote_fence_i, NULL, 1);
> +               int ret = -EINVAL;
> +
> +               if (IS_ENABLED(CONFIG_RISCV_SBI)) {
> +                       cpumask_t hartid_mask;
> +
> +                       riscv_cpuid_to_hartid_mask(&others, &hartid_mask);
> +                       ret = sbi_remote_fence_i(cpumask_bits(&hartid_mask));

This API is removed in sparse hartid series[1]. You may want to rebase
on top of it.

[1] https://patchwork.kernel.org/project/linux-riscv/list/?series=590195

> +               }
> +
> +               if (ret)
> +                       on_each_cpu_mask(&others, ipi_remote_fence_i, NULL, 1);
>         }
>
>         preempt_enable();
> --
> 2.30.2
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv



-- 
Regards,
Atish

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] riscv: prevent null-pointer dereference with sbi_remote_fence_i
@ 2021-12-14  1:51   ` Atish Patra
  0 siblings, 0 replies; 12+ messages in thread
From: Atish Patra @ 2021-12-14  1:51 UTC (permalink / raw)
  To: Heiko Stuebner
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Anup Patel,
	Jisheng Zhang, Christoph Müllner, Philipp Tomsich,
	Nick Kossifidis, linux-riscv, linux-kernel@vger.kernel.org List

On Mon, Dec 13, 2021 at 3:21 AM Heiko Stuebner <heiko@sntech.de> wrote:
>
> The callback used inside sbi_remote_fence_i is set at sbi probe time
> to the needed variant. Before that it is a NULL pointer.
>
> The selection between using sbi_remote_fence_i or ipi_remote_fence_i
> right now is solely done on the presence of the RISCV_SBI config option.
>
> On a multiplatform kernel, SBI will probably always be built in, but
> in the future not all machines using that kernel may have SBI
> on them, so in some cases this setup can lead to NULL pointer dereferences.
>

I didn't quite understand this. The only platforms without SBI are
M-mode Linux based platforms.
All other platforms will require SBI if the kernel is running on
supervisor mode.

They may not need SBI IPI or SBI RFENCE extensions if an alternate
mechanism(ACLINT SSWI or AIA IMSIC)
is already available in hardware. IIRC, Anup's aclint & aia series
already takes care of that.

> Also if in future one of the flush_icache_all / flush_icache_mm functions
> gets called earlier in the boot process - before sbi_init - this would
> trigger the issue.
>

sbi_init is called before setup_smp. Do you think there is a use case where
flush_icache_xxx can be called before smp initialization ?

> To prevent this, add a default __sbi_rfence_none returning an error code
> and adapt the callers to check for an error from the remote fence
> to then fall back to the other method.
>

Having said that, it is always good to have guards to avoid NULL
pointer dereferences.
However, the commit text may be updated based on the above questions.

> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  arch/riscv/kernel/sbi.c    | 10 +++++++++-
>  arch/riscv/mm/cacheflush.c | 25 +++++++++++++++++--------
>  2 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index 7402a417f38e..69d0a96b97d0 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -14,11 +14,19 @@
>  unsigned long sbi_spec_version __ro_after_init = SBI_SPEC_VERSION_DEFAULT;
>  EXPORT_SYMBOL(sbi_spec_version);
>
> +static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
> +                            unsigned long start, unsigned long size,
> +                            unsigned long arg4, unsigned long arg5)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
>  static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init;
>  static int (*__sbi_send_ipi)(const unsigned long *hart_mask) __ro_after_init;
>  static int (*__sbi_rfence)(int fid, const unsigned long *hart_mask,
>                            unsigned long start, unsigned long size,
> -                          unsigned long arg4, unsigned long arg5) __ro_after_init;
> +                          unsigned long arg4, unsigned long arg5)
> +                          __ro_after_init = __sbi_rfence_none;
>
>  struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
>                         unsigned long arg1, unsigned long arg2,
> diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> index 89f81067e09e..128e23c094ea 100644
> --- a/arch/riscv/mm/cacheflush.c
> +++ b/arch/riscv/mm/cacheflush.c
> @@ -16,11 +16,15 @@ static void ipi_remote_fence_i(void *info)
>
>  void flush_icache_all(void)
>  {
> +       int ret = -EINVAL;
> +
>         local_flush_icache_all();
>
>         if (IS_ENABLED(CONFIG_RISCV_SBI))
> -               sbi_remote_fence_i(NULL);
> -       else
> +               ret = sbi_remote_fence_i(NULL);
> +
> +       /* fall back to ipi_remote_fence_i if sbi failed or not available */
> +       if (ret)
>                 on_each_cpu(ipi_remote_fence_i, NULL, 1);
>  }
>  EXPORT_SYMBOL(flush_icache_all);
> @@ -66,13 +70,18 @@ void flush_icache_mm(struct mm_struct *mm, bool local)
>                  * with flush_icache_deferred().
>                  */
>                 smp_mb();
> -       } else if (IS_ENABLED(CONFIG_RISCV_SBI)) {
> -               cpumask_t hartid_mask;
> -
> -               riscv_cpuid_to_hartid_mask(&others, &hartid_mask);
> -               sbi_remote_fence_i(cpumask_bits(&hartid_mask));
>         } else {
> -               on_each_cpu_mask(&others, ipi_remote_fence_i, NULL, 1);
> +               int ret = -EINVAL;
> +
> +               if (IS_ENABLED(CONFIG_RISCV_SBI)) {
> +                       cpumask_t hartid_mask;
> +
> +                       riscv_cpuid_to_hartid_mask(&others, &hartid_mask);
> +                       ret = sbi_remote_fence_i(cpumask_bits(&hartid_mask));

This API is removed in sparse hartid series[1]. You may want to rebase
on top of it.

[1] https://patchwork.kernel.org/project/linux-riscv/list/?series=590195

> +               }
> +
> +               if (ret)
> +                       on_each_cpu_mask(&others, ipi_remote_fence_i, NULL, 1);
>         }
>
>         preempt_enable();
> --
> 2.30.2
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv



-- 
Regards,
Atish

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] riscv: provide default implementations for __sbi_set_timer and __sbi_send_ipi
  2021-12-13 11:20   ` Heiko Stuebner
@ 2021-12-14  2:10     ` Atish Patra
  -1 siblings, 0 replies; 12+ messages in thread
From: Atish Patra @ 2021-12-14  2:10 UTC (permalink / raw)
  To: Heiko Stuebner
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Anup Patel,
	Jisheng Zhang, Christoph Müllner, Philipp Tomsich,
	Nick Kossifidis, linux-riscv, linux-kernel@vger.kernel.org List

On Mon, Dec 13, 2021 at 3:21 AM Heiko Stuebner <heiko@sntech.de> wrote:
>
> The mentioned function pointers get called from different sbi functions
> which may get called from other areas of the kernel without fully
> checking if the sbi initialization was done.

SBI initialization happens in sbi_init which is called from setup_arch.
setup_smp happens after that. Thus, there won't be an IPI issued
before SMP is set up.

For __sbi_set_timer, the first time it will be called from this path.
time_init->timer_probe->riscv_timer_init_dt

time_init is called from start_kernel after setup_arch. In fact,
setup_arch is called very early
in the start_kernel.

Is there any other scenario where these SBI functions can be invoked
before SBI is initialized ?

>
> So similarly to sbi_remote_fence_i, provide empty functions for them
> to prevent any null-pointer dereferences in the future.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  arch/riscv/kernel/sbi.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index 69d0a96b97d0..6a21345c6712 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -14,6 +14,13 @@
>  unsigned long sbi_spec_version __ro_after_init = SBI_SPEC_VERSION_DEFAULT;
>  EXPORT_SYMBOL(sbi_spec_version);
>
> +static void __sbi_set_timer_none(uint64_t stime_value) {}
> +
> +static int __sbi_send_ipi_none(const unsigned long *hart_mask)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
>  static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
>                              unsigned long start, unsigned long size,
>                              unsigned long arg4, unsigned long arg5)
> @@ -21,8 +28,9 @@ static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
>         return -EOPNOTSUPP;
>  }
>
> -static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init;
> -static int (*__sbi_send_ipi)(const unsigned long *hart_mask) __ro_after_init;
> +static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init = __sbi_set_timer_none;
> +static int (*__sbi_send_ipi)(const unsigned long *hart_mask)
> +                           __ro_after_init = __sbi_send_ipi_none;
>  static int (*__sbi_rfence)(int fid, const unsigned long *hart_mask,
>                            unsigned long start, unsigned long size,
>                            unsigned long arg4, unsigned long arg5)
> --
> 2.30.2
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv



-- 
Regards,
Atish

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] riscv: provide default implementations for __sbi_set_timer and __sbi_send_ipi
@ 2021-12-14  2:10     ` Atish Patra
  0 siblings, 0 replies; 12+ messages in thread
From: Atish Patra @ 2021-12-14  2:10 UTC (permalink / raw)
  To: Heiko Stuebner
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Anup Patel,
	Jisheng Zhang, Christoph Müllner, Philipp Tomsich,
	Nick Kossifidis, linux-riscv, linux-kernel@vger.kernel.org List

On Mon, Dec 13, 2021 at 3:21 AM Heiko Stuebner <heiko@sntech.de> wrote:
>
> The mentioned function pointers get called from different sbi functions
> which may get called from other areas of the kernel without fully
> checking if the sbi initialization was done.

SBI initialization happens in sbi_init which is called from setup_arch.
setup_smp happens after that. Thus, there won't be an IPI issued
before SMP is set up.

For __sbi_set_timer, the first time it will be called from this path.
time_init->timer_probe->riscv_timer_init_dt

time_init is called from start_kernel after setup_arch. In fact,
setup_arch is called very early
in the start_kernel.

Is there any other scenario where these SBI functions can be invoked
before SBI is initialized ?

>
> So similarly to sbi_remote_fence_i, provide empty functions for them
> to prevent any null-pointer dereferences in the future.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  arch/riscv/kernel/sbi.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index 69d0a96b97d0..6a21345c6712 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -14,6 +14,13 @@
>  unsigned long sbi_spec_version __ro_after_init = SBI_SPEC_VERSION_DEFAULT;
>  EXPORT_SYMBOL(sbi_spec_version);
>
> +static void __sbi_set_timer_none(uint64_t stime_value) {}
> +
> +static int __sbi_send_ipi_none(const unsigned long *hart_mask)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
>  static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
>                              unsigned long start, unsigned long size,
>                              unsigned long arg4, unsigned long arg5)
> @@ -21,8 +28,9 @@ static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
>         return -EOPNOTSUPP;
>  }
>
> -static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init;
> -static int (*__sbi_send_ipi)(const unsigned long *hart_mask) __ro_after_init;
> +static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init = __sbi_set_timer_none;
> +static int (*__sbi_send_ipi)(const unsigned long *hart_mask)
> +                           __ro_after_init = __sbi_send_ipi_none;
>  static int (*__sbi_rfence)(int fid, const unsigned long *hart_mask,
>                            unsigned long start, unsigned long size,
>                            unsigned long arg4, unsigned long arg5)
> --
> 2.30.2
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv



-- 
Regards,
Atish

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] riscv: prevent null-pointer dereference with sbi_remote_fence_i
  2021-12-14  1:51   ` Atish Patra
@ 2021-12-14 10:41     ` Heiko Stuebner
  -1 siblings, 0 replies; 12+ messages in thread
From: Heiko Stuebner @ 2021-12-14 10:41 UTC (permalink / raw)
  To: Atish Patra
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Anup Patel,
	Jisheng Zhang, Christoph Müllner, Philipp Tomsich,
	Nick Kossifidis, linux-riscv, linux-kernel@vger.kernel.org List

Hi Atish,

Am Dienstag, 14. Dezember 2021, 02:51:17 CET schrieb Atish Patra:
> On Mon, Dec 13, 2021 at 3:21 AM Heiko Stuebner <heiko@sntech.de> wrote:
> >
> > The callback used inside sbi_remote_fence_i is set at sbi probe time
> > to the needed variant. Before that it is a NULL pointer.
> >
> > The selection between using sbi_remote_fence_i or ipi_remote_fence_i
> > right now is solely done on the presence of the RISCV_SBI config option.
> >
> > On a multiplatform kernel, SBI will probably always be built in, but
> > in the future not all machines using that kernel may have SBI
> > on them, so in some cases this setup can lead to NULL pointer dereferences.
> >
> 
> I didn't quite understand this. The only platforms without SBI are
> M-mode Linux based platforms.
> All other platforms will require SBI if the kernel is running on
> supervisor mode.

Ok ... but imagine a single-core board starting in S-mode.
Not doing smp, kvm, etc.

So I can very well imagine the possibility that people
might start it without an sbi instance running ;-) .


> They may not need SBI IPI or SBI RFENCE extensions if an alternate
> mechanism(ACLINT SSWI or AIA IMSIC)
> is already available in hardware. IIRC, Anup's aclint & aia series
> already takes care of that.
> 
> > Also if in future one of the flush_icache_all / flush_icache_mm functions
> > gets called earlier in the boot process - before sbi_init - this would
> > trigger the issue.
> >
> 
> sbi_init is called before setup_smp. Do you think there is a use case where
> flush_icache_xxx can be called before smp initialization ?

I ran into the issue while experimenting with doing alternatives patching
earlier. Works fine, but the flush_icache call then runs into that null-ptr.


Heiko


> > To prevent this, add a default __sbi_rfence_none returning an error code
> > and adapt the callers to check for an error from the remote fence
> > to then fall back to the other method.
> >
> 
> Having said that, it is always good to have guards to avoid NULL
> pointer dereferences.
> However, the commit text may be updated based on the above questions.
> 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> >  arch/riscv/kernel/sbi.c    | 10 +++++++++-
> >  arch/riscv/mm/cacheflush.c | 25 +++++++++++++++++--------
> >  2 files changed, 26 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > index 7402a417f38e..69d0a96b97d0 100644
> > --- a/arch/riscv/kernel/sbi.c
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -14,11 +14,19 @@
> >  unsigned long sbi_spec_version __ro_after_init = SBI_SPEC_VERSION_DEFAULT;
> >  EXPORT_SYMBOL(sbi_spec_version);
> >
> > +static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
> > +                            unsigned long start, unsigned long size,
> > +                            unsigned long arg4, unsigned long arg5)
> > +{
> > +       return -EOPNOTSUPP;
> > +}
> > +
> >  static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init;
> >  static int (*__sbi_send_ipi)(const unsigned long *hart_mask) __ro_after_init;
> >  static int (*__sbi_rfence)(int fid, const unsigned long *hart_mask,
> >                            unsigned long start, unsigned long size,
> > -                          unsigned long arg4, unsigned long arg5) __ro_after_init;
> > +                          unsigned long arg4, unsigned long arg5)
> > +                          __ro_after_init = __sbi_rfence_none;
> >
> >  struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
> >                         unsigned long arg1, unsigned long arg2,
> > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > index 89f81067e09e..128e23c094ea 100644
> > --- a/arch/riscv/mm/cacheflush.c
> > +++ b/arch/riscv/mm/cacheflush.c
> > @@ -16,11 +16,15 @@ static void ipi_remote_fence_i(void *info)
> >
> >  void flush_icache_all(void)
> >  {
> > +       int ret = -EINVAL;
> > +
> >         local_flush_icache_all();
> >
> >         if (IS_ENABLED(CONFIG_RISCV_SBI))
> > -               sbi_remote_fence_i(NULL);
> > -       else
> > +               ret = sbi_remote_fence_i(NULL);
> > +
> > +       /* fall back to ipi_remote_fence_i if sbi failed or not available */
> > +       if (ret)
> >                 on_each_cpu(ipi_remote_fence_i, NULL, 1);
> >  }
> >  EXPORT_SYMBOL(flush_icache_all);
> > @@ -66,13 +70,18 @@ void flush_icache_mm(struct mm_struct *mm, bool local)
> >                  * with flush_icache_deferred().
> >                  */
> >                 smp_mb();
> > -       } else if (IS_ENABLED(CONFIG_RISCV_SBI)) {
> > -               cpumask_t hartid_mask;
> > -
> > -               riscv_cpuid_to_hartid_mask(&others, &hartid_mask);
> > -               sbi_remote_fence_i(cpumask_bits(&hartid_mask));
> >         } else {
> > -               on_each_cpu_mask(&others, ipi_remote_fence_i, NULL, 1);
> > +               int ret = -EINVAL;
> > +
> > +               if (IS_ENABLED(CONFIG_RISCV_SBI)) {
> > +                       cpumask_t hartid_mask;
> > +
> > +                       riscv_cpuid_to_hartid_mask(&others, &hartid_mask);
> > +                       ret = sbi_remote_fence_i(cpumask_bits(&hartid_mask));
> 
> This API is removed in sparse hartid series[1]. You may want to rebase
> on top of it.
> 
> [1] https://patchwork.kernel.org/project/linux-riscv/list/?series=590195
> 
> > +               }
> > +
> > +               if (ret)
> > +                       on_each_cpu_mask(&others, ipi_remote_fence_i, NULL, 1);
> >         }
> >
> >         preempt_enable();
> > --
> > 2.30.2
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 
> 
> 
> 





^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] riscv: prevent null-pointer dereference with sbi_remote_fence_i
@ 2021-12-14 10:41     ` Heiko Stuebner
  0 siblings, 0 replies; 12+ messages in thread
From: Heiko Stuebner @ 2021-12-14 10:41 UTC (permalink / raw)
  To: Atish Patra
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Anup Patel,
	Jisheng Zhang, Christoph Müllner, Philipp Tomsich,
	Nick Kossifidis, linux-riscv, linux-kernel@vger.kernel.org List

Hi Atish,

Am Dienstag, 14. Dezember 2021, 02:51:17 CET schrieb Atish Patra:
> On Mon, Dec 13, 2021 at 3:21 AM Heiko Stuebner <heiko@sntech.de> wrote:
> >
> > The callback used inside sbi_remote_fence_i is set at sbi probe time
> > to the needed variant. Before that it is a NULL pointer.
> >
> > The selection between using sbi_remote_fence_i or ipi_remote_fence_i
> > right now is solely done on the presence of the RISCV_SBI config option.
> >
> > On a multiplatform kernel, SBI will probably always be built in, but
> > in the future not all machines using that kernel may have SBI
> > on them, so in some cases this setup can lead to NULL pointer dereferences.
> >
> 
> I didn't quite understand this. The only platforms without SBI are
> M-mode Linux based platforms.
> All other platforms will require SBI if the kernel is running on
> supervisor mode.

Ok ... but imagine a single-core board starting in S-mode.
Not doing smp, kvm, etc.

So I can very well imagine the possibility that people
might start it without an sbi instance running ;-) .


> They may not need SBI IPI or SBI RFENCE extensions if an alternate
> mechanism(ACLINT SSWI or AIA IMSIC)
> is already available in hardware. IIRC, Anup's aclint & aia series
> already takes care of that.
> 
> > Also if in future one of the flush_icache_all / flush_icache_mm functions
> > gets called earlier in the boot process - before sbi_init - this would
> > trigger the issue.
> >
> 
> sbi_init is called before setup_smp. Do you think there is a use case where
> flush_icache_xxx can be called before smp initialization ?

I ran into the issue while experimenting with doing alternatives patching
earlier. Works fine, but the flush_icache call then runs into that null-ptr.


Heiko


> > To prevent this, add a default __sbi_rfence_none returning an error code
> > and adapt the callers to check for an error from the remote fence
> > to then fall back to the other method.
> >
> 
> Having said that, it is always good to have guards to avoid NULL
> pointer dereferences.
> However, the commit text may be updated based on the above questions.
> 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> >  arch/riscv/kernel/sbi.c    | 10 +++++++++-
> >  arch/riscv/mm/cacheflush.c | 25 +++++++++++++++++--------
> >  2 files changed, 26 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > index 7402a417f38e..69d0a96b97d0 100644
> > --- a/arch/riscv/kernel/sbi.c
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -14,11 +14,19 @@
> >  unsigned long sbi_spec_version __ro_after_init = SBI_SPEC_VERSION_DEFAULT;
> >  EXPORT_SYMBOL(sbi_spec_version);
> >
> > +static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
> > +                            unsigned long start, unsigned long size,
> > +                            unsigned long arg4, unsigned long arg5)
> > +{
> > +       return -EOPNOTSUPP;
> > +}
> > +
> >  static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init;
> >  static int (*__sbi_send_ipi)(const unsigned long *hart_mask) __ro_after_init;
> >  static int (*__sbi_rfence)(int fid, const unsigned long *hart_mask,
> >                            unsigned long start, unsigned long size,
> > -                          unsigned long arg4, unsigned long arg5) __ro_after_init;
> > +                          unsigned long arg4, unsigned long arg5)
> > +                          __ro_after_init = __sbi_rfence_none;
> >
> >  struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
> >                         unsigned long arg1, unsigned long arg2,
> > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > index 89f81067e09e..128e23c094ea 100644
> > --- a/arch/riscv/mm/cacheflush.c
> > +++ b/arch/riscv/mm/cacheflush.c
> > @@ -16,11 +16,15 @@ static void ipi_remote_fence_i(void *info)
> >
> >  void flush_icache_all(void)
> >  {
> > +       int ret = -EINVAL;
> > +
> >         local_flush_icache_all();
> >
> >         if (IS_ENABLED(CONFIG_RISCV_SBI))
> > -               sbi_remote_fence_i(NULL);
> > -       else
> > +               ret = sbi_remote_fence_i(NULL);
> > +
> > +       /* fall back to ipi_remote_fence_i if sbi failed or not available */
> > +       if (ret)
> >                 on_each_cpu(ipi_remote_fence_i, NULL, 1);
> >  }
> >  EXPORT_SYMBOL(flush_icache_all);
> > @@ -66,13 +70,18 @@ void flush_icache_mm(struct mm_struct *mm, bool local)
> >                  * with flush_icache_deferred().
> >                  */
> >                 smp_mb();
> > -       } else if (IS_ENABLED(CONFIG_RISCV_SBI)) {
> > -               cpumask_t hartid_mask;
> > -
> > -               riscv_cpuid_to_hartid_mask(&others, &hartid_mask);
> > -               sbi_remote_fence_i(cpumask_bits(&hartid_mask));
> >         } else {
> > -               on_each_cpu_mask(&others, ipi_remote_fence_i, NULL, 1);
> > +               int ret = -EINVAL;
> > +
> > +               if (IS_ENABLED(CONFIG_RISCV_SBI)) {
> > +                       cpumask_t hartid_mask;
> > +
> > +                       riscv_cpuid_to_hartid_mask(&others, &hartid_mask);
> > +                       ret = sbi_remote_fence_i(cpumask_bits(&hartid_mask));
> 
> This API is removed in sparse hartid series[1]. You may want to rebase
> on top of it.
> 
> [1] https://patchwork.kernel.org/project/linux-riscv/list/?series=590195
> 
> > +               }
> > +
> > +               if (ret)
> > +                       on_each_cpu_mask(&others, ipi_remote_fence_i, NULL, 1);
> >         }
> >
> >         preempt_enable();
> > --
> > 2.30.2
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 
> 
> 
> 





_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] riscv: provide default implementations for __sbi_set_timer and __sbi_send_ipi
  2021-12-14  2:10     ` Atish Patra
@ 2021-12-14 11:08       ` Heiko Stuebner
  -1 siblings, 0 replies; 12+ messages in thread
From: Heiko Stuebner @ 2021-12-14 11:08 UTC (permalink / raw)
  To: Atish Patra
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Anup Patel,
	Jisheng Zhang, Christoph Müllner, Philipp Tomsich,
	Nick Kossifidis, linux-riscv, linux-kernel@vger.kernel.org List

Hi Atish,

Am Dienstag, 14. Dezember 2021, 03:10:21 CET schrieb Atish Patra:
> On Mon, Dec 13, 2021 at 3:21 AM Heiko Stuebner <heiko@sntech.de> wrote:
> >
> > The mentioned function pointers get called from different sbi functions
> > which may get called from other areas of the kernel without fully
> > checking if the sbi initialization was done.
> 
> SBI initialization happens in sbi_init which is called from setup_arch.
> setup_smp happens after that. Thus, there won't be an IPI issued
> before SMP is set up.
> 
> For __sbi_set_timer, the first time it will be called from this path.
> time_init->timer_probe->riscv_timer_init_dt
> 
> time_init is called from start_kernel after setup_arch. In fact,
> setup_arch is called very early
> in the start_kernel.
> 
> Is there any other scenario where these SBI functions can be invoked
> before SBI is initialized ?

This patch is more of a second thought ;-) .

I.e. I ran into the issue fixed in the first patch, and then tought it
might be nice to also not have these other "dangling pointers" around.
But yeah, it's not that probably that these two will get called
accidentially.

So I guess I'll let you decide on these two functions ;-) .


Heiko


> >
> > So similarly to sbi_remote_fence_i, provide empty functions for them
> > to prevent any null-pointer dereferences in the future.
> >
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> >  arch/riscv/kernel/sbi.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > index 69d0a96b97d0..6a21345c6712 100644
> > --- a/arch/riscv/kernel/sbi.c
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -14,6 +14,13 @@
> >  unsigned long sbi_spec_version __ro_after_init = SBI_SPEC_VERSION_DEFAULT;
> >  EXPORT_SYMBOL(sbi_spec_version);
> >
> > +static void __sbi_set_timer_none(uint64_t stime_value) {}
> > +
> > +static int __sbi_send_ipi_none(const unsigned long *hart_mask)
> > +{
> > +       return -EOPNOTSUPP;
> > +}
> > +
> >  static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
> >                              unsigned long start, unsigned long size,
> >                              unsigned long arg4, unsigned long arg5)
> > @@ -21,8 +28,9 @@ static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
> >         return -EOPNOTSUPP;
> >  }
> >
> > -static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init;
> > -static int (*__sbi_send_ipi)(const unsigned long *hart_mask) __ro_after_init;
> > +static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init = __sbi_set_timer_none;
> > +static int (*__sbi_send_ipi)(const unsigned long *hart_mask)
> > +                           __ro_after_init = __sbi_send_ipi_none;
> >  static int (*__sbi_rfence)(int fid, const unsigned long *hart_mask,
> >                            unsigned long start, unsigned long size,
> >                            unsigned long arg4, unsigned long arg5)
> > --
> > 2.30.2
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 
> 
> 
> 





^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] riscv: provide default implementations for __sbi_set_timer and __sbi_send_ipi
@ 2021-12-14 11:08       ` Heiko Stuebner
  0 siblings, 0 replies; 12+ messages in thread
From: Heiko Stuebner @ 2021-12-14 11:08 UTC (permalink / raw)
  To: Atish Patra
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Anup Patel,
	Jisheng Zhang, Christoph Müllner, Philipp Tomsich,
	Nick Kossifidis, linux-riscv, linux-kernel@vger.kernel.org List

Hi Atish,

Am Dienstag, 14. Dezember 2021, 03:10:21 CET schrieb Atish Patra:
> On Mon, Dec 13, 2021 at 3:21 AM Heiko Stuebner <heiko@sntech.de> wrote:
> >
> > The mentioned function pointers get called from different sbi functions
> > which may get called from other areas of the kernel without fully
> > checking if the sbi initialization was done.
> 
> SBI initialization happens in sbi_init which is called from setup_arch.
> setup_smp happens after that. Thus, there won't be an IPI issued
> before SMP is set up.
> 
> For __sbi_set_timer, the first time it will be called from this path.
> time_init->timer_probe->riscv_timer_init_dt
> 
> time_init is called from start_kernel after setup_arch. In fact,
> setup_arch is called very early
> in the start_kernel.
> 
> Is there any other scenario where these SBI functions can be invoked
> before SBI is initialized ?

This patch is more of a second thought ;-) .

I.e. I ran into the issue fixed in the first patch, and then tought it
might be nice to also not have these other "dangling pointers" around.
But yeah, it's not that probably that these two will get called
accidentially.

So I guess I'll let you decide on these two functions ;-) .


Heiko


> >
> > So similarly to sbi_remote_fence_i, provide empty functions for them
> > to prevent any null-pointer dereferences in the future.
> >
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> >  arch/riscv/kernel/sbi.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > index 69d0a96b97d0..6a21345c6712 100644
> > --- a/arch/riscv/kernel/sbi.c
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -14,6 +14,13 @@
> >  unsigned long sbi_spec_version __ro_after_init = SBI_SPEC_VERSION_DEFAULT;
> >  EXPORT_SYMBOL(sbi_spec_version);
> >
> > +static void __sbi_set_timer_none(uint64_t stime_value) {}
> > +
> > +static int __sbi_send_ipi_none(const unsigned long *hart_mask)
> > +{
> > +       return -EOPNOTSUPP;
> > +}
> > +
> >  static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
> >                              unsigned long start, unsigned long size,
> >                              unsigned long arg4, unsigned long arg5)
> > @@ -21,8 +28,9 @@ static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
> >         return -EOPNOTSUPP;
> >  }
> >
> > -static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init;
> > -static int (*__sbi_send_ipi)(const unsigned long *hart_mask) __ro_after_init;
> > +static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init = __sbi_set_timer_none;
> > +static int (*__sbi_send_ipi)(const unsigned long *hart_mask)
> > +                           __ro_after_init = __sbi_send_ipi_none;
> >  static int (*__sbi_rfence)(int fid, const unsigned long *hart_mask,
> >                            unsigned long start, unsigned long size,
> >                            unsigned long arg4, unsigned long arg5)
> > --
> > 2.30.2
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 
> 
> 
> 





_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-12-14 11:08 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-13 11:20 [PATCH 1/2] riscv: prevent null-pointer dereference with sbi_remote_fence_i Heiko Stuebner
2021-12-13 11:20 ` Heiko Stuebner
2021-12-13 11:20 ` [PATCH 2/2] riscv: provide default implementations for __sbi_set_timer and __sbi_send_ipi Heiko Stuebner
2021-12-13 11:20   ` Heiko Stuebner
2021-12-14  2:10   ` Atish Patra
2021-12-14  2:10     ` Atish Patra
2021-12-14 11:08     ` Heiko Stuebner
2021-12-14 11:08       ` Heiko Stuebner
2021-12-14  1:51 ` [PATCH 1/2] riscv: prevent null-pointer dereference with sbi_remote_fence_i Atish Patra
2021-12-14  1:51   ` Atish Patra
2021-12-14 10:41   ` Heiko Stuebner
2021-12-14 10:41     ` Heiko Stuebner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.