Messages in this thread | | | From | Robert Richter <> | Subject | Re: [PATCH v3] EDAC, ghes: Fix locking and memory barrier issues | Date | Tue, 5 Nov 2019 20:11:22 +0000 |
| |
On 05.11.19 20:07:51, Robert Richter wrote: > The ghes registration and refcount is broken in several ways: > > * ghes_edac_register() returns with success for a 2nd instance even > if a first instance is still running. This is not correct as the > first instance may fail later. A subsequent registration may not > finish before the first. Parallel registrations must be avoided. > > * The refcount was increased even if a registration failed. This > leads to stale counters preventing the device from being released. > > * The ghes refcount may not be decremented properly on > unregistration. Always decrement the refcount once > ghes_edac_unregister() is called to keep the refcount sane. > > * The ghes_pvt pointer is handed to the irq handler before > registration finished. > > * The mci structure could be freed while the irq handler is running. > > Fix this by adding a mutex to ghes_edac_register(). This mutex > serializes instances to register and unregister. The refcount is only > increased if the registration succeeded. This makes sure the refcount > is in a consistent state after registering or unregistering a device. > Note: A spinlock cannot be used here as the code section may sleep. > > The ghes_pvt is protected by ghes_lock now. This ensures the pointer > is not updated before registration was finished or while the irq > handler is running. It is unset before unregistering the device > including necessary (implicit) memory barriers making the changes > visible to other cpus. Thus, the device can not be used anymore by an > interrupt. > > Also, rename ghes_init to ghes_refcount for better readability and > switch to refcount API. > > A refcount is needed. There can be multiple GHES structures being > defined (see ACPI 6.3 specification, 18.3.2.7 Generic Hardware Error > Source, "Some platforms may describe multiple Generic Hardware Error > Source structures with different notification types, ..."). > > Another approach to use the mci's device refcount (get_device()) and > have a release function does not work here. A release function will be > called only for device_release() with the last put_device() call. The > device must be deleted *before* that with device_del(). This is only > possible by maintaining an own refcount. > > Fixes: 0fe5f281f749 ("EDAC, ghes: Model a single, logical memory controller") > Fixes: 1e72e673b9d1 ("EDAC/ghes: Fix Use after free in ghes_edac remove path") > Co-developed-by: James Morse <james.morse@arm.com> > Signed-off-by: James Morse <james.morse@arm.com> > Co-developed-by: Borislav Petkov <bp@suse.de> > Signed-off-by: Borislav Petkov <bp@suse.de> > Signed-off-by: Robert Richter <rrichter@marvell.com>
I hope this SOB chain is correct now.
Thanks,
-Robert
> --- > V3: > * fixed SOB chain again (added SOBs according to > submitting-patches.rst) > > V2: > * fixed missing 'goto unlock' in error path > * fixed SOB chain > * added comment on how to protect ghes_pvt with ghes_lock > * renamed ghes_init to ghes_refcount > * switched to refcount API instead of atomic_* > --- > drivers/edac/ghes_edac.c | 89 +++++++++++++++++++++++++++++----------- > 1 file changed, 65 insertions(+), 24 deletions(-)
| |