Date: Fri, 20 Jan 2023 11:04:33 +0000 From: Alexander Chernikov <melifaro@freebsd.org> To: Alan Somers <asomers@freebsd.org> Cc: "Danilo G. Baio" <dbaio@freebsd.org>, dev-commits-src-all@freebsd.org Subject: Re: git: 2c24ad3377a6 - main - ifconfig: abort if loading a module fails other than for ENOENT Message-ID: <1E9FAE83-B5C2-4E1F-8D04-CF4F477F76C7@freebsd.org> In-Reply-To: <CAOtMX2hv182P2HTAPkbYDZiwNxkV2-C%2BWp2%2BL0SpfDpqn2Zccw@mail.gmail.com> References: <202301091857.309Iv87L068285@gitrepo.freebsd.org> <2f4e4ccf-b19a-4f8f-a9e0-72298e500d7c@app.fastmail.com> <CAOtMX2hv182P2HTAPkbYDZiwNxkV2-C%2BWp2%2BL0SpfDpqn2Zccw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
> On 19 Jan 2023, at 17:11, Alan Somers <asomers@freebsd.org> wrote: >=20 > On Thu, Jan 19, 2023 at 7:03 AM Danilo G. Baio <dbaio@freebsd.org> = wrote: >>=20 >>=20 >>=20 >> On Mon, Jan 9, 2023, at 15:57, Alan Somers wrote: >>> The branch main has been updated by asomers: >>>=20 >>> URL: >>> = https://cgit.FreeBSD.org/src/commit/?id=3D2c24ad3377a6f584e484656db8390e4e= b7cfc119 >>>=20 >>> commit 2c24ad3377a6f584e484656db8390e4eb7cfc119 >>> Author: Alan Somers <asomers@FreeBSD.org> >>> AuthorDate: 2022-12-26 02:06:21 +0000 >>> Commit: Alan Somers <asomers@FreeBSD.org> >>> CommitDate: 2023-01-10 02:56:18 +0000 >>>=20 >>> ifconfig: abort if loading a module fails other than for ENOENT >>>=20 >>> If "ifconfig create" tries to load a kernel module, and the module >>> exists but can't be loaded, fail the command with a useful error >>> message. This is helpful, for example, when trying to create a = cloned >>> interface in a vnet jail. But ignore ENOENT, because sometimes = ifconfig >>> can't correctly guess the name of the required kernel module. >>>=20 >>> MFC after: 2 weeks >>> Reviewed by: jhb >>> Differential Revision: https://reviews.freebsd.org/D37873 >>> --- >>> sbin/ifconfig/ifconfig.c | 18 +++++++++++++----- >>> 1 file changed, 13 insertions(+), 5 deletions(-) >>>=20 >>> diff --git a/sbin/ifconfig/ifconfig.c b/sbin/ifconfig/ifconfig.c >>> index 462d543125c4..120207a6927e 100644 >>> --- a/sbin/ifconfig/ifconfig.c >>> +++ b/sbin/ifconfig/ifconfig.c >>> @@ -1719,11 +1719,19 @@ ifmaybeload(const char *name) >>> } >>> } >>>=20 >>> - /* >>> - * Try to load the module. But ignore failures, because = ifconfig can't >>> - * infer the names of all drivers (eg mlx4en(4)). >>> - */ >>> - (void) kldload(ifkind); >>> + /* Try to load the module. */ >>> + if (kldload(ifkind) < 0) { >>> + switch (errno) { >>> + case ENOENT: >>> + /* >>> + * Ignore ENOENT, because ifconfig can't infer = the >>> + * names of all drivers (eg mlx4en(4)). >>> + */ >>> + break; >>> + default: >>> + err(1, "kldload(%s)", ifkind); >>> + } >>> + } >>> } >>>=20 >>> static struct cmd basic_cmds[] =3D { >>=20 >>=20 >> Hi. >>=20 >> I have a jail with vnet where the interface is renamed that stopped = working after this. >>=20 >> from inside the jail: >>=20 >> root@test:/ # ifconfig >> lo0: flags=3D8049<UP,LOOPBACK,RUNNING,MULTICAST> metric 0 mtu 16384 >> options=3D680003<RXCSUM,TXCSUM,LINKSTATE,RXCSUM_IPV6,TXCSUM_IPV6>= >> inet6 ::1 prefixlen 128 >> inet6 fe80::1%lo0 prefixlen 64 scopeid 0x10 >> inet 127.0.0.1 netmask 0xff000000 >> groups: lo >> nd6 options=3D21<PERFORMNUD,AUTO_LINKLOCAL> >> vnet0b_test: flags=3D8862<BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric = 0 mtu 1500 >> options=3D8<VLAN_MTU> >> ether 02:27:72:a7:28:0b >> groups: epair >> media: Ethernet 10Gbase-T (10Gbase-T <full-duplex>) >> status: active >> nd6 options=3D29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL> >>=20 >> root@test:/ # ifconfig vnet0b_test >> ifconfig: kldload(if_vnet): Operation not permitted >>=20 >>=20 >> If I don't rename the interface, that works. >>=20 >> jail.conf: >>=20 >> test { >> vnet; >> $index =3D "0"; >> vnet.interface =3D "vnet${index}b_${name}"; >> exec.prestart +=3D "ifconfig epair${index} create"; >> exec.prestart +=3D "ifconfig ${bridge} addm epair${index}a"; >> exec.prestart +=3D "ifconfig epair${index}a up name = vnet${index}a_${name}"; >> exec.prestart +=3D "ifconfig epair${index}b up name = vnet${index}b_${name}"; >> exec.poststop +=3D "ifconfig ${bridge} deletem = vnet${index}a_${name}"; >> exec.poststop +=3D "ifconfig vnet${index}a_${name} destroy"; >> devfs_ruleset =3D "11"; # add path 'bpf*' unhide (devfs.rules) >> } >>=20 >> That's a bit odd, I know, could be using description instead. >>=20 >> Just reporting. >>=20 >> Regards. >> -- >> Danilo G. Baio >=20 > Ugh, it looks like kldload(2) is doing the privilege check before the > file existence check. I'm not sure of the best solution: > * Change kern_kldload to check for file existence first. This would > ring some alarm bells among security folks, and it isn't totally easy > to do, either. > * Change ifconfig(8) to do an existence check of its own. This would = be ugly. > * Change ifconfig(8) so that it doesn't attempt to load modules when > just listing an interface. This might be incomplete, but is probably > worth doing anyway. I think another question is that if if should be done by ifconfig(8) at = all. Kernel can take care of trying to load the required modules, = checking the privileges. I=E2=80=99m considering adding such code for the netlink-based interface = creation. >=20
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1E9FAE83-B5C2-4E1F-8D04-CF4F477F76C7>