linux-toradex.git/drivers/pci/hotplug/s390_pci_hpc.c, branch v5.12

s390/pci: fix leak of PCI device structure

2021-03-15T18:10:56+00:00

In commit 05bc1be6db4b2 ("s390/pci: create zPCI bus") we removed the
pci_dev_put() call matching the earlier pci_get_slot() done as part of
__zpci_event_availability(). This was based on the wrong understanding
that the device_put() done as part of pci_destroy_device() would counter
the pci_get_slot() when it only counters the initial reference. This
same understanding and existing bad example also lead to not doing
a pci_dev_put() in zpci_remove_device().

Since releasing the PCI devices, unlike releasing the PCI slot, does not
print any debug message for testing I added one in pci_release_dev().
This revealed that we are indeed leaking the PCI device on PCI
hotunplug. Further testing also revealed another missing pci_dev_put() in
disable_slot().

Fix this by adding the missing pci_dev_put() in disable_slot() and fix
zpci_remove_device() with the correct pci_dev_put() calls. Also instead
of calling pci_get_slot() in __zpci_event_availability() to determine if
a PCI device is registered and then doing the same again in
zpci_remove_device() do this once in zpci_remove_device() which makes
sure that the pdev in __zpci_event_availability() is only used for the
result of pci_scan_single_device() which does not need a reference count
decremnt as its ownership goes to the PCI bus.

Also move the check if zdev->zbus->bus is set into zpci_remove_device()
since it may be that we're removing a device with devfn != 0 which never
had a PCI bus. So we can still set the pdev->error_state to indicate
that the device is not usable anymore, add a flag to set the error state.

Fixes: 05bc1be6db4b2 ("s390/pci: create zPCI bus")
Cc:  # 5.8+: e1bff843cde6 s390/pci: remove superfluous zdev->zbus check
Cc:  # 5.8+: ba764dd703fe s390/pci: refactor zpci_create_device()
Cc:  # 5.8+
Reviewed-by: Matthew Rosato 
Signed-off-by: Niklas Schnelle 
Signed-off-by: Heiko Carstens

s390/pci: re-introduce zpci_remove_device()

2020-08-17T11:17:25+00:00

For fixing the PF to VF link removal we need to perform some action on
every removal of a zdev from the common PCI subsystem.
So in preparation re-introduce zpci_remove_device() and use that instead
of directly calling the common code functions. This  was actually still
declared from earlier code but no longer implemented.

Reviewed-by: Pierre Morel 
Signed-off-by: Niklas Schnelle 
Signed-off-by: Heiko Carstens

s390/pci: Do not disable PF when VFs exist

2020-04-28T11:49:47+00:00

The Physical function should not be disabled until no virtual
functions depends on it.
Let's force the user to first use echo 0 > sriov_numfs before
allowing to disable the PF with echo 0 > power.

Signed-off-by: Pierre Morel 
Reviewed-by: Niklas Schnelle 
Signed-off-by: Vasily Gorbik

s390/pci: Handling multifunctions

2020-04-28T11:49:47+00:00

We allow multiple functions on a single bus.
We suppress the ZPCI_DEVFN definition and replace its
occurences with zpci->devfn.

We verify the number of device during the registration.

There can never be more domains in use than existing
devices, so we do not need to verify the count of domain
after having verified the count of devices.

Signed-off-by: Pierre Morel 
Reviewed-by: Niklas Schnelle 
Signed-off-by: Vasily Gorbik

s390/pci: create zPCI bus

2020-04-28T11:49:46+00:00

The zPCI bus is in charge to handle common zPCI resources for
zPCI devices.

Creating the zPCI bus, the PCI bus, the zPCI devices and the
PCI devices and hotplug slots
done in a specific order:
- PCI hotplug slot creation needs a PCI bus
- PCI bus needs a PCI domain
  which is reported by the pci_domain_nr() when setting up the
  host bridge
- PCI domain is set from the zPCI with devfn 0
  this is necessary to have a reproducible enumeration

Therefore we can not create devices or hotplug slots for any PCI
device associated with a zPCI device before having discovered
the function zero of the bus.

The discovery and initialization of devices can be done at several
points in the code:
- On Events, serialized in a thread context
- On initialization, in the kernel init thread context
- When powering on the hotplug slot, in a user thread context

The removal of devices and their parent bus may also be done on
events or for devices when powering down the slot.

To guarantee the existence of the bus and devices until they are
no more needed we use kref in zPCI bus and introduce a reference
count in the zPCI devices.

In this patch the zPCI bus still only accept a device with
a devfn 0.

Signed-off-by: Pierre Morel 
Reviewed-by: Niklas Schnelle 
Signed-off-by: Vasily Gorbik

s390/pci: embedding hotplug_slot in zdev

2020-03-10T14:16:25+00:00

Embedding the hotplug_slot in zdev structure allows to
greatly simplify the hotplug handling by eliminating
the handling of the slot_list.

Signed-off-by: Pierre Morel 
Reviewed-by: Niklas Schnelle 
Signed-off-by: Vasily Gorbik

PCI: hotplug: Embed hotplug_slot

2018-09-18T22:52:15+00:00

When the PCI hotplug core and its first user, cpqphp, were introduced in
February 2002 with historic commit a8a2069f432c, cpqphp allocated a slot
struct for its internal use plus a hotplug_slot struct to be registered
with the hotplug core and linked the two with pointers:
https://git.kernel.org/tglx/history/c/a8a2069f432c

Nowadays, the predominant pattern in the tree is to embed ("subclass")
such structures in one another and cast to the containing struct with
container_of().  But it wasn't until July 2002 that container_of() was
introduced with historic commit ec4f214232cf:
https://git.kernel.org/tglx/history/c/ec4f214232cf

pnv_php, introduced in 2016, did the right thing and embedded struct
hotplug_slot in its internal struct pnv_php_slot, but all other drivers
cargo-culted cpqphp's design and linked separate structs with pointers.

Embedding structs is preferrable to linking them with pointers because
it requires fewer allocations, thereby reducing overhead and simplifying
error paths.  Casting an embedded struct to the containing struct
becomes a cheap subtraction rather than a dereference.  And having fewer
pointers reduces the risk of them pointing nowhere either accidentally
or due to an attack.

Convert all drivers to embed struct hotplug_slot in their internal slot
struct.  The "private" pointer in struct hotplug_slot thereby becomes
unused, so drop it.

Signed-off-by: Lukas Wunner 
Signed-off-by: Bjorn Helgaas 
Reviewed-by: Rafael J. Wysocki 
Acked-by: Tyrel Datwyler   # drivers/pci/hotplug/rpa*
Acked-by: Sebastian Ott         # drivers/pci/hotplug/s390*
Acked-by: Andy Shevchenko  # drivers/platform/x86
Cc: Len Brown 
Cc: Scott Murray 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Michael Ellerman 
Cc: Oliver OHalloran 
Cc: Gavin Shan 
Cc: Gerald Schaefer 
Cc: Corentin Chary 
Cc: Darren Hart

PCI: hotplug: Drop hotplug_slot_info

2018-09-18T22:52:15+00:00

Ever since the PCI hotplug core was introduced in 2002, drivers had to
allocate and register a struct hotplug_slot_info for every slot:
https://git.kernel.org/tglx/history/c/a8a2069f432c

Apparently the idea was that drivers furnish the hotplug core with an
up-to-date card presence status, power status, latch status and
attention indicator status as well as notify the hotplug core of changes
thereof.  However only 4 out of 12 hotplug drivers bother to notify the
hotplug core with pci_hp_change_slot_info() and the hotplug core never
made any use of the information:  There is just a single macro in
pci_hotplug_core.c, GET_STATUS(), which uses the hotplug_slot_info if
the driver lacks the corresponding callback in hotplug_slot_ops.  The
macro is called when the user reads the attribute via sysfs.

Now, if the callback isn't defined, the attribute isn't exposed in sysfs
in the first place (see e.g. has_power_file()).  There are only two
situations when the hotplug_slot_info would actually be accessed:

* If the driver defines ->enable_slot or ->disable_slot but not
  ->get_power_status.

* If the driver defines ->set_attention_status but not
  ->get_attention_status.

There is no driver doing the former and just a single driver doing the
latter, namely pnv_php.c.  Amend it with a ->get_attention_status
callback.  With that, the hotplug_slot_info becomes completely unused by
the PCI hotplug core.  But a few drivers use it internally as a cache:

cpcihp uses it to cache the latch_status and adapter_status.
cpqhp uses it to cache the adapter_status.
pnv_php and rpaphp use it to cache the attention_status.
shpchp uses it to cache all four values.

Amend these drivers to cache the information in their private slot
struct.  shpchp's slot struct already contains members to cache the
power_status and adapter_status, so additional members are only needed
for the other two values.  In the case of cpqphp, the cached value is
only accessed in a single place, so instead of caching it, read the
current value from the hardware.

Caution:  acpiphp, cpci, cpqhp, shpchp, asus-wmi and eeepc-laptop
populate the hotplug_slot_info with initial values on probe.  That code
is herewith removed.  There is a theoretical chance that the code has
side effects without which the driver fails to function, e.g. if the
ACPI method to read the adapter status needs to be executed at least
once on probe.  That seems unlikely to me, still maintainers should
review the changes carefully for this possibility.

Rafael adds: "I'm not aware of any case in which it will break anything,
[...] but if that happens, it may be necessary to add the execution of
the control methods in question directly to the initialization part."

Signed-off-by: Lukas Wunner 
Signed-off-by: Bjorn Helgaas 
Reviewed-by: Rafael J. Wysocki 
Acked-by: Tyrel Datwyler   # drivers/pci/hotplug/rpa*
Acked-by: Sebastian Ott         # drivers/pci/hotplug/s390*
Acked-by: Andy Shevchenko  # drivers/platform/x86
Cc: Len Brown 
Cc: Scott Murray 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Michael Ellerman 
Cc: Oliver OHalloran 
Cc: Gavin Shan 
Cc: Gerald Schaefer 
Cc: Corentin Chary 
Cc: Darren Hart

PCI: hotplug: Constify hotplug_slot_ops

2018-09-18T22:52:15+00:00

Hotplug drivers cannot declare their hotplug_slot_ops const, making them
attractive targets for attackers, because upon registration of a hotplug
slot, __pci_hp_initialize() writes to the "owner" and "mod_name" members
in that struct.

Fix by moving these members to struct hotplug_slot and constify every
driver's hotplug_slot_ops except for pciehp.

pciehp constructs its hotplug_slot_ops at runtime based on the PCIe
port's capabilities, hence cannot declare them const.  It can be
converted to __write_rarely once that's mainlined:
http://www.openwall.com/lists/kernel-hardening/2016/11/16/3

Signed-off-by: Lukas Wunner 
Signed-off-by: Bjorn Helgaas 
Reviewed-by: Rafael J. Wysocki 
Acked-by: Tyrel Datwyler   # drivers/pci/hotplug/rpa*
Acked-by: Andy Shevchenko  # drivers/platform/x86
Cc: Len Brown 
Cc: Scott Murray 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Michael Ellerman 
Cc: Oliver OHalloran 
Cc: Gavin Shan 
Cc: Sebastian Ott 
Cc: Gerald Schaefer 
Cc: Corentin Chary 
Cc: Darren Hart

PCI: hotplug: Demidlayer registration with the core

2018-07-23T22:04:13+00:00

When a hotplug driver calls pci_hp_register(), all steps necessary for
registration are carried out in one go, including creation of a kobject
and addition to sysfs.  That's a problem for pciehp once it's converted
to enable/disable the slot exclusively from the IRQ thread:  The thread
needs to be spawned after creation of the kobject (because it uses the
kobject's name), but before addition to sysfs (because it will handle
enable/disable requests submitted via sysfs).

pci_hp_deregister() does offer a ->release callback that's invoked
after deletion from sysfs and before destruction of the kobject.  But
because pci_hp_register() doesn't offer a counterpart, hotplug drivers'
->probe and ->remove code becomes asymmetric, which is error prone
as recently discovered use-after-free bugs in pciehp's ->remove hook
have shown.

In a sense, this appears to be a case of the midlayer antipattern:

   "The core thesis of the "midlayer mistake" is that midlayers are
    bad and should not exist.  That common functionality which it is
    so tempting to put in a midlayer should instead be provided as
    library routines which can [be] used, augmented, or ignored by
    each bottom level driver independently.  Thus every subsystem
    that supports multiple implementations (or drivers) should
    provide a very thin top layer which calls directly into the
    bottom layer drivers, and a rich library of support code that
    eases the implementation of those drivers.  This library is
    available to, but not forced upon, those drivers."
        --  Neil Brown (2009), https://lwn.net/Articles/336262/

The presence of midlayer traits in the PCI hotplug core might be ascribed
to its age:  When it was introduced in February 2002, the blessings of a
library approach might not have been well known:
https://git.kernel.org/tglx/history/c/a8a2069f432c

For comparison, the driver core does offer split functions for creating
a kobject (device_initialize()) and addition to sysfs (device_add()) as
an alternative to carrying out everything at once (device_register()).
This was introduced in October 2002:
https://git.kernel.org/tglx/history/c/8b290eb19962

The odd ->release callback in the PCI hotplug core was added in 2003:
https://git.kernel.org/tglx/history/c/69f8d663b595

Clearly, a library approach would not force every hotplug driver to
implement a ->release callback, but rather allow the driver to remove
the sysfs files, release its data structures and finally destroy the
kobject.  Alternatively, a driver may choose to remove everything with
pci_hp_deregister(), then release its data structures.

To this end, offer drivers pci_hp_initialize() and pci_hp_add() as a
split-up version of pci_hp_register().  Likewise, offer pci_hp_del()
and pci_hp_destroy() as a split-up version of pci_hp_deregister().

Eliminate the ->release callback and move its code into each driver's
teardown routine.

Declare pci_hp_deregister() void, in keeping with the usual kernel
pattern that enablement can fail, but disablement cannot.  It only
returned an error if the caller passed in a NULL pointer or a slot which
has never or is no longer registered or is sharing its name with another
slot.  Those would be bugs, so WARN about them.  Few hotplug drivers
actually checked the return value and those that did only printed a
useless error message to dmesg.  Remove that.

For most drivers the conversion was straightforward since it doesn't
matter whether the code in the ->release callback is executed before or
after destruction of the kobject.  But in the case of ibmphp, it was
unclear to me whether setting slot_cur->ctrl and slot_cur->bus_on to
NULL needs to happen before the kobject is destroyed, so I erred on
the side of caution and ensured that the order stays the same.  Another
nontrivial case is pnv_php, I've found the list and kref logic difficult
to understand, however my impression was that it is safe to delete the
list element and drop the references until after the kobject is
destroyed.

Signed-off-by: Lukas Wunner 
Signed-off-by: Bjorn Helgaas 
Acked-by: Andy Shevchenko   # drivers/platform/x86
Cc: Rafael J. Wysocki 
Cc: Len Brown 
Cc: Scott Murray 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Michael Ellerman 
Cc: Gavin Shan 
Cc: Sebastian Ott 
Cc: Gerald Schaefer 
Cc: Corentin Chary 
Cc: Darren Hart 
Cc: Andy Shevchenko