Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()
On Mon, Jul 16, 2012 at 6:56 PM, Nicholas A. Bellinger n...@linux-iscsi.org wrote: Do you have a plan for how to handle this? Do we really want to plumb through another callback to tell the fabric driver to free the command in this case? I need to think more about this ahead of changing it back again for-3.6 now that other fabrics have the assumption of target_submit_cmd() would not fail. There is a clear case with qla2xxx for just changing it back (we might end up doing that just yet) but I wanted to get the other important bits into for-next into place first.. Sleeping on things, I now feel pretty strongly that having target_submit_cmd return an error value for immediate errors where the command does not make it into the target core is the right approach. Freeing commands is already one of the most confusing and complex parts of the target code, with check_stop_free, cmd_wait_comp and SCF_ACK_KREF. Adding another code flow back to the fabric driver with yet another set of semantics around freeing a command seems like it's making things even harder to maintain. On the other hand, every caller of target_submit_cmd() in the tree already has an error path where it handles problems it detects right before calling target_submit_cmd(), and so it's trivial to share that for problems detected inside target_submit_cmd(). If you look at patch 4/7, pretty much the only changes are like going from target_submit_cmd(...); to if (target_submit_cmd(...)) goto err; and in fact the -handle_cmd() wrapper that qla_target uses to call target_submit_cmd via tcm_qla2xxx already has a return value that qla_target handled properly! I guess another approach would be to split target_submit_cmd() into the target_get_sess_cmd() part and the rest of it, and have fabric drivers handle errors queueing commands but not have to worry about the submit cmd part failing. But I'm not sure that's any better. Andy, any feelings about this one way or another? Christoph? Thanks, Roland -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()
On Tue, Jul 17, 2012 at 09:34:49AM -0700, Roland Dreier wrote: Sleeping on things, I now feel pretty strongly that having target_submit_cmd return an error value for immediate errors where the command does not make it into the target core is the right approach. I think it is. When I tried to convert other drivers to target_submit_cmd while doing the target processing thread removal that would have simplified a lot of things. Freeing commands is already one of the most confusing and complex parts of the target code, with check_stop_free, cmd_wait_comp and SCF_ACK_KREF. Adding another code flow back to the fabric driver with yet another set of semantics around freeing a command seems like it's making things even harder to maintain. True. And the above mess really needs to be simplified, too. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()
On 07/17/2012 09:39 AM, Christoph Hellwig wrote: On Tue, Jul 17, 2012 at 09:34:49AM -0700, Roland Dreier wrote: Sleeping on things, I now feel pretty strongly that having target_submit_cmd return an error value for immediate errors where the command does not make it into the target core is the right approach. I think it is. When I tried to convert other drivers to target_submit_cmd while doing the target processing thread removal that would have simplified a lot of things. Freeing commands is already one of the most confusing and complex parts of the target code, with check_stop_free, cmd_wait_comp and SCF_ACK_KREF. Adding another code flow back to the fabric driver with yet another set of semantics around freeing a command seems like it's making things even harder to maintain. True. And the above mess really needs to be simplified, too. I'm ok with submit_cmd returning a value for now. Maybe in the end it doesn't, but getting the code working comes first. This is one of those areas that needs a complete rewrite, so who cares if there's some dirt on the floor when the whole joint is due for remodeling. -- Andy -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()
On Tue, 2012-07-17 at 09:34 -0700, Roland Dreier wrote: On Mon, Jul 16, 2012 at 6:56 PM, Nicholas A. Bellinger n...@linux-iscsi.org wrote: Do you have a plan for how to handle this? Do we really want to plumb through another callback to tell the fabric driver to free the command in this case? I need to think more about this ahead of changing it back again for-3.6 now that other fabrics have the assumption of target_submit_cmd() would not fail. There is a clear case with qla2xxx for just changing it back (we might end up doing that just yet) but I wanted to get the other important bits into for-next into place first.. Sleeping on things, I now feel pretty strongly that having target_submit_cmd return an error value for immediate errors where the command does not make it into the target core is the right approach. Freeing commands is already one of the most confusing and complex parts of the target code, with check_stop_free, cmd_wait_comp and SCF_ACK_KREF. Adding another code flow back to the fabric driver with yet another set of semantics around freeing a command seems like it's making things even harder to maintain. I wasn't talking about adding another release path. I was thinking more about a TFO call to optionally abort HW/SW exchange if we can't accept the command in question. Not to mention, this could end up being useful for other purposes beyond the initial target_submit_cmd() failure case due to active I/O shutdown. (aborts + active I/O shutdown with active SRP WRITEs with ib_srpt come to mind needing something like this..) On the other hand, every caller of target_submit_cmd() in the tree already has an error path where it handles problems it detects right before calling target_submit_cmd(), and so it's trivial to share that for problems detected inside target_submit_cmd(). If you look at patch 4/7, pretty much the only changes are like going from target_submit_cmd(...); to if (target_submit_cmd(...)) goto err; and in fact the -handle_cmd() wrapper that qla_target uses to call target_submit_cmd via tcm_qla2xxx already has a return value that qla_target handled properly! Sure, there is no doubting tcm_qla2xxx's usage of this return value back into qla_target.c code.. I guess another approach would be to split target_submit_cmd() into the target_get_sess_cmd() part and the rest of it, and have fabric drivers handle errors queueing commands but not have to worry about the submit cmd part failing. But I'm not sure that's any better. Andy, any feelings about this one way or another? Christoph? Ok, so for-3.6 it makes the most sense to just convert this back to propagate up the return code, but only for target_get_sess_cmd() failure case.. That said, I'll go ahead and respin patch #4 on top of for-next, and post the updated patch shortly with the same CC's and ask for ACKs from the necessary folks. --nab -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()
On Mon, 2012-07-16 at 11:04 -0700, Roland Dreier wrote:
From: Roland Dreier rol...@purestorage.com
Target core code assumes that target_splice_sess_cmd_list() has set
sess_tearing_down and moved the list of pending commands to
sess_wait_list, no more commands will be added to the session; if any
are added, nothing keeps the se_session from being freed while the
command is still in flight, which e.g. leads to use-after-free of
se_cmd-se_sess in target_release_cmd_kref().
To enforce this invariant, put a check of sess_tearing_down inside of
sess_cmd_lock in target_get_sess_cmd(); any checks before this are
racy and can lead to the use-after-free described above. For example,
the qla_target check in qlt_do_work() checks sess_tearing_down from
work thread context but then drops all locks before calling
target_submit_cmd() (as it must, since that is a sleeping function).
However, since no locks are held, anything can happen with respect to
the session it has looked up -- although it does correctly get
sess_kref within its lock, so the memory won't be freed while
target_submit_cmd() is actually running, nothing stops eg an ACL from
being dropped and calling -shutdown_session() (which calls into
target_splice_sess_cmd_list()) before we get to target_get_sess_cmd().
Once this happens, the se_session memory can be freed as soon as
target_submit_cmd() returns and qlt_do_work() drops its reference,
even though we've just added a command to sess_cmd_list.
To prevent this use-after-free, check sess_tearing_down inside of
sess_cmd_lock right before target_get_sess_cmd() adds a command to
sess_cmd_list; this is synchronized with target_splice_sess_cmd_list()
so that every command is either waited for or not added to the queue.
Signed-off-by: Roland Dreier rol...@purestorage.com
---
drivers/target/target_core_transport.c | 24 +++-
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/target/target_core_transport.c
b/drivers/target/target_core_transport.c
index 34ca821..23e6e2d 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -73,7 +73,7 @@ static void transport_complete_task_attr(struct se_cmd
*cmd);
static void transport_handle_queue_full(struct se_cmd *cmd,
struct se_device *dev);
static int transport_generic_get_mem(struct se_cmd *cmd);
-static void target_get_sess_cmd(struct se_session *, struct se_cmd *, bool);
+static int target_get_sess_cmd(struct se_session *, struct se_cmd *, bool);
static void transport_put_cmd(struct se_cmd *cmd);
static void transport_remove_cmd_from_queue(struct se_cmd *cmd);
static int transport_set_sense_codes(struct se_cmd *cmd, u8 asc, u8 ascq);
@@ -1570,7 +1570,9 @@ int target_submit_cmd(struct se_cmd *se_cmd, struct
se_session *se_sess,
* for fabrics using TARGET_SCF_ACK_KREF that expect a second
* kref_put() to happen during fabric packet acknowledgement.
*/
- target_get_sess_cmd(se_sess, se_cmd, (flags TARGET_SCF_ACK_KREF));
+ rc = target_get_sess_cmd(se_sess, se_cmd, (flags
TARGET_SCF_ACK_KREF));
+ if (rc)
+ return rc;
OK, I see where you where going with the target_submit_cmd() failure
here now..
However, I'm still leaning towards a way to do this for tcm_qla2xxx that
does not require all fabric callers to handle target_submit_cmd()
exceptions directly..
How about a special target_get_sess_cmd() failure callback to free
se_cmd when target_get_sess_cmd() fails, but not actually send a SCSI
response back out to the fabric here during session shutdown..?
That said, I'm going to commit this patch (slightly changed to keep
target_submit_cmd() returning void for now) but I'd still like a better
way of handling this particular failure without propagating up the
return value.
/*
* Signal bidirectional data payloads to target-core
*/
@@ -1664,7 +1666,11 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct
se_session *se_sess,
se_cmd-se_tmr_req-ref_task_tag = tag;
/* See target_submit_cmd for commentary */
- target_get_sess_cmd(se_sess, se_cmd, (flags TARGET_SCF_ACK_KREF));
+ ret = target_get_sess_cmd(se_sess, se_cmd, (flags
TARGET_SCF_ACK_KREF));
+ if (ret) {
+ core_tmr_release_req(se_cmd-se_tmr_req);
+ return ret;
+ }
ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
if (ret) {
@@ -3650,10 +3656,11 @@ EXPORT_SYMBOL(transport_generic_free_cmd);
* @se_cmd: command descriptor to add
* @ack_kref:Signal that fabric will perform an ack
target_put_sess_cmd()
*/
-static void target_get_sess_cmd(struct se_session *se_sess, struct se_cmd
*se_cmd,
- bool ack_kref)
+static int target_get_sess_cmd(struct se_session *se_sess, struct se_cmd
*se_cmd,
+bool ack_kref)
{
Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()
On Mon, Jul 16, 2012 at 4:08 PM, Nicholas A. Bellinger n...@linux-iscsi.org wrote: However, I'm still leaning towards a way to do this for tcm_qla2xxx that does not require all fabric callers to handle target_submit_cmd() exceptions directly.. How about a special target_get_sess_cmd() failure callback to free se_cmd when target_get_sess_cmd() fails, but not actually send a SCSI response back out to the fabric here during session shutdown..? I guess that's OK, although I'm not sure it ends up being cleaner. If you look at my 4/7 patch, you see that all target_submit_cmd callers have an error path where they handle terminating commands that fail processing just before calling submit_cmd anyway. eg for qla2xxx we'll need a callback to call qlt_send_term_exchange() in addition to the error path call that qlt_do_work() has anyway. Similarly tcm_fc ends up with a second call to ft_send_resp_code_and_free(). etc. But I don't really have a strong feeling about this, if the view is that submit_cmd() should never return failure directly then I'm OK with that. That said, I'm going to commit this patch (slightly changed to keep target_submit_cmd() returning void for now) but I'd still like a better way of handling this particular failure without propagating up the return value. OK, I'll take a look at how you handle this... - R. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()
OK, I'll take a look at how you handle this... So looking at commit bc187ea6c3b3 in the tree you just pushed out (target: Check sess_tearing_down in target_get_sess_cmd()) it looks like you just return from target_submit_cmd() if we fail to add the command to sess_cmd_list -- doesn't this mean we just leak those commands and possibly leave the HW sitting there with open exchanges? Do you have a plan for how to handle this? Do we really want to plumb through another callback to tell the fabric driver to free the command in this case? - R. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()
On Mon, 2012-07-16 at 18:40 -0700, Roland Dreier wrote: OK, I'll take a look at how you handle this... So looking at commit bc187ea6c3b3 in the tree you just pushed out (target: Check sess_tearing_down in target_get_sess_cmd()) it looks like you just return from target_submit_cmd() if we fail to add the command to sess_cmd_list -- doesn't this mean we just leak those commands and possibly leave the HW sitting there with open exchanges? With dropping patch #5 for now, I assume that would be the case.. Do you have a plan for how to handle this? Do we really want to plumb through another callback to tell the fabric driver to free the command in this case? I need to think more about this ahead of changing it back again for-3.6 now that other fabrics have the assumption of target_submit_cmd() would not fail. There is a clear case with qla2xxx for just changing it back (we might end up doing that just yet) but I wanted to get the other important bits into for-next into place first.. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()
On Mon, 2012-07-16 at 18:56 -0700, Nicholas A. Bellinger wrote: On Mon, 2012-07-16 at 18:40 -0700, Roland Dreier wrote: OK, I'll take a look at how you handle this... So looking at commit bc187ea6c3b3 in the tree you just pushed out (target: Check sess_tearing_down in target_get_sess_cmd()) it looks like you just return from target_submit_cmd() if we fail to add the command to sess_cmd_list -- doesn't this mean we just leak those commands and possibly leave the HW sitting there with open exchanges? With dropping patch #5 for now, I assume that would be the case.. Or with patch #4 even.. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
