On Tue, 2012-11-06 at 17:14 -0800, Roland Dreier wrote:
> On Tue, Nov 6, 2012 at 4:55 PM, Nicholas A. Bellinger
> <nab@xxxxxxxxxxxxxxx> wrote:
> > > - (cmd->stat_sn < exp_statsn)) {
>
> > Is there an actual bug this patch fixes, or some initiator that expects
> > ExpStatSN to be using SNA..? (CC'ing mnc + hare)
>
> Well, consider what happens with the original code as above. stat_sn
> is a u32, so on a connection that lasts a long time (though 4 billion
> commands doesn't last that long at high IOPS), we'll eventually hit a
> case where our stat_sn will wrap around from UINT_MAX to 0. And when
> that happens, if the initiator sends us an exp_stat_sn of UINT_MAX-1
> or something, then the test above will spuriously succeed.
>
Note the cmd->i_state == ISTATE_SENT_STATUS check before ->stat_sn
comparison trys to prevent a iscsi_cmd_t from being improperly acked +
released by an malicious initiator ExpStatSN.
> The iscsi spec talks about using mod(2**32) arithmetic for StatSN. I
> think what that means is that you have to take wraparound into
> account. Strictly speaking that doesn't necessarily mean SNA I guess,
> but that seems to be the easiest way to deal with it.
>
For the wrap around case your definately right though, as the end result
is iscsi_cmd->stat_sn with a larger value not being released by a lower
ExpStatSN, instead of being incorrectly released as mentioned above.
That said, applying the original patch with a proper commit log into
for-next shortly.
Thanks!
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
