svn commit: r324179 - head/sys/netinet

[Julien Charbon]

 Hi Jonathan,

On 10/16/17 7:04 PM, Jonathan Looney wrote:
> I apologize for just getting to this, but your code just made it into my
> local tree.
> 
> The non-INVARIANTS case looks incorrect. Because tw stays on the list,
> it can end up stuck at the front of the queue and block everything
> behind it.
> 
> Personally, I would be more comfortable just panic'ing here. This
> indicates a problem. And, depending on the way things got corrupted, you
> could end up with other hard-to-debug problems if you continue with a
> corrupted state.

 Good remark, actually, this logic:

#ifdef INVARIANTS -> panic()
#else INVARIANTS -> log(LOG_ERR)

 was introduced with the main fix:
https://reviews.freebsd.org/rS307551

 Why not panic()-ing in both cases INVARIANTS and !INVARIANTS already?
The rational is that r307551 fixes an issue introduced with r185775,
pushed 8 years before:
https://reviews.freebsd.org/rS185775

 Obviously, this double free is either rarely reproduced and/or has only
minor effects for most of people (like a connection closes sooner than
expected), only few people got annoying kernel panic/infinite loop.
Thus before introducing a panic() that could potentially impact a fair
amount of people, better introducing a log(LOG_ERR) first.

 Now, when replace these log(LOG_ERR) calls by a proper panic()?  I
would say when 11.0 (which is the last supported release without the
r307551 fix) reaches the EoL state:  End of October 2017 I believe.  And
so far, so good, no log(LOG_ERR)/panic()/similar issues have been
witness with r307551 fix.

 Of course, if you think it is too cautious, you can create a review
with the panic() calls change to get wider point of views.

 My 2 cents.

--
Julien