From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 13F2C35E552 for ; Tue, 27 Jan 2026 15:58:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1769529518; cv=none; b=p8OZ+jDH+BFVsCLhGQo/km9tweMLvvWDCGeQuDGzgqTkZe6S5Rz2tlPAGlN2EOliEnVbWFJ3auMvN17Uy/VjlAYE/UoLp+sh+j2bBH0R1v4jYNJ9N82E1n1owd1zLPyZdtq+ME7R8VTgDiErtfVtB5FsgKh27UfmhuJbdcIdlzA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1769529518; c=relaxed/simple; bh=nVstBk4Y1lGp5Jd2un5oyBfrauMV07MlNCfy0oT7Nrs=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=WEFfUZJXmq1E8CJsKqTcGlOaM4p3zVFlRVSJ8HWpGL4CD1M4KcesYdG6YzfrIaUSvozHES3KvTNBgzr62TQB2Qchwv8ss+mmgVksKbIrnPtTS60VDfqO6pjnEDRiL8/CLKyWuGwHjtZBslRA3SprtdK/zgAeQN+JvRtINHdorzU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=dn5ItrDG; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="dn5ItrDG" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 16200C116C6; Tue, 27 Jan 2026 15:58:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1769529517; bh=nVstBk4Y1lGp5Jd2un5oyBfrauMV07MlNCfy0oT7Nrs=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=dn5ItrDGAoRiK2rLspgWlpzL83EAEKcQ/UxvwLitxSZABIZ981Wp6WxiCFJQ/9kA8 Je2cv4hBalr56LXTpGyj0WX0sHx6VvbaeVyLOAUasNlOYxkeNPevKiLMYzpgYdZtWs 36WUNrRnMptzmkvOVjz+n4qwa1EI2o6HN5uXNu1liF2zucMtb6StxOb8vi6QkQRrM5 wYZgEfkfOCAQtMnsAjhHD3aWE0UpGOIyc3J7TwqgjrokdrqybDzIkP+DnQQk6kbWUh nqRiSI4FhO/dn1Cou2jvphzZjdDrDriMt6iSFefg4O9qooXSQvGZQei0jULBDeRgmQ AiHRHzLtEWh1A== From: Thomas Gleixner To: Yangyu Chen , linux-riscv@lists.infradead.org Cc: linux-kernel@vger.kernel.org, Anup Patel , Samuel Holland , Charles Mirabile , Lucas Zampieri , Paul Walmsley , Palmer Dabbelt , Mason Huo , Zhang Xincheng , Charlie Jenkins , Marc Zyngier , Sia Jee Heng , Ley Foon Tan , Yangyu Chen Subject: Re: [PATCH v2] irqchip/sifive-plic: Fix insufficient irq_groups allocation In-Reply-To: References: Date: Tue, 27 Jan 2026 16:58:33 +0100 Message-ID: <87bjifavyu.ffs@tglx> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain On Wed, Jan 21 2026 at 13:45, Yangyu Chen wrote: > Since the first irq source is 1 instead of 0, when the number of > irqs is multiple of 32, the last irq group will be ignored during > allocation, saving, and restoring. This lead to memory corruption > when accessing enable_save beyond allocated memory after commit > 14ff9e54dd14 ("irqchip/sifive-plic: Cache the interrupt enable state") > which will access enable_save for all sources during plic_probe. > Thus, we should allocate irq_groups based on (nr_irqs + 1) instead of > nr_irqs to avoid this issue. > > This is an long standing bug since Linux v6.4 but since the last irq > source is rarely used, it may not be triggered in practice until commit > 14ff9e54dd14 ("irqchip/sifive-plic: Cache the interrupt enable state"). I'm absolutely not convinced that this is the right fix. The handling of nr_irqs in this driver is completely inconsistent: 1) static int plic_irq_suspend(void *data) /* irq ID 0 is reserved */ for (unsigned int i = 1; i < priv->nr_irqs; i++) { 2) static void plic_irq_resume(void *data) /* irq ID 0 is reserved */ for (unsigned int i = 1; i < priv->nr_irqs; i++) { ... for (i = 0; i < DIV_ROUND_UP(priv->nr_irqs, 32); i++) { 3) static irq_hw_number_t cp100_get_hwirq(struct plic_handler *handler, void __iomem *claim) int nr_irq_groups = DIV_ROUND_UP(handler->priv->nr_irqs, 32); for (i = 0; i < nr_irq_groups; i++) { 4) static int plic_probe(struct fwnode_handle *fwnode) priv->prio_save = bitmap_zalloc(nr_irqs, GFP_KERNEL); ... for (int j = 0; j <= nr_irqs / 32; j++) writel(0, enable_base + j); ... handler->enable_save = kcalloc(DIV_ROUND_UP(nr_irqs, 32),... ... for (hwirq = 1; hwirq <= nr_irqs; hwirq++) { ... priv->irqdomain = irq_domain_create_linear(fwnode, nr_irqs + 1, So can the SIFIVE people please clarify once and forever what's the actual meaning of nr_irqs: A) The actual number of hardware interrupts available for drivers starting from hwirq=1 B) The total number of hardware interrupts, where hwirq=0 is reserved and therefore the number of interrupts available to drivers is nr_irqs - 1. That needs to be clarified first and then subsequently every single instance which deals with nr_irqs has to be updated accordingly. A random fix which cures the symptom of an out of bounds memory access is not helpful at all to cure the underlying root cause of inconsistency. Thanks, tglx