illumos gate

Upon code inspection (as part of a review of code introduced in builds post-134), we find that there is an ASSERT in iscsit's iscsit_add_pdu_to_queue() function... specifically at the end it ASSERT's that the queue entry at the index is NULL.

However, if a duplicate PDU is received by the target with the cmdsn > expsn, then this will assert which is evil. (We can't trust the initiator, right. :-)

Even in a non-debug kernel this is bad, because the original PDU will be lost, and hence leaked. Memory leaks in error conditions are not good (although clearly not as evil as ASSERT.)

Here's my example diffs:

@@ -21,6 +21,9 @@
 /*
  * Copyright (c) 2008, 2010, Oracle and/or its affiliates. All rights reserved.
  */
+/*
+ * Copyright 2011 Nexenta Systems, Inc.  All rights reserved.
+ */

 #include <sys/cpuvar.h>
 #include <sys/types.h>
@@ -3167,9 +3170,22 @@
     mutex_exit(&ict->ict_mutex);

     index = ntohl(cmdsn) % ISCSIT_RXPDU_QUEUE_LEN;
-    ASSERT(cbuf->cb_buffer[index] == NULL);
-    cbuf->cb_buffer[index] = rx_pdu;
-    cbuf->cb_num_elems++;
+
+    /*
+     * In the normal case, assuming that the Initiator is not
+     * buggy and that we don't have packet duplication occuring,
+     * the entry in the array will be NULL.  However, we may have
+     * received a duplicate PDU with cmdsn > expsn , and in that
+     * case we just ignore this PDU -- the previously received one
+     * remains queued for processing.  We need to be careful not
+     * to leak this one however.
+     */
+    if (cbuf->cb_buffer[index] != NULL) {
+        idm_pdu_free(rx_pdu);
+    } else {
+        cbuf->cb_buffer[index] = rx_pdu;
+        cbuf->cb_num_elems++;
+    }
 }

 static idm_pdu_t *