Re: [PATCH net] net: ethernet: ti: cpts: drop stale events before the misc IRQ can fire

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix narrows the window but leaves the NULL-pointer write in
  cpts_fifo_read() reachable.

--

net: ethernet: ti: cpts: drop stale events before the misc IRQ can fire

After a kexec, the previous kernel can leave a misc IRQ pending. That IRQ
can run cpts_fifo_read() before cpts_register() has set up the event
pool. This patch masks the CPTS interrupt and drains the CPTS event FIFO
in cpts_create() to avoid that. CPTS_EN is deliberately left set.

> diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
> index 2ba4c8795d605..3cc908ac90e2b 100644
> --- a/drivers/net/ethernet/ti/cpts.c
> +++ b/drivers/net/ethernet/ti/cpts.c

[ ... ]

> @@ -779,6 +780,17 @@ struct cpts *cpts_create(struct device *dev, void __iomem *regs,
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> +	/* After a kexec the CPTS may still be running from the previous
> +	 * kernel, with its interrupt enabled and events in the FIFO. The misc
> +	 * IRQ is requested before cpts_register() sets up the event pool, so
> +	 * mask the interrupt and drop the stale events now.
> +	 */
> +	cpts_write32(cpts, 0, int_enable);
> +	for (i = 0; i < CPTS_FIFO_DEPTH; i++) {
> +		if (cpts_fifo_pop(cpts, &high, &low))
> +			break;
> +	}

[Severity: High]
Does this close the window completely, or only make it smaller?

CPTS_EN stays set, so the CPTS can keep queueing events after this loop
finishes. cpts->pool and cpts->events are still zeroed from devm_kzalloc().
Only cpts_register() initializes them, and it runs later from ndo_open.

A fair amount of work happens between cpts_create() and the misc IRQ
request. In cpsw_new.c:cpsw_probe():

    cpsw_init_common()        <- calls cpts_create()
    ...
    cpsw_create_ports()
    ...
    devm_request_irq(dev, cpsw->misc_irq, cpsw_misc_interrupt, ...)

In cpsw.c:cpsw_probe() the same window also covers register_netdev() and
cpsw_probe_dual_emac().

Several things could add new events to the FIFO in that window:

 - half-rollover and rollover events from the free-running counter
   (CPTS_EV_HALF / CPTS_EV_ROLL)
 - HW_TS_PUSH events, if the previous kernel left HWx_TS_PUSH_EN set
 - RX/TX timestamp events, if the previous kernel left port timestamping
   enabled

Port timestamping is only cleared by cpsw_soft_reset() in ndo_open, and
cpsw has no .shutdown handler. So after a kexec, a peer that sends PTP
event frames would keep generating RX events.

The commit message notes that the interrupt may already be latched. If
it is delivered when devm_request_irq() runs, the handler does:

cpsw_misc_interrupt()
  cpts_misc_interrupt()
    cpts_fifo_read()
      cpts_fifo_pop()

cpts_fifo_pop() tests intstat_raw & TS_PEND_RAW, so writing 0 to
int_enable does not stop the pop. Then in cpts_fifo_read():

		if (list_empty(&cpts->pool) && cpts_purge_events(cpts)) {
			dev_warn(cpts->dev, "cpts: event pool empty\n");
			break;
		}

		event = list_first_entry(&cpts->pool, struct cpts_event, list);
		event->high = hi;

On a zeroed list_head, list_empty() returns false, because NULL is not
equal to &cpts->pool. list_first_entry() then gives
container_of(NULL, struct cpts_event, list), so the write to event->high
lands near address 0 in hardirq context.

Isn't that the same oops the commit message describes?

Would it be more robust to make the handler safe to run before
cpts_register()? One option is to move INIT_LIST_HEAD(&cpts->events) and
INIT_LIST_HEAD(&cpts->pool) into cpts_create(). Another is to have
cpts_misc_interrupt() return early until registration has happened.
Either would replace the drain approach rather than extend it.

>  	cpts->cc.read = cpts_systim_read;
>  	cpts->cc.mask = CLOCKSOURCE_MASK(32);
>  	cpts->info = cpts_info;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261026072314.663980-1-alan.tseng.cs%40gmail.com