From owner-svn-src-head@freebsd.org Mon Oct 16 21:32:16 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id B2055E45B47; Mon, 16 Oct 2017 21:32:16 +0000 (UTC) (envelope-from julien.charbon@gmail.com) Received: from mail-wm0-f67.google.com (mail-wm0-f67.google.com [74.125.82.67]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 507FF6E131; Mon, 16 Oct 2017 21:32:15 +0000 (UTC) (envelope-from julien.charbon@gmail.com) Received: by mail-wm0-f67.google.com with SMTP id b189so40342wmd.4; Mon, 16 Oct 2017 14:32:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=KuAjdq1ZIBVBcOSrnZlhSYZM7qg7a8ISv1qIITMZkqQ=; b=jbTqLHtLLn8bkvfKFYEUZOl9ha5tGFH54myQM2DoU+KYVF9QJDmxdHb9/+LVD9JQQ5 Ut0pA5wKWMWlzOQvOjrZI6n3LD0lzOjfsUtDDclNnnF1bOUT4xHFHugBxLX68Bueyiox lQAAh/6JgPWCXUXdcAbv0nnRpOzCMhckodEMCFZKuh9uMejkcicuCjH0Ygyu+jNtxfI2 Txd+PjcgklbAvo7I5BoivggifUMTsZrEmfKNEk2/C5Q/ZW4OSOzgMnSsxOO+tYTDW1fc kbtJ97IUIibqnDqlZPRE/tNJNUumWA1gptHDaM9NErVsqwBiGaOjmYWN3idD4s/+OkVc rp4w== X-Gm-Message-State: AMCzsaV7txp0iANO3BSPkUcC5Tfu9iloZIV/93HxoXBEst1GJpDqK/9P KdidgQNjLtHOODOEPTecJdbHTo7H X-Google-Smtp-Source: ABhQp+Rl7D91Sa0UrtsmsqYkoDJXKUISwwqE4g+AsBeUR48/6Ahn++BnGFN4fHT6YYrTlN323zpt/A== X-Received: by 10.28.135.209 with SMTP id j200mr1794910wmd.40.1508189527490; Mon, 16 Oct 2017 14:32:07 -0700 (PDT) Received: from ?IPv6:2a00:b420:1:9700:9160:e6b4:e62a:107f? ([2a00:b420:1:9700:9160:e6b4:e62a:107f]) by smtp.gmail.com with ESMTPSA id o24sm11152290wmi.39.2017.10.16.14.32.06 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 16 Oct 2017 14:32:07 -0700 (PDT) Subject: Re: svn commit: r324179 - head/sys/netinet To: Jonathan Looney Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201710012120.v91LKSH1050145@repo.freebsd.org> From: Julien Charbon Message-ID: <7dfb2ff7-7038-bae7-3505-bc1de21464fa@freebsd.org> Date: Mon, 16 Oct 2017 23:32:05 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Oct 2017 21:32:16 -0000 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