Re: [PATCH v3 2/7] PCI: xilinx-nwl: Fix off-by-one in IRQ handler

From: Sean Anderson <sean.anderson@linux.dev>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: "Bjorn Helgaas" <helgaas@kernel.org>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Krzysztof Wilczyński" <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
Date: Fri, 24 May 2024 11:03:01 -0400	[thread overview]
Message-ID: <3e7a23ae-6423-4455-9ffb-4820ee2dc92d@linux.dev> (raw)
In-Reply-To: <c2e1d87c-14e2-4efd-a5cd-f173b52dad35@moroto.mountain>

On 5/24/24 10:56, Dan Carpenter wrote:
> 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.  :/

If I had to guess it's because most PCIe hardware uses MSIs. Unless you
plugged in a PCI bridge there's almost no reason to use INTx at all.

--Sean