Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1221505 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix  unsafe list usage Chaitra Basappa <chaitra.basappa@avagotech.com> - 2015-09-09 16:40 +0200
    Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix  unsafe list usage "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2015-09-10 00:10 +0200
      Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix  unsafe list usage "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2015-09-11 09:00 +0200
        Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix  unsafe list usage James Bottomley <James.Bottomley@HansenPartnership.com> - 2015-09-11 20:00 +0200
          Re: [PATCH-v2 1/2] mpt3sas: Refcount sas_device objects and fix  unsafe list usage "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2015-09-11 22:10 +0200

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

From: Sreekanth Reddy [mailto:sreekanth.reddy@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 <nab@daterainc.com>
wrote:
> From: Nicholas Bellinger <nab@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 <calvinowens@fb.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Sreekanth Reddy <sreekanth.reddy@avagotech.com>
> Cc: MPT-FusionLinux.pdl <MPT-FusionLinux.pdl@avagotech.com>
> Signed-off-by: Nicholas Bellinger <nab@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1221760

On Wed, 2015-09-09 at 19:59 +0530, Chaitra Basappa wrote:
> From: Sreekanth Reddy [mailto:sreekanth.reddy@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 <nab@daterainc.com>
> wrote:
> > From: Nicholas Bellinger <nab@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 <calvinowens@fb.com>
> > Cc: Christoph Hellwig <hch@infradead.org>
> > Cc: Sreekanth Reddy <sreekanth.reddy@avagotech.com>
> > Cc: MPT-FusionLinux.pdl <MPT-FusionLinux.pdl@avagotech.com>
> > Signed-off-by: Nicholas Bellinger <nab@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 <nab@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 &ioc->sas_device_list or &ioc->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.reddy@avagotech.com>
Reported-by: Chaitra Basappa <chaitra.basappa@avagotech.com>
Cc: Calvin Owens <calvinowens@fb.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Nicholas Bellinger <nab@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 @@ 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222565

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.reddy@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 <nab@daterainc.com>
> > wrote:
> > > From: Nicholas Bellinger <nab@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 <calvinowens@fb.com>
> > > Cc: Christoph Hellwig <hch@infradead.org>
> > > Cc: Sreekanth Reddy <sreekanth.reddy@avagotech.com>
> > > Cc: MPT-FusionLinux.pdl <MPT-FusionLinux.pdl@avagotech.com>
> > > Signed-off-by: Nicholas Bellinger <nab@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223015

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.reddy@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 <nab@daterainc.com>
> > > wrote:
> > > > From: Nicholas Bellinger <nab@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 <calvinowens@fb.com>
> > > > Cc: Christoph Hellwig <hch@infradead.org>
> > > > Cc: Sreekanth Reddy <sreekanth.reddy@avagotech.com>
> > > > Cc: MPT-FusionLinux.pdl <MPT-FusionLinux.pdl@avagotech.com>
> > > > Signed-off-by: Nicholas Bellinger <nab@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223075

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.reddy@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 <nab@daterainc.com>
> > > > wrote:
> > > > > From: Nicholas Bellinger <nab@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 <calvinowens@fb.com>
> > > > > Cc: Christoph Hellwig <hch@infradead.org>
> > > > > Cc: Sreekanth Reddy <sreekanth.reddy@avagotech.com>
> > > > > Cc: MPT-FusionLinux.pdl <MPT-FusionLinux.pdl@avagotech.com>
> > > > > Signed-off-by: Nicholas Bellinger <nab@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.
> 

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 code,
and it's in linux-next.

Currently there are no outstanding comments to be addressed for -v2.  

> 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.
> 

As mentioned, there is not a scenario where keeping this broken list
handling code around for -rc1 makes any sense, when the exact same
change for mpt2sas minus two simple cases is (I assume) going to Linus
shortly.

Do you have any specific code comments on the series, or not..?

> 
> > 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.
> 

Believe me, fixing up LLD code and arguing with you over why utterly
broken list handling code should be left as-is for another few weeks in
mainline is the last thing I'd like to be spending time on right now.

Unfortunately, I've got 100+ HBAs that use this driver, and the current
state of broken list handling is completely unacceptable.

--nab

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web