| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 20 Oct 2010 13:05:45 -0600
From: "Tantilov, Emil S" <emil.s.tantilov@...el.com>
To: Jesse Barnes <jbarnes@...tuousgeek.org>,
Emil S Tantilov <emils.tantilov@...il.com>
CC: Ben Hutchings <bhutchings@...arflare.com>,
Michael Chan <mchan@...adcom.com>,
Matthew Wilcox <matthew@....cx>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
NetDev <netdev@...r.kernel.org>,
"Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
Subject: RE: [PATCH] PCI: MSI: Remove unsafe and unnecessary hardware access
>-----Original Message-----
>From: Jesse Barnes [mailto:jbarnes@...tuousgeek.org]
>Sent: Friday, October 15, 2010 1:06 PM
>To: Emil S Tantilov
>Cc: Ben Hutchings; Michael Chan; Matthew Wilcox; linux-pci@...r.kernel.org;
>NetDev; Tantilov, Emil S; Brandeburg, Jesse; Kirsher, Jeffrey T
>Subject: Re: [PATCH] PCI: MSI: Remove unsafe and unnecessary hardware
>access
>
>On Fri, 15 Oct 2010 11:26:08 -0700
>Emil S Tantilov <emils.tantilov@...il.com> wrote:
>
>> On Thu, Jun 17, 2010 at 12:16 PM, Ben Hutchings
>> <bhutchings@...arflare.com> wrote:
>> > During suspend on an SMP system, {read,write}_msi_msg_desc() may be
>> > called to mask and unmask interrupts on a device that is already in a
>> > reduced power state. At this point memory-mapped registers including
>> > MSI-X tables are not accessible, and config space may not be fully
>> > functional either.
>> >
>> > While a device is in a reduced power state its interrupts are
>> > effectively masked and its MSI(-X) state will be restored when it is
>> > brought back to D0. Therefore these functions can simply read and
>> > write msi_desc::msg for devices not in D0.
>> >
>> > Further, read_msi_msg_desc() should only ever be used to update a
>> > previously written message, so it can always read msi_desc::msg
>> > and never needs to touch the hardware.
>> >
>> > Signed-off-by: Ben Hutchings <bhutchings@...arflare.com>
>> > ---
>> > On Mon, 2010-06-14 at 18:13 -0700, Michael Chan wrote:
>> >> I'm debugging the bnx2 driver which doesn't work after suspend/resume
>if
>> >> it is running in MSI-X mode. The problem is that during suspend, the
>> >> MSI-X vectors are disabled by the following sequence on x86:
>> >>
>> >> take_cpu_down() -> cpu_disable_common() -> fixup_irqs()
>> >>
>> >> The MSI-X address/data used to disable the vectors are remembered in
>the
>> >> above sequence. During resume, these address/data are then programmed
>> >> back to the device during pci_restore_state(), causing all the vectors
>> >> to remain disabled.
>> >
>> > That's not quite what I see. What I see is that the message is read
>> > back from the table *after* the driver's suspend method has been
>called.
>> > At this point the device is already in D3 and memory-mapped registers
>> > are not accessible, so we get random bits as the message. At least,
>> > that's what I see happening with the sfc driver.
>> >
>> >> Some drivers call free_irq() during suspend and request_irq() during
>> >> resume, and that should avoid the problem. bnx2 and some other
>drivers
>> >> do not do that. These drivers rely on pci_restore_state() to restore
>> >> the MSI-X vectors to the same working state before suspend.
>> >>
>> >> What's the right way to fix this? Thanks.
>> >
>> > This is my attempt, which works for sfc. See if it works for bnx2.
>> >
>> > Ben.
>> >
>> > drivers/pci/msi.c | 34 +++++++++++-----------------------
>> > 1 files changed, 11 insertions(+), 23 deletions(-)
>> >
>> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> > index 77b68ea..03f04dc 100644
>> > --- a/drivers/pci/msi.c
>> > +++ b/drivers/pci/msi.c
>> > @@ -196,30 +196,15 @@ void unmask_msi_irq(unsigned int irq)
>> > void read_msi_msg_desc(struct irq_desc *desc, struct msi_msg *msg)
>> > {
>> > struct msi_desc *entry = get_irq_desc_msi(desc);
>> > - if (entry->msi_attrib.is_msix) {
>> > - void __iomem *base = entry->mask_base +
>> > - entry->msi_attrib.entry_nr *
>PCI_MSIX_ENTRY_SIZE;
>> >
>> > - msg->address_lo = readl(base +
>PCI_MSIX_ENTRY_LOWER_ADDR);
>> > - msg->address_hi = readl(base +
>PCI_MSIX_ENTRY_UPPER_ADDR);
>> > - msg->data = readl(base + PCI_MSIX_ENTRY_DATA);
>> > - } else {
>> > - struct pci_dev *dev = entry->dev;
>> > - int pos = entry->msi_attrib.pos;
>> > - u16 data;
>> > + /* We do not touch the hardware (which may not even be
>> > + * accessible at the moment) but return the last message
>> > + * written. Assert that this is valid, assuming that
>> > + * valid messages are not all-zeroes. */
>> > + BUG_ON(!(entry->msg.address_hi | entry->msg.address_lo |
>> > + entry->msg.data));
>> >
>> > - pci_read_config_dword(dev, msi_lower_address_reg(pos),
>> > - &msg->address_lo);
>> > - if (entry->msi_attrib.is_64) {
>> > - pci_read_config_dword(dev,
>msi_upper_address_reg(pos),
>> > - &msg->address_hi);
>> > - pci_read_config_word(dev, msi_data_reg(pos, 1),
>&data);
>> > - } else {
>> > - msg->address_hi = 0;
>> > - pci_read_config_word(dev, msi_data_reg(pos, 0),
>&data);
>> > - }
>> > - msg->data = data;
>> > - }
>> > + *msg = entry->msg;
>> > }
>> >
>> > void read_msi_msg(unsigned int irq, struct msi_msg *msg)
>> > @@ -232,7 +217,10 @@ void read_msi_msg(unsigned int irq, struct msi_msg
>*msg)
>> > void write_msi_msg_desc(struct irq_desc *desc, struct msi_msg *msg)
>> > {
>> > struct msi_desc *entry = get_irq_desc_msi(desc);
>> > - if (entry->msi_attrib.is_msix) {
>> > +
>> > + if (entry->dev->current_state != PCI_D0) {
>>
>> This check exposed a problem in ixgb (patch is on the way) where
>> pci_disable_device() was not being called in ixgb_remove(). As a
>> result the current_state was set to PCI_UNKNOWN and the interface
>> failed to work on subsequent load of the driver.
>>
>> Even though the problem was in ixgb, it made me wonder about this
>> check as the presumption here (low power state) may not always be
>> true. Like in the case of unloading a driver, which sets
>> dev->current_state to PCI_UNKNOWN which is not a representation of the
>> _real_ state of the device (actual state could be D0).
>>
>> BTW - quick search shows other drivers that could potentially suffer
>> the faith of ixgb due to lack of pci_disable_device() call on removal.
>
>Yeah we just ran into this in the DRM layer as well; which does a
>pci_enable_device but never calls _disable, so we're stuck with
>potentially stale state.
>
>I came up with the below to address that, but really I don't like the
>idea of nested pci_enable_device() calls at all. But I haven't looked
>at the latest Wireless USB stuff to see if those drivers still rely on
>it.
>
>--
>Jesse Barnes, Intel Open Source Technology Center
>
>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>index 7fa3cbd..37facc1 100644
>--- a/drivers/pci/pci.c
>+++ b/drivers/pci/pci.c
>@@ -994,6 +994,18 @@ static int __pci_enable_device_flags(struct pci_dev
>*dev,
> int err;
> int i, bars = 0;
>
>+ /*
>+ * Power state could be unknown at this point, either due to a fresh
>+ * boot or a device removal call. So get the current power state
>+ * so that things like MSI message writing will behave as expected
>+ * (e.g. if the device really is in D0 at enable time).
>+ */
>+ if (dev->pm_cap) {
>+ u16 pmcsr;
>+ pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
>+ dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
>+ }
>+
> if (atomic_add_return(1, &dev->enable_cnt) > 1)
> return 0; /* already enabled */
>
With this patch applied I could reload the driver and confirmed that current_state is set to the actual power state.
Thanks,
Emil
Powered by blists - more mailing lists