Date: Mon, 16 Oct 2017 23:32:05 +0200 From: Julien Charbon <jch@freebsd.org> To: Jonathan Looney <jonlooney@gmail.com> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r324179 - head/sys/netinet Message-ID: <7dfb2ff7-7038-bae7-3505-bc1de21464fa@freebsd.org> In-Reply-To: <CADrOrmvRTRim3DaD01ZKOrNRrfCq7GKqtFqsxsV-yCRvAppLvA@mail.gmail.com> References: <201710012120.v91LKSH1050145@repo.freebsd.org> <CADrOrmvRTRim3DaD01ZKOrNRrfCq7GKqtFqsxsV-yCRvAppLvA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?7dfb2ff7-7038-bae7-3505-bc1de21464fa>