Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix unsafe list usage

[Nicholas A. Bellinger]

On Fri, 2015-09-11 at 10:50 -0700, James Bottomley wrote:
> On Thu, 2015-09-10 at 23:55 -0700, Nicholas A. Bellinger wrote:
> > On Wed, 2015-09-09 at 15:03 -0700, Nicholas A. Bellinger wrote:
> > > On Wed, 2015-09-09 at 19:59 +0530, Chaitra Basappa wrote:
> > > > From: Sreekanth Reddy [mailto:sreekanth.re...@avagotech.com]
> > > > Sent: Tuesday, September 08, 2015 5:26 PM
> > > > To: Nicholas A. Bellinger
> > > > Cc: linux-scsi; linux-kernel; James Bottomley; Calvin Owens; Christoph
> > > > Hellwig; MPT-FusionLinux.pdl; kernel-team; Nicholas Bellinger; Chaitra
> > > > Basappa
> > > > Subject: Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix
> > > > unsafe list usage
> > > > 
> > > > On Sun, Aug 30, 2015 at 1:24 PM, Nicholas A. Bellinger 
> > > > 
> > > > wrote:
> > > > > From: Nicholas Bellinger 
> > > > >
> > > > > These objects can be referenced concurrently throughout the driver, we
> > > > > need a way to make sure threads can't delete them out from under each
> > > > > other. This patch adds the refcount, and refactors the code to use it.
> > > > >
> > > > > Additionally, we cannot iterate over the sas_device_list without
> > > > > holding the lock, or we risk corrupting random memory if items are
> > > > > added or deleted as we iterate. This patch refactors
> > > > > _scsih_probe_sas() to use the sas_device_list in a safe way.
> > > > >
> > > > > This patch is a port of Calvin's PATCH-v4 for mpt2sas code, atop
> > > > > mpt3sas changes in scsi.git/for-next.
> > > > >
> > > > > Cc: Calvin Owens 
> > > > > Cc: Christoph Hellwig 
> > > > > Cc: Sreekanth Reddy 
> > > > > Cc: MPT-FusionLinux.pdl 
> > > > > Signed-off-by: Nicholas Bellinger 
> > > > > ---
> > > > >  drivers/scsi/mpt3sas/mpt3sas_base.h  |  25 +-
> > > > >  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 479
> > > > > +--
> > > > >  drivers/scsi/mpt3sas/mpt3sas_transport.c |  18 +-
> > > > >  3 files changed, 364 insertions(+), 158 deletions(-)
> > > > >
> > > > > @@ -2763,7 +2874,7 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER 
> > > > > *ioc,
> > > > > u16 handle)
> > > > > struct scsi_device *sdev;
> > > > > struct _sas_device *sas_device;
> > > > >
> > > > 
> > > > [Sreekanth] Here sas_device_lock spin lock needs to be acquired before
> > > > calling
> > > >   __mpt3sas_get_sdev_by_addr() function.
> > > > 
> > > > [Chaitra]Here instead of calling " __mpt3sas_get_sdev_by_handle()" 
> > > > function
> > > > calling
> > > > "mpt3sas_get_sdev_by_handle()" function will fixes "invalid 
> > > > page access"
> > > > type of kernel panic
> > > > 
> > > > > -   sas_device = _scsih_sas_device_find_by_handle(ioc, handle);
> > > > > +   sas_device = __mpt3sas_get_sdev_by_handle(ioc, handle);
> > > > > if (!sas_device)
> > > > > return;
> > > > >
> > > 
> > > Whoops, missed this comment in _scsih_block_io_device() from Sreekanth's
> > > earlier reply.
> > > 
> > > Here's the updated incremental patch atop target-pending/for-next-merge
> > > to use the protected callers for both cases.
> > > 
> > > Please review + ACK ASAP.
> > 
> > The mpt3sas -v2 series + v4.3-rc0 breakage incremental patch here made
> > it into linux-next-09102015, and at this point I don't see a scenario
> > where keeping around the broken list_head dereferences makes sense.
> 
> I already explained the dangers of what the patch does.  Separated
> lifetime objects need to be treated very carefully.  Rushing this in to
> -rc1 without an Avago soak test is irresponsible.  Two issues have
> already turned up in this thanks to inspection and as a bug fix it's not
> bound by the merge window anyway so there's no reason to rush it into
> -rc1 without the proper testing.
> 

It's not being 'rushed in'.  The changes have being run continuously on
60+ HBAs w/ 720+ HDDs using v3.14.y code for the last 3 weeks.

Calvin reviewed the code, the Avago folks have commented on the co

Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix unsafe list usage

[James Bottomley]

On Thu, 2015-09-10 at 23:55 -0700, Nicholas A. Bellinger wrote:
> On Wed, 2015-09-09 at 15:03 -0700, Nicholas A. Bellinger wrote:
> > On Wed, 2015-09-09 at 19:59 +0530, Chaitra Basappa wrote:
> > > From: Sreekanth Reddy [mailto:sreekanth.re...@avagotech.com]
> > > Sent: Tuesday, September 08, 2015 5:26 PM
> > > To: Nicholas A. Bellinger
> > > Cc: linux-scsi; linux-kernel; James Bottomley; Calvin Owens; Christoph
> > > Hellwig; MPT-FusionLinux.pdl; kernel-team; Nicholas Bellinger; Chaitra
> > > Basappa
> > > Subject: Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix
> > > unsafe list usage
> > > 
> > > On Sun, Aug 30, 2015 at 1:24 PM, Nicholas A. Bellinger 
> > > 
> > > wrote:
> > > > From: Nicholas Bellinger 
> > > >
> > > > These objects can be referenced concurrently throughout the driver, we
> > > > need a way to make sure threads can't delete them out from under each
> > > > other. This patch adds the refcount, and refactors the code to use it.
> > > >
> > > > Additionally, we cannot iterate over the sas_device_list without
> > > > holding the lock, or we risk corrupting random memory if items are
> > > > added or deleted as we iterate. This patch refactors
> > > > _scsih_probe_sas() to use the sas_device_list in a safe way.
> > > >
> > > > This patch is a port of Calvin's PATCH-v4 for mpt2sas code, atop
> > > > mpt3sas changes in scsi.git/for-next.
> > > >
> > > > Cc: Calvin Owens 
> > > > Cc: Christoph Hellwig 
> > > > Cc: Sreekanth Reddy 
> > > > Cc: MPT-FusionLinux.pdl 
> > > > Signed-off-by: Nicholas Bellinger 
> > > > ---
> > > >  drivers/scsi/mpt3sas/mpt3sas_base.h  |  25 +-
> > > >  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 479
> > > > +--
> > > >  drivers/scsi/mpt3sas/mpt3sas_transport.c |  18 +-
> > > >  3 files changed, 364 insertions(+), 158 deletions(-)
> > > >
> > > > @@ -2763,7 +2874,7 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER 
> > > > *ioc,
> > > > u16 handle)
> > > > struct scsi_device *sdev;
> > > > struct _sas_device *sas_device;
> > > >
> > > 
> > > [Sreekanth] Here sas_device_lock spin lock needs to be acquired before
> > > calling
> > >   __mpt3sas_get_sdev_by_addr() function.
> > > 
> > > [Chaitra]Here instead of calling " __mpt3sas_get_sdev_by_handle()" 
> > > function
> > > calling
> > >   "mpt3sas_get_sdev_by_handle()" function will fixes "invalid page access"
> > > type of kernel panic
> > > 
> > > > -   sas_device = _scsih_sas_device_find_by_handle(ioc, handle);
> > > > +   sas_device = __mpt3sas_get_sdev_by_handle(ioc, handle);
> > > > if (!sas_device)
> > > > return;
> > > >
> > 
> > Whoops, missed this comment in _scsih_block_io_device() from Sreekanth's
> > earlier reply.
> > 
> > Here's the updated incremental patch atop target-pending/for-next-merge
> > to use the protected callers for both cases.
> > 
> > Please review + ACK ASAP.
> 
> The mpt3sas -v2 series + v4.3-rc0 breakage incremental patch here made
> it into linux-next-09102015, and at this point I don't see a scenario
> where keeping around the broken list_head dereferences makes sense.

I already explained the dangers of what the patch does.  Separated
lifetime objects need to be treated very carefully.  Rushing this in to
-rc1 without an Avago soak test is irresponsible.  Two issues have
already turned up in this thanks to inspection and as a bug fix it's not
bound by the merge window anyway so there's no reason to rush it into
-rc1 without the proper testing.

The reason for wanting to do this right is not to create a bisection
black hole: if we create an unreliable base storage driver by rushing
this into -rc1 it makes bisection very difficult for people who use mpt3
gear because they won't know if it's the bug they're chasing or the one
we introduced which they can't avoid because they have to use a storage
driver to boot the kernel.


> So that said, I'd like to send a target-pending/for-next-merge PULL
> request out to Linus in the next 48 hours.

How about no: it's not a target patch, it's an initiator patch, which
makes it my decision not yours.  The Maintainers are being responsive,
so there's no reason to override their request for a soak test, even if
you are the patch author.  It will get pushed once they confirm.

James



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix unsafe list usage

[Nicholas A. Bellinger]

On Wed, 2015-09-09 at 15:03 -0700, Nicholas A. Bellinger wrote:
> On Wed, 2015-09-09 at 19:59 +0530, Chaitra Basappa wrote:
> > From: Sreekanth Reddy [mailto:sreekanth.re...@avagotech.com]
> > Sent: Tuesday, September 08, 2015 5:26 PM
> > To: Nicholas A. Bellinger
> > Cc: linux-scsi; linux-kernel; James Bottomley; Calvin Owens; Christoph
> > Hellwig; MPT-FusionLinux.pdl; kernel-team; Nicholas Bellinger; Chaitra
> > Basappa
> > Subject: Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix
> > unsafe list usage
> > 
> > On Sun, Aug 30, 2015 at 1:24 PM, Nicholas A. Bellinger 
> > wrote:
> > > From: Nicholas Bellinger 
> > >
> > > These objects can be referenced concurrently throughout the driver, we
> > > need a way to make sure threads can't delete them out from under each
> > > other. This patch adds the refcount, and refactors the code to use it.
> > >
> > > Additionally, we cannot iterate over the sas_device_list without
> > > holding the lock, or we risk corrupting random memory if items are
> > > added or deleted as we iterate. This patch refactors
> > > _scsih_probe_sas() to use the sas_device_list in a safe way.
> > >
> > > This patch is a port of Calvin's PATCH-v4 for mpt2sas code, atop
> > > mpt3sas changes in scsi.git/for-next.
> > >
> > > Cc: Calvin Owens 
> > > Cc: Christoph Hellwig 
> > > Cc: Sreekanth Reddy 
> > > Cc: MPT-FusionLinux.pdl 
> > > Signed-off-by: Nicholas Bellinger 
> > > ---
> > >  drivers/scsi/mpt3sas/mpt3sas_base.h  |  25 +-
> > >  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 479
> > > +--
> > >  drivers/scsi/mpt3sas/mpt3sas_transport.c |  18 +-
> > >  3 files changed, 364 insertions(+), 158 deletions(-)
> > >
> > > @@ -2763,7 +2874,7 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER *ioc,
> > > u16 handle)
> > > struct scsi_device *sdev;
> > > struct _sas_device *sas_device;
> > >
> > 
> > [Sreekanth] Here sas_device_lock spin lock needs to be acquired before
> > calling
> >   __mpt3sas_get_sdev_by_addr() function.
> > 
> > [Chaitra]Here instead of calling " __mpt3sas_get_sdev_by_handle()" function
> > calling
> > "mpt3sas_get_sdev_by_handle()" function will fixes "invalid page access"
> > type of kernel panic
> > 
> > > -   sas_device = _scsih_sas_device_find_by_handle(ioc, handle);
> > > +   sas_device = __mpt3sas_get_sdev_by_handle(ioc, handle);
> > > if (!sas_device)
> > > return;
> > >
> 
> Whoops, missed this comment in _scsih_block_io_device() from Sreekanth's
> earlier reply.
> 
> Here's the updated incremental patch atop target-pending/for-next-merge
> to use the protected callers for both cases.
> 
> Please review + ACK ASAP.

The mpt3sas -v2 series + v4.3-rc0 breakage incremental patch here made
it into linux-next-09102015, and at this point I don't see a scenario
where keeping around the broken list_head dereferences makes sense.

So that said, I'd like to send a target-pending/for-next-merge PULL
request out to Linus in the next 48 hours.

Any objections from Avago folks..?

Thank you,

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix unsafe list usage

[Nicholas A. Bellinger]

On Wed, 2015-09-09 at 15:03 -0700, Nicholas A. Bellinger wrote:
> On Wed, 2015-09-09 at 19:59 +0530, Chaitra Basappa wrote:
> > From: Sreekanth Reddy [mailto:sreekanth.re...@avagotech.com]
> > Sent: Tuesday, September 08, 2015 5:26 PM
> > To: Nicholas A. Bellinger
> > Cc: linux-scsi; linux-kernel; James Bottomley; Calvin Owens; Christoph
> > Hellwig; MPT-FusionLinux.pdl; kernel-team; Nicholas Bellinger; Chaitra
> > Basappa
> > Subject: Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix
> > unsafe list usage
> > 
> > On Sun, Aug 30, 2015 at 1:24 PM, Nicholas A. Bellinger <n...@daterainc.com>
> > wrote:
> > > From: Nicholas Bellinger <n...@linux-iscsi.org>
> > >
> > > These objects can be referenced concurrently throughout the driver, we
> > > need a way to make sure threads can't delete them out from under each
> > > other. This patch adds the refcount, and refactors the code to use it.
> > >
> > > Additionally, we cannot iterate over the sas_device_list without
> > > holding the lock, or we risk corrupting random memory if items are
> > > added or deleted as we iterate. This patch refactors
> > > _scsih_probe_sas() to use the sas_device_list in a safe way.
> > >
> > > This patch is a port of Calvin's PATCH-v4 for mpt2sas code, atop
> > > mpt3sas changes in scsi.git/for-next.
> > >
> > > Cc: Calvin Owens <calvinow...@fb.com>
> > > Cc: Christoph Hellwig <h...@infradead.org>
> > > Cc: Sreekanth Reddy <sreekanth.re...@avagotech.com>
> > > Cc: MPT-FusionLinux.pdl <mpt-fusionlinux@avagotech.com>
> > > Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
> > > ---
> > >  drivers/scsi/mpt3sas/mpt3sas_base.h  |  25 +-
> > >  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 479
> > > +--
> > >  drivers/scsi/mpt3sas/mpt3sas_transport.c |  18 +-
> > >  3 files changed, 364 insertions(+), 158 deletions(-)
> > >
> > > @@ -2763,7 +2874,7 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER *ioc,
> > > u16 handle)
> > > struct scsi_device *sdev;
> > > struct _sas_device *sas_device;
> > >
> > 
> > [Sreekanth] Here sas_device_lock spin lock needs to be acquired before
> > calling
> >   __mpt3sas_get_sdev_by_addr() function.
> > 
> > [Chaitra]Here instead of calling " __mpt3sas_get_sdev_by_handle()" function
> > calling
> > "mpt3sas_get_sdev_by_handle()" function will fixes "invalid page access"
> > type of kernel panic
> > 
> > > -   sas_device = _scsih_sas_device_find_by_handle(ioc, handle);
> > > +   sas_device = __mpt3sas_get_sdev_by_handle(ioc, handle);
> > > if (!sas_device)
> > > return;
> > >
> 
> Whoops, missed this comment in _scsih_block_io_device() from Sreekanth's
> earlier reply.
> 
> Here's the updated incremental patch atop target-pending/for-next-merge
> to use the protected callers for both cases.
> 
> Please review + ACK ASAP.

The mpt3sas -v2 series + v4.3-rc0 breakage incremental patch here made
it into linux-next-09102015, and at this point I don't see a scenario
where keeping around the broken list_head dereferences makes sense.

So that said, I'd like to send a target-pending/for-next-merge PULL
request out to Linus in the next 48 hours.

Any objections from Avago folks..?

Thank you,

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix unsafe list usage

[James Bottomley]

On Thu, 2015-09-10 at 23:55 -0700, Nicholas A. Bellinger wrote:
> On Wed, 2015-09-09 at 15:03 -0700, Nicholas A. Bellinger wrote:
> > On Wed, 2015-09-09 at 19:59 +0530, Chaitra Basappa wrote:
> > > From: Sreekanth Reddy [mailto:sreekanth.re...@avagotech.com]
> > > Sent: Tuesday, September 08, 2015 5:26 PM
> > > To: Nicholas A. Bellinger
> > > Cc: linux-scsi; linux-kernel; James Bottomley; Calvin Owens; Christoph
> > > Hellwig; MPT-FusionLinux.pdl; kernel-team; Nicholas Bellinger; Chaitra
> > > Basappa
> > > Subject: Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix
> > > unsafe list usage
> > > 
> > > On Sun, Aug 30, 2015 at 1:24 PM, Nicholas A. Bellinger 
> > > <n...@daterainc.com>
> > > wrote:
> > > > From: Nicholas Bellinger <n...@linux-iscsi.org>
> > > >
> > > > These objects can be referenced concurrently throughout the driver, we
> > > > need a way to make sure threads can't delete them out from under each
> > > > other. This patch adds the refcount, and refactors the code to use it.
> > > >
> > > > Additionally, we cannot iterate over the sas_device_list without
> > > > holding the lock, or we risk corrupting random memory if items are
> > > > added or deleted as we iterate. This patch refactors
> > > > _scsih_probe_sas() to use the sas_device_list in a safe way.
> > > >
> > > > This patch is a port of Calvin's PATCH-v4 for mpt2sas code, atop
> > > > mpt3sas changes in scsi.git/for-next.
> > > >
> > > > Cc: Calvin Owens <calvinow...@fb.com>
> > > > Cc: Christoph Hellwig <h...@infradead.org>
> > > > Cc: Sreekanth Reddy <sreekanth.re...@avagotech.com>
> > > > Cc: MPT-FusionLinux.pdl <mpt-fusionlinux@avagotech.com>
> > > > Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
> > > > ---
> > > >  drivers/scsi/mpt3sas/mpt3sas_base.h  |  25 +-
> > > >  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 479
> > > > +--
> > > >  drivers/scsi/mpt3sas/mpt3sas_transport.c |  18 +-
> > > >  3 files changed, 364 insertions(+), 158 deletions(-)
> > > >
> > > > @@ -2763,7 +2874,7 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER 
> > > > *ioc,
> > > > u16 handle)
> > > > struct scsi_device *sdev;
> > > > struct _sas_device *sas_device;
> > > >
> > > 
> > > [Sreekanth] Here sas_device_lock spin lock needs to be acquired before
> > > calling
> > >   __mpt3sas_get_sdev_by_addr() function.
> > > 
> > > [Chaitra]Here instead of calling " __mpt3sas_get_sdev_by_handle()" 
> > > function
> > > calling
> > >   "mpt3sas_get_sdev_by_handle()" function will fixes "invalid page access"
> > > type of kernel panic
> > > 
> > > > -   sas_device = _scsih_sas_device_find_by_handle(ioc, handle);
> > > > +   sas_device = __mpt3sas_get_sdev_by_handle(ioc, handle);
> > > > if (!sas_device)
> > > > return;
> > > >
> > 
> > Whoops, missed this comment in _scsih_block_io_device() from Sreekanth's
> > earlier reply.
> > 
> > Here's the updated incremental patch atop target-pending/for-next-merge
> > to use the protected callers for both cases.
> > 
> > Please review + ACK ASAP.
> 
> The mpt3sas -v2 series + v4.3-rc0 breakage incremental patch here made
> it into linux-next-09102015, and at this point I don't see a scenario
> where keeping around the broken list_head dereferences makes sense.

I already explained the dangers of what the patch does.  Separated
lifetime objects need to be treated very carefully.  Rushing this in to
-rc1 without an Avago soak test is irresponsible.  Two issues have
already turned up in this thanks to inspection and as a bug fix it's not
bound by the merge window anyway so there's no reason to rush it into
-rc1 without the proper testing.

The reason for wanting to do this right is not to create a bisection
black hole: if we create an unreliable base storage driver by rushing
this into -rc1 it makes bisection very difficult for people who use mpt3
gear because they won't know if it's the bug they're chasing or the one
we introduced which they can't avoid because they have to use a storage
driver to boot the kernel.


> So that said, I'd like to send a target-pending/for-next-merge PULL
> request out to Linus in the next 48 hours.

How about no: it's not a target patch, it's an initiator patch, which
makes it my decision not yours.  The Maintainers are being responsive,
so there's no reason to override their request for a soak test, even if
you are the patch author.  It will get pushed once they confirm.

James



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix unsafe list usage

[Nicholas A. Bellinger]

On Fri, 2015-09-11 at 10:50 -0700, James Bottomley wrote:
> On Thu, 2015-09-10 at 23:55 -0700, Nicholas A. Bellinger wrote:
> > On Wed, 2015-09-09 at 15:03 -0700, Nicholas A. Bellinger wrote:
> > > On Wed, 2015-09-09 at 19:59 +0530, Chaitra Basappa wrote:
> > > > From: Sreekanth Reddy [mailto:sreekanth.re...@avagotech.com]
> > > > Sent: Tuesday, September 08, 2015 5:26 PM
> > > > To: Nicholas A. Bellinger
> > > > Cc: linux-scsi; linux-kernel; James Bottomley; Calvin Owens; Christoph
> > > > Hellwig; MPT-FusionLinux.pdl; kernel-team; Nicholas Bellinger; Chaitra
> > > > Basappa
> > > > Subject: Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix
> > > > unsafe list usage
> > > > 
> > > > On Sun, Aug 30, 2015 at 1:24 PM, Nicholas A. Bellinger 
> > > > <n...@daterainc.com>
> > > > wrote:
> > > > > From: Nicholas Bellinger <n...@linux-iscsi.org>
> > > > >
> > > > > These objects can be referenced concurrently throughout the driver, we
> > > > > need a way to make sure threads can't delete them out from under each
> > > > > other. This patch adds the refcount, and refactors the code to use it.
> > > > >
> > > > > Additionally, we cannot iterate over the sas_device_list without
> > > > > holding the lock, or we risk corrupting random memory if items are
> > > > > added or deleted as we iterate. This patch refactors
> > > > > _scsih_probe_sas() to use the sas_device_list in a safe way.
> > > > >
> > > > > This patch is a port of Calvin's PATCH-v4 for mpt2sas code, atop
> > > > > mpt3sas changes in scsi.git/for-next.
> > > > >
> > > > > Cc: Calvin Owens <calvinow...@fb.com>
> > > > > Cc: Christoph Hellwig <h...@infradead.org>
> > > > > Cc: Sreekanth Reddy <sreekanth.re...@avagotech.com>
> > > > > Cc: MPT-FusionLinux.pdl <mpt-fusionlinux@avagotech.com>
> > > > > Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
> > > > > ---
> > > > >  drivers/scsi/mpt3sas/mpt3sas_base.h  |  25 +-
> > > > >  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 479
> > > > > +--
> > > > >  drivers/scsi/mpt3sas/mpt3sas_transport.c |  18 +-
> > > > >  3 files changed, 364 insertions(+), 158 deletions(-)
> > > > >
> > > > > @@ -2763,7 +2874,7 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER 
> > > > > *ioc,
> > > > > u16 handle)
> > > > > struct scsi_device *sdev;
> > > > > struct _sas_device *sas_device;
> > > > >
> > > > 
> > > > [Sreekanth] Here sas_device_lock spin lock needs to be acquired before
> > > > calling
> > > >   __mpt3sas_get_sdev_by_addr() function.
> > > > 
> > > > [Chaitra]Here instead of calling " __mpt3sas_get_sdev_by_handle()" 
> > > > function
> > > > calling
> > > > "mpt3sas_get_sdev_by_handle()" function will fixes "invalid 
> > > > page access"
> > > > type of kernel panic
> > > > 
> > > > > -   sas_device = _scsih_sas_device_find_by_handle(ioc, handle);
> > > > > +   sas_device = __mpt3sas_get_sdev_by_handle(ioc, handle);
> > > > > if (!sas_device)
> > > > > return;
> > > > >
> > > 
> > > Whoops, missed this comment in _scsih_block_io_device() from Sreekanth's
> > > earlier reply.
> > > 
> > > Here's the updated incremental patch atop target-pending/for-next-merge
> > > to use the protected callers for both cases.
> > > 
> > > Please review + ACK ASAP.
> > 
> > The mpt3sas -v2 series + v4.3-rc0 breakage incremental patch here made
> > it into linux-next-09102015, and at this point I don't see a scenario
> > where keeping around the broken list_head dereferences makes sense.
> 
> I already explained the dangers of what the patch does.  Separated
> lifetime objects need to be treated very carefully.  Rushing this in to
> -rc1 without an Avago soak test is irresponsible.  Two issues have
> already turned up in this thanks to inspection and as a bug fix it's not
> bound by the merge window anyway so there's no reason to rush it into
> -rc1 without the proper testing.
&

Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix unsafe list usage

[Nicholas A. Bellinger]

On Wed, 2015-09-09 at 19:59 +0530, Chaitra Basappa wrote:
> From: Sreekanth Reddy [mailto:sreekanth.re...@avagotech.com]
> Sent: Tuesday, September 08, 2015 5:26 PM
> To: Nicholas A. Bellinger
> Cc: linux-scsi; linux-kernel; James Bottomley; Calvin Owens; Christoph
> Hellwig; MPT-FusionLinux.pdl; kernel-team; Nicholas Bellinger; Chaitra
> Basappa
> Subject: Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix
> unsafe list usage
> 
> On Sun, Aug 30, 2015 at 1:24 PM, Nicholas A. Bellinger 
> wrote:
> > From: Nicholas Bellinger 
> >
> > These objects can be referenced concurrently throughout the driver, we
> > need a way to make sure threads can't delete them out from under each
> > other. This patch adds the refcount, and refactors the code to use it.
> >
> > Additionally, we cannot iterate over the sas_device_list without
> > holding the lock, or we risk corrupting random memory if items are
> > added or deleted as we iterate. This patch refactors
> > _scsih_probe_sas() to use the sas_device_list in a safe way.
> >
> > This patch is a port of Calvin's PATCH-v4 for mpt2sas code, atop
> > mpt3sas changes in scsi.git/for-next.
> >
> > Cc: Calvin Owens 
> > Cc: Christoph Hellwig 
> > Cc: Sreekanth Reddy 
> > Cc: MPT-FusionLinux.pdl 
> > Signed-off-by: Nicholas Bellinger 
> > ---
> >  drivers/scsi/mpt3sas/mpt3sas_base.h  |  25 +-
> >  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 479
> > +--
> >  drivers/scsi/mpt3sas/mpt3sas_transport.c |  18 +-
> >  3 files changed, 364 insertions(+), 158 deletions(-)
> >
> > @@ -2763,7 +2874,7 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER *ioc,
> > u16 handle)
> > struct scsi_device *sdev;
> > struct _sas_device *sas_device;
> >
> 
> [Sreekanth] Here sas_device_lock spin lock needs to be acquired before
> calling
>   __mpt3sas_get_sdev_by_addr() function.
> 
> [Chaitra]Here instead of calling " __mpt3sas_get_sdev_by_handle()" function
> calling
>   "mpt3sas_get_sdev_by_handle()" function will fixes "invalid page access"
> type of kernel panic
> 
> > -   sas_device = _scsih_sas_device_find_by_handle(ioc, handle);
> > +   sas_device = __mpt3sas_get_sdev_by_handle(ioc, handle);
> > if (!sas_device)
> > return;
> >

Whoops, missed this comment in _scsih_block_io_device() from Sreekanth's
earlier reply.

Here's the updated incremental patch atop target-pending/for-next-merge
to use the protected callers for both cases.

Please review + ACK ASAP.

Thank you,

--nab

>From 8edb1554f7c2eb73cf70c9856aec01e786b9bcf9 Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger 
Date: Tue, 8 Sep 2015 23:05:49 -0700
Subject: [PATCH] mpt3sas: Fix unprotected list lookup in v4.3-rc0 changes

This patch adds the missing mpt3sas_get_sdev_by_addr() protected
lookup usage in mpt3sas_transport_port_add() to avoid a NULL pointer
dereference when >sas_device_list or >sas_device_init_list
changes from below without a proper sas_device_get(sas_device)
reference held.

Also, use the protected mpt3sas_get_sdev_by_handle() lookup within
_scsih_block_io_device() as well.

Reported-by: Sreekanth Reddy 
Reported-by: Chaitra Basappa 
Cc: Calvin Owens 
Cc: Christoph Hellwig 
Cc: Martin K. Petersen 
Signed-off-by: Nicholas Bellinger 
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 2 +-
 drivers/scsi/mpt3sas/mpt3sas_transport.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 0431cd0..9e68432 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -2933,7 +2933,7 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER *ioc, u16 
handle)
struct scsi_device *sdev;
struct _sas_device *sas_device;
 
-   sas_device = __mpt3sas_get_sdev_by_handle(ioc, handle);
+   sas_device = mpt3sas_get_sdev_by_handle(ioc, handle);
if (!sas_device)
return;
 
diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c 
b/drivers/scsi/mpt3sas/mpt3sas_transport.c
index 6074b11..ca36d7e 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
@@ -734,7 +734,7 @@ mpt3sas_transport_port_add(struct MPT3SAS_ADAPTER *ioc, u16 
handle,
rphy->identify = mpt3sas_port->remote_identify;
 
if (mpt3sas_port->remote_identify.device_type == SAS_END_DEVICE) {
-   sas_device = __mpt3sas_get_sdev_by_addr(ioc,
+   sas_device = mpt3sas_get_sdev_by_addr(ioc,
mpt3sas_port->remote_identify.sas_address);
if (!sas_device) {
dfailprintk(ioc, printk(MPT3SAS_FMT
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix unsafe list usage

[Chaitra Basappa]

From: Sreekanth Reddy [mailto:sreekanth.re...@avagotech.com]
Sent: Tuesday, September 08, 2015 5:26 PM
To: Nicholas A. Bellinger
Cc: linux-scsi; linux-kernel; James Bottomley; Calvin Owens; Christoph
Hellwig; MPT-FusionLinux.pdl; kernel-team; Nicholas Bellinger; Chaitra
Basappa
Subject: Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix
unsafe list usage

On Sun, Aug 30, 2015 at 1:24 PM, Nicholas A. Bellinger 
wrote:
> From: Nicholas Bellinger 
>
> These objects can be referenced concurrently throughout the driver, we
> need a way to make sure threads can't delete them out from under each
> other. This patch adds the refcount, and refactors the code to use it.
>
> Additionally, we cannot iterate over the sas_device_list without
> holding the lock, or we risk corrupting random memory if items are
> added or deleted as we iterate. This patch refactors
> _scsih_probe_sas() to use the sas_device_list in a safe way.
>
> This patch is a port of Calvin's PATCH-v4 for mpt2sas code, atop
> mpt3sas changes in scsi.git/for-next.
>
> Cc: Calvin Owens 
> Cc: Christoph Hellwig 
> Cc: Sreekanth Reddy 
> Cc: MPT-FusionLinux.pdl 
> Signed-off-by: Nicholas Bellinger 
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.h  |  25 +-
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 479
> +--
>  drivers/scsi/mpt3sas/mpt3sas_transport.c |  18 +-
>  3 files changed, 364 insertions(+), 158 deletions(-)
>
> @@ -2763,7 +2874,7 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER *ioc,
> u16 handle)
> struct scsi_device *sdev;
> struct _sas_device *sas_device;
>

[Sreekanth] Here sas_device_lock spin lock needs to be acquired before
calling
  __mpt3sas_get_sdev_by_addr() function.

[Chaitra]Here instead of calling " __mpt3sas_get_sdev_by_handle()" function
calling
"mpt3sas_get_sdev_by_handle()" function will fixes "invalid page access"
type of kernel panic

> -   sas_device = _scsih_sas_device_find_by_handle(ioc, handle);
> +   sas_device = __mpt3sas_get_sdev_by_handle(ioc, handle);
> if (!sas_device)
> return;
>
> @@ -2779,6 +2890,8 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER *ioc,
> u16 handle)
> continue;
> _scsih_internal_device_block(sdev, sas_device_priv_data);
> }
> +
> +   sas_device_put(sas_device);
>  }
>


Regards,
Chaitra
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix unsafe list usage

[Nicholas A. Bellinger]

Hi Sreekanth,

On Tue, 2015-09-08 at 17:25 +0530, Sreekanth Reddy wrote:
> On Sun, Aug 30, 2015 at 1:24 PM, Nicholas A. Bellinger
>  wrote:
> > From: Nicholas Bellinger 
> >
> > These objects can be referenced concurrently throughout the driver, we
> > need a way to make sure threads can't delete them out from under
> > each other. This patch adds the refcount, and refactors the code to use
> > it.
> >
> > Additionally, we cannot iterate over the sas_device_list without
> > holding the lock, or we risk corrupting random memory if items are
> > added or deleted as we iterate. This patch refactors _scsih_probe_sas()
> > to use the sas_device_list in a safe way.
> >
> > This patch is a port of Calvin's PATCH-v4 for mpt2sas code, atop
> > mpt3sas changes in scsi.git/for-next.
> >
> > Cc: Calvin Owens 
> > Cc: Christoph Hellwig 
> > Cc: Sreekanth Reddy 
> > Cc: MPT-FusionLinux.pdl 
> > Signed-off-by: Nicholas Bellinger 
> > ---
> >  drivers/scsi/mpt3sas/mpt3sas_base.h  |  25 +-
> >  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 479 
> > +--
> >  drivers/scsi/mpt3sas/mpt3sas_transport.c |  18 +-
> >  3 files changed, 364 insertions(+), 158 deletions(-)




> > diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c 
> > b/drivers/scsi/mpt3sas/mpt3sas_transport.c
> > index 70fd019..6074b11 100644
> > --- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
> > +++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
> > @@ -734,7 +734,7 @@ mpt3sas_transport_port_add(struct MPT3SAS_ADAPTER *ioc, 
> > u16 handle,
> > rphy->identify = mpt3sas_port->remote_identify;
> >
> > if (mpt3sas_port->remote_identify.device_type == SAS_END_DEVICE) {
> 
> [Sreekanth] Here also sas_device_lock spin lock needs to be acquired
> before calling
>   __mpt3sas_get_sdev_by_addr() function.
> 

Thanks for your review.

Fixed with the following incremental patch atop for-next-merge code.

Please review.

Thank you,

>From 4004584675d24d8e886b03c013074cdfb76a0864 Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger 
Date: Tue, 8 Sep 2015 23:05:49 -0700
Subject: [PATCH] mpt3sas: Add missing ioc->sas_device_lock in
 mpt3sas_transport_port_add

This patch adds the missing struct MPT3SAS_ADAPTER->sas_device_lock
usage in __mpt3sas_get_sdev_by_addr() lookup to avoid a NULL pointer
dereference when >sas_device_list or >sas_device_init_list
changes from below without a proper sas_device_get(sas_device)
reference held.

Reported-by: Sreekanth Reddy 
Cc: Calvin Owens 
Cc: Christoph Hellwig 
Cc: Martin K. Petersen 
Signed-off-by: Nicholas Bellinger 
---
 drivers/scsi/mpt3sas/mpt3sas_transport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c 
b/drivers/scsi/mpt3sas/mpt3sas_transport.c
index 6074b11..cc55907 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
@@ -734,8 +734,10 @@ mpt3sas_transport_port_add(struct MPT3SAS_ADAPTER *ioc, 
u16 handle,
rphy->identify = mpt3sas_port->remote_identify;
 
if (mpt3sas_port->remote_identify.device_type == SAS_END_DEVICE) {
+   spin_lock_irqsave(>sas_device_lock, flags);
sas_device = __mpt3sas_get_sdev_by_addr(ioc,
mpt3sas_port->remote_identify.sas_address);
+   spin_unlock_irqrestore(>sas_device_lock, flags);
if (!sas_device) {
dfailprintk(ioc, printk(MPT3SAS_FMT
"failure at %s:%d/%s()!\n",
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix unsafe list usage

[Nicholas A. Bellinger]

Hi Sreekanth,

On Tue, 2015-09-08 at 17:25 +0530, Sreekanth Reddy wrote:
> On Sun, Aug 30, 2015 at 1:24 PM, Nicholas A. Bellinger
>  wrote:
> > From: Nicholas Bellinger 
> >
> > These objects can be referenced concurrently throughout the driver, we
> > need a way to make sure threads can't delete them out from under
> > each other. This patch adds the refcount, and refactors the code to use
> > it.
> >
> > Additionally, we cannot iterate over the sas_device_list without
> > holding the lock, or we risk corrupting random memory if items are
> > added or deleted as we iterate. This patch refactors _scsih_probe_sas()
> > to use the sas_device_list in a safe way.
> >
> > This patch is a port of Calvin's PATCH-v4 for mpt2sas code, atop
> > mpt3sas changes in scsi.git/for-next.
> >
> > Cc: Calvin Owens 
> > Cc: Christoph Hellwig 
> > Cc: Sreekanth Reddy 
> > Cc: MPT-FusionLinux.pdl 
> > Signed-off-by: Nicholas Bellinger 
> > ---
> >  drivers/scsi/mpt3sas/mpt3sas_base.h  |  25 +-
> >  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 479 
> > +--
> >  drivers/scsi/mpt3sas/mpt3sas_transport.c |  18 +-
> >  3 files changed, 364 insertions(+), 158 deletions(-)




> > diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c 
> > b/drivers/scsi/mpt3sas/mpt3sas_transport.c
> > index 70fd019..6074b11 100644
> > --- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
> > +++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
> > @@ -734,7 +734,7 @@ mpt3sas_transport_port_add(struct MPT3SAS_ADAPTER *ioc, 
> > u16 handle,
> > rphy->identify = mpt3sas_port->remote_identify;
> >
> > if (mpt3sas_port->remote_identify.device_type == SAS_END_DEVICE) {
> 
> [Sreekanth] Here also sas_device_lock spin lock needs to be acquired
> before calling
>   __mpt3sas_get_sdev_by_addr() function.
> 

Thanks for your review.

Fixed with the following incremental patch atop for-next-merge code.

Please review.

Thank you,

>From 4004584675d24d8e886b03c013074cdfb76a0864 Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger 
Date: Tue, 8 Sep 2015 23:05:49 -0700
Subject: [PATCH] mpt3sas: Add missing ioc->sas_device_lock in
 mpt3sas_transport_port_add

This patch adds the missing struct MPT3SAS_ADAPTER->sas_device_lock
usage in __mpt3sas_get_sdev_by_addr() lookup to avoid a NULL pointer
dereference when >sas_device_list or >sas_device_init_list
changes from below without a proper sas_device_get(sas_device)
reference held.

Reported-by: Sreekanth Reddy 
Cc: Calvin Owens 
Cc: Christoph Hellwig 
Cc: Martin K. Petersen 
Signed-off-by: Nicholas Bellinger 
---
 drivers/scsi/mpt3sas/mpt3sas_transport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c 
b/drivers/scsi/mpt3sas/mpt3sas_transport.c
index 6074b11..cc55907 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
@@ -734,8 +734,10 @@ mpt3sas_transport_port_add(struct MPT3SAS_ADAPTER *ioc, 
u16 handle,
rphy->identify = mpt3sas_port->remote_identify;
 
if (mpt3sas_port->remote_identify.device_type == SAS_END_DEVICE) {
+   spin_lock_irqsave(>sas_device_lock, flags);
sas_device = __mpt3sas_get_sdev_by_addr(ioc,
mpt3sas_port->remote_identify.sas_address);
+   spin_unlock_irqrestore(>sas_device_lock, flags);
if (!sas_device) {
dfailprintk(ioc, printk(MPT3SAS_FMT
"failure at %s:%d/%s()!\n",
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix unsafe list usage

[Chaitra Basappa]

From: Sreekanth Reddy [mailto:sreekanth.re...@avagotech.com]
Sent: Tuesday, September 08, 2015 5:26 PM
To: Nicholas A. Bellinger
Cc: linux-scsi; linux-kernel; James Bottomley; Calvin Owens; Christoph
Hellwig; MPT-FusionLinux.pdl; kernel-team; Nicholas Bellinger; Chaitra
Basappa
Subject: Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix
unsafe list usage

On Sun, Aug 30, 2015 at 1:24 PM, Nicholas A. Bellinger <n...@daterainc.com>
wrote:
> From: Nicholas Bellinger <n...@linux-iscsi.org>
>
> These objects can be referenced concurrently throughout the driver, we
> need a way to make sure threads can't delete them out from under each
> other. This patch adds the refcount, and refactors the code to use it.
>
> Additionally, we cannot iterate over the sas_device_list without
> holding the lock, or we risk corrupting random memory if items are
> added or deleted as we iterate. This patch refactors
> _scsih_probe_sas() to use the sas_device_list in a safe way.
>
> This patch is a port of Calvin's PATCH-v4 for mpt2sas code, atop
> mpt3sas changes in scsi.git/for-next.
>
> Cc: Calvin Owens <calvinow...@fb.com>
> Cc: Christoph Hellwig <h...@infradead.org>
> Cc: Sreekanth Reddy <sreekanth.re...@avagotech.com>
> Cc: MPT-FusionLinux.pdl <mpt-fusionlinux@avagotech.com>
> Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.h  |  25 +-
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 479
> +--
>  drivers/scsi/mpt3sas/mpt3sas_transport.c |  18 +-
>  3 files changed, 364 insertions(+), 158 deletions(-)
>
> @@ -2763,7 +2874,7 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER *ioc,
> u16 handle)
> struct scsi_device *sdev;
> struct _sas_device *sas_device;
>

[Sreekanth] Here sas_device_lock spin lock needs to be acquired before
calling
  __mpt3sas_get_sdev_by_addr() function.

[Chaitra]Here instead of calling " __mpt3sas_get_sdev_by_handle()" function
calling
"mpt3sas_get_sdev_by_handle()" function will fixes "invalid page access"
type of kernel panic

> -   sas_device = _scsih_sas_device_find_by_handle(ioc, handle);
> +   sas_device = __mpt3sas_get_sdev_by_handle(ioc, handle);
> if (!sas_device)
> return;
>
> @@ -2779,6 +2890,8 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER *ioc,
> u16 handle)
> continue;
> _scsih_internal_device_block(sdev, sas_device_priv_data);
> }
> +
> +   sas_device_put(sas_device);
>  }
>


Regards,
Chaitra
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix unsafe list usage

[Nicholas A. Bellinger]

On Wed, 2015-09-09 at 19:59 +0530, Chaitra Basappa wrote:
> From: Sreekanth Reddy [mailto:sreekanth.re...@avagotech.com]
> Sent: Tuesday, September 08, 2015 5:26 PM
> To: Nicholas A. Bellinger
> Cc: linux-scsi; linux-kernel; James Bottomley; Calvin Owens; Christoph
> Hellwig; MPT-FusionLinux.pdl; kernel-team; Nicholas Bellinger; Chaitra
> Basappa
> Subject: Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix
> unsafe list usage
> 
> On Sun, Aug 30, 2015 at 1:24 PM, Nicholas A. Bellinger <n...@daterainc.com>
> wrote:
> > From: Nicholas Bellinger <n...@linux-iscsi.org>
> >
> > These objects can be referenced concurrently throughout the driver, we
> > need a way to make sure threads can't delete them out from under each
> > other. This patch adds the refcount, and refactors the code to use it.
> >
> > Additionally, we cannot iterate over the sas_device_list without
> > holding the lock, or we risk corrupting random memory if items are
> > added or deleted as we iterate. This patch refactors
> > _scsih_probe_sas() to use the sas_device_list in a safe way.
> >
> > This patch is a port of Calvin's PATCH-v4 for mpt2sas code, atop
> > mpt3sas changes in scsi.git/for-next.
> >
> > Cc: Calvin Owens <calvinow...@fb.com>
> > Cc: Christoph Hellwig <h...@infradead.org>
> > Cc: Sreekanth Reddy <sreekanth.re...@avagotech.com>
> > Cc: MPT-FusionLinux.pdl <mpt-fusionlinux@avagotech.com>
> > Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
> > ---
> >  drivers/scsi/mpt3sas/mpt3sas_base.h  |  25 +-
> >  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 479
> > +--
> >  drivers/scsi/mpt3sas/mpt3sas_transport.c |  18 +-
> >  3 files changed, 364 insertions(+), 158 deletions(-)
> >
> > @@ -2763,7 +2874,7 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER *ioc,
> > u16 handle)
> > struct scsi_device *sdev;
> > struct _sas_device *sas_device;
> >
> 
> [Sreekanth] Here sas_device_lock spin lock needs to be acquired before
> calling
>   __mpt3sas_get_sdev_by_addr() function.
> 
> [Chaitra]Here instead of calling " __mpt3sas_get_sdev_by_handle()" function
> calling
>   "mpt3sas_get_sdev_by_handle()" function will fixes "invalid page access"
> type of kernel panic
> 
> > -   sas_device = _scsih_sas_device_find_by_handle(ioc, handle);
> > +   sas_device = __mpt3sas_get_sdev_by_handle(ioc, handle);
> > if (!sas_device)
> > return;
> >

Whoops, missed this comment in _scsih_block_io_device() from Sreekanth's
earlier reply.

Here's the updated incremental patch atop target-pending/for-next-merge
to use the protected callers for both cases.

Please review + ACK ASAP.

Thank you,

--nab

>From 8edb1554f7c2eb73cf70c9856aec01e786b9bcf9 Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger <n...@linux-iscsi.org>
Date: Tue, 8 Sep 2015 23:05:49 -0700
Subject: [PATCH] mpt3sas: Fix unprotected list lookup in v4.3-rc0 changes

This patch adds the missing mpt3sas_get_sdev_by_addr() protected
lookup usage in mpt3sas_transport_port_add() to avoid a NULL pointer
dereference when >sas_device_list or >sas_device_init_list
changes from below without a proper sas_device_get(sas_device)
reference held.

Also, use the protected mpt3sas_get_sdev_by_handle() lookup within
_scsih_block_io_device() as well.

Reported-by: Sreekanth Reddy <sreekanth.re...@avagotech.com>
Reported-by: Chaitra Basappa <chaitra.basa...@avagotech.com>
Cc: Calvin Owens <calvinow...@fb.com>
Cc: Christoph Hellwig <h...@infradead.org>
Cc: Martin K. Petersen <martin.peter...@oracle.com>
Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 2 +-
 drivers/scsi/mpt3sas/mpt3sas_transport.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 0431cd0..9e68432 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -2933,7 +2933,7 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER *ioc, u16 
handle)
struct scsi_device *sdev;
struct _sas_device *sas_device;
 
-   sas_device = __mpt3sas_get_sdev_by_handle(ioc, handle);
+   sas_device = mpt3sas_get_sdev_by_handle(ioc, handle);
if (!sas_device)
return;
 
diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c 
b/drivers/scsi/mpt3sas/mpt3sas_transport.c
index 6074b11..ca36d7e 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
@@ -734,7 +734,7 

Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix unsafe list usage

[Sreekanth Reddy]

On Sun, Aug 30, 2015 at 1:24 PM, Nicholas A. Bellinger
 wrote:
> From: Nicholas Bellinger 
>
> These objects can be referenced concurrently throughout the driver, we
> need a way to make sure threads can't delete them out from under
> each other. This patch adds the refcount, and refactors the code to use
> it.
>
> Additionally, we cannot iterate over the sas_device_list without
> holding the lock, or we risk corrupting random memory if items are
> added or deleted as we iterate. This patch refactors _scsih_probe_sas()
> to use the sas_device_list in a safe way.
>
> This patch is a port of Calvin's PATCH-v4 for mpt2sas code, atop
> mpt3sas changes in scsi.git/for-next.
>
> Cc: Calvin Owens 
> Cc: Christoph Hellwig 
> Cc: Sreekanth Reddy 
> Cc: MPT-FusionLinux.pdl 
> Signed-off-by: Nicholas Bellinger 
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.h  |  25 +-
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 479 
> +--
>  drivers/scsi/mpt3sas/mpt3sas_transport.c |  18 +-
>  3 files changed, 364 insertions(+), 158 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h 
> b/drivers/scsi/mpt3sas/mpt3sas_base.h
> index f0e462b..9a73c8b 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.h
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
> @@ -248,6 +248,7 @@ struct Mpi2ManufacturingPage11_t {
>   * @flags: MPT_TARGET_FLAGS_XXX flags
>   * @deleted: target flaged for deletion
>   * @tm_busy: target is busy with TM request.
> + * @sdev: The sas_device associated with this target
>   */
>  struct MPT3SAS_TARGET {
> struct scsi_target *starget;
> @@ -257,6 +258,7 @@ struct MPT3SAS_TARGET {
> u32 flags;
> u8  deleted;
> u8  tm_busy;
> +   struct  _sas_device *sdev;
>  };
>
>
> @@ -335,6 +337,9 @@ struct _internal_cmd {
>   * @pfa_led_on: flag for PFA LED status
>   * @pend_sas_rphy_add: flag to check if device is in sas_rphy_add()
>   * addition routine.
> + * @enclosure_level: used for enclosure services
> + * @connector_name: ASCII value from pg0.ConnectorName
> + * @refcount: reference count for deletion
>   */
>  struct _sas_device {
> struct list_head list;
> @@ -358,8 +363,24 @@ struct _sas_device {
> u8  pend_sas_rphy_add;
> u8  enclosure_level;
> u8  connector_name[4];
> +   struct kref refcount;
>  };
>
> +static inline void sas_device_get(struct _sas_device *s)
> +{
> +   kref_get(>refcount);
> +}
> +
> +static inline void sas_device_free(struct kref *r)
> +{
> +   kfree(container_of(r, struct _sas_device, refcount));
> +}
> +
> +static inline void sas_device_put(struct _sas_device *s)
> +{
> +   kref_put(>refcount, sas_device_free);
> +}
> +
>  /**
>   * struct _raid_device - raid volume link list
>   * @list: sas device list
> @@ -1090,7 +,9 @@ struct _sas_node *mpt3sas_scsih_expander_find_by_handle(
> struct MPT3SAS_ADAPTER *ioc, u16 handle);
>  struct _sas_node *mpt3sas_scsih_expander_find_by_sas_address(
> struct MPT3SAS_ADAPTER *ioc, u64 sas_address);
> -struct _sas_device *mpt3sas_scsih_sas_device_find_by_sas_address(
> +struct _sas_device *mpt3sas_get_sdev_by_addr(
> +struct MPT3SAS_ADAPTER *ioc, u64 sas_address);
> +struct _sas_device *__mpt3sas_get_sdev_by_addr(
> struct MPT3SAS_ADAPTER *ioc, u64 sas_address);
>
>  void mpt3sas_port_enable_complete(struct MPT3SAS_ADAPTER *ioc);
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
> b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 8ccef38..897153b 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -518,8 +518,61 @@ _scsih_determine_boot_device(struct MPT3SAS_ADAPTER *ioc,
> }
>  }
>
> +static struct _sas_device *
> +__mpt3sas_get_sdev_from_target(struct MPT3SAS_ADAPTER *ioc,
> +   struct MPT3SAS_TARGET *tgt_priv)
> +{
> +   struct _sas_device *ret;
> +
> +   assert_spin_locked(>sas_device_lock);
> +
> +   ret = tgt_priv->sdev;
> +   if (ret)
> +   sas_device_get(ret);
> +
> +   return ret;
> +}
> +
> +static struct _sas_device *
> +mpt3sas_get_sdev_from_target(struct MPT3SAS_ADAPTER *ioc,
> +   struct MPT3SAS_TARGET *tgt_priv)
> +{
> +   struct _sas_device *ret;
> +   unsigned long flags;
> +
> +   spin_lock_irqsave(>sas_device_lock, flags);
> +   ret = __mpt3sas_get_sdev_from_target(ioc, tgt_priv);
> +   spin_unlock_irqrestore(>sas_device_lock, flags);
> +
> +   return ret;
> +}
> +
> +struct _sas_device *
> +__mpt3sas_get_sdev_by_addr(struct MPT3SAS_ADAPTER *ioc,
> +   u64 sas_address)
> +{
> +   struct _sas_device *sas_device;
> +
> +   assert_spin_locked(>sas_device_lock);
> +
> +   list_for_each_entry(sas_device, >sas_device_list, list)
> +   if (sas_device->sas_address == sas_address)
> +   goto found_device;
> +
> +   list_for_each_entry(sas_device, >sas_device_init_list, list)
> +   if 

Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix unsafe list usage

[Sreekanth Reddy]

On Sun, Aug 30, 2015 at 1:24 PM, Nicholas A. Bellinger
 wrote:
> From: Nicholas Bellinger 
>
> These objects can be referenced concurrently throughout the driver, we
> need a way to make sure threads can't delete them out from under
> each other. This patch adds the refcount, and refactors the code to use
> it.
>
> Additionally, we cannot iterate over the sas_device_list without
> holding the lock, or we risk corrupting random memory if items are
> added or deleted as we iterate. This patch refactors _scsih_probe_sas()
> to use the sas_device_list in a safe way.
>
> This patch is a port of Calvin's PATCH-v4 for mpt2sas code, atop
> mpt3sas changes in scsi.git/for-next.
>
> Cc: Calvin Owens 
> Cc: Christoph Hellwig 
> Cc: Sreekanth Reddy 
> Cc: MPT-FusionLinux.pdl 
> Signed-off-by: Nicholas Bellinger 
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.h  |  25 +-
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 479 
> +--
>  drivers/scsi/mpt3sas/mpt3sas_transport.c |  18 +-
>  3 files changed, 364 insertions(+), 158 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h 
> b/drivers/scsi/mpt3sas/mpt3sas_base.h
> index f0e462b..9a73c8b 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.h
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
> @@ -248,6 +248,7 @@ struct Mpi2ManufacturingPage11_t {
>   * @flags: MPT_TARGET_FLAGS_XXX flags
>   * @deleted: target flaged for deletion
>   * @tm_busy: target is busy with TM request.
> + * @sdev: The sas_device associated with this target
>   */
>  struct MPT3SAS_TARGET {
> struct scsi_target *starget;
> @@ -257,6 +258,7 @@ struct MPT3SAS_TARGET {
> u32 flags;
> u8  deleted;
> u8  tm_busy;
> +   struct  _sas_device *sdev;
>  };
>
>
> @@ -335,6 +337,9 @@ struct _internal_cmd {
>   * @pfa_led_on: flag for PFA LED status
>   * @pend_sas_rphy_add: flag to check if device is in sas_rphy_add()
>   * addition routine.
> + * @enclosure_level: used for enclosure services
> + * @connector_name: ASCII value from pg0.ConnectorName
> + * @refcount: reference count for deletion
>   */
>  struct _sas_device {
> struct list_head list;
> @@ -358,8 +363,24 @@ struct _sas_device {
> u8  pend_sas_rphy_add;
> u8  enclosure_level;
> u8  connector_name[4];
> +   struct kref refcount;
>  };
>
> +static inline void sas_device_get(struct _sas_device *s)
> +{
> +   kref_get(>refcount);
> +}
> +
> +static inline void sas_device_free(struct kref *r)
> +{
> +   kfree(container_of(r, struct _sas_device, refcount));
> +}
> +
> +static inline void sas_device_put(struct _sas_device *s)
> +{
> +   kref_put(>refcount, sas_device_free);
> +}
> +
>  /**
>   * struct _raid_device - raid volume link list
>   * @list: sas device list
> @@ -1090,7 +,9 @@ struct _sas_node *mpt3sas_scsih_expander_find_by_handle(
> struct MPT3SAS_ADAPTER *ioc, u16 handle);
>  struct _sas_node *mpt3sas_scsih_expander_find_by_sas_address(
> struct MPT3SAS_ADAPTER *ioc, u64 sas_address);
> -struct _sas_device *mpt3sas_scsih_sas_device_find_by_sas_address(
> +struct _sas_device *mpt3sas_get_sdev_by_addr(
> +struct MPT3SAS_ADAPTER *ioc, u64 sas_address);
> +struct _sas_device *__mpt3sas_get_sdev_by_addr(
> struct MPT3SAS_ADAPTER *ioc, u64 sas_address);
>
>  void mpt3sas_port_enable_complete(struct MPT3SAS_ADAPTER *ioc);
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
> b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 8ccef38..897153b 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -518,8 +518,61 @@ _scsih_determine_boot_device(struct MPT3SAS_ADAPTER *ioc,
> }
>  }
>
> +static struct _sas_device *
> +__mpt3sas_get_sdev_from_target(struct MPT3SAS_ADAPTER *ioc,
> +   struct MPT3SAS_TARGET *tgt_priv)
> +{
> +   struct _sas_device *ret;
> +
> +   assert_spin_locked(>sas_device_lock);
> +
> +   ret = tgt_priv->sdev;
> +   if (ret)
> +   sas_device_get(ret);
> +
> +   return ret;
> +}
> +
> +static struct _sas_device *
> +mpt3sas_get_sdev_from_target(struct MPT3SAS_ADAPTER *ioc,
> +   struct MPT3SAS_TARGET *tgt_priv)
> +{
> +   struct _sas_device *ret;
> +   unsigned long flags;
> +
> +   spin_lock_irqsave(>sas_device_lock, flags);
> +   ret = __mpt3sas_get_sdev_from_target(ioc, tgt_priv);
> +   spin_unlock_irqrestore(>sas_device_lock, flags);
> +
> +   return ret;
> +}
> +
> +struct _sas_device *
> +__mpt3sas_get_sdev_by_addr(struct MPT3SAS_ADAPTER *ioc,
> +   u64 sas_address)
> +{
> +   struct _sas_device *sas_device;
> +
> +   assert_spin_locked(>sas_device_lock);
> +
> +   list_for_each_entry(sas_device, >sas_device_list, list)
> +   if