Project

General

Profile

Bug #653

incorrect handling in iscsit of duplicate PDUs with cmdsn > expsn

Added by Garrett D'Amore almost 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 *
#1

Updated by Garrett D'Amore almost 10 years ago

  • Assignee changed from Garrett D'Amore to Dan McDonald

Dan, assigning to you. I think mostly we just need to have this code tested. It has been reviewed on list, and offlist. Ping me out of band if you have questions.

#2

Updated by Dan McDonald almost 10 years ago

What file is this in? Your diff doesn't have a path.

Ahhh:

usr/src/uts/common/io/comstar/port/iscsit/iscsit.c

#3

Updated by Yuri Pankov over 8 years ago

  • Difficulty set to Medium
  • Tags set to needs-triage
#4

Updated by Dan McDonald over 8 years ago

  • Status changed from New to Resolved
  • % Done changed from 0 to 100

Forgot to close this bug:

changeset: 13817:32a39c8dbb42
user: Dan McDonald <>
date: Thu Jul 26 12:14:58 2012 -0400
description:
653 incorrect handling in iscsit of duplicate PDUs with cmdsn > expsn
Reviewed by: Jason King <>
Reviewed by: Gary Mills <>
Reviewed by: Vitaliy Gusev <>
Approved by: Garrett D'Amore <>

Also available in: Atom PDF