[illumos-Developer] review for 653: https://www.illumos.org/issues/653

[Garrett D'Amore]

On Fri, 2011-01-21 at 10:28 -0800, Eric Schrock wrote:
> This looks good to me.  Is it always the correct thing to do to keep
> the original PDU, and not discard it in favor of the new one?  My
> understanding is that the initiator is already broken so we're just
> trying to avoid catastrophic failure on the server; precise semantics
> like this are (rightfully) undefined.  It might be good to add "The
> behavior in this case is undefined, so we choose to ignore this
> PDU ...".

The second PDU *should* be a copy of the first, so I think it shouldn't
matter which one we keep.  Unless the Initiator is totally busted and
incorrectly using cmdsns.

I think there is a situation where the Initiator isn't broken, but may
have sent a duplicate PDU due to lack of response/timeout.  Admittedly
I'm not an expert on this part of the protocol, so my understanding may
be flawed.

> I assume that there's no way to test this other than writing a custom
> broken initiator (likely overkill for such a simple fix)?

I am not aware of any other way to test it.  This was just something I
noticed by code inspection, not something we've actually seen in the
field.

	- Garrett
> 
> 
> Thanks,
> 
> 
> - Eric
> 
> On Thu, Jan 20, 2011 at 10:06 AM, Garrett D'Amore
> <garrett at nexenta.com> wrote:
>         There's a potential leak, or even worse, fatal assertion, in
>         iSCSI.
>         
>         I've described this in https://www.illumos.org/issues/653 but
>         here's a
>         diff for the fix, which I think makes it easier to see.
>         
>         I would greatly appreciate feedback on this.
>         
>                - Garrett
>         
>         -r b749961aba64
>         usr/src/uts/common/io/comstar/port/iscsit/iscsit.c
>         --- a/usr/src/uts/common/io/comstar/port/iscsit/iscsit.c
>            Wed Jan 19
>         08:12:06 2011 -0800
>         +++ b/usr/src/uts/common/io/comstar/port/iscsit/iscsit.c
>            Thu Jan 20
>         10:04:19 2011 -0800
>         @@ -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 *
>         
>         
>         
>         _______________________________________________
>         Developer mailing list
>         Developer at lists.illumos.org
>         http://lists.illumos.org/m/listinfo/developer
> 
> 
> 
> -- 
> Eric Schrock, Delphix
> 
> 
> _______________________________________________
> Developer mailing list
> Developer at lists.illumos.org
> http://lists.illumos.org/m/listinfo/developer