LinuxLists.cc - [PATCH] fix NULL-pointer dereference on scsi_run

[permalink] [raw]

Subject: Re: [PATCH] fix NULL-pointer dereference on scsi_run_queue

On Thu, Aug 2, 2012 at 5:57 PM, James Bottomley
<[email protected]> wrote:
> On Thu, 2012-08-02 at 17:41 +0900, Chanho Min wrote:
>> This patch is to fix a oops from a torn down device. When
>> scsi_run_queue process starved queues, scsi_request_fn can race with
>> scsi_remove_device. In this case, rarely, scsi_request_fn release the
>> last reference and set sdev->request_queue to NULL. It result in
>> NULL-pointer dereference when spin_unlock is tried with (NULL)->
>> queue_lock. We need to add an extra reference to the device on both
>> sides of the __blk_run_queue to hold reference until scsi_request_fn
>> is finished.
>
> You need a recent kernel with this patch:
>
> commit 940f5d47e2f2e1fa00443921a0abf4822335b54d
> Author: Bart Van Assche <[email protected]>
> Date: Fri Jun 29 15:34:26 2012 +0000
>
> [SCSI] Avoid dangling pointer in scsi_requeue_command()
>
> James
It is different from my case. This is occured inside scsi_run_queue
and on processing starved_list.
Another sdev is obtained from starved_list.

2012-08-02 09:34:59

by James Bottomley

[permalink] [raw]

Subject: Re: [PATCH] fix NULL-pointer dereference on scsi_run_queue

On Thu, 2012-08-02 at 18:28 +0900, Chanho Min wrote:
> On Thu, Aug 2, 2012 at 5:57 PM, James Bottomley
> <[email protected]> wrote:
> > On Thu, 2012-08-02 at 17:41 +0900, Chanho Min wrote:
> >> This patch is to fix a oops from a torn down device. When
> >> scsi_run_queue process starved queues, scsi_request_fn can race with
> >> scsi_remove_device. In this case, rarely, scsi_request_fn release the
> >> last reference and set sdev->request_queue to NULL. It result in
> >> NULL-pointer dereference when spin_unlock is tried with (NULL)->
> >> queue_lock. We need to add an extra reference to the device on both
> >> sides of the __blk_run_queue to hold reference until scsi_request_fn
> >> is finished.
> >
> > You need a recent kernel with this patch:
> >
> > commit 940f5d47e2f2e1fa00443921a0abf4822335b54d
> > Author: Bart Van Assche <[email protected]>
> > Date: Fri Jun 29 15:34:26 2012 +0000
> >
> > [SCSI] Avoid dangling pointer in scsi_requeue_command()
> >
> > James
> It is different from my case. This is occured inside scsi_run_queue
> and on processing starved_list.
> Another sdev is obtained from starved_list.

Does it occur with that patch applied?

If it does, the likely fix would be to take a copy of the queue ... but
I'd like to understand why first. An active command has an automatic
reference to the sdev_gendev, so it shouldn't be the normal case. This
was broken by unprep because it releases the command from the queue and
drops the reference. We may have another case like unjprep, but in that
case, we need to find it ... trying to add extra get/put_device() calls
will paper over the problem.

James

2012-08-03 02:28:49

[permalink] [raw]

Subject: Re: [PATCH] fix NULL-pointer dereference on scsi_run_queue

> Does it occur with that patch applied?
I'm trying to reproduce it with that patch. but, It is unlikely to be fixed.
because scsi_run_queue is invoked from scsi_requeue_run_queue,
not scsi_requeue_command.

> If it does, the likely fix would be to take a copy of the queue ... but
> I'd like to understand why first. An active command has an automatic
> reference to the sdev_gendev, so it shouldn't be the normal case. This
> was broken by unprep because it releases the command from the queue and
> drops the reference. We may have another case like unjprep, but in that
__scsi_remove_device drops the last reference under race condition.

> case, we need to find it ... trying to add extra get/put_device() calls
> will paper over the problem.
yes, extra reference is not good to fix.
But, As long as scsi_device_dev_release_usercontext set request_queue
to NULL, Isn't it necessary to ensure that __blk_run_queue don't release device?

Thanks a lot!
Chanho Min

2012-08-03 03:01:57

[permalink] [raw]

Subject: Re: [PATCH] fix NULL-pointer dereference on scsi_run_queue

On 08/02/2012 04:34 AM, James Bottomley wrote:
> On Thu, 2012-08-02 at 18:28 +0900, Chanho Min wrote:
>> On Thu, Aug 2, 2012 at 5:57 PM, James Bottomley
>> <[email protected]> wrote:
>>> On Thu, 2012-08-02 at 17:41 +0900, Chanho Min wrote:
>>>> This patch is to fix a oops from a torn down device. When
>>>> scsi_run_queue process starved queues, scsi_request_fn can race with
>>>> scsi_remove_device. In this case, rarely, scsi_request_fn release the
>>>> last reference and set sdev->request_queue to NULL. It result in
>>>> NULL-pointer dereference when spin_unlock is tried with (NULL)->
>>>> queue_lock. We need to add an extra reference to the device on both
>>>> sides of the __blk_run_queue to hold reference until scsi_request_fn
>>>> is finished.
>>>
>>> You need a recent kernel with this patch:
>>>
>>> commit 940f5d47e2f2e1fa00443921a0abf4822335b54d
>>> Author: Bart Van Assche <[email protected]>
>>> Date: Fri Jun 29 15:34:26 2012 +0000
>>>
>>> [SCSI] Avoid dangling pointer in scsi_requeue_command()
>>>
>>> James
>> It is different from my case. This is occured inside scsi_run_queue
>> and on processing starved_list.
>> Another sdev is obtained from starved_list.
>
> Does it occur with that patch applied?
>
> If it does, the likely fix would be to take a copy of the queue ... but
> I'd like to understand why first. An active command has an automatic
> reference to the sdev_gendev, so it shouldn't be the normal case. This
> was broken by unprep because it releases the command from the queue and
> drops the reference. We may have another case like unjprep, but in that
> case, we need to find it ... trying to add extra get/put_device() calls
> will paper over the problem.
>

I think the problem is that __scsi_remove_device will now wait for
commands to get dequeued and run, before proceeding but we do not take a
device off the starved list until scsi_device_dev_release_usercontext is
run, or maybe thinking about it another way scsi_kill_request does not
remove sdevs from the starved list if the device is being removed.

So lets say we hit the not_ready path in scsi_request_fn and put the
sdev on the starved list. Then we remove the device. We could end up
putting the device in SDEV_DEL, and then calling scsi_request_fn via
blk_cleanup_queue's drain queue call. scsi_request_fn would hit the
scsi_device_online check and fail the IO, but we never took the sdev off
the starved list from what I can tell.

Now, there is no IO in the queue and so __scsi_remove_device continues.
It then calls scsi_device_dev_release_usercontext at the same time some
other thread is calling scsi_run_queue. We then race. scsi_run_queue
splices the starved list with the sdev we are trying to remove and
deletes the list entry from the list and drops the host lock. But then
scsi_device_dev_release_usercontext grabs the host lock and ends up
running the entire function and freeing the queue. Then scsi_run_queue
tries to access the sdev and queue so it can grab the queue lock that
was just freed and kablewy.

2012-08-04 09:01:36

[permalink] [raw]

Subject: Re: [PATCH] fix NULL-pointer dereference on scsi_run_queue

On 08/02/12 08:41, Chanho Min wrote:
> This patch is to fix a oops from a torn down device. When
> scsi_run_queue process starved queues, scsi_request_fn can race with
> scsi_remove_device. In this case, rarely, scsi_request_fn release the
> last reference and set sdev->request_queue to NULL. It result in
> NULL-pointer dereference when spin_unlock is tried with (NULL)->
> queue_lock. We need to add an extra reference to the device on both
> sides of the __blk_run_queue to hold reference until scsi_request_fn
> is finished.

Good catch. So far I haven't been able to trigger this issue in my
tests. So it would be appreciated if you could verify whether the patch
below helps (patch is based on 3.6-rc1):

---
drivers/scsi/scsi_sysfs.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 093d4f6..59e523c 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -348,7 +348,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
starget->reap_ref++;
list_del(&sdev->siblings);
list_del(&sdev->same_target_siblings);
- list_del(&sdev->starved_entry);
spin_unlock_irqrestore(sdev->host->host_lock, flags);

cancel_work_sync(&sdev->event_work);
@@ -956,6 +955,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
void __scsi_remove_device(struct scsi_device *sdev)
{
struct device *dev = &sdev->sdev_gendev;
+ struct Scsi_Host *shost = sdev->host;
+ unsigned long flags;

if (sdev->is_visible) {
if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
@@ -977,6 +978,11 @@ void __scsi_remove_device(struct scsi_device *sdev)
blk_cleanup_queue(sdev->request_queue);
cancel_work_sync(&sdev->requeue_work);

+ spin_lock_irqsave(shost->host_lock, flags);
+ if (!list_empty(&sdev->starved_entry))
+ list_del(&sdev->starved_entry);
+ spin_unlock_irqrestore(shost->host_lock, flags);
+
if (sdev->host->hostt->slave_destroy)
sdev->host->hostt->slave_destroy(sdev);
transport_destroy_device(dev);
--
1.7.7

2012-08-04 16:46:32

[permalink] [raw]

Subject: Re: [PATCH] fix NULL-pointer dereference on scsi_run_queue

On 08/04/2012 04:01 AM, Bart Van Assche wrote:
> On 08/02/12 08:41, Chanho Min wrote:
>> This patch is to fix a oops from a torn down device. When
>> scsi_run_queue process starved queues, scsi_request_fn can race with
>> scsi_remove_device. In this case, rarely, scsi_request_fn release the
>> last reference and set sdev->request_queue to NULL. It result in
>> NULL-pointer dereference when spin_unlock is tried with (NULL)->
>> queue_lock. We need to add an extra reference to the device on both
>> sides of the __blk_run_queue to hold reference until scsi_request_fn
>> is finished.
>
> Good catch. So far I haven't been able to trigger this issue in my
> tests. So it would be appreciated if you could verify whether the patch
> below helps (patch is based on 3.6-rc1):
>
> ---
> drivers/scsi/scsi_sysfs.c | 8 +++++++-
> 1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 093d4f6..59e523c 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -348,7 +348,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
> starget->reap_ref++;
> list_del(&sdev->siblings);
> list_del(&sdev->same_target_siblings);
> - list_del(&sdev->starved_entry);
> spin_unlock_irqrestore(sdev->host->host_lock, flags);
>
> cancel_work_sync(&sdev->event_work);
> @@ -956,6 +955,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
> void __scsi_remove_device(struct scsi_device *sdev)
> {
> struct device *dev = &sdev->sdev_gendev;
> + struct Scsi_Host *shost = sdev->host;
> + unsigned long flags;
>
> if (sdev->is_visible) {
> if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
> @@ -977,6 +978,11 @@ void __scsi_remove_device(struct scsi_device *sdev)
> blk_cleanup_queue(sdev->request_queue);
> cancel_work_sync(&sdev->requeue_work);
>
> + spin_lock_irqsave(shost->host_lock, flags);
> + if (!list_empty(&sdev->starved_entry))
> + list_del(&sdev->starved_entry);
> + spin_unlock_irqrestore(shost->host_lock, flags);
> +

I do not think it's that simple. If scsi_run_queue is running right now
and that function has deleted the starved entry and is now about to
access the sdev or queue, then this code above does not help and
__scsi_remove_device could just continue on and end up calling
scsi_device_dev_release_usercontext and freeing the device from under
scsi_run_queue.

I think we have to have scsi-ml do a get_device when a sdev is added to
the starved entry and then do a put_device when it is removed (must do
these under the host lock for the starved entry case too). I am not sure
if that is just a hack/papering-over of the problem and there are more
issues like this.

2012-08-04 20:18:51

[permalink] [raw]

Subject: Re: [PATCH] fix NULL-pointer dereference on scsi_run_queue

On 08/04/12 16:46, Mike Christie wrote:
> I think we have to have scsi-ml do a get_device when a sdev is added to
> the starved entry and then do a put_device when it is removed (must do
> these under the host lock for the starved entry case too). I am not sure
> if that is just a hack/papering-over of the problem and there are more
> issues like this.

That would result in a more complex patch than the patch at the start of
this thread, isn't it ? Also, IMHO it would help to document which
functions in the scsi-ml are called with an sdev reference and which
ones not. That would make the scsi-ml code easier to verify for issues
like the one reported at the start of this thread.

Bart.

2012-08-04 22:36:50

[permalink] [raw]

Subject: Re: [PATCH] fix NULL-pointer dereference on scsi_run_queue

On 08/04/2012 03:18 PM, Bart Van Assche wrote:
> On 08/04/12 16:46, Mike Christie wrote:
>> I think we have to have scsi-ml do a get_device when a sdev is added to
>> the starved entry and then do a put_device when it is removed (must do
>> these under the host lock for the starved entry case too). I am not sure
>> if that is just a hack/papering-over of the problem and there are more
>> issues like this.
>
> That would result in a more complex patch than the patch at the start of
> this thread, isn't it ? Also, IMHO it would help to document which

Yaah, but the original patch in this thread is still racey isn't it?

spin_unlock(shost->host_lock);

The sdev/queue could get freed by some other thread when this function
is right here, so the get_device call is now going to try to access
freed memory.

+ /* hold a reference on the device so it doesn't release device */
+ get_device(&sdev->sdev_gendev);

2012-08-06 17:56:57

[permalink] [raw]

Subject: Re: [PATCH] fix NULL-pointer dereference on scsi_run_queue

On 08/04/12 22:36, Mike Christie wrote:
> On 08/04/2012 03:18 PM, Bart Van Assche wrote:
>> On 08/04/12 16:46, Mike Christie wrote:
>>> I think we have to have scsi-ml do a get_device when a sdev is added to
>>> the starved entry and then do a put_device when it is removed (must do
>>> these under the host lock for the starved entry case too). I am not sure
>>> if that is just a hack/papering-over of the problem and there are more
>>> issues like this.
>>
>> That would result in a more complex patch than the patch at the start of
>> this thread, isn't it ?
>
> Yaah, but the original patch in this thread is still racey isn't it?

Indeed. How about the patch below ? Scsi devices are removed from
starved_list after blk_cleanup_queue() and before put_device(). That
guarantees that inside scsi_run_queue() get_device() under host lock
will succeed.

---
drivers/scsi/scsi_lib.c | 5 +++++
drivers/scsi/scsi_sysfs.c | 7 ++++++-
2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ffd7773..bd7daec 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -452,10 +452,15 @@ static void scsi_run_queue(struct request_queue *q)
continue;
}

+ get_device(&sdev->sdev_gendev);
spin_unlock(shost->host_lock);
+
spin_lock(sdev->request_queue->queue_lock);
__blk_run_queue(sdev->request_queue);
spin_unlock(sdev->request_queue->queue_lock);
+
+ put_device(&sdev->sdev_gendev);
+
spin_lock(shost->host_lock);
}
/* put any unprocessed entries back */
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 093d4f6..44f232e 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -348,7 +348,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
starget->reap_ref++;
list_del(&sdev->siblings);
list_del(&sdev->same_target_siblings);
- list_del(&sdev->starved_entry);
spin_unlock_irqrestore(sdev->host->host_lock, flags);

cancel_work_sync(&sdev->event_work);
@@ -956,6 +955,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
void __scsi_remove_device(struct scsi_device *sdev)
{
struct device *dev = &sdev->sdev_gendev;
+ struct Scsi_Host *shost = sdev->host;
+ unsigned long flags;

if (sdev->is_visible) {
if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
@@ -977,6 +978,10 @@ void __scsi_remove_device(struct scsi_device *sdev)
blk_cleanup_queue(sdev->request_queue);
cancel_work_sync(&sdev->requeue_work);

+ spin_lock_irqsave(shost->host_lock, flags);
+ list_del(&sdev->starved_entry);
+ spin_unlock_irqrestore(shost->host_lock, flags);
+
if (sdev->host->hostt->slave_destroy)
sdev->host->hostt->slave_destroy(sdev);
transport_destroy_device(dev);
--
1.7.7

2012-08-07 08:53:53

[permalink] [raw]

Subject: Re: [PATCH] fix NULL-pointer dereference on scsi_run_queue

On Tue, Aug 7, 2012 at 2:56 AM, Bart Van Assche <[email protected]> wrote:
> Indeed. How about the patch below ? Scsi devices are removed from
> starved_list after blk_cleanup_queue() and before put_device(). That
> guarantees that inside scsi_run_queue() get_device() under host lock
> will succeed.
Thanks, IMHO, it's harmless and the simple way to solve this issue.
But, I think the second half of your patches are not required, extra
referecne is might suffice?

In addition, Is it ironic that we are careful to use put_device at
scsi_request_fn?. If we trigger the ->remove(),
It occur a oops. What about the removal of unlock/lock as patch bellow?

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 4037fd5..8d9eccd 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1608,11 +1608,7 @@ out_delay:
if (sdev->device_busy == 0)
blk_delay_queue(q, SCSI_QUEUE_DELAY);
out:
- /* must be careful here...if we trigger the ->remove() function
- * we cannot be holding the q lock */
- spin_unlock_irq(q->queue_lock);
put_device(&sdev->sdev_gendev);
- spin_lock_irq(q->queue_lock);
}

u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)

2012-08-07 09:30:34

[permalink] [raw]

Subject: Re: [PATCH] fix NULL-pointer dereference on scsi_run_queue

On 08/07/12 08:53, Chanho Min wrote:
> On Tue, Aug 7, 2012 at 2:56 AM, Bart Van Assche <[email protected]> wrote:
>> Indeed. How about the patch below ? Scsi devices are removed from
>> starved_list after blk_cleanup_queue() and before put_device(). That
>> guarantees that inside scsi_run_queue() get_device() under host lock
>> will succeed.
>
> Thanks, IMHO, it's harmless and the simple way to solve this issue.
> But, I think the second half of your patches are not required, extra
> referecne is might suffice?

I'm afraid that without the second half of that patch the following race
is still possible:
- sdev reference count drops to zero while scsi_run_queue() is in
progress and while that sdev is on the starved_list of its SCSI host;
scsi_device_dev_release_usercontext() call is scheduled but not yet
executed.
- scsi_run_queue() takes that sdev off the local starved_list.
- scsi_run_queue() calls get_device() and that call fails since the
sdev reference count is zero.
- scsi_device_dev_release_usercontext() gets scheduled and frees the
sdev.
- scsi_run_queue() proceeds and calls __blk_run_queue() on a freed
queue, which is what we were trying to avoid.

Bart.

2012-08-07 09:43:24

[permalink] [raw]

Subject: Re: [PATCH] fix NULL-pointer dereference on scsi_run_queue

On 08/07/12 08:53, Chanho Min wrote:
> In addition, Is it ironic that we are careful to use put_device at
> scsi_request_fn?. If we trigger the ->remove(),
> It occur a oops. What about the removal of unlock/lock as patch bellow?
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 4037fd5..8d9eccd 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1608,11 +1608,7 @@ out_delay:
> if (sdev->device_busy == 0)
> blk_delay_queue(q, SCSI_QUEUE_DELAY);
> out:
> - /* must be careful here...if we trigger the ->remove() function
> - * we cannot be holding the q lock */
> - spin_unlock_irq(q->queue_lock);
> put_device(&sdev->sdev_gendev);
> - spin_lock_irq(q->queue_lock);
> }

As far as I can see the comment in the above code was added before
scsi_device_dev_release() was moved to user context, so it might be
outdated. See also
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=65110b2168950a19cc78b5027ed18cb811fbdae8.

Bart.

2012-08-07 16:16:41

[permalink] [raw]

Subject: Re: [PATCH] fix NULL-pointer dereference on scsi_run_queue

On 08/06/2012 12:56 PM, Bart Van Assche wrote:
> On 08/04/12 22:36, Mike Christie wrote:
>> On 08/04/2012 03:18 PM, Bart Van Assche wrote:
>>> On 08/04/12 16:46, Mike Christie wrote:
>>>> I think we have to have scsi-ml do a get_device when a sdev is added to
>>>> the starved entry and then do a put_device when it is removed (must do
>>>> these under the host lock for the starved entry case too). I am not sure
>>>> if that is just a hack/papering-over of the problem and there are more
>>>> issues like this.
>>>
>>> That would result in a more complex patch than the patch at the start of
>>> this thread, isn't it ?
>>
>> Yaah, but the original patch in this thread is still racey isn't it?
>
> Indeed. How about the patch below ? Scsi devices are removed from
> starved_list after blk_cleanup_queue() and before put_device(). That
> guarantees that inside scsi_run_queue() get_device() under host lock
> will succeed.
>
> ---
> drivers/scsi/scsi_lib.c | 5 +++++
> drivers/scsi/scsi_sysfs.c | 7 ++++++-
> 2 files changed, 11 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index ffd7773..bd7daec 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -452,10 +452,15 @@ static void scsi_run_queue(struct request_queue *q)
> continue;
> }
>
> + get_device(&sdev->sdev_gendev);
> spin_unlock(shost->host_lock);
> +
> spin_lock(sdev->request_queue->queue_lock);
> __blk_run_queue(sdev->request_queue);
> spin_unlock(sdev->request_queue->queue_lock);
> +
> + put_device(&sdev->sdev_gendev);
> +
> spin_lock(shost->host_lock);
> }
> /* put any unprocessed entries back */
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 093d4f6..44f232e 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -348,7 +348,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
> starget->reap_ref++;
> list_del(&sdev->siblings);
> list_del(&sdev->same_target_siblings);
> - list_del(&sdev->starved_entry);
> spin_unlock_irqrestore(sdev->host->host_lock, flags);
>
> cancel_work_sync(&sdev->event_work);
> @@ -956,6 +955,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
> void __scsi_remove_device(struct scsi_device *sdev)
> {
> struct device *dev = &sdev->sdev_gendev;
> + struct Scsi_Host *shost = sdev->host;
> + unsigned long flags;
>
> if (sdev->is_visible) {
> if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
> @@ -977,6 +978,10 @@ void __scsi_remove_device(struct scsi_device *sdev)
> blk_cleanup_queue(sdev->request_queue);
> cancel_work_sync(&sdev->requeue_work);
>
> + spin_lock_irqsave(shost->host_lock, flags);
> + list_del(&sdev->starved_entry);
> + spin_unlock_irqrestore(shost->host_lock, flags);
> +
> if (sdev->host->hostt->slave_destroy)
> sdev->host->hostt->slave_destroy(sdev);
> transport_destroy_device(dev);
>

I think the patch will work now.

Reviewed-by: Mike Christie <[email protected]>

2012-08-08 03:42:47

[permalink] [raw]

Subject: Re: [PATCH] fix NULL-pointer dereference on scsi_run_queue

> I'm afraid that without the second half of that patch the following race
> is still possible:
> - sdev reference count drops to zero while scsi_run_queue() is in
> progress and while that sdev is on the starved_list of its SCSI host;
> scsi_device_dev_release_usercontext() call is scheduled but not yet
> executed.
> - scsi_run_queue() takes that sdev off the local starved_list.
> - scsi_run_queue() calls get_device() and that call fails since the
> sdev reference count is zero.
> - scsi_device_dev_release_usercontext() gets scheduled and frees the
> sdev.
> - scsi_run_queue() proceeds and calls __blk_run_queue() on a freed
> queue, which is what we were trying to avoid.
Thank you for the explanation. It look correct. Let's check one more thing.
What If __scsi_remove_device doesn't release device? : reference count
is more than 2.
So We lost starved_list but device is exist. Is there any issue about this?

Chanho,

2012-08-08 07:37:35