Re: [PATCH v2] mpt2sas: setpci reset kernel oops fix

[Hannes Reinecke <hare@suse.de>]

On 07/14/2015 01:23 PM, Nagarajkumar Narayanan wrote:
> 
> Patch Description:
> 
> In mpt2sas driver due to lack of synchronization between ioctl,
> BRM status access through sysfs, pci resource removal kernel oops
> happen as ioctl path and BRM status sysfs access path still tries
> to access the removed resources
> 
> kernel: BUG: unable to handle kernel paging request at ffffc900171e0000
> 
> Oops: 0000 [#1] SMP
> 
> Two locks added to provide syncrhonization
> 
> 1. pci_access_mutex: Mutex to synchronize ioctl,sysfs show path and
> pci resource handling. PCI resource freeing will lead to free
> vital hardware/memory resource, which might be in use by cli/sysfs
> path functions resulting in Null pointer reference followed by kernel
> crash. To avoid the above race condition we use mutex syncrhonization
> which ensures the syncrhonization between cli/sysfs_show path
> 
> 2. spinlock on list operations over IOCs
> 
> Case: when multiple warpdrive cards(IOCs) are in use
> Each IOC will added to the ioc list stucture on initialization.
> Watchdog threads run at regular intervals to check IOC for any
> fault conditions which will trigger the dead_ioc thread to
> deallocate pci resource, resulting deleting the IOC netry from list,
> this deletion need to protected by spinlock to enusre that
> ioc removal is syncrhonized, if not synchronized it might lead to
> list_del corruption as the ioc list is traversed in cli path
> 
> 
> From 8db4d8194276ba420a4e93de4b09df6da5a934e4 Mon Sep 17 00:00:00 2001
> From: Nagarajkumar Narayanan <nagarajkumar.narayanan@seagate.com>
> Date: Tue, 14 Jul 2015 16:33:56 +0530
> Subject: [PATCH] mpt2sas setpci reset oops fix
> 
> setpci reset on nytro warpdrive card along with sysfs access and
> cli ioctl access resulted in kernel oops
> 
> 1. pci_access_mutex lock added to provide synchronization between IOCTL,
>    sysfs, PCI resource handling path
> 
> 2. gioc_lock spinlock to protect list operations over multiple
> controllers
> 
> Signed-off-by: Nagarajkumar Narayanan <nagarajkumar.narayanan@seagate.com>
> ---
> * v2
> - removed is_warpdrive condition for pci_access_mutex lock
> 
> * v1
> - using DEFINE_SPINLOCK() to initialize the lock at compile time instead
>   of using spin_lock_init
> 
>  drivers/scsi/mpt2sas/mpt2sas_base.c  |    7 +++++++
>  drivers/scsi/mpt2sas/mpt2sas_base.h  |   19 ++++++++++++++++++-
>  drivers/scsi/mpt2sas/mpt2sas_ctl.c   |   33 +++++++++++++++++++++++++++++----
>  drivers/scsi/mpt2sas/mpt2sas_scsih.c |   15 ++++++++++++++-
>  4 files changed, 68 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
> index 11248de..f04dcc0 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c
> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
> @@ -108,13 +108,17 @@ _scsih_set_fwfault_debug(const char *val, struct kernel_param *kp)
>  {
>  	int ret = param_set_int(val, kp);
>  	struct MPT2SAS_ADAPTER *ioc;
> +	unsigned long flags;
>  
>  	if (ret)
>  		return ret;
>  
> +	/* global ioc spinlock to protect controller list on list operations */
>  	printk(KERN_INFO "setting fwfault_debug(%d)\n", mpt2sas_fwfault_debug);
> +	spin_lock_irqsave(&gioc_lock, flags);
>  	list_for_each_entry(ioc, &mpt2sas_ioc_list, list)
>  		ioc->fwfault_debug = mpt2sas_fwfault_debug;
> +	spin_unlock_irqrestore(&gioc_lock, flags);
>  	return 0;
>  }
>  
> @@ -4436,6 +4440,8 @@ mpt2sas_base_free_resources(struct MPT2SAS_ADAPTER *ioc)
>  	    __func__));
>  
>  	if (ioc->chip_phys && ioc->chip) {
> +		/* synchronizing freeing resource with pci_access_mutex lock */
> +		mutex_lock(&ioc->pci_access_mutex);
>  		_base_mask_interrupts(ioc);
>  		ioc->shost_recovery = 1;
>  		_base_make_ioc_ready(ioc, CAN_SLEEP, SOFT_RESET);
> @@ -4454,6 +4460,7 @@ mpt2sas_base_free_resources(struct MPT2SAS_ADAPTER *ioc)
>  		pci_disable_pcie_error_reporting(pdev);
>  		pci_disable_device(pdev);
>  	}
> +	mutex_unlock(&ioc->pci_access_mutex);
>  	return;
>  }
>  
Lock imbalance. Please move the call to 'mutex_lock()' out of the
'if' clause.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html