Re: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command

[Dan Carpenter <dan.carpenter@linaro.org>]

Sorry for the delayed response.  I was on vacation.

On Fri, Dec 20, 2024 at 02:31:20AM +0000, Thinh Nguyen wrote:
> On Thu, Dec 19, 2024, Dan Carpenter wrote:
> > Hi Thinh,
> > 
> > kernel test robot noticed the following build warnings:
> > 
> > url:    https://github.com/intel-lab-lkp/linux/commits/Thinh-Nguyen/usb-gadget-f_tcm-Don-t-free-command-immediately/20241211-092317 
> > base:   d8d936c51388442f769a81e512b505dcf87c6a51
> > patch link:    https://lore.kernel.org/r/6bffc2903d0cd1e7c7afca837053a48e883d8903.1733876548.git.Thinh.Nguyen%40synopsys.com 
> > patch subject: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command
> > config: nios2-randconfig-r071-20241219 (https://download.01.org/0day-ci/archive/20241219/202412192132.XB16SilM-lkp@intel.com/config )
> > compiler: nios2-linux-gcc (GCC) 14.2.0
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > | Closes: https://lore.kernel.org/r/202412192132.XB16SilM-lkp@intel.com/ 
> > 
> > smatch warnings:
> > drivers/usb/gadget/function/f_tcm.c:1308 usbg_cmd_work() error: we previously assumed 'active_cmd' could be null (see line 1265)
> > 
> > vim +/active_cmd +1308 drivers/usb/gadget/function/f_tcm.c
> > 
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1227  static void usbg_cmd_work(struct work_struct *work)
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1228  {
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1229  	struct usbg_cmd *cmd = container_of(work, struct usbg_cmd, work);
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1230  
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1231  	/*
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1232  	 * Failure is detected by f_tcm here. Skip submitting the command to the
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1233  	 * target core if we already know the failing response and send the usb
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1234  	 * response to the host directly.
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1235  	 */
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1236  	if (cmd->tmr_rsp != RC_RESPONSE_UNKNOWN)
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1237  		goto skip;
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1238  
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1239  	if (cmd->tmr_func)
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1240  		usbg_submit_tmr(cmd);
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1241  	else
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1242  		usbg_submit_cmd(cmd);
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1243  
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1244  	return;
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1245  
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1246  skip:
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1247  	if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1248  		struct f_uas *fu = cmd->fu;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1249  		struct se_session *se_sess;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1250  		struct uas_stream *stream = NULL;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1251  		struct hlist_node *tmp;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1252  		struct usbg_cmd *active_cmd = NULL;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1253  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1254  		se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1255  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1256  		hash_for_each_possible_safe(fu->stream_hash, stream, tmp, node, cmd->tag) {
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1257  			int i = stream - &fu->stream[0];
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1258  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1259  			active_cmd = &((struct usbg_cmd *)se_sess->sess_cmd_map)[i];
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1260  			if (active_cmd->tag == cmd->tag)
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1261  				break;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1262  		}
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1263  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1264  		/* Sanity check */
> > 7735c10c74d903 Thinh Nguyen          2024-12-11 @1265  		if (!stream || (active_cmd && active_cmd->tag != cmd->tag)) {
> > 
> > Testing for !stream is sufficient.  Another option would be to write this
> 
> Just testing for !stream is sufficient to know whether active_cmd is
> NULL, but we still need to check for matching tag also.

Yes.  Sorry, I was unclear.  That's what I meant.  We could write it
as:

	if (!stream || active_cmd->tag != cmd->tag) {

There is no need to check if active_cmd is non-NULL since we know that
stream is non-NULL.  However, if we DO check it, then we should check
it consistently everywhere.

> 
> > as:
> > 	if (!stream || !active_cmd || active_cmd->tag != cmd->tag)) {
> 
> Perhaps we can just do this:
> 
> 	if (!active_cmd || active_cmd->tag != cmd->tag)) {
> 
> If active_cmd is NULL, then the stream variable must also be NULL. This
> may not be obvious.
> 
> > 
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1266  			usbg_submit_command(cmd->fu, cmd->req);
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1267  			return;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1268  		}
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1269  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1270  		reinit_completion(&stream->cmd_completion);
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1271  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1272  		/*
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1273  		 * A UASP command consists of the command, data, and status
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1274  		 * stages, each operating sequentially from different endpoints.
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1275  		 *
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1276  		 * Each USB endpoint operates independently, and depending on
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1277  		 * hardware implementation, a completion callback for a transfer
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1278  		 * from one endpoint may not reflect the order of completion on
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1279  		 * the wire. This is particularly true for devices with
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1280  		 * endpoints that have independent interrupts and event buffers.
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1281  		 *
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1282  		 * The driver must still detect misbehaving hosts and respond
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1283  		 * with an overlap status. To reduce false overlap failures,
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1284  		 * allow the active and matching stream ID a brief 1ms to
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1285  		 * complete before responding with an overlap command failure.
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1286  		 * Overlap failure should be rare.
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1287  		 */
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1288  		wait_for_completion_timeout(&stream->cmd_completion, msecs_to_jiffies(1));
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1289  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1290  		/* If the previous stream is completed, retry the command. */
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1291  		if (!hash_hashed(&stream->node)) {
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1292  			usbg_submit_command(cmd->fu, cmd->req);
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1293  			return;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1294  		}
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1295  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1296  		/*
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1297  		 * The command isn't submitted to the target core, so we're safe
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1298  		 * to remove the bitmap index from the session tag pool.
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1299  		 */
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1300  		sbitmap_queue_clear(&se_sess->sess_tag_pool,
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1301  				    cmd->se_cmd.map_tag,
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1302  				    cmd->se_cmd.map_cpu);
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1303  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1304  		/*
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1305  		 * Overlap command tag detected. Cancel any pending transfer of
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1306  		 * the command submitted to target core.
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1307  		 */
> > 7735c10c74d903 Thinh Nguyen          2024-12-11 @1308  		active_cmd->tmr_rsp = RC_OVERLAPPED_TAG;
> > 
> > The inconsistent NULL check triggers a warning here.
> > 
> 
> We already check for !stream prior, so I didn't check for active_cmd
> here. This is more of a consistency issue. If possible and if needed, we
> can make this more consistent after the merge?

This is not a run time bug, yes.  It's just an inconsistent NULL check,
but the NULL check is not necessary so that's not a problem.  I don't
have a vote on how you merge it.  ;)  You can do that however you want.

regards,
dan carpenter