| Message ID | 20170202005853.23456-17-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Feb 01, 2017 at 04:58:33PM -0800, Bart Van Assche wrote: > Stop execution in the unlikely scenario that CMD_T_STOP has been > set for a command just after the command has been added to the > device command list and before .write_pending() is called. How could this happen? -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2017-02-06 at 01:14 -0800, Christoph Hellwig wrote: > On Wed, Feb 01, 2017 at 04:58:33PM -0800, Bart Van Assche wrote: > > Stop execution in the unlikely scenario that CMD_T_STOP has been > > set for a command just after the command has been added to the > > device command list and before .write_pending() is called. > > How could this happen? Hello Christoph, Since this call of transport_cmd_check_stop() occurs before command execution starts and hence before CMD_T_ACTIVE is set __transport_wait_for_tasks() cannot yet have set CMD_T_STOP. If you want I can drop this patch. Bart.-- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 06, 2017 at 05:29:30PM +0000, Bart Van Assche wrote: > On Mon, 2017-02-06 at 01:14 -0800, Christoph Hellwig wrote: > > On Wed, Feb 01, 2017 at 04:58:33PM -0800, Bart Van Assche wrote: > > > Stop execution in the unlikely scenario that CMD_T_STOP has been > > > set for a command just after the command has been added to the > > > device command list and before .write_pending() is called. > > > > How could this happen? > > Hello Christoph, > > Since this call of transport_cmd_check_stop() occurs before command > execution starts and hence before CMD_T_ACTIVE is set > __transport_wait_for_tasks() cannot yet have set CMD_T_STOP. If you > want I can drop this patch. I think checking return values is always a good thing, so I like this patch in principle. Just my detector for how could this obvious bug ever be unnoticed was tripped off by it. So maybe just add a blurb that this is currently impossible, but checking is still sensible to the patch description. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2017-02-01 at 16:58 -0800, Bart Van Assche wrote: > Stop execution in the unlikely scenario that CMD_T_STOP has been > set for a command just after the command has been added to the > device command list and before .write_pending() is called. > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Reviewed-by: Hannes Reinecke <hare@suse.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Andy Grover <agrover@redhat.com> > Cc: David Disseldorp <ddiss@suse.de> > --- > drivers/target/target_core_transport.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 782b511c4f5f..d241c4d27352 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -2456,7 +2456,8 @@ transport_generic_new_cmd(struct se_cmd *cmd) > target_execute_cmd(cmd); > return 0; > } > - transport_cmd_check_stop(cmd, false, true); > + if (transport_cmd_check_stop(cmd, false, true)) > + return 0; > > ret = cmd->se_tfo->write_pending(cmd); > if (ret == -EAGAIN || ret == -ENOMEM) Nice catch on this btw. I've not seen folks being able to hit this, which it could certainly result in unexpected behavior if the CMD_T_STOP check was true in transport_cmd_check_stop() -> complete_all(&cmd->t_transport_stop_comp) happened, but this caller did not return and ->write_pending() was invoked after CMD_T_STOP. That said, it might be a good candidate for stable. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 782b511c4f5f..d241c4d27352 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -2456,7 +2456,8 @@ transport_generic_new_cmd(struct se_cmd *cmd) target_execute_cmd(cmd); return 0; } - transport_cmd_check_stop(cmd, false, true); + if (transport_cmd_check_stop(cmd, false, true)) + return 0; ret = cmd->se_tfo->write_pending(cmd); if (ret == -EAGAIN || ret == -ENOMEM)