Re: tcp options length with SACK

[Nick Hudson <skrll%netbsd.org@localhost>]

On 01/04/17 21:01, Robert Elz wrote:
> While I was (fairly ineffectively) looking to see if I could spot
> a cause for PR51767 (which Martin has since fixed) I came across ...
>
> /*
>   * Determine the length of the TCP options for this connection.
>   *
>   * XXX:  What do we do for SACK, when we add that?  Just reserve
>   *       all of the space?  Otherwise we can't exactly be incrementing
>   *       cwnd by an amount that varies depending on the amount we last
>   *       had to SACK!
>   */
>          
> u_int
> tcp_optlen(struct tcpcb *tp)
> {
> 	/* ... */
>
> (and sure enough SACK is not in any way accounted for in tcp_optlen() nor
> is it handled separately at the couple of places that tcp_oplen() is called,
> but the "when we add that" is now long past.)
>
> This made me wonder if this could be related to the ocasional problems
> people report when using SACK - my impression is that the effect might be
> to allow packets bigger than the MSS to be transmitted (or attempted to).

Could be... I've actually turned SACK off on my desktop.

I also noticed

    985 #define    TCP_SACK_OPTLEN(nblks)    ((nblks) * 8 + 2 + 2)

which can't be right

I'd like to see this fixed.

> Of course, I may be misreading things, in which case at the very least that
> comment ought to be corrected.
>
>
> Also, while I was looking at the optlen calcs (and use) for TCP, it occurred
> to me that with all of SACK, TIMESTAMP, and TCP_SIGNATURE in use, the TCP
> options space would be very close to (if not beyond) filled, a situation
> we do not handle gracefully at all ...

I was reading RFC2018 section 3 about this today while hunting the bug 
with martin.

>   as a first step I'd suggest deleting
> all the padding NOP options that are now always inserted for alignment
> (they certainly can make processing faster, but they are not required)
> rather than just bailing if the options exceed the space allowed, and if
> even that is not going to allow things to work, it might be better to
> disable one of the options (SACK or SIGNATURE I'd guess, TIMESTAMP is
> too useful) rather than just bailing (generating a send error.)

SIGNATURE isn't that common I guess, but the code should add as many 
SACK blocks as it can

> kre
Nick