kern/75122: [PATCH] Incorrect inflight bandwidth calculation on first packet

[Uwe Doering]

Uwe Doering wrote:
> Dan Nelson wrote:
>> In the last episode (Dec 18), Uwe Doering said:
>>> Dan Nelson wrote:
>>>
>>>> Updated patch including Matt's recommended fix:
>>>>
>>>> +    /*
>>>> +     * Sanity check, plus ignore pure window update acks.
>>>> +     */
>>>> +    if ((int)(ack_seq - tp->t_bw_rtseq) <= 0)
>>>> +        return;
>>>
>>> I wonder, isn't there a flaw in the logic with regard to the sequence
>>> number handling?  If the sequence number wraps around 't_bw_rtseq' no
>>> longer gets set and therefore the bandwidth calculation stops until
>>> 'ack_seq' either catches up with 't_bw_rtseq' again (which would take
>>> quite a while), or 'ticks' wraps around as well, or there is
>>> inactivity for more than 10 seconds.  This is probably not the
>>> intended behavior.
>>
>> I think the code works as-is.  ack_seq and tp->t_bw_rtseq are both of
>> type "tcp_seq" which is a u_int32_t.  Wrap-around is handled
>> transparently when your variables are unsigned and your sequence space
>> covers all possible values.  It's the magic of mod(2^32) arithmetic :)
>> The (int) cast just makes the if simpler.  Without the cast it would
>> read
>>
>>  if (ack_seq - tp->t_bw_rtseq > 2147483648U || ack_seq == tp->t_bw_rtseq)
>>
>> The sanity check is probably not even necessary, as any really invalid
>> sequence numbers would have caused the packet to be dropped before it
>> got this far.
> 
> You are correct.  Unlike 'ticks', which is of type 'int', a sequence 
> number wrap-around has no effect on the calculation of the byte count 
> due to using 'u_int32_t'.  Tricky. ;-)
> 
> Therefore, I propose testing for equality only (window update acks). 
> Checking for byte counts of more than 2147483648U and declaring that 
> bogus is a little arbitrary.  If we are concerned about invalid sequence 
> numbers, they can result in a lower byte count as well.  The way it is 
> now (with the patch installed) it looks too much like a wrap-around 
> check and is therefore misleading.  So I suggest to either get rid of it 
> or add a comment that explains the situation.

On second thought, checking for less than zero (by means of the 'int' 
cast) might have its merits.  This may be a protection against 
out-of-order ACKs, which could in fact be valid, but calculating a byte 
count from an ACK for an earlier packet when we've already processed a 
later ACK would be bogus.

Hard to tell what Matt's intentions were.  In the end the patch appears 
to be okay as it is.  Nevertheless, my suggestion to comment this 
sufficiently still stands.

    Uwe
-- 
Uwe Doering         |  EscapeBox - Managed On-Demand UNIX Servers
gemini at geminix.org  |  http://www.escapebox.net