[PATCH 1/1] [PATCH REGRESSION] alua: fix bus detach oops

[PATCH 1/1] [PATCH REGRESSION] alua: fix bus detach oops

[michaelc]

From: Mike Christie <michaelc@cs.wisc.edu>

This fixes a regression caused by commit
1d5203284d8acbdfdf9b478d434450b34f338f28

The bug is that the alua detach() callout will try to access the
sddev->scsi_dh_data, but we have already set it to NULL. This patch
moves the clearing of that field to after detach() is called.

It looks like the regression was added during 3.19 development,
so it has not been in a released kernel, and so I did not cc
stable.

Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>

---
 drivers/scsi/device_handler/scsi_dh.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index 1dba62c..1efebc9 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -136,11 +136,12 @@ static void __detach_handler (struct kref *kref)
 	struct scsi_device_handler *scsi_dh = scsi_dh_data->scsi_dh;
 	struct scsi_device *sdev = scsi_dh_data->sdev;
 
+	scsi_dh->detach(sdev);
+
 	spin_lock_irq(sdev->request_queue->queue_lock);
 	sdev->scsi_dh_data = NULL;
 	spin_unlock_irq(sdev->request_queue->queue_lock);
 
-	scsi_dh->detach(sdev);
 	sdev_printk(KERN_NOTICE, sdev, "%s: Detached\n", scsi_dh->name);
 	module_put(scsi_dh->module);
 }
-- 
1.8.3.1

Re: [PATCH 1/1] [PATCH REGRESSION] alua: fix bus detach oops

[Hannes Reinecke]

On 01/28/2015 10:46 AM, michaelc@cs.wisc.edu wrote:
> From: Mike Christie <michaelc@cs.wisc.edu>
> 
> This fixes a regression caused by commit
> 1d5203284d8acbdfdf9b478d434450b34f338f28
> 
> The bug is that the alua detach() callout will try to access the
> sddev->scsi_dh_data, but we have already set it to NULL. This patch
> moves the clearing of that field to after detach() is called.
> 
> It looks like the regression was added during 3.19 development,
> so it has not been in a released kernel, and so I did not cc
> stable.
> 
> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
> 
> ---
>  drivers/scsi/device_handler/scsi_dh.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
> index 1dba62c..1efebc9 100644
> --- a/drivers/scsi/device_handler/scsi_dh.c
> +++ b/drivers/scsi/device_handler/scsi_dh.c
> @@ -136,11 +136,12 @@ static void __detach_handler (struct kref *kref)
>  	struct scsi_device_handler *scsi_dh = scsi_dh_data->scsi_dh;
>  	struct scsi_device *sdev = scsi_dh_data->sdev;
>  
> +	scsi_dh->detach(sdev);
> +
>  	spin_lock_irq(sdev->request_queue->queue_lock);
>  	sdev->scsi_dh_data = NULL;
>  	spin_unlock_irq(sdev->request_queue->queue_lock);
>  
> -	scsi_dh->detach(sdev);
>  	sdev_printk(KERN_NOTICE, sdev, "%s: Detached\n", scsi_dh->name);
>  	module_put(scsi_dh->module);
>  }
> 
Errm.

We save the contents first:

>  	struct scsi_device_handler *scsi_dh = scsi_dh_data->scsi_dh;

Then set the pointer to NULL:

>  	sdev->scsi_dh_data = NULL;

and then call 'detach':

> 	scsi_dh->detach(sdev);

So scsi_dh is _not_ NULL, hence it shouldn't oops.

NACK.

Cheers,

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

Re: [PATCH 1/1] [PATCH REGRESSION] alua: fix bus detach oops

[Christoph Hellwig]

On Thu, Jan 29, 2015 at 09:45:24AM +0100, Hannes Reinecke wrote:
> Errm.
> 
> We save the contents first:
> 
> >  	struct scsi_device_handler *scsi_dh = scsi_dh_data->scsi_dh;
> 
> Then set the pointer to NULL:
> 
> >  	sdev->scsi_dh_data = NULL;
> 
> and then call 'detach':
> 
> > 	scsi_dh->detach(sdev);
> 
> So scsi_dh is _not_ NULL, hence it shouldn't oops.

The actual ->detach method can, and usually does access it.

Re: [PATCH 1/1] [PATCH REGRESSION] alua: fix bus detach oops

[Mike Christie]

On 01/29/2015 02:45 AM, Hannes Reinecke wrote:
> On 01/28/2015 10:46 AM, michaelc@cs.wisc.edu wrote:
>> From: Mike Christie <michaelc@cs.wisc.edu>
>>
>> This fixes a regression caused by commit
>> 1d5203284d8acbdfdf9b478d434450b34f338f28
>>
>> The bug is that the alua detach() callout will try to access the
>> sddev->scsi_dh_data, but we have already set it to NULL. This patch
>> moves the clearing of that field to after detach() is called.
>>
>> It looks like the regression was added during 3.19 development,
>> so it has not been in a released kernel, and so I did not cc
>> stable.
>>
>> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
>>
>> ---
>>  drivers/scsi/device_handler/scsi_dh.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
>> index 1dba62c..1efebc9 100644
>> --- a/drivers/scsi/device_handler/scsi_dh.c
>> +++ b/drivers/scsi/device_handler/scsi_dh.c
>> @@ -136,11 +136,12 @@ static void __detach_handler (struct kref *kref)
>>  	struct scsi_device_handler *scsi_dh = scsi_dh_data->scsi_dh;
>>  	struct scsi_device *sdev = scsi_dh_data->sdev;
>>  
>> +	scsi_dh->detach(sdev);
>> +
>>  	spin_lock_irq(sdev->request_queue->queue_lock);
>>  	sdev->scsi_dh_data = NULL;
>>  	spin_unlock_irq(sdev->request_queue->queue_lock);
>>  
>> -	scsi_dh->detach(sdev);
>>  	sdev_printk(KERN_NOTICE, sdev, "%s: Detached\n", scsi_dh->name);
>>  	module_put(scsi_dh->module);
>>  }
>>
> Errm.
> 
> We save the contents first:
> 
>>  	struct scsi_device_handler *scsi_dh = scsi_dh_data->scsi_dh;
> 
> Then set the pointer to NULL:
> 
>>  	sdev->scsi_dh_data = NULL;
> 
> and then call 'detach':
> 
>> 	scsi_dh->detach(sdev);
> 
> So scsi_dh is _not_ NULL, hence it shouldn't oops.
> 

The problem is the actual detach() functions are the ones that are
accessing the NULL'd scsi_dh_data->scsi_dh pointer.

So above we have set sdev->scsi_dh_data to NULL and then are calling
detach(). In scsi_dh_alua.c, get_alua_data() we will then access the
NULL'd pointer.

static void alua_bus_detach(struct scsi_device *sdev)
{
        struct alua_dh_data *h = get_alua_data(sdev);

	if (h->buff && h->inq != h->buff)
                kfree(h->buff);
        kfree(h);