[v2,24/25] usb: gadget: f_tcm: Check overlapped command

Commit Message

Thinh Nguyen July 19, 2022, 1:28 a.m. UTC

If there's an overlapped command tag, cancel the command and respond
with RC_OVERLAPPED_TAG to host.

Signed-off-by: Thinh Nguyen <[e-mail]>
---
 Changes in v2:
 - New patch. It was part of TASK MANAGEMENT change.
 - Directly respond to host on overlapped tag instead of reporting to target
   core.

 drivers/usb/gadget/function/f_tcm.c | 75 ++++++++++++++++++++++++++++-
 1 file changed, 73 insertions(+), 2 deletions(-)

Comments

Sebastian Andrzej Siewior Aug. 26, 2022, 10:46 a.m. UTC | #1

On 2022-07-18 18:28:34 [-0700], Thinh Nguyen wrote:
> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index 8b99ee07df87..dfa3e7c043a3 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -1214,6 +1233,51 @@ static void usbg_cmd_work(struct work_struct *work)
>  	return;
>  
>  skip:
> +	if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
> +		struct se_session *se_sess;
> +		struct uas_stream *stream;
> +
> +		se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
> +		stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag);
> +
> +		/*
> +		 * There's no guarantee of a matching completion order between
> +		 * different endpoints. i.e. The device may receive a new (CDB)
> +		 * command request completion of the command endpoint before it
> +		 * gets notified of the previous command status completion from
> +		 * a status endpoint. The driver still needs to detect
> +		 * misbehaving host and respond with an overlap command tag. To
> +		 * prevent false overlapped tag failure, give the active and
> +		 * matching stream id a short time (1ms) to complete before
> +		 * respond with overlapped command failure.
> +		 */
> +		msleep(1);

How likely is it for this to happen? Is there maybe some synchronisation
missing? If I see this right, in order to get here, you will already
spill the message "Command tag %d overlapped" which does not look good.
Why should the host re-use a tag which did not yet complete?

Sebastian

Thinh Nguyen Aug. 26, 2022, 7:27 p.m. UTC | #2

On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-07-18 18:28:34 [-0700], Thinh Nguyen wrote:
> > diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> > index 8b99ee07df87..dfa3e7c043a3 100644
> > --- a/drivers/usb/gadget/function/f_tcm.c
> > +++ b/drivers/usb/gadget/function/f_tcm.c
> > @@ -1214,6 +1233,51 @@ static void usbg_cmd_work(struct work_struct *work)
> >  	return;
> >  
> >  skip:
> > +	if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
> > +		struct se_session *se_sess;
> > +		struct uas_stream *stream;
> > +
> > +		se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
> > +		stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag);
> > +
> > +		/*
> > +		 * There's no guarantee of a matching completion order between
> > +		 * different endpoints. i.e. The device may receive a new (CDB)
> > +		 * command request completion of the command endpoint before it
> > +		 * gets notified of the previous command status completion from
> > +		 * a status endpoint. The driver still needs to detect
> > +		 * misbehaving host and respond with an overlap command tag. To
> > +		 * prevent false overlapped tag failure, give the active and
> > +		 * matching stream id a short time (1ms) to complete before
> > +		 * respond with overlapped command failure.
> > +		 */
> > +		msleep(1);
> 
> How likely is it for this to happen? Is there maybe some synchronisation
> missing? If I see this right, in order to get here, you will already
> spill the message "Command tag %d overlapped" which does not look good.
> Why should the host re-use a tag which did not yet complete?
> 

Not often, but it can happen. For example, even though the device sent a
status on the wire and the host received it. The host may send a new
command with the same tag before the device is notified of the previous
command completion. Since they operate in different endpoints, the
device driver may get notification of the new command from the command
endpoint before the status completion of the status endpoint.

This is an attempt to maintain synchronization and prevent false overlap
check. It's a quick fix. I feel that we can handle this better. If you
can think of a better way to handle this, let me know.

BR,
Thinh

Patch

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 8b99ee07df87..dfa3e7c043a3 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -692,6 +692,15 @@  static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
 		break;
 
 	case UASP_QUEUE_COMMAND:
+		/*
+		 * Overlapped command detected and cancelled.
+		 * So send overlapped attempted status.
+		 */
+		if (cmd->tmr_rsp == RC_OVERLAPPED_TAG &&
+		    req->status == -ECONNRESET) {
+			uasp_send_tm_response(cmd);
+			return;
+		}
 
 		stream->cmd = NULL;
 
@@ -700,7 +709,8 @@  static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
 		 * bitmap index. This is for the cases where f_tcm handles
 		 * status response instead of the target core.
 		 */
-		if (cmd->tmr_rsp != RC_RESPONSE_UNKNOWN) {
+		if (cmd->tmr_rsp != RC_OVERLAPPED_TAG &&
+		    cmd->tmr_rsp != RC_RESPONSE_UNKNOWN) {
 			struct se_session *se_sess;
 
 			se_sess = fu->tpg->tpg_nexus->tvn_se_sess;
@@ -1080,6 +1090,14 @@  static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
 
 cleanup:
 	target_put_sess_cmd(se_cmd);
+
+	/* Command was aborted due to overlapped tag */
+	if (cmd->state == UASP_QUEUE_COMMAND &&
+	    cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
+		uasp_send_tm_response(cmd);
+		return;
+	}
+
 	transport_send_check_condition_and_sense(se_cmd,
 			TCM_CHECK_CONDITION_ABORT_CMD, 0);
 }
@@ -1148,9 +1166,10 @@  static int usbg_send_read_response(struct se_cmd *se_cmd)
 		return uasp_send_read_response(cmd);
 }
 
+static void usbg_aborted_task(struct se_cmd *se_cmd);
+
 static void usbg_submit_tmr(struct usbg_cmd *cmd)
 {
-	struct usbg_cmd *cmd = container_of(work, struct usbg_cmd, work);
 	struct se_session *se_sess;
 	struct se_cmd *se_cmd;
 	int flags = TARGET_SCF_ACK_KREF;
@@ -1214,6 +1233,51 @@  static void usbg_cmd_work(struct work_struct *work)
 	return;
 
 skip:
+	if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
+		struct se_session *se_sess;
+		struct uas_stream *stream;
+
+		se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
+		stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag);
+
+		/*
+		 * There's no guarantee of a matching completion order between
+		 * different endpoints. i.e. The device may receive a new (CDB)
+		 * command request completion of the command endpoint before it
+		 * gets notified of the previous command status completion from
+		 * a status endpoint. The driver still needs to detect
+		 * misbehaving host and respond with an overlap command tag. To
+		 * prevent false overlapped tag failure, give the active and
+		 * matching stream id a short time (1ms) to complete before
+		 * respond with overlapped command failure.
+		 */
+		msleep(1);
+
+		/* If the stream is completed, retry the command. */
+		if (!stream->cmd) {
+			usbg_submit_command(cmd->fu, cmd->req);
+			return;
+		}
+
+		/*
+		 * The command isn't submitted to the target core, so we're safe
+		 * to remove the bitmap index from the session tag pool.
+		 */
+		sbitmap_queue_clear(&se_sess->sess_tag_pool,
+				    cmd->se_cmd.map_tag,
+				    cmd->se_cmd.map_cpu);
+
+		/*
+		 * Overlap command tag detected. Cancel any pending transfer of
+		 * the command submitted to target core.
+		 */
+		stream->cmd->tmr_rsp = RC_OVERLAPPED_TAG;
+		usbg_aborted_task(&stream->cmd->se_cmd);
+
+		/* Send the response after the transfer is aborted. */
+		return;
+	}
+
 	uasp_send_tm_response(cmd);
 }
 
@@ -1282,6 +1346,13 @@  static int usbg_submit_command(struct f_uas *fu, struct usb_request *req)
 		goto skip;
 	}
 
+	stream = uasp_get_stream_by_tag(fu, scsi_tag);
+	if (stream->cmd) {
+		pr_err("Command tag %d overlapped\n", scsi_tag);
+		cmd->tmr_rsp = RC_OVERLAPPED_TAG;
+		goto skip;
+	}
+
 	stream->cmd = cmd;
 
 	if (iu->iu_id == IU_ID_TASK_MGMT) {