Thread (39 messages) flat view 39 messages, 3 authors, 2021-08-24

Re: [PATCH 05/13] clocksource/arm_arch_timer: Fix MMIO base address vs callback ordering issue

From: Oliver Upton <hidden>
Date: 2021-08-09 16:52:16
Also in: lkml

On Mon, Aug 9, 2021 at 8:27 AM Marc Zyngier [off-list ref] 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 <maz@kernel.org>
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 <redacted>
quoted hunk ↗ jump to hunk
---
 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help