From owner-svn-src-all@freebsd.org Wed May 3 19:22:04 2017 Return-Path: Delivered-To: svn-src-all@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 276CBD57E57; Wed, 3 May 2017 19:22:04 +0000 (UTC) (envelope-from asomers@gmail.com) Received: from mail-yb0-x241.google.com (mail-yb0-x241.google.com [IPv6:2607:f8b0:4002:c09::241]) (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 D7EE2B34; Wed, 3 May 2017 19:22:03 +0000 (UTC) (envelope-from asomers@gmail.com) Received: by mail-yb0-x241.google.com with SMTP id l192so8595885ybl.1; Wed, 03 May 2017 12:22:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=Df0fyDNfaKLR3l9pecApJPOqRwZ34MDYVaO+35/m9U8=; b=cdmDmjGFOAoYo52SO/bgymiEkCyQeX3m3xQ7dyKhGYorZFc8dCxbcmUMRf0S+l8Uzy B1LWbzxqEnzWBju6Yp8TqvL1RDGAZFIY4txiI/BM21FA5Au1fzJJqx6nkhKw1x4tdi5X atPjVLQ1sqlQLFHHEV2PW1xfkSaZbOTb8EcQNjY0dfMmI0nqRtuBc4fbcxVkDoPCSYpN cqNiuAWJu5jKLwUyuS9JtIsp7NtFjsNKAzTYe+NP7L6Xt/AUMYw4nIMmc3Kf/DXG3y2a 1e5zQso/c0AijK3ewuWywFA5oyVHXCc5ZE5EkiodQUR/wFG5fxRZpkxghELl+hygVJQs QSgQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=Df0fyDNfaKLR3l9pecApJPOqRwZ34MDYVaO+35/m9U8=; b=EsRkKaVrNcQUyV51mCrHX+YO3h/R0V0lGJJX8LirKWUgM/7i4Sg88kxvgXGEJg4bwW oZQCybqFPyt/IdsKfp6Ws11u6qScef761myS3fZpCrxi6MR2kyUBLqT8OJFrKPUUftUo W4wHGND7Lnv2mmSCMzhEfq5vkIsxgx/ayPvRda3bmYuMRMCQ7VB3052BU8tGtWw55Bh4 rm0oTSZStrY9PomAZ4cFSXIEdoy6dUJT/k461Cz5yQ14tfaZdkWVzsVuimqRHgtehMm1 BmQz0FcyBNoTfyKdNcu/uD0RLWuKbhEPQzaCzMi6LkAQp6hkZ3n5AbHmY2KULW/WTfBU OEfw== X-Gm-Message-State: AN3rC/4SyQ9lBooq9UWgOpe8OZVGye9i20vCPl6T0f0Ac0IiUb89VOxE cQyNy8xldhfX0U5AOdarq8hpXmRflm4w X-Received: by 10.37.5.209 with SMTP id 200mr31619233ybf.139.1493839322082; Wed, 03 May 2017 12:22:02 -0700 (PDT) MIME-Version: 1.0 Sender: asomers@gmail.com Received: by 10.129.20.214 with HTTP; Wed, 3 May 2017 12:22:01 -0700 (PDT) In-Reply-To: <1493838199.80042.14.camel@freebsd.org> References: <201705031721.v43HL2vS071819@repo.freebsd.org> <1493838199.80042.14.camel@freebsd.org> From: Alan Somers Date: Wed, 3 May 2017 13:22:01 -0600 X-Google-Sender-Auth: xB4KqykgbWKeSMnbnSJTx65WEiw Message-ID: Subject: Re: svn commit: r317755 - head/sbin/ifconfig To: Ian Lepore Cc: Ryan Stone , "src-committers@freebsd.org" , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" Content-Type: text/plain; charset=UTF-8 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 May 2017 19:22:04 -0000 On Wed, May 3, 2017 at 1:03 PM, Ian Lepore wrote: > On Wed, 2017-05-03 at 14:07 -0400, Ryan Stone wrote: >> On Wed, May 3, 2017 at 1:39 PM, Ryan Stone wrote: >> >> > >> > >> > >> > On Wed, May 3, 2017 at 1:21 PM, Alan Somers wrote: >> > >> > > >> > > Author: asomers >> > > Date: Wed May 3 17:21:01 2017 >> > > New Revision: 317755 >> > > URL: https://svnweb.freebsd.org/changeset/base/317755 >> > > >> > > Log: >> > > Various Coverity fixes in ifconfig(8) >> > > >> > > * Exit early if kldload(2) fails (1011259). This is the only change that >> > > affects ifconfig's behavior. >> > > >> > > >> > Please revert this ASAP. kldload is expected to fail for a number of >> > benign reasons and this change is likely to prevent any network >> > configuration from being applied to systems, breaking remote access. >> > >> > >> It's been pointed out to me off-list that the situation is not quite as >> dire as I had originally believed. The ifconfig code in question already >> searches to check if the module in question is loaded before calling >> kldload. However, there is at least one driver (mlx4_en) that does not >> follow the "if_" kld module naming convention that this code depends >> on, so this change will make it impossible to apply configuration to >> mlx4_en interfaces. Additionally, it is possible that other drivers use >> the naming convention for their kld file but not for the module declared in >> the C code, in which case this change would also break configuration of >> those interfaces. >> >> jhb@ suggests that ifconfig should only attempt to load a module if the >> interface doesn't already exist, by calling if_nametoindex to check for the >> existence of the interface. That seems to be a reasonable fix for me, but >> in the interest of not breaking users' networking configuration >> (potentially making it impossible to fix a remote machine), I'd recommend >> that the part of the change that checks the return code from kldload() be >> reverted while a fix for this issue is worked on. > > It should be noted that the existing code uses if_nametoindex() > immediately after ifmaybeload() returns, and handles errors > accordingly. I.e., there wasn't really anything wrong with the code as > originally written/structured. > > -- Ian The problem with the original code is that module load errors would be caught too late, and as a result the error printed would be less useful. For example, if you forgot to put "kld_list=if_igb" in /etc/rc.conf and run "ifconfig igb0", it would print "ifconfig: interface igb7 does not exist" instead of the more useful "ifconfig: kldload(if_igb): Operation not permitted". But I didn't know that there were drivers like mlx4en where ifconfig can't load the correct module. So I'll take Ryan's suggestion and revert that part for now. -Alan