diff mbox

[v4] mpt2sas: setpci reset kernel oops fix

Message ID 20150818075710.GA11947@nagalsi.ban.indi.seagate.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nagarajkumar Narayanan Aug. 18, 2015, 7:57 a.m. UTC
mpt2sas: 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


From c53a1cff4c07528b8b9ec7f6716e94950283e8f9 Mon Sep 17 00:00:00 2001
From: Nagarajkumar Narayanan <nagarajkumar.narayanan@seagate.com>
Date: Tue, 18 Aug 2015 11:58:13 +0530
Subject: [PATCH] mpt2sas setpci reset oops fix

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

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

Note: pci_access_mutex is used only if nytro warpdrive cards
(ioc->is_warpdrive based on device id) are used
as we could not test this case with other SAS2 HBA cards
We can remove this check if this behaviour confirmed from other
cards.

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

Signed-off-by: Nagarajkumar Narayanan <nagarajkumar.narayanan@seagate.com>
---
* v4
- spinlock function moved from spinlock_irqsave to spinlock on gioc lock 
  as it was not   used in interrupt context
- added common exit point for pci_access_mutex unlock

* v3
- fixed lock imbalance, moved acquiring mutex lock out of if condition

* 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  |    6 +++++
 drivers/scsi/mpt2sas/mpt2sas_base.h  |   19 ++++++++++++++++-
 drivers/scsi/mpt2sas/mpt2sas_ctl.c   |   38 +++++++++++++++++++++++++++------
 drivers/scsi/mpt2sas/mpt2sas_scsih.c |   13 ++++++++++-
 4 files changed, 67 insertions(+), 9 deletions(-)

Comments

Johannes Thumshirn Sept. 1, 2015, 8:22 a.m. UTC | #1
Nagarajkumar Narayanan <nagarajkumar.narayanan@seagate.com> writes:

> mpt2sas: 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
>
>
> From c53a1cff4c07528b8b9ec7f6716e94950283e8f9 Mon Sep 17 00:00:00 2001
> From: Nagarajkumar Narayanan <nagarajkumar.narayanan@seagate.com>
> Date: Tue, 18 Aug 2015 11:58:13 +0530
> Subject: [PATCH] mpt2sas setpci reset oops fix
>
> 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
>
> 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
>
> Note: pci_access_mutex is used only if nytro warpdrive cards
> (ioc->is_warpdrive based on device id) are used
> as we could not test this case with other SAS2 HBA cards
> We can remove this check if this behaviour confirmed from other
> cards.
>
> 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
>
> Signed-off-by: Nagarajkumar Narayanan <nagarajkumar.narayanan@seagate.com>
> ---
> * v4
> - spinlock function moved from spinlock_irqsave to spinlock on gioc lock 
>   as it was not   used in interrupt context
> - added common exit point for pci_access_mutex unlock
>
> * v3
> - fixed lock imbalance, moved acquiring mutex lock out of if condition
>
> * 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  |    6 +++++
>  drivers/scsi/mpt2sas/mpt2sas_base.h  |   19 ++++++++++++++++-
>  drivers/scsi/mpt2sas/mpt2sas_ctl.c   |   38 +++++++++++++++++++++++++++------
>  drivers/scsi/mpt2sas/mpt2sas_scsih.c |   13 ++++++++++-
>  4 files changed, 67 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
> index 11248de..60fefca 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c
> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
> @@ -112,9 +112,12 @@ _scsih_set_fwfault_debug(const char *val, struct kernel_param *kp)
>  	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(&gioc_lock);
>  	list_for_each_entry(ioc, &mpt2sas_ioc_list, list)
>  		ioc->fwfault_debug = mpt2sas_fwfault_debug;
> +	spin_unlock(&gioc_lock);
>  	return 0;
>  }
>  
> @@ -4435,6 +4438,8 @@ mpt2sas_base_free_resources(struct MPT2SAS_ADAPTER *ioc)
>  	dexitprintk(ioc, printk(MPT2SAS_INFO_FMT "%s\n", ioc->name,
>  	    __func__));
>  
> +	/* synchronizing freeing resource with pci_access_mutex lock */
> +	mutex_lock(&ioc->pci_access_mutex);
>  	if (ioc->chip_phys && ioc->chip) {
>  		_base_mask_interrupts(ioc);
>  		ioc->shost_recovery = 1;
> @@ -4454,6 +4459,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;
>  }
>  
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.h b/drivers/scsi/mpt2sas/mpt2sas_base.h
> index caff8d1..c82bdb3 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_base.h
> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.h
> @@ -799,6 +799,12 @@ typedef void (*MPT2SAS_FLUSH_RUNNING_CMDS)(struct MPT2SAS_ADAPTER *ioc);
>   * @delayed_tr_list: target reset link list
>   * @delayed_tr_volume_list: volume target reset link list
>   * @@temp_sensors_count: flag to carry the number of temperature sensors
> + * @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
>   */
>  struct MPT2SAS_ADAPTER {
>  	struct list_head list;
> @@ -1015,6 +1021,7 @@ struct MPT2SAS_ADAPTER {
>  	u8		mfg_pg10_hide_flag;
>  	u8		hide_drives;
>  
> +	struct mutex pci_access_mutex;
>  };
>  
>  typedef u8 (*MPT_CALLBACK)(struct MPT2SAS_ADAPTER *ioc, u16 smid, u8 msix_index,
> @@ -1023,6 +1030,17 @@ typedef u8 (*MPT_CALLBACK)(struct MPT2SAS_ADAPTER *ioc, u16 smid, u8 msix_index,
>  
>  /* base shared API */
>  extern struct list_head mpt2sas_ioc_list;
> +/* 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
> + */
> +extern spinlock_t gioc_lock;
>  void mpt2sas_base_start_watchdog(struct MPT2SAS_ADAPTER *ioc);
>  void mpt2sas_base_stop_watchdog(struct MPT2SAS_ADAPTER *ioc);
>  
> @@ -1099,7 +1117,6 @@ struct _sas_device *mpt2sas_scsih_sas_device_find_by_sas_address(
>      struct MPT2SAS_ADAPTER *ioc, u64 sas_address);
>  
>  void mpt2sas_port_enable_complete(struct MPT2SAS_ADAPTER *ioc);
> -
>  void mpt2sas_scsih_reset_handler(struct MPT2SAS_ADAPTER *ioc, int reset_phase);
>  
>  /* config shared API */
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_ctl.c b/drivers/scsi/mpt2sas/mpt2sas_ctl.c
> index 4e50960..3694b63 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_ctl.c
> +++ b/drivers/scsi/mpt2sas/mpt2sas_ctl.c
> @@ -427,13 +427,16 @@ static int
>  _ctl_verify_adapter(int ioc_number, struct MPT2SAS_ADAPTER **iocpp)
>  {
>  	struct MPT2SAS_ADAPTER *ioc;
> -
> +	/* global ioc lock to protect controller on list operations */
> +	spin_lock(&gioc_lock);
>  	list_for_each_entry(ioc, &mpt2sas_ioc_list, list) {
>  		if (ioc->id != ioc_number)
>  			continue;
> +		spin_unlock(&gioc_lock);
>  		*iocpp = ioc;
>  		return ioc_number;
>  	}
> +	spin_unlock(&gioc_lock);
>  	*iocpp = NULL;
>  	return -1;
>  }
> @@ -522,10 +525,15 @@ _ctl_poll(struct file *filep, poll_table *wait)
>  
>  	poll_wait(filep, &ctl_poll_wait, wait);
>  
> +	/* global ioc lock to protect controller on list operations */
> +	spin_lock(&gioc_lock);
>  	list_for_each_entry(ioc, &mpt2sas_ioc_list, list) {
> -		if (ioc->aen_event_read_flag)
> +		if (ioc->aen_event_read_flag) {
> +			spin_unlock(&gioc_lock);
>  			return POLLIN | POLLRDNORM;
> +		}
>  	}
> +	spin_unlock(&gioc_lock);
>  	return 0;
>  }
>  
> @@ -2168,16 +2176,23 @@ _ctl_ioctl_main(struct file *file, unsigned int cmd, void __user *arg,
>  
>  	if (_ctl_verify_adapter(ioctl_header.ioc_number, &ioc) == -1 || !ioc)
>  		return -ENODEV;
> +	/* pci_access_mutex lock acquired by ioctl path */
> +	mutex_lock(&ioc->pci_access_mutex);
>  	if (ioc->shost_recovery || ioc->pci_error_recovery ||
> -	    ioc->is_driver_loading)
> -		return -EAGAIN;
> +		ioc->is_driver_loading || ioc->remove_host) {
> +		ret = -EAGAIN;
> +		goto out_unlock_pciaccess;
> +	}
>  
>  	state = (file->f_flags & O_NONBLOCK) ? NON_BLOCKING : BLOCKING;
>  	if (state == NON_BLOCKING) {
> -		if (!mutex_trylock(&ioc->ctl_cmds.mutex))
> -			return -EAGAIN;
> +		if (!mutex_trylock(&ioc->ctl_cmds.mutex)) {
> +			ret = -EAGAIN;
> +			goto out_unlock_pciaccess;
> +		}
>  	} else if (mutex_lock_interruptible(&ioc->ctl_cmds.mutex)) {
> -		return -ERESTARTSYS;
> +		ret = -ERESTARTSYS;
> +		goto out_unlock_pciaccess;
>  	}
>  
>  	switch (cmd) {
> @@ -2258,6 +2273,8 @@ _ctl_ioctl_main(struct file *file, unsigned int cmd, void __user *arg,
>  	}
>  
>  	mutex_unlock(&ioc->ctl_cmds.mutex);
> +out_unlock_pciaccess:
> +	mutex_unlock(&ioc->pci_access_mutex);
>  	return ret;
>  }
>  
> @@ -2711,6 +2728,12 @@ _ctl_BRM_status_show(struct device *cdev, struct device_attribute *attr,
>  		    "warpdrive\n", ioc->name, __func__);
>  		goto out;
>  	}
> +	/* pci_access_mutex lock acquired by sysfs show path */
> +	mutex_lock(&ioc->pci_access_mutex);
> +	if (ioc->pci_error_recovery || ioc->remove_host) {
> +		mutex_unlock(&ioc->pci_access_mutex);
> +		return 0;
> +	}
>  
>  	/* allocate upto GPIOVal 36 entries */
>  	sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36);
> @@ -2749,6 +2772,7 @@ _ctl_BRM_status_show(struct device *cdev, struct device_attribute *attr,
>  
>   out:
>  	kfree(io_unit_pg3);
> +	mutex_unlock(&ioc->pci_access_mutex);
>  	return rc;
>  }
>  static DEVICE_ATTR(BRM_status, S_IRUGO, _ctl_BRM_status_show, NULL);
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> index 3f26147..2124e08 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> @@ -79,7 +79,8 @@ static int _scsih_scan_finished(struct Scsi_Host *shost, unsigned long time);
>  
>  /* global parameters */
>  LIST_HEAD(mpt2sas_ioc_list);
> -
> +/* global ioc lock for list operations */
> +DEFINE_SPINLOCK(gioc_lock);
>  /* local parameters */
>  static u8 scsi_io_cb_idx = -1;
>  static u8 tm_cb_idx = -1;
> @@ -293,8 +294,10 @@ _scsih_set_debug_level(const char *val, struct kernel_param *kp)
>  		return ret;
>  
>  	printk(KERN_INFO "setting logging_level(0x%08x)\n", logging_level);
> +	spin_lock(&gioc_lock);
>  	list_for_each_entry(ioc, &mpt2sas_ioc_list, list)
>  		ioc->logging_level = logging_level;
> +	spin_unlock(&gioc_lock);
>  	return 0;
>  }
>  module_param_call(logging_level, _scsih_set_debug_level, param_get_int,
> @@ -7867,7 +7870,9 @@ _scsih_remove(struct pci_dev *pdev)
>  	sas_remove_host(shost);
>  	scsi_remove_host(shost);
>  	mpt2sas_base_detach(ioc);
> +	spin_lock(&gioc_lock);
>  	list_del(&ioc->list);
> +	spin_unlock(&gioc_lock);
>  	scsi_host_put(shost);
>  }
>  
> @@ -8142,7 +8147,9 @@ _scsih_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	ioc = shost_priv(shost);
>  	memset(ioc, 0, sizeof(struct MPT2SAS_ADAPTER));
>  	INIT_LIST_HEAD(&ioc->list);
> +	spin_lock(&gioc_lock);
>  	list_add_tail(&ioc->list, &mpt2sas_ioc_list);
> +	spin_unlock(&gioc_lock);
>  	ioc->shost = shost;
>  	ioc->id = mpt_ids++;
>  	sprintf(ioc->name, "%s%d", MPT2SAS_DRIVER_NAME, ioc->id);
> @@ -8167,6 +8174,8 @@ _scsih_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	ioc->schedule_dead_ioc_flush_running_cmds = &_scsih_flush_running_cmds;
>  	/* misc semaphores and spin locks */
>  	mutex_init(&ioc->reset_in_progress_mutex);
> +	/* initializing pci_access_mutex lock */
> +	mutex_init(&ioc->pci_access_mutex);
>  	spin_lock_init(&ioc->ioc_reset_in_progress_lock);
>  	spin_lock_init(&ioc->scsi_lookup_lock);
>  	spin_lock_init(&ioc->sas_device_lock);
> @@ -8269,7 +8278,9 @@ _scsih_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   out_attach_fail:
>  	destroy_workqueue(ioc->firmware_event_thread);
>   out_thread_fail:
> +	spin_lock(&gioc_lock);
>  	list_del(&ioc->list);
> +	spin_unlock(&gioc_lock);
>  	scsi_host_put(shost);
>  	return rv;
>  }


Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Sreekanth Reddy Sept. 4, 2015, 2:37 p.m. UTC | #2
On Tue, Sep 1, 2015 at 1:52 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
> Nagarajkumar Narayanan <nagarajkumar.narayanan@seagate.com> writes:
>
>> mpt2sas: 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
>>
>>
>> From c53a1cff4c07528b8b9ec7f6716e94950283e8f9 Mon Sep 17 00:00:00 2001
>> From: Nagarajkumar Narayanan <nagarajkumar.narayanan@seagate.com>
>> Date: Tue, 18 Aug 2015 11:58:13 +0530
>> Subject: [PATCH] mpt2sas setpci reset oops fix
>>
>> 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
>>
>> 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
>>
>> Note: pci_access_mutex is used only if nytro warpdrive cards
>> (ioc->is_warpdrive based on device id) are used
>> as we could not test this case with other SAS2 HBA cards
>> We can remove this check if this behaviour confirmed from other
>> cards.
>>
>> 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
>>
>> Signed-off-by: Nagarajkumar Narayanan <nagarajkumar.narayanan@seagate.com>
>> ---
>> * v4
>> - spinlock function moved from spinlock_irqsave to spinlock on gioc lock
>>   as it was not   used in interrupt context
>> - added common exit point for pci_access_mutex unlock
>>
>> * v3
>> - fixed lock imbalance, moved acquiring mutex lock out of if condition
>>
>> * 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  |    6 +++++
>>  drivers/scsi/mpt2sas/mpt2sas_base.h  |   19 ++++++++++++++++-
>>  drivers/scsi/mpt2sas/mpt2sas_ctl.c   |   38 +++++++++++++++++++++++++++------
>>  drivers/scsi/mpt2sas/mpt2sas_scsih.c |   13 ++++++++++-
>>  4 files changed, 67 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
>> index 11248de..60fefca 100644
>> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c
>> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
>> @@ -112,9 +112,12 @@ _scsih_set_fwfault_debug(const char *val, struct kernel_param *kp)
>>       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(&gioc_lock);
>>       list_for_each_entry(ioc, &mpt2sas_ioc_list, list)
>>               ioc->fwfault_debug = mpt2sas_fwfault_debug;
>> +     spin_unlock(&gioc_lock);
>>       return 0;
>>  }
>>
>> @@ -4435,6 +4438,8 @@ mpt2sas_base_free_resources(struct MPT2SAS_ADAPTER *ioc)
>>       dexitprintk(ioc, printk(MPT2SAS_INFO_FMT "%s\n", ioc->name,
>>           __func__));
>>
>> +     /* synchronizing freeing resource with pci_access_mutex lock */
>> +     mutex_lock(&ioc->pci_access_mutex);
>>       if (ioc->chip_phys && ioc->chip) {
>>               _base_mask_interrupts(ioc);
>>               ioc->shost_recovery = 1;
>> @@ -4454,6 +4459,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;
>>  }
>>
>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.h b/drivers/scsi/mpt2sas/mpt2sas_base.h
>> index caff8d1..c82bdb3 100644
>> --- a/drivers/scsi/mpt2sas/mpt2sas_base.h
>> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.h
>> @@ -799,6 +799,12 @@ typedef void (*MPT2SAS_FLUSH_RUNNING_CMDS)(struct MPT2SAS_ADAPTER *ioc);
>>   * @delayed_tr_list: target reset link list
>>   * @delayed_tr_volume_list: volume target reset link list
>>   * @@temp_sensors_count: flag to carry the number of temperature sensors
>> + * @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
>>   */
>>  struct MPT2SAS_ADAPTER {
>>       struct list_head list;
>> @@ -1015,6 +1021,7 @@ struct MPT2SAS_ADAPTER {
>>       u8              mfg_pg10_hide_flag;
>>       u8              hide_drives;
>>
>> +     struct mutex pci_access_mutex;
>>  };
>>
>>  typedef u8 (*MPT_CALLBACK)(struct MPT2SAS_ADAPTER *ioc, u16 smid, u8 msix_index,
>> @@ -1023,6 +1030,17 @@ typedef u8 (*MPT_CALLBACK)(struct MPT2SAS_ADAPTER *ioc, u16 smid, u8 msix_index,
>>
>>  /* base shared API */
>>  extern struct list_head mpt2sas_ioc_list;
>> +/* 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
>> + */
>> +extern spinlock_t gioc_lock;
>>  void mpt2sas_base_start_watchdog(struct MPT2SAS_ADAPTER *ioc);
>>  void mpt2sas_base_stop_watchdog(struct MPT2SAS_ADAPTER *ioc);
>>
>> @@ -1099,7 +1117,6 @@ struct _sas_device *mpt2sas_scsih_sas_device_find_by_sas_address(
>>      struct MPT2SAS_ADAPTER *ioc, u64 sas_address);
>>
>>  void mpt2sas_port_enable_complete(struct MPT2SAS_ADAPTER *ioc);
>> -
>>  void mpt2sas_scsih_reset_handler(struct MPT2SAS_ADAPTER *ioc, int reset_phase);
>>
>>  /* config shared API */
>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_ctl.c b/drivers/scsi/mpt2sas/mpt2sas_ctl.c
>> index 4e50960..3694b63 100644
>> --- a/drivers/scsi/mpt2sas/mpt2sas_ctl.c
>> +++ b/drivers/scsi/mpt2sas/mpt2sas_ctl.c
>> @@ -427,13 +427,16 @@ static int
>>  _ctl_verify_adapter(int ioc_number, struct MPT2SAS_ADAPTER **iocpp)
>>  {
>>       struct MPT2SAS_ADAPTER *ioc;
>> -
>> +     /* global ioc lock to protect controller on list operations */
>> +     spin_lock(&gioc_lock);
>>       list_for_each_entry(ioc, &mpt2sas_ioc_list, list) {
>>               if (ioc->id != ioc_number)
>>                       continue;
>> +             spin_unlock(&gioc_lock);
>>               *iocpp = ioc;
>>               return ioc_number;
>>       }
>> +     spin_unlock(&gioc_lock);
>>       *iocpp = NULL;
>>       return -1;
>>  }
>> @@ -522,10 +525,15 @@ _ctl_poll(struct file *filep, poll_table *wait)
>>
>>       poll_wait(filep, &ctl_poll_wait, wait);
>>
>> +     /* global ioc lock to protect controller on list operations */
>> +     spin_lock(&gioc_lock);
>>       list_for_each_entry(ioc, &mpt2sas_ioc_list, list) {
>> -             if (ioc->aen_event_read_flag)
>> +             if (ioc->aen_event_read_flag) {
>> +                     spin_unlock(&gioc_lock);
>>                       return POLLIN | POLLRDNORM;
>> +             }
>>       }
>> +     spin_unlock(&gioc_lock);
>>       return 0;
>>  }
>>
>> @@ -2168,16 +2176,23 @@ _ctl_ioctl_main(struct file *file, unsigned int cmd, void __user *arg,
>>
>>       if (_ctl_verify_adapter(ioctl_header.ioc_number, &ioc) == -1 || !ioc)
>>               return -ENODEV;
>> +     /* pci_access_mutex lock acquired by ioctl path */
>> +     mutex_lock(&ioc->pci_access_mutex);
>>       if (ioc->shost_recovery || ioc->pci_error_recovery ||
>> -         ioc->is_driver_loading)
>> -             return -EAGAIN;
>> +             ioc->is_driver_loading || ioc->remove_host) {
>> +             ret = -EAGAIN;
>> +             goto out_unlock_pciaccess;
>> +     }
>>
>>       state = (file->f_flags & O_NONBLOCK) ? NON_BLOCKING : BLOCKING;
>>       if (state == NON_BLOCKING) {
>> -             if (!mutex_trylock(&ioc->ctl_cmds.mutex))
>> -                     return -EAGAIN;
>> +             if (!mutex_trylock(&ioc->ctl_cmds.mutex)) {
>> +                     ret = -EAGAIN;
>> +                     goto out_unlock_pciaccess;
>> +             }
>>       } else if (mutex_lock_interruptible(&ioc->ctl_cmds.mutex)) {
>> -             return -ERESTARTSYS;
>> +             ret = -ERESTARTSYS;
>> +             goto out_unlock_pciaccess;
>>       }
>>
>>       switch (cmd) {
>> @@ -2258,6 +2273,8 @@ _ctl_ioctl_main(struct file *file, unsigned int cmd, void __user *arg,
>>       }
>>
>>       mutex_unlock(&ioc->ctl_cmds.mutex);
>> +out_unlock_pciaccess:
>> +     mutex_unlock(&ioc->pci_access_mutex);
>>       return ret;
>>  }
>>
>> @@ -2711,6 +2728,12 @@ _ctl_BRM_status_show(struct device *cdev, struct device_attribute *attr,
>>                   "warpdrive\n", ioc->name, __func__);
>>               goto out;
>>       }
>> +     /* pci_access_mutex lock acquired by sysfs show path */
>> +     mutex_lock(&ioc->pci_access_mutex);
>> +     if (ioc->pci_error_recovery || ioc->remove_host) {
>> +             mutex_unlock(&ioc->pci_access_mutex);
>> +             return 0;
>> +     }
>>
>>       /* allocate upto GPIOVal 36 entries */
>>       sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36);
>> @@ -2749,6 +2772,7 @@ _ctl_BRM_status_show(struct device *cdev, struct device_attribute *attr,
>>
>>   out:
>>       kfree(io_unit_pg3);
>> +     mutex_unlock(&ioc->pci_access_mutex);
>>       return rc;
>>  }
>>  static DEVICE_ATTR(BRM_status, S_IRUGO, _ctl_BRM_status_show, NULL);
>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
>> index 3f26147..2124e08 100644
>> --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
>> +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
>> @@ -79,7 +79,8 @@ static int _scsih_scan_finished(struct Scsi_Host *shost, unsigned long time);
>>
>>  /* global parameters */
>>  LIST_HEAD(mpt2sas_ioc_list);
>> -
>> +/* global ioc lock for list operations */
>> +DEFINE_SPINLOCK(gioc_lock);
>>  /* local parameters */
>>  static u8 scsi_io_cb_idx = -1;
>>  static u8 tm_cb_idx = -1;
>> @@ -293,8 +294,10 @@ _scsih_set_debug_level(const char *val, struct kernel_param *kp)
>>               return ret;
>>
>>       printk(KERN_INFO "setting logging_level(0x%08x)\n", logging_level);
>> +     spin_lock(&gioc_lock);
>>       list_for_each_entry(ioc, &mpt2sas_ioc_list, list)
>>               ioc->logging_level = logging_level;
>> +     spin_unlock(&gioc_lock);
>>       return 0;
>>  }
>>  module_param_call(logging_level, _scsih_set_debug_level, param_get_int,
>> @@ -7867,7 +7870,9 @@ _scsih_remove(struct pci_dev *pdev)
>>       sas_remove_host(shost);
>>       scsi_remove_host(shost);
>>       mpt2sas_base_detach(ioc);
>> +     spin_lock(&gioc_lock);
>>       list_del(&ioc->list);
>> +     spin_unlock(&gioc_lock);
>>       scsi_host_put(shost);
>>  }
>>
>> @@ -8142,7 +8147,9 @@ _scsih_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>       ioc = shost_priv(shost);
>>       memset(ioc, 0, sizeof(struct MPT2SAS_ADAPTER));
>>       INIT_LIST_HEAD(&ioc->list);
>> +     spin_lock(&gioc_lock);
>>       list_add_tail(&ioc->list, &mpt2sas_ioc_list);
>> +     spin_unlock(&gioc_lock);
>>       ioc->shost = shost;
>>       ioc->id = mpt_ids++;
>>       sprintf(ioc->name, "%s%d", MPT2SAS_DRIVER_NAME, ioc->id);
>> @@ -8167,6 +8174,8 @@ _scsih_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>       ioc->schedule_dead_ioc_flush_running_cmds = &_scsih_flush_running_cmds;
>>       /* misc semaphores and spin locks */
>>       mutex_init(&ioc->reset_in_progress_mutex);
>> +     /* initializing pci_access_mutex lock */
>> +     mutex_init(&ioc->pci_access_mutex);
>>       spin_lock_init(&ioc->ioc_reset_in_progress_lock);
>>       spin_lock_init(&ioc->scsi_lookup_lock);
>>       spin_lock_init(&ioc->sas_device_lock);
>> @@ -8269,7 +8278,9 @@ _scsih_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   out_attach_fail:
>>       destroy_workqueue(ioc->firmware_event_thread);
>>   out_thread_fail:
>> +     spin_lock(&gioc_lock);
>>       list_del(&ioc->list);
>> +     spin_unlock(&gioc_lock);
>>       scsi_host_put(shost);
>>       return rv;
>>  }
>
>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

ACK-by: Sreekanth Reddy <sreekanth.reddy@avagotech.com>

>
> --
--
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
diff mbox

Patch

diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
index 11248de..60fefca 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
@@ -112,9 +112,12 @@  _scsih_set_fwfault_debug(const char *val, struct kernel_param *kp)
 	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(&gioc_lock);
 	list_for_each_entry(ioc, &mpt2sas_ioc_list, list)
 		ioc->fwfault_debug = mpt2sas_fwfault_debug;
+	spin_unlock(&gioc_lock);
 	return 0;
 }
 
@@ -4435,6 +4438,8 @@  mpt2sas_base_free_resources(struct MPT2SAS_ADAPTER *ioc)
 	dexitprintk(ioc, printk(MPT2SAS_INFO_FMT "%s\n", ioc->name,
 	    __func__));
 
+	/* synchronizing freeing resource with pci_access_mutex lock */
+	mutex_lock(&ioc->pci_access_mutex);
 	if (ioc->chip_phys && ioc->chip) {
 		_base_mask_interrupts(ioc);
 		ioc->shost_recovery = 1;
@@ -4454,6 +4459,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;
 }
 
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.h b/drivers/scsi/mpt2sas/mpt2sas_base.h
index caff8d1..c82bdb3 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.h
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.h
@@ -799,6 +799,12 @@  typedef void (*MPT2SAS_FLUSH_RUNNING_CMDS)(struct MPT2SAS_ADAPTER *ioc);
  * @delayed_tr_list: target reset link list
  * @delayed_tr_volume_list: volume target reset link list
  * @@temp_sensors_count: flag to carry the number of temperature sensors
+ * @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
  */
 struct MPT2SAS_ADAPTER {
 	struct list_head list;
@@ -1015,6 +1021,7 @@  struct MPT2SAS_ADAPTER {
 	u8		mfg_pg10_hide_flag;
 	u8		hide_drives;
 
+	struct mutex pci_access_mutex;
 };
 
 typedef u8 (*MPT_CALLBACK)(struct MPT2SAS_ADAPTER *ioc, u16 smid, u8 msix_index,
@@ -1023,6 +1030,17 @@  typedef u8 (*MPT_CALLBACK)(struct MPT2SAS_ADAPTER *ioc, u16 smid, u8 msix_index,
 
 /* base shared API */
 extern struct list_head mpt2sas_ioc_list;
+/* 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
+ */
+extern spinlock_t gioc_lock;
 void mpt2sas_base_start_watchdog(struct MPT2SAS_ADAPTER *ioc);
 void mpt2sas_base_stop_watchdog(struct MPT2SAS_ADAPTER *ioc);
 
@@ -1099,7 +1117,6 @@  struct _sas_device *mpt2sas_scsih_sas_device_find_by_sas_address(
     struct MPT2SAS_ADAPTER *ioc, u64 sas_address);
 
 void mpt2sas_port_enable_complete(struct MPT2SAS_ADAPTER *ioc);
-
 void mpt2sas_scsih_reset_handler(struct MPT2SAS_ADAPTER *ioc, int reset_phase);
 
 /* config shared API */
diff --git a/drivers/scsi/mpt2sas/mpt2sas_ctl.c b/drivers/scsi/mpt2sas/mpt2sas_ctl.c
index 4e50960..3694b63 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_ctl.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_ctl.c
@@ -427,13 +427,16 @@  static int
 _ctl_verify_adapter(int ioc_number, struct MPT2SAS_ADAPTER **iocpp)
 {
 	struct MPT2SAS_ADAPTER *ioc;
-
+	/* global ioc lock to protect controller on list operations */
+	spin_lock(&gioc_lock);
 	list_for_each_entry(ioc, &mpt2sas_ioc_list, list) {
 		if (ioc->id != ioc_number)
 			continue;
+		spin_unlock(&gioc_lock);
 		*iocpp = ioc;
 		return ioc_number;
 	}
+	spin_unlock(&gioc_lock);
 	*iocpp = NULL;
 	return -1;
 }
@@ -522,10 +525,15 @@  _ctl_poll(struct file *filep, poll_table *wait)
 
 	poll_wait(filep, &ctl_poll_wait, wait);
 
+	/* global ioc lock to protect controller on list operations */
+	spin_lock(&gioc_lock);
 	list_for_each_entry(ioc, &mpt2sas_ioc_list, list) {
-		if (ioc->aen_event_read_flag)
+		if (ioc->aen_event_read_flag) {
+			spin_unlock(&gioc_lock);
 			return POLLIN | POLLRDNORM;
+		}
 	}
+	spin_unlock(&gioc_lock);
 	return 0;
 }
 
@@ -2168,16 +2176,23 @@  _ctl_ioctl_main(struct file *file, unsigned int cmd, void __user *arg,
 
 	if (_ctl_verify_adapter(ioctl_header.ioc_number, &ioc) == -1 || !ioc)
 		return -ENODEV;
+	/* pci_access_mutex lock acquired by ioctl path */
+	mutex_lock(&ioc->pci_access_mutex);
 	if (ioc->shost_recovery || ioc->pci_error_recovery ||
-	    ioc->is_driver_loading)
-		return -EAGAIN;
+		ioc->is_driver_loading || ioc->remove_host) {
+		ret = -EAGAIN;
+		goto out_unlock_pciaccess;
+	}
 
 	state = (file->f_flags & O_NONBLOCK) ? NON_BLOCKING : BLOCKING;
 	if (state == NON_BLOCKING) {
-		if (!mutex_trylock(&ioc->ctl_cmds.mutex))
-			return -EAGAIN;
+		if (!mutex_trylock(&ioc->ctl_cmds.mutex)) {
+			ret = -EAGAIN;
+			goto out_unlock_pciaccess;
+		}
 	} else if (mutex_lock_interruptible(&ioc->ctl_cmds.mutex)) {
-		return -ERESTARTSYS;
+		ret = -ERESTARTSYS;
+		goto out_unlock_pciaccess;
 	}
 
 	switch (cmd) {
@@ -2258,6 +2273,8 @@  _ctl_ioctl_main(struct file *file, unsigned int cmd, void __user *arg,
 	}
 
 	mutex_unlock(&ioc->ctl_cmds.mutex);
+out_unlock_pciaccess:
+	mutex_unlock(&ioc->pci_access_mutex);
 	return ret;
 }
 
@@ -2711,6 +2728,12 @@  _ctl_BRM_status_show(struct device *cdev, struct device_attribute *attr,
 		    "warpdrive\n", ioc->name, __func__);
 		goto out;
 	}
+	/* pci_access_mutex lock acquired by sysfs show path */
+	mutex_lock(&ioc->pci_access_mutex);
+	if (ioc->pci_error_recovery || ioc->remove_host) {
+		mutex_unlock(&ioc->pci_access_mutex);
+		return 0;
+	}
 
 	/* allocate upto GPIOVal 36 entries */
 	sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36);
@@ -2749,6 +2772,7 @@  _ctl_BRM_status_show(struct device *cdev, struct device_attribute *attr,
 
  out:
 	kfree(io_unit_pg3);
+	mutex_unlock(&ioc->pci_access_mutex);
 	return rc;
 }
 static DEVICE_ATTR(BRM_status, S_IRUGO, _ctl_BRM_status_show, NULL);
diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
index 3f26147..2124e08 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
@@ -79,7 +79,8 @@  static int _scsih_scan_finished(struct Scsi_Host *shost, unsigned long time);
 
 /* global parameters */
 LIST_HEAD(mpt2sas_ioc_list);
-
+/* global ioc lock for list operations */
+DEFINE_SPINLOCK(gioc_lock);
 /* local parameters */
 static u8 scsi_io_cb_idx = -1;
 static u8 tm_cb_idx = -1;
@@ -293,8 +294,10 @@  _scsih_set_debug_level(const char *val, struct kernel_param *kp)
 		return ret;
 
 	printk(KERN_INFO "setting logging_level(0x%08x)\n", logging_level);
+	spin_lock(&gioc_lock);
 	list_for_each_entry(ioc, &mpt2sas_ioc_list, list)
 		ioc->logging_level = logging_level;
+	spin_unlock(&gioc_lock);
 	return 0;
 }
 module_param_call(logging_level, _scsih_set_debug_level, param_get_int,
@@ -7867,7 +7870,9 @@  _scsih_remove(struct pci_dev *pdev)
 	sas_remove_host(shost);
 	scsi_remove_host(shost);
 	mpt2sas_base_detach(ioc);
+	spin_lock(&gioc_lock);
 	list_del(&ioc->list);
+	spin_unlock(&gioc_lock);
 	scsi_host_put(shost);
 }
 
@@ -8142,7 +8147,9 @@  _scsih_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	ioc = shost_priv(shost);
 	memset(ioc, 0, sizeof(struct MPT2SAS_ADAPTER));
 	INIT_LIST_HEAD(&ioc->list);
+	spin_lock(&gioc_lock);
 	list_add_tail(&ioc->list, &mpt2sas_ioc_list);
+	spin_unlock(&gioc_lock);
 	ioc->shost = shost;
 	ioc->id = mpt_ids++;
 	sprintf(ioc->name, "%s%d", MPT2SAS_DRIVER_NAME, ioc->id);
@@ -8167,6 +8174,8 @@  _scsih_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	ioc->schedule_dead_ioc_flush_running_cmds = &_scsih_flush_running_cmds;
 	/* misc semaphores and spin locks */
 	mutex_init(&ioc->reset_in_progress_mutex);
+	/* initializing pci_access_mutex lock */
+	mutex_init(&ioc->pci_access_mutex);
 	spin_lock_init(&ioc->ioc_reset_in_progress_lock);
 	spin_lock_init(&ioc->scsi_lookup_lock);
 	spin_lock_init(&ioc->sas_device_lock);
@@ -8269,7 +8278,9 @@  _scsih_probe(struct pci_dev *pdev, const struct pci_device_id *id)
  out_attach_fail:
 	destroy_workqueue(ioc->firmware_event_thread);
  out_thread_fail:
+	spin_lock(&gioc_lock);
 	list_del(&ioc->list);
+	spin_unlock(&gioc_lock);
 	scsi_host_put(shost);
 	return rv;
 }