Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 3 May 2017 13:22:01 -0600
From:      Alan Somers <asomers@freebsd.org>
To:        Ian Lepore <ian@freebsd.org>
Cc:        Ryan Stone <rysto32@gmail.com>,  "src-committers@freebsd.org" <src-committers@freebsd.org>,  "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>,  "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>
Subject:   Re: svn commit: r317755 - head/sbin/ifconfig
Message-ID:  <CAOtMX2irVPoGYHB0pNmWYNdc7-G7R4XTXexz77hsaurUCefm2A@mail.gmail.com>
In-Reply-To: <1493838199.80042.14.camel@freebsd.org>
References:  <201705031721.v43HL2vS071819@repo.freebsd.org> <CAFMmRNyHc7qgeidDTZss%2Bo=56Uc-Xgyu_voj4ybHt=mEJa6c%2Bg@mail.gmail.com> <CAFMmRNyUXZz==Xdh8fqbw7BqYCHig%2BXAEOj1dxcJ2p0H6_qWeA@mail.gmail.com> <1493838199.80042.14.camel@freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, May 3, 2017 at 1:03 PM, Ian Lepore <ian@freebsd.org> wrote:
> On Wed, 2017-05-03 at 14:07 -0400, Ryan Stone wrote:
>> On Wed, May 3, 2017 at 1:39 PM, Ryan Stone <rysto32@gmail.com> wrote:
>>
>> >
>> >
>> >
>> > On Wed, May 3, 2017 at 1:21 PM, Alan Somers <asomers@freebsd.org> 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



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAOtMX2irVPoGYHB0pNmWYNdc7-G7R4XTXexz77hsaurUCefm2A>