The MMIO timer base address gets published after we have registered
the callbacks and the interrupt handler, which is... a bit dangerous.
Fix this by moving the base address publication to the point where
we register the timer, and expose a pointer to the timer structure
itself rather than a naked value.
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/clocksource/arm_arch_timer.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 160464f75017..ca7761d8459a 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -54,13 +54,13 @@
static unsigned arch_timers_present __initdata;
-static void __iomem *arch_counter_base __ro_after_init;
-
struct arch_timer {
void __iomem *base;
struct clock_event_device evt;
};
+static struct arch_timer *arch_timer_mem __ro_after_init;
+
#define to_arch_timer(e) container_of(e, struct arch_timer, evt)
static u32 arch_timer_rate __ro_after_init;
@@ -975,9 +975,9 @@ static u64 arch_counter_get_cntvct_mem(void)
u32 vct_lo, vct_hi, tmp_hi;
do {
- vct_hi = readl_relaxed(arch_counter_base + CNTVCT_HI);
- vct_lo = readl_relaxed(arch_counter_base + CNTVCT_LO);
- tmp_hi = readl_relaxed(arch_counter_base + CNTVCT_HI);
+ vct_hi = readl_relaxed(arch_timer_mem->base + CNTVCT_HI);
+ vct_lo = readl_relaxed(arch_timer_mem->base + CNTVCT_LO);
+ tmp_hi = readl_relaxed(arch_timer_mem->base + CNTVCT_HI);
} while (vct_hi != tmp_hi);
return ((u64) vct_hi << 32) | vct_lo;
@@ -1168,25 +1168,25 @@ static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq)
{
int ret;
irq_handler_t func;
- struct arch_timer *t;
- t = kzalloc(sizeof(*t), GFP_KERNEL);
- if (!t)
+ arch_timer_mem = kzalloc(sizeof(*arch_timer_mem), GFP_KERNEL);
+ if (!arch_timer_mem)
return -ENOMEM;
- t->base = base;
- t->evt.irq = irq;
- __arch_timer_setup(ARCH_TIMER_TYPE_MEM, &t->evt);
+ arch_timer_mem->base = base;
+ arch_timer_mem->evt.irq = irq;
+ __arch_timer_setup(ARCH_TIMER_TYPE_MEM, &arch_timer_mem->evt);
if (arch_timer_mem_use_virtual)
func = arch_timer_handler_virt_mem;
else
func = arch_timer_handler_phys_mem;
- ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &t->evt);
+ ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &arch_timer_mem->evt);
if (ret) {
pr_err("Failed to request mem timer irq\n");
- kfree(t);
+ kfree(arch_timer_mem);
+ arch_timer_mem = NULL;
}
return ret;
@@ -1444,7 +1444,6 @@ arch_timer_mem_frame_register(struct arch_timer_mem_frame *frame)
return ret;
}
- arch_counter_base = base;
arch_timers_present |= ARCH_TIMER_TYPE_MEM;
return 0;
--
2.30.2
On Mon, Aug 9, 2021 at 8:27 AM Marc Zyngier <[email protected]> wrote:
>
> The MMIO timer base address gets published after we have registered
> the callbacks and the interrupt handler, which is... a bit dangerous.
>
> Fix this by moving the base address publication to the point where
> we register the timer, and expose a pointer to the timer structure
> itself rather than a naked value.
>
> Signed-off-by: Marc Zyngier <[email protected]>
Is this patch stable-worthy? I take it there haven't been any reports
of issues, though this seems rather perilous.
Reviewed-by: Oliver Upton <[email protected]>
> ---
> drivers/clocksource/arm_arch_timer.c | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 160464f75017..ca7761d8459a 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -54,13 +54,13 @@
>
> static unsigned arch_timers_present __initdata;
>
> -static void __iomem *arch_counter_base __ro_after_init;
> -
> struct arch_timer {
> void __iomem *base;
> struct clock_event_device evt;
> };
>
> +static struct arch_timer *arch_timer_mem __ro_after_init;
> +
> #define to_arch_timer(e) container_of(e, struct arch_timer, evt)
>
> static u32 arch_timer_rate __ro_after_init;
> @@ -975,9 +975,9 @@ static u64 arch_counter_get_cntvct_mem(void)
> u32 vct_lo, vct_hi, tmp_hi;
>
> do {
> - vct_hi = readl_relaxed(arch_counter_base + CNTVCT_HI);
> - vct_lo = readl_relaxed(arch_counter_base + CNTVCT_LO);
> - tmp_hi = readl_relaxed(arch_counter_base + CNTVCT_HI);
> + vct_hi = readl_relaxed(arch_timer_mem->base + CNTVCT_HI);
> + vct_lo = readl_relaxed(arch_timer_mem->base + CNTVCT_LO);
> + tmp_hi = readl_relaxed(arch_timer_mem->base + CNTVCT_HI);
> } while (vct_hi != tmp_hi);
>
> return ((u64) vct_hi << 32) | vct_lo;
> @@ -1168,25 +1168,25 @@ static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq)
> {
> int ret;
> irq_handler_t func;
> - struct arch_timer *t;
>
> - t = kzalloc(sizeof(*t), GFP_KERNEL);
> - if (!t)
> + arch_timer_mem = kzalloc(sizeof(*arch_timer_mem), GFP_KERNEL);
> + if (!arch_timer_mem)
> return -ENOMEM;
>
> - t->base = base;
> - t->evt.irq = irq;
> - __arch_timer_setup(ARCH_TIMER_TYPE_MEM, &t->evt);
> + arch_timer_mem->base = base;
> + arch_timer_mem->evt.irq = irq;
> + __arch_timer_setup(ARCH_TIMER_TYPE_MEM, &arch_timer_mem->evt);
>
> if (arch_timer_mem_use_virtual)
> func = arch_timer_handler_virt_mem;
> else
> func = arch_timer_handler_phys_mem;
>
> - ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &t->evt);
> + ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &arch_timer_mem->evt);
> if (ret) {
> pr_err("Failed to request mem timer irq\n");
> - kfree(t);
> + kfree(arch_timer_mem);
> + arch_timer_mem = NULL;
> }
>
> return ret;
> @@ -1444,7 +1444,6 @@ arch_timer_mem_frame_register(struct arch_timer_mem_frame *frame)
> return ret;
> }
>
> - arch_counter_base = base;
> arch_timers_present |= ARCH_TIMER_TYPE_MEM;
>
> return 0;
> --
> 2.30.2
>
On Mon, 09 Aug 2021 17:52:00 +0100,
Oliver Upton <[email protected]> wrote:
>
> On Mon, Aug 9, 2021 at 8:27 AM Marc Zyngier <[email protected]> wrote:
> >
> > The MMIO timer base address gets published after we have registered
> > the callbacks and the interrupt handler, which is... a bit dangerous.
> >
> > Fix this by moving the base address publication to the point where
> > we register the timer, and expose a pointer to the timer structure
> > itself rather than a naked value.
> >
> > Signed-off-by: Marc Zyngier <[email protected]>
>
> Is this patch stable-worthy? I take it there haven't been any reports
> of issues, though this seems rather perilous.
It *could* deserve a Cc stable, although I suspect it doesn't easily
fall over with the current code:
- When programming a timer, the driver uses the base contained in
struct arch_timer, and derived from the clock_event_device.
- As long as you don't need to read the counter, you are good (the
whole point of using TVAL is that you avoid reading the counter).
It is only if someone called into the standalone counter accessor that
there would be a firework, and that's rather unlikely.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On Mon, Aug 09, 2021 at 04:26:43PM +0100, Marc Zyngier wrote:
> The MMIO timer base address gets published after we have registered
> the callbacks and the interrupt handler, which is... a bit dangerous.
>
> Fix this by moving the base address publication to the point where
> we register the timer, and expose a pointer to the timer structure
> itself rather than a naked value.
>
> Signed-off-by: Marc Zyngier <[email protected]>
I don't have agood setup to test this with, but this looks good to me,
and builds cleanly for arm/arm64, so:
Reviewed-by: Mark Rutland <[email protected]>
Mark.
> ---
> drivers/clocksource/arm_arch_timer.c | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 160464f75017..ca7761d8459a 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -54,13 +54,13 @@
>
> static unsigned arch_timers_present __initdata;
>
> -static void __iomem *arch_counter_base __ro_after_init;
> -
> struct arch_timer {
> void __iomem *base;
> struct clock_event_device evt;
> };
>
> +static struct arch_timer *arch_timer_mem __ro_after_init;
> +
> #define to_arch_timer(e) container_of(e, struct arch_timer, evt)
>
> static u32 arch_timer_rate __ro_after_init;
> @@ -975,9 +975,9 @@ static u64 arch_counter_get_cntvct_mem(void)
> u32 vct_lo, vct_hi, tmp_hi;
>
> do {
> - vct_hi = readl_relaxed(arch_counter_base + CNTVCT_HI);
> - vct_lo = readl_relaxed(arch_counter_base + CNTVCT_LO);
> - tmp_hi = readl_relaxed(arch_counter_base + CNTVCT_HI);
> + vct_hi = readl_relaxed(arch_timer_mem->base + CNTVCT_HI);
> + vct_lo = readl_relaxed(arch_timer_mem->base + CNTVCT_LO);
> + tmp_hi = readl_relaxed(arch_timer_mem->base + CNTVCT_HI);
> } while (vct_hi != tmp_hi);
>
> return ((u64) vct_hi << 32) | vct_lo;
> @@ -1168,25 +1168,25 @@ static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq)
> {
> int ret;
> irq_handler_t func;
> - struct arch_timer *t;
>
> - t = kzalloc(sizeof(*t), GFP_KERNEL);
> - if (!t)
> + arch_timer_mem = kzalloc(sizeof(*arch_timer_mem), GFP_KERNEL);
> + if (!arch_timer_mem)
> return -ENOMEM;
>
> - t->base = base;
> - t->evt.irq = irq;
> - __arch_timer_setup(ARCH_TIMER_TYPE_MEM, &t->evt);
> + arch_timer_mem->base = base;
> + arch_timer_mem->evt.irq = irq;
> + __arch_timer_setup(ARCH_TIMER_TYPE_MEM, &arch_timer_mem->evt);
>
> if (arch_timer_mem_use_virtual)
> func = arch_timer_handler_virt_mem;
> else
> func = arch_timer_handler_phys_mem;
>
> - ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &t->evt);
> + ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &arch_timer_mem->evt);
> if (ret) {
> pr_err("Failed to request mem timer irq\n");
> - kfree(t);
> + kfree(arch_timer_mem);
> + arch_timer_mem = NULL;
> }
>
> return ret;
> @@ -1444,7 +1444,6 @@ arch_timer_mem_frame_register(struct arch_timer_mem_frame *frame)
> return ret;
> }
>
> - arch_counter_base = base;
> arch_timers_present |= ARCH_TIMER_TYPE_MEM;
>
> return 0;
> --
> 2.30.2
>