| Message ID | 1515413086-34256-1-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Mon, 2018-01-08 at 13:04 +0100, Hannes Reinecke wrote: > The abort handler might be racing with command completion, so the > task might already be NULL by the time the abort handler is called. Hi, I tried the patch on top of 4.15-rc7 (and without the revert of 90965761). I don't have the NULL pointer deref anymore, but the box timeouts after SCSI init. It's not completely dead, but I don't have any meaningful log from the kernel. Regards,
Hannes, On 2018/1/8 20:04, Hannes Reinecke wrote: > The abort handler might be racing with command completion, so the > task might already be NULL by the time the abort handler is called. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > drivers/scsi/libsas/sas_scsi_host.c | 22 +++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c > index 58476b7..08203fb 100644 > --- a/drivers/scsi/libsas/sas_scsi_host.c > +++ b/drivers/scsi/libsas/sas_scsi_host.c > @@ -486,18 +486,34 @@ static int sas_queue_reset(struct domain_device *dev, int reset_type, > > int sas_eh_abort_handler(struct scsi_cmnd *cmd) > { > - int res; > - struct sas_task *task = TO_SAS_TASK(cmd); > + int res = TMF_RESP_FUNC_COMPLETE; > + struct sas_task *task; > struct Scsi_Host *host = cmd->device->host; > struct sas_internal *i = to_sas_internal(host->transportt); > + struct domain_device *dev = cmd_to_domain_dev(cmd); > + struct sas_ha_struct *ha = dev->port->ha; > + unsigned long flags; > > if (!i->dft->lldd_abort_task) > return FAILED; > > - res = i->dft->lldd_abort_task(task); > + /* Avoid sas_scsi_task_done() interfering */ > + spin_lock_irqsave(&dev->done_lock, flags); > + task = TO_SAS_TASK(cmd); > + if (test_bit(SAS_HA_FROZEN, &ha->state)) { > + res = TMF_RESP_FUNC_FAILED; > + task = NULL; > + } else > + ASSIGN_SAS_TASK(cmd, NULL); > + spin_unlock_irqrestore(&dev->done_lock, flags); > + if (task) > + res = i->dft->lldd_abort_task(task); > if (res == TMF_RESP_FUNC_SUCC || res == TMF_RESP_FUNC_COMPLETE) > return SUCCESS; > > + spin_lock_irqsave(&dev->done_lock, flags); > + ASSIGN_SAS_TASK(cmd, task); > + spin_unlock_irqrestore(&dev->done_lock, flags); Why do you assign task back? As I remember, when this cmd dispatch again, we will create a new task and assign to it again. So should we end this task here? > return FAILED; > } > EXPORT_SYMBOL_GPL(sas_eh_abort_handler); >
On 01/09/2018 05:09 AM, Jason Yan wrote: > Hannes, > > On 2018/1/8 20:04, Hannes Reinecke wrote: >> The abort handler might be racing with command completion, so the >> task might already be NULL by the time the abort handler is called. >> >> Signed-off-by: Hannes Reinecke <hare@suse.com> >> --- >> drivers/scsi/libsas/sas_scsi_host.c | 22 +++++++++++++++++++--- >> 1 file changed, 19 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/scsi/libsas/sas_scsi_host.c >> b/drivers/scsi/libsas/sas_scsi_host.c >> index 58476b7..08203fb 100644 >> --- a/drivers/scsi/libsas/sas_scsi_host.c >> +++ b/drivers/scsi/libsas/sas_scsi_host.c >> @@ -486,18 +486,34 @@ static int sas_queue_reset(struct domain_device >> *dev, int reset_type, >> >> int sas_eh_abort_handler(struct scsi_cmnd *cmd) >> { >> - int res; >> - struct sas_task *task = TO_SAS_TASK(cmd); >> + int res = TMF_RESP_FUNC_COMPLETE; >> + struct sas_task *task; >> struct Scsi_Host *host = cmd->device->host; >> struct sas_internal *i = to_sas_internal(host->transportt); >> + struct domain_device *dev = cmd_to_domain_dev(cmd); >> + struct sas_ha_struct *ha = dev->port->ha; >> + unsigned long flags; >> >> if (!i->dft->lldd_abort_task) >> return FAILED; >> >> - res = i->dft->lldd_abort_task(task); >> + /* Avoid sas_scsi_task_done() interfering */ >> + spin_lock_irqsave(&dev->done_lock, flags); >> + task = TO_SAS_TASK(cmd); >> + if (test_bit(SAS_HA_FROZEN, &ha->state)) { >> + res = TMF_RESP_FUNC_FAILED; >> + task = NULL; >> + } else >> + ASSIGN_SAS_TASK(cmd, NULL); >> + spin_unlock_irqrestore(&dev->done_lock, flags); >> + if (task) >> + res = i->dft->lldd_abort_task(task); >> if (res == TMF_RESP_FUNC_SUCC || res == TMF_RESP_FUNC_COMPLETE) >> return SUCCESS; >> >> + spin_lock_irqsave(&dev->done_lock, flags); >> + ASSIGN_SAS_TASK(cmd, task); >> + spin_unlock_irqrestore(&dev->done_lock, flags); > > Why do you assign task back? As I remember, when this cmd dispatch > again, we will create a new task and assign to it again. So should we > end this task here? > We only will create a new task if we decide to retry the command. But if we return FAILED here the command is not retried but rather the SCSI EH is invoked. Cheers, Hannes
On 2018/1/9 15:34, Hannes Reinecke wrote: > On 01/09/2018 05:09 AM, Jason Yan wrote: >> Hannes, >> >> On 2018/1/8 20:04, Hannes Reinecke wrote: >>> The abort handler might be racing with command completion, so the >>> task might already be NULL by the time the abort handler is called. >>> >>> Signed-off-by: Hannes Reinecke <hare@suse.com> >>> --- >>> drivers/scsi/libsas/sas_scsi_host.c | 22 +++++++++++++++++++--- >>> 1 file changed, 19 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/scsi/libsas/sas_scsi_host.c >>> b/drivers/scsi/libsas/sas_scsi_host.c >>> index 58476b7..08203fb 100644 >>> --- a/drivers/scsi/libsas/sas_scsi_host.c >>> +++ b/drivers/scsi/libsas/sas_scsi_host.c >>> @@ -486,18 +486,34 @@ static int sas_queue_reset(struct domain_device >>> *dev, int reset_type, >>> >>> int sas_eh_abort_handler(struct scsi_cmnd *cmd) >>> { >>> - int res; >>> - struct sas_task *task = TO_SAS_TASK(cmd); >>> + int res = TMF_RESP_FUNC_COMPLETE; >>> + struct sas_task *task; >>> struct Scsi_Host *host = cmd->device->host; >>> struct sas_internal *i = to_sas_internal(host->transportt); >>> + struct domain_device *dev = cmd_to_domain_dev(cmd); >>> + struct sas_ha_struct *ha = dev->port->ha; >>> + unsigned long flags; >>> >>> if (!i->dft->lldd_abort_task) >>> return FAILED; >>> >>> - res = i->dft->lldd_abort_task(task); >>> + /* Avoid sas_scsi_task_done() interfering */ >>> + spin_lock_irqsave(&dev->done_lock, flags); >>> + task = TO_SAS_TASK(cmd); >>> + if (test_bit(SAS_HA_FROZEN, &ha->state)) { >>> + res = TMF_RESP_FUNC_FAILED; >>> + task = NULL; >>> + } else >>> + ASSIGN_SAS_TASK(cmd, NULL); >>> + spin_unlock_irqrestore(&dev->done_lock, flags); >>> + if (task) >>> + res = i->dft->lldd_abort_task(task); >>> if (res == TMF_RESP_FUNC_SUCC || res == TMF_RESP_FUNC_COMPLETE) >>> return SUCCESS; >>> >>> + spin_lock_irqsave(&dev->done_lock, flags); >>> + ASSIGN_SAS_TASK(cmd, task); >>> + spin_unlock_irqrestore(&dev->done_lock, flags); >> >> Why do you assign task back? As I remember, when this cmd dispatch >> again, we will create a new task and assign to it again. So should we >> end this task here? >> > We only will create a new task if we decide to retry the command. > But if we return FAILED here the command is not retried but rather the > SCSI EH is invoked. > I got it. The SCSI EH will handle this. But when we return SUCCESS above, shall we free the task? I don't see that LLDDs free it. Jason > Cheers, > > Hannes >
On 01/09/2018 10:04 AM, Jason Yan wrote: > > On 2018/1/9 15:34, Hannes Reinecke wrote: >> On 01/09/2018 05:09 AM, Jason Yan wrote: >>> Hannes, >>> >>> On 2018/1/8 20:04, Hannes Reinecke wrote: >>>> The abort handler might be racing with command completion, so the >>>> task might already be NULL by the time the abort handler is called. >>>> >>>> Signed-off-by: Hannes Reinecke <hare@suse.com> >>>> --- >>>> drivers/scsi/libsas/sas_scsi_host.c | 22 +++++++++++++++++++--- >>>> 1 file changed, 19 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/scsi/libsas/sas_scsi_host.c >>>> b/drivers/scsi/libsas/sas_scsi_host.c >>>> index 58476b7..08203fb 100644 >>>> --- a/drivers/scsi/libsas/sas_scsi_host.c >>>> +++ b/drivers/scsi/libsas/sas_scsi_host.c >>>> @@ -486,18 +486,34 @@ static int sas_queue_reset(struct domain_device >>>> *dev, int reset_type, >>>> >>>> int sas_eh_abort_handler(struct scsi_cmnd *cmd) >>>> { >>>> - int res; >>>> - struct sas_task *task = TO_SAS_TASK(cmd); >>>> + int res = TMF_RESP_FUNC_COMPLETE; >>>> + struct sas_task *task; >>>> struct Scsi_Host *host = cmd->device->host; >>>> struct sas_internal *i = to_sas_internal(host->transportt); >>>> + struct domain_device *dev = cmd_to_domain_dev(cmd); >>>> + struct sas_ha_struct *ha = dev->port->ha; >>>> + unsigned long flags; >>>> >>>> if (!i->dft->lldd_abort_task) >>>> return FAILED; >>>> >>>> - res = i->dft->lldd_abort_task(task); >>>> + /* Avoid sas_scsi_task_done() interfering */ >>>> + spin_lock_irqsave(&dev->done_lock, flags); >>>> + task = TO_SAS_TASK(cmd); >>>> + if (test_bit(SAS_HA_FROZEN, &ha->state)) { >>>> + res = TMF_RESP_FUNC_FAILED; >>>> + task = NULL; >>>> + } else >>>> + ASSIGN_SAS_TASK(cmd, NULL); >>>> + spin_unlock_irqrestore(&dev->done_lock, flags); >>>> + if (task) >>>> + res = i->dft->lldd_abort_task(task); >>>> if (res == TMF_RESP_FUNC_SUCC || res == TMF_RESP_FUNC_COMPLETE) >>>> return SUCCESS; >>>> >>>> + spin_lock_irqsave(&dev->done_lock, flags); >>>> + ASSIGN_SAS_TASK(cmd, task); >>>> + spin_unlock_irqrestore(&dev->done_lock, flags); >>> >>> Why do you assign task back? As I remember, when this cmd dispatch >>> again, we will create a new task and assign to it again. So should we >>> end this task here? >>> >> We only will create a new task if we decide to retry the command. >> But if we return FAILED here the command is not retried but rather the >> SCSI EH is invoked. >> > > I got it. The SCSI EH will handle this. > > But when we return SUCCESS above, shall we free the task? I don't see > that LLDDs free it. > Hmm. Possibly. I'll check. Cheers, Hannes
On 01/08/2018 06:11 PM, Yves-Alexis Perez wrote: > On Mon, 2018-01-08 at 13:04 +0100, Hannes Reinecke wrote: >> The abort handler might be racing with command completion, so the >> task might already be NULL by the time the abort handler is called. > > Hi, > > I tried the patch on top of 4.15-rc7 (and without the revert of 90965761). I > don't have the NULL pointer deref anymore, but the box timeouts after SCSI > init. It's not completely dead, but I don't have any meaningful log from the > kernel. > Can you try to boot the stock 4.15 kernel (without any patches) with scsi_mod.scsi_logging_level=9411 on the kernel commandline and send me tha output? I really would like to see which command fails. THX. Cheers, Hannes
On Tue, 2018-01-09 at 10:30 +0100, Hannes Reinecke wrote: > Can you try to boot the stock 4.15 kernel (without any patches) with > scsi_mod.scsi_logging_level=9411 > on the kernel commandline and send me tha output? > I really would like to see which command fails. > THX. Here it is: [ 204.960508] sd 0:0:1:0: [sdb] tag#0 Send: scmd 0x000000006f2d047e [ 204.960510] sd 0:0:0:0: [sda] tag#1 Send: scmd 0x000000009840a325 [ 204.960511] sd 0:0:1:0: [sdb] tag#0 CDB: ATA command pass through(16) 85 06 20 00 05 00 fe 00 00 00 00 00 00 40 ef 00 [ 204.960512] sd 0:0:0:0: [sda] tag#1 CDB: ATA command pass through(16) 85 06 20 00 05 00 fe 00 00 00 00 00 00 40 ef 00 [ 204.960583] sd 0:0:1:0: [sdb] tag#0 Done: TIMEOUT_ERROR Result: hostbyte=DID_OK driverbyte=DRIVER_OK [ 204.960586] sd 0:0:1:0: [sdb] tag#0 CDB: ATA command pass through(16) 85 06 20 00 05 00 fe 00 00 00 00 00 00 40 ef 00 [ 204.983681] BUG: unable to handle kernel NULL pointer dereference at (null) [ 204.983690] IP: isci_task_abort_task+0x30/0x4b0 [isci] [ 204.983691] PGD 0 P4D 0 [ 204.983693] Oops: 0000 [#1] SMP PTI [ 204.983695] Modules linked in: snd_hwdep(+) snd_hda_core snd_pcm_oss snd_mixer_oss iTCO_wdt snd_pcm iTCO_vendor_support mei_me snd_timer snd lpc_ich shpchp sg mei joydev evdev mfd_core soundcore dcdbas serio_raw intel_rapl_perf(+) iptable_filter(+) nls_cp437 nls_utf8 vfat fat coretemp dell_smm_hwmon parport_pc ppdev lp parport ip_tables x_tables autofs4 ext4 crc16 mbcache jbd2 fscrypto algif_skcipher af_alg dm_crypt dm_mod tpm_rng rng_core uhci_hcd pcrypt sr_mod cdrom sd_mod hid_lenovo usbhid hid nouveau crct10dif_pclmul crc32_pclmul video isci crc32c_intel mxm_wmi ahci xhci_pci libsas ehci_pci ghash_clmulni_intel wmi libahci scsi_transport_sas xhci_hcd ehci_hcd i2c_algo_bit pcbc drm_kms_helper libata e1000e ttm aesni_intel aes_x86_64 crypto_simd cryptd psmouse ptp glue_helper i2c_i801 usbcore scsi_mod [ 204.983738] pps_core drm button [ 204.983741] CPU: 11 PID: 262 Comm: kworker/u64:5 Not tainted 4.15.0-rc7 #2 [ 204.983742] Hardware name: Dell Inc. Precision T5600/0Y56T3, BIOS A09 05/03/2013 [ 204.983750] Workqueue: scsi_tmf_0 scmd_eh_abort_handler [scsi_mod] [ 204.983755] RIP: 0010:isci_task_abort_task+0x30/0x4b0 [isci] [ 204.983756] RSP: 0018:ffffb0504152fcd8 EFLAGS: 00010246 [ 204.983757] RAX: 0000000000000000 RBX: ffff8a8f842d79a8 RCX: 00000000ffffffff [ 204.983758] RDX: ffff8a8f80f7f800 RSI: 0000000000000000 RDI: 0000000000000000 [ 204.983759] RBP: ffff8a8f842d7948 R08: ffff8a8f870e1f60 R09: 0000000000000000 [ 204.983759] R10: 0000000000000000 R11: 0000000000000f27 R12: 0000000000000008 [ 204.983760] R13: 0000000000000000 R14: ffff8a8f80b51540 R15: 0000000000000000 [ 204.983761] FS: 0000000000000000(0000) GS:ffff8a8f870c0000(0000) knlGS:0000000000000000 [ 204.983774] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 204.983775] CR2: 0000000000000000 CR3: 0000000200a0a005 CR4: 00000000000606e0 [ 204.983775] Call Trace: [ 204.983781] ? sched_clock+0x5/0x10 [ 204.983784] ? sched_clock_cpu+0xc/0xb0 [ 204.983787] ? sched_clock+0x5/0x10 [ 204.983788] ? sched_clock+0x5/0x10 [ 204.983790] ? sched_clock_cpu+0xc/0xb0 [ 204.983791] ? pick_next_task_fair+0x4de/0x5f0 [ 204.983794] ? __switch_to+0xa2/0x450 [ 204.983795] ? put_prev_entity+0x1e/0xe0 [ 204.983799] sas_eh_abort_handler+0x2f/0x50 [libsas] [ 204.983805] scmd_eh_abort_handler+0x56/0x210 [scsi_mod] [ 204.983809] process_one_work+0x188/0x380 [ 204.983811] worker_thread+0x2e/0x390 [ 204.983812] ? process_one_work+0x380/0x380 [ 204.983814] kthread+0x111/0x130 [ 204.983815] ? kthread_create_worker_on_cpu+0x70/0x70 [ 204.983819] ret_from_fork+0x1f/0x30 [ 204.983820] Code: 41 57 41 56 49 89 ff 41 55 41 54 4d 8d 67 08 55 53 48 81 ec 68 01 00 00 65 48 8b 04 25 28 00 00 00 48 89 84 24 60 01 00 00 31 c0 <48> 8b 07 c7 44 24 08 00 00 00 00 c7 44 24 10 00 00 00 00 48 8b [ 204.983843] RIP: isci_task_abort_task+0x30/0x4b0 [isci] RSP: ffffb0504152fcd8 [ 204.983843] CR2: 0000000000000000 [ 204.983845] ---[ end trace c55806a9bed49dc4 ]--- I have the full log from boot (2.2M) but I'm unsure how much private data appears in it so I'd rather not send it to a mailing list. If you need it I'll send it privately. Regards,
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c index 58476b7..08203fb 100644 --- a/drivers/scsi/libsas/sas_scsi_host.c +++ b/drivers/scsi/libsas/sas_scsi_host.c @@ -486,18 +486,34 @@ static int sas_queue_reset(struct domain_device *dev, int reset_type, int sas_eh_abort_handler(struct scsi_cmnd *cmd) { - int res; - struct sas_task *task = TO_SAS_TASK(cmd); + int res = TMF_RESP_FUNC_COMPLETE; + struct sas_task *task; struct Scsi_Host *host = cmd->device->host; struct sas_internal *i = to_sas_internal(host->transportt); + struct domain_device *dev = cmd_to_domain_dev(cmd); + struct sas_ha_struct *ha = dev->port->ha; + unsigned long flags; if (!i->dft->lldd_abort_task) return FAILED; - res = i->dft->lldd_abort_task(task); + /* Avoid sas_scsi_task_done() interfering */ + spin_lock_irqsave(&dev->done_lock, flags); + task = TO_SAS_TASK(cmd); + if (test_bit(SAS_HA_FROZEN, &ha->state)) { + res = TMF_RESP_FUNC_FAILED; + task = NULL; + } else + ASSIGN_SAS_TASK(cmd, NULL); + spin_unlock_irqrestore(&dev->done_lock, flags); + if (task) + res = i->dft->lldd_abort_task(task); if (res == TMF_RESP_FUNC_SUCC || res == TMF_RESP_FUNC_COMPLETE) return SUCCESS; + spin_lock_irqsave(&dev->done_lock, flags); + ASSIGN_SAS_TASK(cmd, task); + spin_unlock_irqrestore(&dev->done_lock, flags); return FAILED; } EXPORT_SYMBOL_GPL(sas_eh_abort_handler);
The abort handler might be racing with command completion, so the task might already be NULL by the time the abort handler is called. Signed-off-by: Hannes Reinecke <hare@suse.com> --- drivers/scsi/libsas/sas_scsi_host.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)