* [PATCH 0/2] _scsih_sas_host_add early exits can crash system
@ 2016-05-25 19:14 Joe Lawrence
2016-05-25 19:14 ` [PATCH 1/2] mpt3sas - set num_phys after allocating phy[] space Joe Lawrence
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Joe Lawrence @ 2016-05-25 19:14 UTC (permalink / raw)
To: linux-scsi
Cc: sathya.prakash, chaitra.basappa, suganath-prabu.subramani, Joe Lawrence
There are many error paths in _scsih_sas_host_add that lead to an early
exit and a few that leave IOC resources uninitialized, setting the stage
for a later crash.
This can be emulated using a systemtap script like:
% stap -g -e \
'probe module("mpt3sas").function("mpt3sas_config_get_sas_iounit_pg0").return { $return = -1 }'
to force early exit, while remove/re-adding an MPT3 adapter:
% lspci -D | grep MPT
0000:54:00.0 Mass storage controller: LSI Logic / Symbios Logic SAS3008 PCI-Express Fusion-MPT SAS-3 (rev 02)
% SYSFS=$(find /sys/devices -name 0000:54:00.0)
% SYSFS_PARENT=$(dirname $SYSFS)
% echo 1 > $SYSFS/remove
% sleep 1m
% echo 1 > $SYSFS_PARENT/rescan
These two patches fix:
1) referencing unallocated ioc->sas_hba.phy[] space
2) passing a NULL ioc->sas_hba.parent_dev to the scsi_transport_sas
layer.
Note: these changes don't improve or retry adapter initialization, but
only try to prevent the system from crashing
Joe Lawrence (2):
mpt3sas - set num_phys after allocating phy[] space
mpt3sas - avoid mpt3sas_transport_port_add NULL parent_dev
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 20 +++++++++++---------
drivers/scsi/mpt3sas/mpt3sas_transport.c | 5 +++++
2 files changed, 16 insertions(+), 9 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] mpt3sas - set num_phys after allocating phy[] space 2016-05-25 19:14 [PATCH 0/2] _scsih_sas_host_add early exits can crash system Joe Lawrence @ 2016-05-25 19:14 ` Joe Lawrence 2016-05-27 8:18 ` Sreekanth Reddy 2016-05-25 19:14 ` [PATCH 2/2] mpt3sas - avoid mpt3sas_transport_port_add NULL parent_dev Joe Lawrence 2016-06-01 2:41 ` [PATCH 0/2] _scsih_sas_host_add early exits can crash system Martin K. Petersen 2 siblings, 1 reply; 6+ messages in thread From: Joe Lawrence @ 2016-05-25 19:14 UTC (permalink / raw) To: linux-scsi Cc: sathya.prakash, chaitra.basappa, suganath-prabu.subramani, Joe Lawrence In _scsih_sas_host_add, the number of HBA phys are determined and then later used to allocate an array of struct _sas_phy's. If the routine sets ioc->sas_hba.num_phys, but then fails to allocate the ioc->sas_hba.phy array (by kcalloc error or other intermediate error/exit path), ioc->sas_hba is left in a dangerous state: all readers of ioc->sas_hba.phy[] do so by indexing it from 0..ioc->sas_hba.num_phys without checking that the space was ever allocated. Modify _scsih_sas_host_add to set ioc->sas_hba.num_phys only after successfully allocating ioc->sas_hba.phy[]. Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com> --- drivers/scsi/mpt3sas/mpt3sas_scsih.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c index f2139e5604a3..6e36d20c9e0b 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c @@ -4893,13 +4893,22 @@ _scsih_sas_host_add(struct MPT3SAS_ADAPTER *ioc) u16 ioc_status; u16 sz; u8 device_missing_delay; + u8 num_phys; - mpt3sas_config_get_number_hba_phys(ioc, &ioc->sas_hba.num_phys); - if (!ioc->sas_hba.num_phys) { + mpt3sas_config_get_number_hba_phys(ioc, &num_phys); + if (!num_phys) { pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n", ioc->name, __FILE__, __LINE__, __func__); return; } + ioc->sas_hba.phy = kcalloc(num_phys, + sizeof(struct _sas_phy), GFP_KERNEL); + if (!ioc->sas_hba.phy) { + pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n", + ioc->name, __FILE__, __LINE__, __func__); + goto out; + } + ioc->sas_hba.num_phys = num_phys; /* sas_iounit page 0 */ sz = offsetof(Mpi2SasIOUnitPage0_t, PhyData) + (ioc->sas_hba.num_phys * @@ -4959,13 +4968,6 @@ _scsih_sas_host_add(struct MPT3SAS_ADAPTER *ioc) MPI2_SASIOUNIT1_REPORT_MISSING_TIMEOUT_MASK; ioc->sas_hba.parent_dev = &ioc->shost->shost_gendev; - ioc->sas_hba.phy = kcalloc(ioc->sas_hba.num_phys, - sizeof(struct _sas_phy), GFP_KERNEL); - if (!ioc->sas_hba.phy) { - pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n", - ioc->name, __FILE__, __LINE__, __func__); - goto out; - } for (i = 0; i < ioc->sas_hba.num_phys ; i++) { if ((mpt3sas_config_get_phy_pg0(ioc, &mpi_reply, &phy_pg0, i))) { -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] mpt3sas - set num_phys after allocating phy[] space 2016-05-25 19:14 ` [PATCH 1/2] mpt3sas - set num_phys after allocating phy[] space Joe Lawrence @ 2016-05-27 8:18 ` Sreekanth Reddy 0 siblings, 0 replies; 6+ messages in thread From: Sreekanth Reddy @ 2016-05-27 8:18 UTC (permalink / raw) To: Joe Lawrence Cc: linux-scsi, Sathya Prakash Veerichetty, Chaitra Basappa, Suganath Prabu Subramani Hi, This patch looks good. Please consider this patch as Ack-by: Sreekanth Reddy <sreekanth.reddy@broadcom.com> Thanks, Sreekanth On Thu, May 26, 2016 at 12:44 AM, Joe Lawrence <joe.lawrence@stratus.com> wrote: > In _scsih_sas_host_add, the number of HBA phys are determined and then > later used to allocate an array of struct _sas_phy's. If the routine > sets ioc->sas_hba.num_phys, but then fails to allocate the > ioc->sas_hba.phy array (by kcalloc error or other intermediate > error/exit path), ioc->sas_hba is left in a dangerous state: all readers > of ioc->sas_hba.phy[] do so by indexing it from 0..ioc->sas_hba.num_phys > without checking that the space was ever allocated. > > Modify _scsih_sas_host_add to set ioc->sas_hba.num_phys only after > successfully allocating ioc->sas_hba.phy[]. > > Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com> > --- > drivers/scsi/mpt3sas/mpt3sas_scsih.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c > index f2139e5604a3..6e36d20c9e0b 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c > @@ -4893,13 +4893,22 @@ _scsih_sas_host_add(struct MPT3SAS_ADAPTER *ioc) > u16 ioc_status; > u16 sz; > u8 device_missing_delay; > + u8 num_phys; > > - mpt3sas_config_get_number_hba_phys(ioc, &ioc->sas_hba.num_phys); > - if (!ioc->sas_hba.num_phys) { > + mpt3sas_config_get_number_hba_phys(ioc, &num_phys); > + if (!num_phys) { > pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n", > ioc->name, __FILE__, __LINE__, __func__); > return; > } > + ioc->sas_hba.phy = kcalloc(num_phys, > + sizeof(struct _sas_phy), GFP_KERNEL); > + if (!ioc->sas_hba.phy) { > + pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n", > + ioc->name, __FILE__, __LINE__, __func__); > + goto out; > + } > + ioc->sas_hba.num_phys = num_phys; > > /* sas_iounit page 0 */ > sz = offsetof(Mpi2SasIOUnitPage0_t, PhyData) + (ioc->sas_hba.num_phys * > @@ -4959,13 +4968,6 @@ _scsih_sas_host_add(struct MPT3SAS_ADAPTER *ioc) > MPI2_SASIOUNIT1_REPORT_MISSING_TIMEOUT_MASK; > > ioc->sas_hba.parent_dev = &ioc->shost->shost_gendev; > - ioc->sas_hba.phy = kcalloc(ioc->sas_hba.num_phys, > - sizeof(struct _sas_phy), GFP_KERNEL); > - if (!ioc->sas_hba.phy) { > - pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n", > - ioc->name, __FILE__, __LINE__, __func__); > - goto out; > - } > for (i = 0; i < ioc->sas_hba.num_phys ; i++) { > if ((mpt3sas_config_get_phy_pg0(ioc, &mpi_reply, &phy_pg0, > i))) { > -- > 1.8.3.1 > > -- > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] mpt3sas - avoid mpt3sas_transport_port_add NULL parent_dev 2016-05-25 19:14 [PATCH 0/2] _scsih_sas_host_add early exits can crash system Joe Lawrence 2016-05-25 19:14 ` [PATCH 1/2] mpt3sas - set num_phys after allocating phy[] space Joe Lawrence @ 2016-05-25 19:14 ` Joe Lawrence 2016-05-27 8:18 ` Sreekanth Reddy 2016-06-01 2:41 ` [PATCH 0/2] _scsih_sas_host_add early exits can crash system Martin K. Petersen 2 siblings, 1 reply; 6+ messages in thread From: Joe Lawrence @ 2016-05-25 19:14 UTC (permalink / raw) To: linux-scsi Cc: sathya.prakash, chaitra.basappa, suganath-prabu.subramani, Joe Lawrence If _scsih_sas_host_add's call to mpt3sas_config_get_sas_iounit_pg0 fails, ioc->sas_hba.parent_dev may be left uninitialized. A later device probe could invoke mpt3sas_transport_port_add which will call sas_port_alloc_num [scsi_transport_sas] with a NULL parent_dev pointer. Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com> --- drivers/scsi/mpt3sas/mpt3sas_transport.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c b/drivers/scsi/mpt3sas/mpt3sas_transport.c index 6a84b82d71bb..ff93286bc32f 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_transport.c +++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c @@ -705,6 +705,11 @@ mpt3sas_transport_port_add(struct MPT3SAS_ADAPTER *ioc, u16 handle, goto out_fail; } + if (!sas_node->parent_dev) { + pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n", + ioc->name, __FILE__, __LINE__, __func__); + goto out_fail; + } port = sas_port_alloc_num(sas_node->parent_dev); if ((sas_port_add(port))) { pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n", -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] mpt3sas - avoid mpt3sas_transport_port_add NULL parent_dev 2016-05-25 19:14 ` [PATCH 2/2] mpt3sas - avoid mpt3sas_transport_port_add NULL parent_dev Joe Lawrence @ 2016-05-27 8:18 ` Sreekanth Reddy 0 siblings, 0 replies; 6+ messages in thread From: Sreekanth Reddy @ 2016-05-27 8:18 UTC (permalink / raw) To: Joe Lawrence Cc: linux-scsi, Sathya Prakash Veerichetty, Chaitra Basappa, Suganath Prabu Subramani Hi, This patch looks good. Please consider this patch as Ack-by: Sreekanth Reddy <sreekanth.reddy@broadcom.com> Thanks, Sreekanth On Thu, May 26, 2016 at 12:44 AM, Joe Lawrence <joe.lawrence@stratus.com> wrote: > If _scsih_sas_host_add's call to mpt3sas_config_get_sas_iounit_pg0 > fails, ioc->sas_hba.parent_dev may be left uninitialized. A later > device probe could invoke mpt3sas_transport_port_add which will call > sas_port_alloc_num [scsi_transport_sas] with a NULL parent_dev pointer. > > Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com> > --- > drivers/scsi/mpt3sas/mpt3sas_transport.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c b/drivers/scsi/mpt3sas/mpt3sas_transport.c > index 6a84b82d71bb..ff93286bc32f 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_transport.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c > @@ -705,6 +705,11 @@ mpt3sas_transport_port_add(struct MPT3SAS_ADAPTER *ioc, u16 handle, > goto out_fail; > } > > + if (!sas_node->parent_dev) { > + pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n", > + ioc->name, __FILE__, __LINE__, __func__); > + goto out_fail; > + } > port = sas_port_alloc_num(sas_node->parent_dev); > if ((sas_port_add(port))) { > pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n", > -- > 1.8.3.1 > > -- > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] _scsih_sas_host_add early exits can crash system 2016-05-25 19:14 [PATCH 0/2] _scsih_sas_host_add early exits can crash system Joe Lawrence 2016-05-25 19:14 ` [PATCH 1/2] mpt3sas - set num_phys after allocating phy[] space Joe Lawrence 2016-05-25 19:14 ` [PATCH 2/2] mpt3sas - avoid mpt3sas_transport_port_add NULL parent_dev Joe Lawrence @ 2016-06-01 2:41 ` Martin K. Petersen 2 siblings, 0 replies; 6+ messages in thread From: Martin K. Petersen @ 2016-06-01 2:41 UTC (permalink / raw) To: Joe Lawrence Cc: linux-scsi, sathya.prakash, chaitra.basappa, suganath-prabu.subramani >>>>> "Joe" == Joe Lawrence <joe.lawrence@stratus.com> writes: Joe> There are many error paths in _scsih_sas_host_add that lead to an Joe> early exit and a few that leave IOC resources uninitialized, Joe> setting the stage for a later crash. Applied to 4.8/scsi-queue. Please make sure to use "mpt3sas: foo bar" in the patch descriptions. You had used "mpt3sas - foo bar". Thanks! -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-01 2:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-25 19:14 [PATCH 0/2] _scsih_sas_host_add early exits can crash system Joe Lawrence 2016-05-25 19:14 ` [PATCH 1/2] mpt3sas - set num_phys after allocating phy[] space Joe Lawrence 2016-05-27 8:18 ` Sreekanth Reddy 2016-05-25 19:14 ` [PATCH 2/2] mpt3sas - avoid mpt3sas_transport_port_add NULL parent_dev Joe Lawrence 2016-05-27 8:18 ` Sreekanth Reddy 2016-06-01 2:41 ` [PATCH 0/2] _scsih_sas_host_add early exits can crash system Martin K. Petersen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.