LinuxLists.cc - __elv_add

[permalink] [raw]

Subject: Re: __elv_add_request OOPS

On 2011-05-24 06:29, Parag Warudkar wrote:
>
> External DVD drive - connected when suspended, removed before resume.
> Results in NULL pointer dereference in __blk_add_request on resume.
>
> *ffffffff811d6503: 48 89 58 08 mov %rbx,0x8(%rax) |
> %ebx = ffff880131559020 <--- faulting instruction
>
> 48 89 58 08 appears only in list_add :
>
> static inline void list_add(struct list_head *new, struct list_head *head)
> {
> __list_add(new, head, head->next);
> ffffffff81ac012c: 49 8b 04 24 mov (%r12),%rax
> #ifndef CONFIG_DEBUG_LIST
> static inline void __list_add(struct list_head *new,
> struct list_head *prev,
> struct list_head *next)
> {
> next->prev = new;
> ffffffff81ac0130: 48 89 58 08 mov %rbx,0x8(%rax)
>
> AFAICS list_add is only called from one place in __elv_add_request :
>
> switch (where) {
> case ELEVATOR_INSERT_REQUEUE:
> case ELEVATOR_INSERT_FRONT:
> rq->cmd_flags |= REQ_SOFTBARRIER;
> ** list_add(&rq->queuelist, &q->queue_head);
> break;
>
> Now, where is the patch? :)

You forgot to attach it?

This is clearly q == NULL, CC'ing James/linux-scsi. Oops left below.

> [18682.256362] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> [18682.256535] IP: [<ffffffff811d6503>] __elv_add_request+0x1e3/0x270
> [18682.256603] PGD 0
> [18682.256632] Oops: 0002 [#1] SMP
> [18682.256686] CPU 2
> [18682.256714] Modules linked in: nls_utf8 udf crc_itu_t usb_storage cryptd aes_x86_64 aes_generic fuse parport_pc ppdev dm_crypt kvm_intel joydev kvm binfmt_misc snd_hda_codec_hdmi snd_hda_codec_realtek arc4 snd_hda_intel snd_hda_codec iwlagn snd_hwdep snd_pcm mac80211 snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq snd_timer snd_seq_device snd cfg80211 soundcore btusb uvcvideo snd_page_alloc bluetooth videodev v4l2_compat_ioctl32 psmouse ideapad_laptop serio_raw sparse_keymap lp intel_ips mac_hid parport ext4 mbcache jbd2 i915 ahci libahci libata drm_kms_helper drm i2c_algo_bit cfbcopyarea video cfbimgblt cfbfillrect atl1c
> [18682.257659]
> [18682.257685] Pid: 14069, comm: xdg-screensaver Not tainted 2.6.39+ #4 LENOVO 0876 /Base Board Product Name
> [18682.257845] RIP: 0010:[<ffffffff811d6503>] [<ffffffff811d6503>] __elv_add_request+0x1e3/0x270
> [18682.257964] RSP: 0018:ffff88009b3a19e8 EFLAGS: 00010006
> [18682.258056] RAX: 0000000000000000 RBX: ffff880131559020 RCX: 0000000000000001
> [18682.258152] RDX: 0000000000000001 RSI: ffff880131559020 RDI: ffff8801315f77d0
> [18682.258248] RBP: ffff88009b3a1a08 R08: ffffffff811e1000 R09: ffff8801315f77d0
> [18682.258343] R10: ffff8800b5085e40 R11: ffff8800b5085e40 R12: ffff8801315f77d0
> [18682.258437] R13: 0000000000000001 R14: 0000000000000001 R15: ffff8800b5085e40
> [18682.258529] FS: 0000000000000000(0000) GS:ffff880137c80000(0000) knlGS:0000000000000000
> [18682.258636] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [18682.258712] CR2: 0000000000000008 CR3: 0000000001a03000 CR4: 00000000000006e0
> [18682.258807] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [18682.258898] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> [18682.258994] Process xdg-screensaver (pid: 14069, threadinfo ffff88009b3a0000, task ffff8800aff096b0)
> [18682.259112] Stack:
> [18682.259140] ffff8801315f77d0 ffff880131559020 0000000000000001 ffff88009b3a1c48
> [18682.259249] ffff88009b3a1a38 ffffffff811e10a0 0000000000000000 ffff88009b3a1a48
> [18682.259354] ffff880131559020 0000000000000000 ffff88009b3a1af8 ffffffff811e118e
> [18682.259460] Call Trace:
> [18682.259504] [<ffffffff811e10a0>] blk_execute_rq_nowait+0x60/0xc0
> [18682.259587] [<ffffffff811e118e>] blk_execute_rq+0x8e/0x130
> [18682.259668] [<ffffffff812db5bc>] scsi_execute+0xfc/0x160
> [18682.259742] [<ffffffff812dc18f>] scsi_execute_req+0xbf/0x130
> [18682.259821] [<ffffffff812d5c71>] ioctl_internal_command.clone.4+0x61/0x1b0
> [18682.259914] [<ffffffff812d5e3e>] scsi_set_medium_removal+0x7e/0xb0
> [18682.260000] [<ffffffff812ecfa0>] sr_lock_door+0x20/0x30
> [18682.260075] [<ffffffff812f9da7>] cdrom_release+0x147/0x270
> [18682.260153] [<ffffffff812ebac8>] sr_block_release+0x38/0x60
> [18682.260233] [<ffffffff811730ac>] __blkdev_put+0x16c/0x1b0
> [18682.260308] [<ffffffff81173129>] blkdev_put+0x39/0x150
> [18682.260379] [<ffffffff81173264>] blkdev_close+0x24/0x30
> [18682.260455] [<ffffffff81140fba>] fput+0xea/0x220
> [18682.260521] [<ffffffff8113d396>] filp_close+0x66/0x90
> [18682.260592] [<ffffffff8105c117>] put_files_struct+0x87/0xf0
> [18682.260668] [<ffffffff8105c244>] exit_files+0x54/0x70
> [18682.264275] [<ffffffff8105c72b>] do_exit+0x16b/0x860
> [18682.267802] [<ffffffff811f6c2a>] ? trace_hardirqs_off_thunk+0x3a/0x6c
> [18682.271512] [<ffffffff8105d0e8>] do_group_exit+0x58/0xd0
> [18682.276948] [<ffffffff8105d177>] sys_exit_group+0x17/0x20
> [18682.281121] [<ffffffff81485d42>] system_call_fastpath+0x16/0x1b
> [18682.284603] Code: ff ff e9 90 fe ff ff 90 81 4b 40 00 08 00 00 48 89 df e8 c1 93 00 00 eb c1 0f 1f 80 00 00 00 00 81 4b 40 00 08 00 00 49 8b 04 24
> [18682.284903] 89 58 08 48 89 03 4c 89 63 08 49 89 1c 24 eb 9e 0f 1f 40 00
> [18682.290727] RIP [<ffffffff811d6503>] __elv_add_request+0x1e3/0x270
> [18682.293189] RSP <ffff88009b3a19e8>
> [18682.296075] CR2: 0000000000000008
> [18682.358582] ---[ end trace 82dd699fdeb50b72 ]---

--
Jens Axboe

2011-05-25 01:41:27

[permalink] [raw]

Subject: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Tue, 24 May 2011, Jens Axboe wrote:

> On 2011-05-24 06:29, Parag Warudkar wrote:
> >
> > External DVD drive - connected when suspended, removed before resume.
> > Results in NULL pointer dereference in __blk_add_request on resume.
> >
> > *ffffffff811d6503: 48 89 58 08 mov %rbx,0x8(%rax) |
> > %ebx = ffff880131559020 <--- faulting instruction
> >
> > 48 89 58 08 appears only in list_add :
> >
> > static inline void list_add(struct list_head *new, struct list_head *head)
> > {
> > __list_add(new, head, head->next);
> > ffffffff81ac012c: 49 8b 04 24 mov (%r12),%rax
> > #ifndef CONFIG_DEBUG_LIST
> > static inline void __list_add(struct list_head *new,
> > struct list_head *prev,
> > struct list_head *next)
> > {
> > next->prev = new;
> > ffffffff81ac0130: 48 89 58 08 mov %rbx,0x8(%rax)
> >
> > AFAICS list_add is only called from one place in __elv_add_request :
> >
> > switch (where) {
> > case ELEVATOR_INSERT_REQUEUE:
> > case ELEVATOR_INSERT_FRONT:
> > rq->cmd_flags |= REQ_SOFTBARRIER;
> > ** list_add(&rq->queuelist, &q->queue_head);
> > break;
> >
> > Now, where is the patch? :)
>
> You forgot to attach it?
>
> This is clearly q == NULL [snip]

OK, I think this patch should do the trick.

Signed-off-by: Parag Warudkar <[email protected]>
Reported-and-tested-by: Parag Warudkar <[email protected]>

Check for device deletion before sending it a scsi command. This fixes an
OOPS I was seeing during resume when the external dvd drive was removed
while suspended.

diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
index d9564fb..cc4edbf 100644
--- a/drivers/scsi/scsi_ioctl.c
+++ b/drivers/scsi/scsi_ioctl.c
@@ -136,7 +136,7 @@ int scsi_set_medium_removal(struct scsi_device *sdev, char state)
char scsi_cmd[MAX_COMMAND_SIZE];
int ret;

- if (!sdev->removable || !sdev->lockable)
+ if (!sdev->removable || !sdev->lockable || sdev->sdev_state == SDEV_DEL)
return 0;

scsi_cmd[0] = ALLOW_MEDIUM_REMOVAL;

2011-05-25 07:37:51

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On 2011-05-25 03:41, Parag Warudkar wrote:
>
>
> On Tue, 24 May 2011, Jens Axboe wrote:
>
>> On 2011-05-24 06:29, Parag Warudkar wrote:
>>>
>>> External DVD drive - connected when suspended, removed before resume.
>>> Results in NULL pointer dereference in __blk_add_request on resume.
>>>
>>> *ffffffff811d6503: 48 89 58 08 mov %rbx,0x8(%rax) |
>>> %ebx = ffff880131559020 <--- faulting instruction
>>>
>>> 48 89 58 08 appears only in list_add :
>>>
>>> static inline void list_add(struct list_head *new, struct list_head *head)
>>> {
>>> __list_add(new, head, head->next);
>>> ffffffff81ac012c: 49 8b 04 24 mov (%r12),%rax
>>> #ifndef CONFIG_DEBUG_LIST
>>> static inline void __list_add(struct list_head *new,
>>> struct list_head *prev,
>>> struct list_head *next)
>>> {
>>> next->prev = new;
>>> ffffffff81ac0130: 48 89 58 08 mov %rbx,0x8(%rax)
>>>
>>> AFAICS list_add is only called from one place in __elv_add_request :
>>>
>>> switch (where) {
>>> case ELEVATOR_INSERT_REQUEUE:
>>> case ELEVATOR_INSERT_FRONT:
>>> rq->cmd_flags |= REQ_SOFTBARRIER;
>>> ** list_add(&rq->queuelist, &q->queue_head);
>>> break;
>>>
>>> Now, where is the patch? :)
>>
>> You forgot to attach it?
>>
>> This is clearly q == NULL [snip]
>
> OK, I think this patch should do the trick.
>
> Signed-off-by: Parag Warudkar <[email protected]>
> Reported-and-tested-by: Parag Warudkar <[email protected]>
>
> Check for device deletion before sending it a scsi command. This fixes an
> OOPS I was seeing during resume when the external dvd drive was removed
> while suspended.
>
> diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
> index d9564fb..cc4edbf 100644
> --- a/drivers/scsi/scsi_ioctl.c
> +++ b/drivers/scsi/scsi_ioctl.c
> @@ -136,7 +136,7 @@ int scsi_set_medium_removal(struct scsi_device *sdev, char state)
> char scsi_cmd[MAX_COMMAND_SIZE];
> int ret;
>
> - if (!sdev->removable || !sdev->lockable)
> + if (!sdev->removable || !sdev->lockable || sdev->sdev_state == SDEV_DEL)
> return 0;
>
> scsi_cmd[0] = ALLOW_MEDIUM_REMOVAL;

While this will fix your particular oops, I don't think it's quite
right. It's fixing one particular piece of fall out from attempting to
talk to a removed device, it's not necessarily fixing the full class of
them. The other checks in scsi_set_medium_removal() aren't related to a
changing state of the device, they are capability checks.

--
Jens Axboe

2011-05-25 18:44:57

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, 25 May 2011, Jens Axboe wrote:

> While this will fix your particular oops, I don't think it's quite
> right. It's fixing one particular piece of fall out from attempting to
> talk to a removed device, it's not necessarily fixing the full class of
> them. The other checks in scsi_set_medium_removal() aren't related to a
> changing state of the device, they are capability checks.

Below patch pushes the check down to ioctl functions and hopefully should
cover more ioctl-on-gone-device cases by returning -ENXIO if an attempt
was made to submit request to a non-running device.

With this patch in place - instead of oopsing I now get a proper I/O error
on resume.

AFAICT from my limited SCSI layer knowledge - these seem to be the right
places to check. Would be interested in knowing if there are better/more
complete ways to handle this but if they are going to take time IMHO it
certainly doesn't hurt to keep the below patch in.

Signed-off-by: Parag Warudkar <[email protected]>

diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
index d9564fb..c32cade 100644
--- a/drivers/scsi/scsi_ioctl.c
+++ b/drivers/scsi/scsi_ioctl.c
@@ -91,6 +91,10 @@ static int ioctl_internal_command(struct scsi_device *sdev, char *cmd,
int result;
struct scsi_sense_hdr sshdr;

+ if (!sdev || sdev->sdev_state == SDEV_DEL
+ || sdev->sdev_state > SDEV_QUIESCE)
+ return -ENXIO;
+
SCSI_LOG_IOCTL(1, printk("Trying ioctl with scsi command %d\n", *cmd));

result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL, 0,
@@ -195,8 +199,9 @@ int scsi_ioctl(struct scsi_device *sdev, int cmd, void __user *arg)
{
char scsi_cmd[MAX_COMMAND_SIZE];

- /* No idea how this happens.... */
- if (!sdev)
+ /* No idea how !sdev happens.... */
+ if (!sdev || sdev->sdev_state == SDEV_DEL
+ || sdev->sdev_state > SDEV_QUIESCE)
return -ENXIO;

/*
@@ -288,6 +293,10 @@ int scsi_nonblockable_ioctl(struct scsi_device *sdev, int cmd,
{
int val, result;

+ if (!sdev || sdev->sdev_state == SDEV_DEL
+ || sdev->sdev_state > SDEV_QUIESCE)
+ return -ENXIO;
+
/* The first set of iocts may be executed even if we're doing
* error processing, as long as the device was opened
* non-blocking */

2011-05-25 18:56:51

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, May 25, 2011 at 11:44 AM, Parag Warudkar <[email protected]> wrote:
>
> Below patch pushes the check down to ioctl functions and hopefully should
> cover more ioctl-on-gone-device cases by returning -ENXIO if an attempt
> was made to submit request to a non-running device.

Can we please not duplicate complicated logic like that?

IOW, just make a helper function for it.

That said, isn't this all *exactly* what scsi_prep_state_check() is
supposed to check for? Why isn't that called, or if called, why isn't
it doing the right thing?

Linus

2011-05-25 19:02:30

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On 2011-05-25 20:55, Linus Torvalds wrote:
> On Wed, May 25, 2011 at 11:44 AM, Parag Warudkar <[email protected]> wrote:
>>
>> Below patch pushes the check down to ioctl functions and hopefully should
>> cover more ioctl-on-gone-device cases by returning -ENXIO if an attempt
>> was made to submit request to a non-running device.
>
> Can we please not duplicate complicated logic like that?
>
> IOW, just make a helper function for it.
>
> That said, isn't this all *exactly* what scsi_prep_state_check() is
> supposed to check for? Why isn't that called, or if called, why isn't
> it doing the right thing?

This is before you get that far, it's actually oopsing on inserting the
request on sdev->sdev_queue that is now NULL. The prep state checking
happens when sr/sd pulls the request off the queue for processing.

--
Jens Axboe

2011-05-25 19:14:36

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, May 25, 2011 at 12:02 PM, Jens Axboe <[email protected]> wrote:
>
> This is before you get that far, it's actually oopsing on inserting the
> request on sdev->sdev_queue that is now NULL. The prep state checking
> happens when sr/sd pulls the request off the queue for processing.

Ok, please just add a "sdev_alive(sdev)" helper that checks for sdev
&& sdev->sdev_queue. Apparently the issue isn't "this device is
SDEV_DEL, so don't send commands". Apparently the issue is literally
"don't oops".

Or maybe the prep state checking could be moved earlier. It seems
stupid to have a "can I do this command" function that is run too late
to actually catch the problems..

Linus

2011-05-25 19:17:47

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On 2011-05-25 21:13, Linus Torvalds wrote:
> On Wed, May 25, 2011 at 12:02 PM, Jens Axboe <[email protected]> wrote:
>>
>> This is before you get that far, it's actually oopsing on inserting the
>> request on sdev->sdev_queue that is now NULL. The prep state checking
>> happens when sr/sd pulls the request off the queue for processing.
>
> Ok, please just add a "sdev_alive(sdev)" helper that checks for sdev
> && sdev->sdev_queue. Apparently the issue isn't "this device is
> SDEV_DEL, so don't send commands". Apparently the issue is literally
> "don't oops".

Yep, will do.

> Or maybe the prep state checking could be moved earlier. It seems
> stupid to have a "can I do this command" function that is run too late
> to actually catch the problems..

I don't think we can move it earlier, we essentially have to do the
check at both ends. The "normal" IO path would see that the queue is
dead and error the IO, but this is an internal setup that sets up the
request and then adds it to the stored queue pointer. So it needs to
check this state one way or the other. I think the above fix with the
sdev_alive() will be good enough.

--
Jens Axboe

2011-05-25 19:52:26

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, 25 May 2011, Jens Axboe wrote:

> I don't think we can move it earlier, we essentially have to do the
> check at both ends. The "normal" IO path would see that the queue is
> dead and error the IO, but this is an internal setup that sets up the
> request and then adds it to the stored queue pointer. So it needs to
> check this state one way or the other. I think the above fix with the
> sdev_alive() will be good enough.
>

If I understood it right we need checks in both places (kind of makes
sense as the device could be gone *after* we checked in ioctl function and
before the request queue was dereferenced). So here is the ioctl check
with a helper function for state check that Linus asked for.

Signed-off-by: Parag Warudkar <[email protected]>

diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
index d9564fb..8615e63 100644
--- a/drivers/scsi/scsi_ioctl.c
+++ b/drivers/scsi/scsi_ioctl.c
@@ -30,6 +30,13 @@

#define MAX_BUF PAGE_SIZE

+static inline int sdev_early_check(struct scsi_device *sdev)
+{
+ if (!sdev || sdev->sdev_state == SDEV_DEL
+ || sdev->sdev_state > SDEV_QUIESCE)
+ return -ENXIO;
+ return 0;
+}
/**
* ioctl_probe -- return host identification
* @host: host to identify
@@ -91,6 +98,10 @@ static int ioctl_internal_command(struct scsi_device *sdev, char *cmd,
int result;
struct scsi_sense_hdr sshdr;

+ result = sdev_early_check(sdev);
+ if (result)
+ return result;
+
SCSI_LOG_IOCTL(1, printk("Trying ioctl with scsi command %d\n", *cmd));

result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL, 0,
@@ -194,10 +205,11 @@ static int scsi_ioctl_get_pci(struct scsi_device *sdev, void __user *arg)
int scsi_ioctl(struct scsi_device *sdev, int cmd, void __user *arg)
{
char scsi_cmd[MAX_COMMAND_SIZE];
+ int result;

- /* No idea how this happens.... */
- if (!sdev)
- return -ENXIO;
+ result = sdev_early_check(sdev);
+ if (result)
+ return result;

/*
* If we are in the middle of error recovery, don't let anyone
@@ -288,6 +300,10 @@ int scsi_nonblockable_ioctl(struct scsi_device *sdev, int cmd,
{
int val, result;

+ result = sdev_early_check(sdev);
+ if (result)
+ return result;
+
/* The first set of iocts may be executed even if we're doing
* error processing, as long as the device was opened
* non-blocking */

2011-05-25 20:04:34

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, May 25, 2011 at 12:52 PM, Parag Warudkar <[email protected]> wrote:
>
> +static inline int sdev_early_check(struct scsi_device *sdev)
> +{
> + ? ? ? if (!sdev || sdev->sdev_state == SDEV_DEL
> + ? ? ? ? ? ? ? || sdev->sdev_state > SDEV_QUIESCE)
> + ? ? ? ? ? ? ? return -ENXIO;
> + ? ? ? return 0;
> +}

Can somebody explain why it's those states, and nothing else?

Quite frankly, if it's about "don't oops on sdev->queue not existing",
then test for that. Not for some random and uncommented list of
states.

Linus

2011-05-25 20:18:34

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, 25 May 2011, Linus Torvalds wrote:

> On Wed, May 25, 2011 at 12:52 PM, Parag Warudkar <[email protected]> wrote:
> >
> > +static inline int sdev_early_check(struct scsi_device *sdev)
> > +{
> > + ? ? ? if (!sdev || sdev->sdev_state == SDEV_DEL
> > + ? ? ? ? ? ? ? || sdev->sdev_state > SDEV_QUIESCE)
> > + ? ? ? ? ? ? ? return -ENXIO;
> > + ? ? ? return 0;
> > +}
>
> Can somebody explain why it's those states, and nothing else?
>
> Quite frankly, if it's about "don't oops on sdev->queue not existing",
> then test for that. Not for some random and uncommented list of
> states.

List of states and what commands if any are accepeted for each state are
explained in scsi_device.h enum definition of scsi_device_state :

enum scsi_device_state {
SDEV_CREATED = 1, /* device created but not added to sysfs
* Only internal commands allowed (for
inq) */
SDEV_RUNNING, /* device properly configured
* All commands allowed */
SDEV_CANCEL, /* beginning to delete device
* Only error handler commands allowed */
SDEV_DEL, /* device deleted
* no commands allowed */
SDEV_QUIESCE, /* Device quiescent. No block commands
* will be accepted, only specials (which
* originate in the mid-layer) */
SDEV_OFFLINE, /* Device offlined (by error handling or
* user request */
SDEV_BLOCK, /* Device blocked by scsi lld. No
* scsi commands from user or midlayer
* should be issued to the scsi
* lld. */
SDEV_CREATED_BLOCK, /* same as above but for created devices
*/
};

In this case - we check for cases where no commands are allowed and reject
those right away, leaving prep_state_check to do filtering at lower level
for which commands are allowed and which are not as the state permits.

Parag

2011-05-25 20:20:23

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, 2011-05-25 at 13:03 -0700, Linus Torvalds wrote:
> On Wed, May 25, 2011 at 12:52 PM, Parag Warudkar <[email protected]> wrote:
> >
> > +static inline int sdev_early_check(struct scsi_device *sdev)
> > +{
> > + if (!sdev || sdev->sdev_state == SDEV_DEL
> > + || sdev->sdev_state > SDEV_QUIESCE)
> > + return -ENXIO;
> > + return 0;
> > +}
>
> Can somebody explain why it's those states, and nothing else?

The states are definitely wrong. QUIESCE shouldn't cause an error.

Can we back up a bit. I agree with Jens that sdev->request_queue looks
to be NULL, but that *only* happens in the final sdev release, so this
must be a refcounting problem higher up the stack. Something is holding
an unrefcounted pointer to the sdev.

James

2011-05-25 20:23:01

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, 25 May 2011, James Bottomley wrote:

> On Wed, 2011-05-25 at 13:03 -0700, Linus Torvalds wrote:
> > On Wed, May 25, 2011 at 12:52 PM, Parag Warudkar <[email protected]> wrote:
> > >
> > > +static inline int sdev_early_check(struct scsi_device *sdev)
> > > +{
> > > + if (!sdev || sdev->sdev_state == SDEV_DEL
> > > + || sdev->sdev_state > SDEV_QUIESCE)
> > > + return -ENXIO;
> > > + return 0;
> > > +}
> >
> > Can somebody explain why it's those states, and nothing else?
>
> The states are definitely wrong. QUIESCE shouldn't cause an error.
>
QUIESCE is not causing an error above - > QUIESCE is which per the
documentation is correct.

Parag

2011-05-25 20:26:19

[permalink] [raw]

Subject: Re: __elv_add_request OOPS

On Tue, 2011-05-24 at 12:44 +0200, Jens Axboe wrote:
> On 2011-05-24 06:29, Parag Warudkar wrote:
> >
> > External DVD drive - connected when suspended, removed before resume.
> > Results in NULL pointer dereference in __blk_add_request on resume.
> >
> > *ffffffff811d6503: 48 89 58 08 mov %rbx,0x8(%rax) |
> > %ebx = ffff880131559020 <--- faulting instruction
> >
> > 48 89 58 08 appears only in list_add :
> >
> > static inline void list_add(struct list_head *new, struct list_head *head)
> > {
> > __list_add(new, head, head->next);
> > ffffffff81ac012c: 49 8b 04 24 mov (%r12),%rax
> > #ifndef CONFIG_DEBUG_LIST
> > static inline void __list_add(struct list_head *new,
> > struct list_head *prev,
> > struct list_head *next)
> > {
> > next->prev = new;
> > ffffffff81ac0130: 48 89 58 08 mov %rbx,0x8(%rax)
> >
> > AFAICS list_add is only called from one place in __elv_add_request :
> >
> > switch (where) {
> > case ELEVATOR_INSERT_REQUEUE:
> > case ELEVATOR_INSERT_FRONT:
> > rq->cmd_flags |= REQ_SOFTBARRIER;
> > ** list_add(&rq->queuelist, &q->queue_head);
> > break;
> >
> > Now, where is the patch? :)
>
> You forgot to attach it?
>
> This is clearly q == NULL, CC'ing James/linux-scsi. Oops left below.

Something strange is going on here. the q can't have been NULL when we
called blk_get_request() in scsi_execute() otherwise we'd have oopsed on
q->queue_lock, so it's not as simple as sdev->request_queue being NULL.

James

> > [18682.256362] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> > [18682.256535] IP: [<ffffffff811d6503>] __elv_add_request+0x1e3/0x270
> > [18682.256603] PGD 0
> > [18682.256632] Oops: 0002 [#1] SMP
> > [18682.256686] CPU 2
> > [18682.256714] Modules linked in: nls_utf8 udf crc_itu_t usb_storage cryptd aes_x86_64 aes_generic fuse parport_pc ppdev dm_crypt kvm_intel joydev kvm binfmt_misc snd_hda_codec_hdmi snd_hda_codec_realtek arc4 snd_hda_intel snd_hda_codec iwlagn snd_hwdep snd_pcm mac80211 snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq snd_timer snd_seq_device snd cfg80211 soundcore btusb uvcvideo snd_page_alloc bluetooth videodev v4l2_compat_ioctl32 psmouse ideapad_laptop serio_raw sparse_keymap lp intel_ips mac_hid parport ext4 mbcache jbd2 i915 ahci libahci libata drm_kms_helper drm i2c_algo_bit cfbcopyarea video cfbimgblt cfbfillrect atl1c
> > [18682.257659]
> > [18682.257685] Pid: 14069, comm: xdg-screensaver Not tainted 2.6.39+ #4 LENOVO 0876 /Base Board Product Name
> > [18682.257845] RIP: 0010:[<ffffffff811d6503>] [<ffffffff811d6503>] __elv_add_request+0x1e3/0x270
> > [18682.257964] RSP: 0018:ffff88009b3a19e8 EFLAGS: 00010006
> > [18682.258056] RAX: 0000000000000000 RBX: ffff880131559020 RCX: 0000000000000001
> > [18682.258152] RDX: 0000000000000001 RSI: ffff880131559020 RDI: ffff8801315f77d0
> > [18682.258248] RBP: ffff88009b3a1a08 R08: ffffffff811e1000 R09: ffff8801315f77d0
> > [18682.258343] R10: ffff8800b5085e40 R11: ffff8800b5085e40 R12: ffff8801315f77d0
> > [18682.258437] R13: 0000000000000001 R14: 0000000000000001 R15: ffff8800b5085e40
> > [18682.258529] FS: 0000000000000000(0000) GS:ffff880137c80000(0000) knlGS:0000000000000000
> > [18682.258636] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [18682.258712] CR2: 0000000000000008 CR3: 0000000001a03000 CR4: 00000000000006e0
> > [18682.258807] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > [18682.258898] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> > [18682.258994] Process xdg-screensaver (pid: 14069, threadinfo ffff88009b3a0000, task ffff8800aff096b0)
> > [18682.259112] Stack:
> > [18682.259140] ffff8801315f77d0 ffff880131559020 0000000000000001 ffff88009b3a1c48
> > [18682.259249] ffff88009b3a1a38 ffffffff811e10a0 0000000000000000 ffff88009b3a1a48
> > [18682.259354] ffff880131559020 0000000000000000 ffff88009b3a1af8 ffffffff811e118e
> > [18682.259460] Call Trace:
> > [18682.259504] [<ffffffff811e10a0>] blk_execute_rq_nowait+0x60/0xc0
> > [18682.259587] [<ffffffff811e118e>] blk_execute_rq+0x8e/0x130
> > [18682.259668] [<ffffffff812db5bc>] scsi_execute+0xfc/0x160
> > [18682.259742] [<ffffffff812dc18f>] scsi_execute_req+0xbf/0x130
> > [18682.259821] [<ffffffff812d5c71>] ioctl_internal_command.clone.4+0x61/0x1b0
> > [18682.259914] [<ffffffff812d5e3e>] scsi_set_medium_removal+0x7e/0xb0
> > [18682.260000] [<ffffffff812ecfa0>] sr_lock_door+0x20/0x30
> > [18682.260075] [<ffffffff812f9da7>] cdrom_release+0x147/0x270
> > [18682.260153] [<ffffffff812ebac8>] sr_block_release+0x38/0x60
> > [18682.260233] [<ffffffff811730ac>] __blkdev_put+0x16c/0x1b0
> > [18682.260308] [<ffffffff81173129>] blkdev_put+0x39/0x150
> > [18682.260379] [<ffffffff81173264>] blkdev_close+0x24/0x30
> > [18682.260455] [<ffffffff81140fba>] fput+0xea/0x220
> > [18682.260521] [<ffffffff8113d396>] filp_close+0x66/0x90
> > [18682.260592] [<ffffffff8105c117>] put_files_struct+0x87/0xf0
> > [18682.260668] [<ffffffff8105c244>] exit_files+0x54/0x70
> > [18682.264275] [<ffffffff8105c72b>] do_exit+0x16b/0x860
> > [18682.267802] [<ffffffff811f6c2a>] ? trace_hardirqs_off_thunk+0x3a/0x6c
> > [18682.271512] [<ffffffff8105d0e8>] do_group_exit+0x58/0xd0
> > [18682.276948] [<ffffffff8105d177>] sys_exit_group+0x17/0x20
> > [18682.281121] [<ffffffff81485d42>] system_call_fastpath+0x16/0x1b
> > [18682.284603] Code: ff ff e9 90 fe ff ff 90 81 4b 40 00 08 00 00 48 89 df e8 c1 93 00 00 eb c1 0f 1f 80 00 00 00 00 81 4b 40 00 08 00 00 49 8b 04 24
> > [18682.284903] 89 58 08 48 89 03 4c 89 63 08 49 89 1c 24 eb 9e 0f 1f 40 00
> > [18682.290727] RIP [<ffffffff811d6503>] __elv_add_request+0x1e3/0x270
> > [18682.293189] RSP <ffff88009b3a19e8>
> > [18682.296075] CR2: 0000000000000008
> > [18682.358582] ---[ end trace 82dd699fdeb50b72 ]---
>

2011-05-25 20:34:52

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, May 25, 2011 at 1:18 PM, Parag Warudkar <[email protected]> wrote:
> ? ? ? ?SDEV_OFFLINE, ? ? ? ? ? /* Device offlined (by error handling or
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? * user request */

So how do you fix the error or online it again? No chance it should
have commands passed to it to do that?

I don't know. My point is that I don't see why we should look at the
state at this point at all. We do that _later_.

Linus

2011-05-25 20:29:54

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, 2011-05-25 at 16:22 -0400, Parag Warudkar wrote:
>
> On Wed, 25 May 2011, James Bottomley wrote:
>
> > On Wed, 2011-05-25 at 13:03 -0700, Linus Torvalds wrote:
> > > On Wed, May 25, 2011 at 12:52 PM, Parag Warudkar <[email protected]> wrote:
> > > >
> > > > +static inline int sdev_early_check(struct scsi_device *sdev)
> > > > +{
> > > > + if (!sdev || sdev->sdev_state == SDEV_DEL
> > > > + || sdev->sdev_state > SDEV_QUIESCE)
> > > > + return -ENXIO;
> > > > + return 0;
> > > > +}
> > >
> > > Can somebody explain why it's those states, and nothing else?
> >
> > The states are definitely wrong. QUIESCE shouldn't cause an error.
> >
> QUIESCE is not causing an error above - > QUIESCE is which per the
> documentation is correct.

BLOCK means a similar thing.

James

2011-05-25 20:42:22

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, 25 May 2011, Linus Torvalds wrote:

> On Wed, May 25, 2011 at 1:18 PM, Parag Warudkar <[email protected]> wrote:
> > ? ? ? ?SDEV_OFFLINE, ? ? ? ? ? /* Device offlined (by error handling or
> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? * user request */
>
> So how do you fix the error or online it again? No chance it should
> have commands passed to it to do that?

Yeah - that makes sense. By that logic, looks like we can only disallow
for SDEV_DEL (if we decide to do that check here).

> I don't know. My point is that I don't see why we should look at the
> state at this point at all. We do that _later_.

If we can do it later, well and good. I just didn't think it would hurt to
check upfront in ioctl if we are dealing with a dead device.

Parag

2011-05-25 20:52:20

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, 2011-05-25 at 16:42 -0400, Parag Warudkar wrote:
> Yeah - that makes sense. By that logic, looks like we can only disallow
> for SDEV_DEL (if we decide to do that check here).

I don't think this is the root cause. I think q is non-NULL but has
already been released, so we're just getting a NULL deref on the actual
list head.

Does this fix it? It adds the refcounting at approximately the correct
places. Of course, we'll now be trying elevator operations on an extant
queue but one whose elevator functions have been destroyed, so there are
probably additional state guards to place.

James

---

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 58584dc..44e8ca3 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -297,7 +297,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
kfree(sdev);
goto out;
}
-
+ blk_get_queue(sdev->request_queue);
sdev->request_queue->queuedata = sdev;
scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index e639125..e0bd3f7 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -322,6 +322,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
kfree(evt);
}

+ blk_put_queue(sdev->request_queue);
/* NULL queue means the device can't be used */
sdev->request_queue = NULL;

2011-05-25 23:00:16

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, May 25, 2011 at 4:52 PM, James Bottomley
<[email protected]> wrote:
> On Wed, 2011-05-25 at 16:42 -0400, Parag Warudkar wrote:
>> Yeah - that makes sense. By that logic, looks like we can only disallow
>> for SDEV_DEL (if we decide to do that check here).
>
> I don't think this is the root cause. ?I think q is non-NULL but has
> already been released, so we're just getting a NULL deref on the actual
> list head.
>
> Does this fix it? ?It adds the refcounting at approximately the correct
> places. ?Of course, we'll now be trying elevator operations on an extant
> queue but one whose elevator functions have been destroyed, so there are
> probably additional state guards to place.

[ Resend with URL - I got a bounce back from vger for attaching the OOPS JPEG ]

It doesn't seem to help. Machine locked up and I was dropped to text
console. I could only capture the oops by a camera.

OOPS available at below URLs [*]

Parag

[*] https://picasaweb.google.com/lh/photo/yccmqaz3xwumwjvecb-b0pTdDOYIfFRksMrZBIo-Inw?feat=directlink

[*] https://picasaweb.google.com/lh/photo/Lim6d3b8p7Lj4D3nHmkbFJTdDOYIfFRksMrZBIo-Inw?feat=directlink

2011-05-25 23:15:20

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, May 25, 2011 at 4:00 PM, Parag Warudkar <[email protected]> wrote:
>
> It doesn't seem to help. Machine locked up and I was dropped to text
> console. I could only capture the oops by a camera.

Hmm. That's a different oops. It is now in scsi_prep_state_check().

At the beginning of it too. You can't see the "Code: " line in the
pictures, but the only dereference I see there is "sdev" itself being
NULL. And %cr2 is 0x640, which would seem to agree (the 'sdev'
structure is absolutely disgustingly big, and sdev->sdev_state is
indeed at an offset in that region.

That 'sdev' comes from scsi_prep_fn() doing

struct scsi_device *sdev = q->queuedata;

is that queuedata perhaps cleared even if the queue itself stays
around due to refcounts?

Linus

2011-05-25 23:45:35

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, 25 May 2011, Linus Torvalds wrote:

> On Wed, May 25, 2011 at 4:00 PM, Parag Warudkar <[email protected]> wrote:
> >
> > It doesn't seem to help. Machine locked up and I was dropped to text
> > console. I could only capture the oops by a camera.
>
> Hmm. That's a different oops. It is now in scsi_prep_state_check().

Different at top but the call trace upto scsi_set_medium_removal is the
same.

And since now we are upping the refcnt we keep the request queue around it
was kind of expected that it won't oops on the request queue.

>
> At the beginning of it too. You can't see the "Code: " line in the
> pictures, but the only dereference I see there is "sdev" itself being
> NULL. And %cr2 is 0x640, which would seem to agree (the 'sdev'
> structure is absolutely disgustingly big, and sdev->sdev_state is
> indeed at an offset in that region.
>
> That 'sdev' comes from scsi_prep_fn() doing
>
> struct scsi_device *sdev = q->queuedata;
>
> is that queuedata perhaps cleared even if the queue itself stays
> around due to refcounts?

So now the issue is with scsi_device refcnt?

Parag

2011-05-25 23:53:35

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, May 25, 2011 at 4:45 PM, Parag Warudkar <[email protected]> wrote:
>
> So now the issue is with scsi_device refcnt?

No, I'd guess that the problem is just that __scsi_remove_device()
sets queuedata to NULL. Afaik, both sdev and the queue actually stay
around.

That said, I don't have very high opinions of some of the SCSI code.
And that's me being polite. The thing is a overcomplicated mess of
indirection and pointers back-and-forth. The refcounting is crazy
and/or impossible, since the pointers really do go back-and-forth.

Linus

2011-05-26 01:01:38

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, May 25, 2011 at 5:35 PM, James Bottomley
<[email protected]> wrote:
>
> Jens, can you look at this, please ... would you prefer a separate DEAD
> test instead? ?Incidentally, this is in addition to the other DEAD test
> you still haven't applied ...

Ugh.

Can you please just always set STOPPED at the same time you set DEAD?

Linus

2011-05-26 01:06:11

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, 2011-05-25 at 18:01 -0700, Linus Torvalds wrote:
> On Wed, May 25, 2011 at 5:35 PM, James Bottomley
> <[email protected]> wrote: > > Jens, can you look
> at this, please ... would you prefer a separate DEAD > test instead?
> Incidentally, this is in addition to the other DEAD test > you still
> haven't applied ...
>
> Ugh.
>
> Can you please just always set STOPPED at the same time you set DEAD?

Well, Jens wondered the same thing in this thread:

http://marc.info/?l=linux-scsi&m=130632292705404

It's just an RFC patch to see if I can fix an oops ... how it's
ultimately implemented will be up to Jens.

James

2011-05-26 01:43:36

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, 25 May 2011, James Bottomley wrote:

> so, hopefully, applying this on top will now fix it.
>

Yeah seems to be OK with that patch on top of the previous one - thanks.

Now I am left with 2 more unrelated suspend/resume issues to report.

Parag

2011-05-27 03:53:10

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Wed, 2011-05-25 at 15:52 -0500, James Bottomley wrote:
> On Wed, 2011-05-25 at 16:42 -0400, Parag Warudkar wrote:
> > Yeah - that makes sense. By that logic, looks like we can only disallow
> > for SDEV_DEL (if we decide to do that check here).
>
> I don't think this is the root cause. I think q is non-NULL but has
> already been released, so we're just getting a NULL deref on the actual
> list head.
>
> Does this fix it? It adds the refcounting at approximately the correct
> places. Of course, we'll now be trying elevator operations on an extant
> queue but one whose elevator functions have been destroyed, so there are
> probably additional state guards to place.
>
> James
>
> ---
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 58584dc..44e8ca3 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -297,7 +297,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
> kfree(sdev);
> goto out;
> }
> -
> + blk_get_queue(sdev->request_queue);

Apparently we can't do this because blk_[put|get]_queue aren't exported
for use in modules (and SCSI can be modular), so this caused a build
failure in linux-next.

James

2011-05-27 05:43:42

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On 2011-05-27 05:53, James Bottomley wrote:
> On Wed, 2011-05-25 at 15:52 -0500, James Bottomley wrote:
>> On Wed, 2011-05-25 at 16:42 -0400, Parag Warudkar wrote:
>>> Yeah - that makes sense. By that logic, looks like we can only disallow
>>> for SDEV_DEL (if we decide to do that check here).
>>
>> I don't think this is the root cause. I think q is non-NULL but has
>> already been released, so we're just getting a NULL deref on the actual
>> list head.
>>
>> Does this fix it? It adds the refcounting at approximately the correct
>> places. Of course, we'll now be trying elevator operations on an extant
>> queue but one whose elevator functions have been destroyed, so there are
>> probably additional state guards to place.
>>
>> James
>>
>> ---
>>
>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>> index 58584dc..44e8ca3 100644
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -297,7 +297,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
>> kfree(sdev);
>> goto out;
>> }
>> -
>> + blk_get_queue(sdev->request_queue);
>
> Apparently we can't do this because blk_[put|get]_queue aren't exported
> for use in modules (and SCSI can be modular), so this caused a build
> failure in linux-next.

I'll just export them.

--
Jens Axboe

2011-05-27 20:21:25

[permalink] [raw]

Subject: Re: [PATCH] SCSI IOCTL: Check for device deletion [was Re: __elv_add_request OOPS]

On Fri, 2011-05-27 at 07:43 +0200, Jens Axboe wrote:
> On 2011-05-27 05:53, James Bottomley wrote:
> > On Wed, 2011-05-25 at 15:52 -0500, James Bottomley wrote:
> >> On Wed, 2011-05-25 at 16:42 -0400, Parag Warudkar wrote:
> >>> Yeah - that makes sense. By that logic, looks like we can only disallow
> >>> for SDEV_DEL (if we decide to do that check here).
> >>
> >> I don't think this is the root cause. I think q is non-NULL but has
> >> already been released, so we're just getting a NULL deref on the actual
> >> list head.
> >>
> >> Does this fix it? It adds the refcounting at approximately the correct
> >> places. Of course, we'll now be trying elevator operations on an extant
> >> queue but one whose elevator functions have been destroyed, so there are
> >> probably additional state guards to place.
> >>
> >> James
> >>
> >> ---
> >>
> >> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> >> index 58584dc..44e8ca3 100644
> >> --- a/drivers/scsi/scsi_scan.c
> >> +++ b/drivers/scsi/scsi_scan.c
> >> @@ -297,7 +297,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
> >> kfree(sdev);
> >> goto out;
> >> }
> >> -
> >> + blk_get_queue(sdev->request_queue);
> >
> > Apparently we can't do this because blk_[put|get]_queue aren't exported
> > for use in modules (and SCSI can be modular), so this caused a build
> > failure in linux-next.
>
> I'll just export them.

OK, if you're fine with that, how does this patch look?

I can take it through my rc-fixes tree (with your ack), since I'm a bit
late on the merge window now.

James

---

From: James Bottomley <[email protected]>
Subject: [PATCH] [SCSI] Fix oops caused by queue refcounting failure

In certain circumstances, we can get an oops from a torn down device.
Most notably this is from CD roms trying to call scsi_ioctl. The root
cause of the problem is the fact that after scsi_remove_device() has
been called, the queue is fully torn down. This is actually wrong
since the queue can be used until the sdev release function is called.
Therefore, we add an extra reference to the queue which is released in
sdev->release, so the queue always exists.

Also export blk_queue_get/blk_queue_put for the refcounting.

Reported-by: Parag Warudkar <[email protected]>
Signed-off-by: James Bottomley <[email protected]>

diff --git a/block/blk-core.c b/block/blk-core.c
index 3fe00a1..1aa776c 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -345,6 +345,7 @@ void blk_put_queue(struct request_queue *q)
{
kobject_put(&q->kobj);
}
+EXPORT_SYMBOL_GPL(blk_put_queue);

/*
* Note: If a driver supplied the queue lock, it should not zap that lock
@@ -566,6 +567,7 @@ int blk_get_queue(struct request_queue *q)

return 1;
}
+EXPORT_SYMBOL_GPL(blk_get_queue);

static inline void blk_free_request(struct request_queue *q, struct request *rq)
{
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 58584dc..44e8ca3 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -297,7 +297,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
kfree(sdev);
goto out;
}
-
+ blk_get_queue(sdev->request_queue);
sdev->request_queue->queuedata = sdev;
scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index e639125..e0bd3f7 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -322,6 +322,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
kfree(evt);
}

+ blk_put_queue(sdev->request_queue);
/* NULL queue means the device can't be used */
sdev->request_queue = NULL;

2011-05-28 12:42:28