On Wednesday, May 03, 2017 01:03:19 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 <ryst...@gmail.com> wrote:
> >
> > >
> > >
> > >
> > > On Wed, May 3, 2017 at 1:21 PM, Alan Somers <asom...@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.
Except it's really klunky. The loop searching the module list is a lot more
code than ignoring EEXIST errors from kldload. I would structure the code
like this:
ifindex = if_nametoindex(ifname);
if (ifindex == 0) {
ifmaybeload(ifname);
ifindex = if_nametoindex(ifname);
}
if (ifindex == 0) {
/* existing code */
}
Further, I would probably simplify ifmaybeload() to be something like this:
static void
ifmaybeload(const char *name)
{
if (noload)
return;
/* existing code to generate 'ifkind' */
if (kldload(ifkind) == -1) {
if (errno != EEXIST)
err(...);
}
}
One could argue for ignoring ENOENT errors as well, but Alan's specific use case
is one that wanted an explicit kldload error for ENOENT.
--
John Baldwin
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
