Re: [PATCH] Fix piggybacked ACKs

["Doug Graham" <dgraham@nortel.com>]

On Thu, Jul 30, 2009 at 05:51:18PM +0800, Wei Yongjun wrote:
> The sender should create a SACK only if the size of the final SCTP
> packet does not exceed the current MTU. Base on RFC 4960:
> 
>   6.1.  Transmission of DATA Chunks
> 
>     Before an endpoint transmits a DATA chunk, if any received DATA
>     chunks have not been acknowledged (e.g., due to delayed ack), the
>     sender should create a SACK and bundle it with the outbound DATA
>     chunk, as long as the size of the final SCTP packet does not exceed
>     the current MTU.

[patch deleted]

I think you're right that there's a real problem here, and that a patch
similar to yours is needed, but this is not a new problem introduced
with my patch.  I only changed the conditions under which a SACK chunk
was bundled with a DATA chunk, but the same bundling would have been
happening before under different conditions.

I'd really need to study the code more than I have to be able to say
whether or not your fix is correct (and who cares what I think anyway
:-)), but I've just spent all of about 15 minutes looking at parts of the
code that I'd never looked at before, and I see something that off the
top of my head looks a bit scary.  That is that sctp_packet_bundle_sack()
calls sctp_packet_append_chunk(), which calls sctp_packet_bundle_sack().
Aside from the possibility of infinite recursion (presumably this must
be prevented somehow, because it doesn't seem to happen), the logic of
this seems strangely circular to me.  If bundle_sack() is going to call
append_chunk(), I'd have guessed that append_chunk() would be a lower
level routine that just appends the chunk you give it, not one that
itself tries to bundle other chunks in.

Anyway, you've got me curious now.  I'll have a go at better understanding
the code when I get some time.

--Doug.