Project

General

Profile

Bug #653

incorrect handling in iscsit of duplicate PDUs with cmdsn > expsn

Added by Garrett D'Amore about 10 years ago. Updated over 8 years ago.

Status:
Resolved
Priority:
High
Assignee:
Category:
kernel
Start date:
2011-01-20
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
needs-triage
Gerrit CR:

Description

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 *

Also available in: Atom PDF