Received-SPF: pass (google.com: domain of linux-kernel+bounces-188819-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 24 May 2024 17:56:21 +0300
From: Dan Carpenter <dan.carpenter@linaro.org>
To: Sean Anderson <sean.anderson@linux.dev>
Cc: Bjorn Helgaas <helgaas@kernel.org>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
	Rob Herring <robh@kernel.org>, linux-pci@vger.kernel.org,
	Michal Simek <michal.simek@amd.com>,
	Thippeswamy Havalige <thippeswamy.havalige@amd.com>,
	linux-arm-kernel@lists.infradead.org,
	Bjorn Helgaas <bhelgaas@google.com>, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org, Bharat Kumar Gogada <bharatku@xilinx.com>
Subject: Re: [PATCH v3 2/7] PCI: xilinx-nwl: Fix off-by-one in IRQ handler
Message-ID: <c2e1d87c-14e2-4efd-a5cd-f173b52dad35@moroto.mountain>
References: <20240522222834.GA101664@bhelgaas>
 <9299ee92-a32b-4b82-aa37-c7087a5c1376@linux.dev>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9299ee92-a32b-4b82-aa37-c7087a5c1376@linux.dev>

On Thu, May 23, 2024 at 11:21:52AM -0400, Sean Anderson wrote:
> On 5/22/24 18:28, Bjorn Helgaas wrote:
> > On Mon, May 20, 2024 at 10:53:57AM -0400, Sean Anderson wrote:
> >> MSGF_LEG_MASK is laid out with INTA in bit 0, INTB in bit 1, INTC in bit
> >> 2, and INTD in bit 3. Hardware IRQ numbers start at 0, and we register
> >> PCI_NUM_INTX irqs. So to enable INTA (aka hwirq 0) we should set bit 0.
> >> Remove the subtraction of one. This fixes the following UBSAN error:
> > 
> > Thanks for these details!
> > 
> > I guess UBSAN == "undefined behavior sanitizer", right?  That sounds
> > like an easy way to find this but not the way users are likely to find
> > it.
> 
> It's pretty likely they will find it this way, since I found it this way
> and no one else had ;)
> 
> > I assume users would notice spurious and missing interrupts, e.g.,
> > a driver that tried to enable INTB would have actually enabled INTA,
> > so we'd see spurious INTA interrupts and the driver would never see
> > the INTB it expected.
> > 
> > And a driver that tried to enable INTA would never see that interrupt,
> > and we might not set any bit in MSGF_LEG_MASK?
> 
> And yes, this would manifest as INTx interrupts being broken.
> 

It's so weird that it's been broken for seven years and no one reported
it.  :/

regards,
dan carpenter