On Wed, 2017-11-08 at 09:15 +0800, Ming Lei wrote: > On Wed, Nov 08, 2017 at 01:06:44AM +0000, Bart Van Assche wrote: > > > > On Wed, 2017-11-08 at 08:59 +0800, Ming Lei wrote: > > > > > > On Tue, Nov 07, 2017 at 04:13:48PM +0000, Bart Van Assche wrote: > > > > > > > > On Tue, 2017-11-07 at 23:21 +0800, Ming Lei wrote: > > > > > > > > > > diff --git a/drivers/scsi/scsi_debugfs.c > > > > > b/drivers/scsi/scsi_debugfs.c > > > > > index 5e9755008aed..7a50878446b4 100644 > > > > > --- a/drivers/scsi/scsi_debugfs.c > > > > > +++ b/drivers/scsi/scsi_debugfs.c > > > > > @@ -9,7 +9,10 @@ void scsi_show_rq(struct seq_file *m, struct > > > > > request *rq) > > > > > int msecs = jiffies_to_msecs(jiffies - cmd- > > > > > >jiffies_at_alloc); > > > > > char buf[80]; > > > > > > > > > > - __scsi_format_command(buf, sizeof(buf), cmd->cmnd, > > > > > cmd->cmd_len); > > > > > + if (cmd->cmnd == scsi_req(rq)->cmd) > > > > > + __scsi_format_command(buf, sizeof(buf), cmd- > > > > > >cmnd, cmd->cmd_len); > > > > > + else > > > > > + strcpy(buf, "unknown"); > > > > > seq_printf(m, ", .cmd=%s, .retries=%d, allocated > > > > > %d.%03d s ago", buf, > > > > > cmd->retries, msecs / 1000, msecs % > > > > > 1000); > > > > > } > > > > > > > > This change introduces a new bug, namely that "unknown" will > > > > appear in the debugfs output if (cmd->cmnd != scsi_req(rq)- > > > > >cmd). Have you considered to use > > > > > > Because there isn't reliable way to get the command safely, and I > > > don't think it is a new bug. > > > > > > > > > > > the test (cmd->cmnd != NULL) instead? > > > > > > No, that is worse, because you may cause use-after-free if you > > > read a freed buffer. > > > > It seems like your operating mode is still to contradict all > > feedback you get instead of trying to address the feedback you get? > > > > Anyway, this is a debugging facility so I'm not convinced that > > avoiding a (very sporadic) use-after-free in this code is better > > than omitting very useful output. > > OK, if no one objects the use-after-free, because this way may > trigger warning from some utility, such as KASAN. Good grief, this list is supposed to be for technical discussions not kindergarten playground squabbles; could you both please act your age? The patch as proposed would lose us all information about PI commands, hence the objection. For the concern about use after free on the NULL check, what about modifying sd_uninit_command() to NULL SCpnt->cmnd before it calls mempool_free()? That will likely eliminate the race window for printing the command. James
