When ABE was added (rS331214) to New Reno and leak fixed (rS333699) , some assumptions about the state of it as the default and always ready cc seem to have changed, notably it now has a constructor and destructor (newreno_cb_destroy) for per connection state.

Not true for either commit. 331214 still allowed for an "always available" NewReno in the event of malloc failure in the constructor, and r333699 goes one step better and killed off the constructor and allocs on demand only when a setting that requires state is changed.

As to the actual problem you're trying to nut out, I suspect the problematic interaction is that NewReno is now (post r333699) assuming that if cc_data != NULL that it has malloced that memory and can use it, which is indeed a bug in the context of falling back to NewReno. Your change is therefore correct but, should be enforced in the framework i.e. I'd suggest removing the changes to NULL cc_data from the individual modules and explicitly do it anywhere cb_destroy() is called in framework code.

To clarify though, are you actually using ABE or changing NewReno's beta/beta_ecn?

If the cb_destroy virtual function shouldn't null the pointer, I assert that it shouldn't free it either.

There should be another cc_assert wrapper function that checks for not null, frees then nulls, after calling the virtual function.

In D16282#347262, @jason_eggnet.com wrote:

If the cb_destroy virtual function shouldn't null the pointer, I assert that it shouldn't free it either.

There should be another cc_assert wrapper function that checks for not null, frees then nulls, after calling the virtual function.

I disagree. Only the module itself knows the nature of the memory it has allcoated and hung off the cc_data pointer, and the module is therefore the only thing that has any business freeing that memory. NULLing the pointer on transition between modules is something the framework should be doing as a contract/guarantee for any incoming module to know that if the cc_data pointer is not NULL, it can be assured it owns that memory, not some previous module.

Good point on sub structures.

I suppose, cb_destroy is equivalent to free. Your suggestion makes sense.

@lstewart we debated who should NULL before submitting this, I think it can be moved out of the destructors to the places I put the KASSERTs but I'll have to double check everything. One issue I foresaw NULLing in the framework is that we have to trust that the cc_cb_destroy destructor did indeed free() and can't catch mistakes with a KASSERT. But writing a new CC isn't exactly common and will probably be cribbed from an existing one so maybe I was over thinking that angle.

To answer your question, no we don't use the NewReno functions that would cause a malloc. We set cc_cubic as our default, but we noticed some transition during timewait that removes the CC_ALGO and sets it back to NewReno, and then double frees.

In D16282#347300, @kbowling wrote:

@lstewart we debated who should NULL before submitting this, I think it can be moved out of the destructors to the places I put the KASSERTs but I'll have to double check everything. One issue I foresaw NULLing in the framework is that we have to trust that the cc_cb_destroy destructor did indeed free() and can't catch mistakes with a KASSERT. But writing a new CC isn't exactly common and will probably be cribbed from an existing one so maybe I was over thinking that angle.

Right, if a module leaks memory, that's really its problem, and if the module gets unloaded and INVARIANTS is on, the kernel prints a warning about the leaked memory anyway (assuming the module declares its own MALLOC_TYPE which it should) so I think that's an adequate safety belt.

Thanks for finding + fixing this and apologies for introducing the bug.

One final nit: I think the change added to cdg's cb_destroy() checking for NULL cc_data is not needed - its cb_init() doesn't allow the algo to be used if malloc() fails.

I would prefer to leave the check in cc_cdg until we run to ground why the callback was getting hit twice (in the case someone has cc_cdg as default).

To leave a paper trail, the option we employ that prompted this initial investigation is net.inet.tcp.nolocaltimewait=1.

Ah I think you are indeed correct, I looked at the QA test suite and they have a stress for the TCP_CONGESTION sockopt from our default cc_cubic to new reno. So we were coming into reno with freed but not NULL junk in cc_data and it therefore the framework tried to call newreno's destructor later on. Thanks for the insight!