Re: [PATCH 1/3] Remove two cancel_delayed_work() calls from the error handler

[James Bottomley <jbottomley@parallels.com>]

On Tue, 2014-05-27 at 10:06 +0200, Bart Van Assche wrote:
> On 05/26/14 17:23, Paolo Bonzini wrote:
> > Il 26/05/2014 17:14, Bart Van Assche ha scritto:
> >> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> >> index 88d46fe..c972eab 100644
> >> --- a/drivers/scsi/scsi.c
> >> +++ b/drivers/scsi/scsi.c
> >> @@ -334,7 +334,7 @@ void scsi_put_command(struct scsi_cmnd *cmd)
> >>      list_del_init(&cmd->list);
> >>      spin_unlock_irqrestore(&cmd->device->list_lock, flags);
> >>
> >> -    cancel_delayed_work(&cmd->abort_work);
> >> +    WARN_ON_ONCE(delayed_work_pending(&cmd->abort_work));
> >>
> >>      __scsi_put_command(cmd->device->host, cmd);
> >>  }
> >> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> >> index f17aa7a..5232583 100644
> >> --- a/drivers/scsi/scsi_error.c
> >> +++ b/drivers/scsi/scsi_error.c
> >> @@ -193,7 +193,7 @@ scsi_abort_command(struct scsi_cmnd *scmd)
> >>          SCSI_LOG_ERROR_RECOVERY(3,
> >>              scmd_printk(KERN_INFO, scmd,
> >>                      "scmd %p previous abort failed\n", scmd));
> >> -        cancel_delayed_work(&scmd->abort_work);
> >> +        WARN_ON_ONCE(delayed_work_pending(&scmd->abort_work));
> >>          return FAILED;
> >>      }
> >>
> >>
> > 
> > I still prefer a BUG_ON in scsi_put_command, but anyway: series
> > 
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Hello Paolo,
> 
> As you probably know scsi_put_command() can get called from softirq
> context. A BUG_ON() in that context might make it unnecessary hard for a
> user to collect call traces.

Why?  The messages dumped are the same, the trace just starts from the
IRQ context ... I don't see what the problem is.

The question isn't ease of gathering the data, it's correctness.  The
point is that if the assert fails we have a free of an in-use command
leading to a nasty use after free ... the machine state is hosed at that
point.

James