Bug 206583 - Unable to load ip_mroute kernel module if VIMAGE is enabled in kernel
Summary: Unable to load ip_mroute kernel module if VIMAGE is enabled in kernel
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Bjoern A. Zeeb
URL:
Keywords: patch, vimage
: 242035 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-01-24 16:53 UTC by Ben Woods
Modified: 2020-07-31 09:50 UTC (History)
5 users (show)

See Also:
bz: mfc-stable12+


Attachments
malloc() array space to unbreak kldloading on VNET kernels (2.29 KB, patch)
2016-01-24 17:24 UTC, Marko Zec
no flags Details | Diff
HEAD: malloc() array space to unbreak kldloading on VNET kernels (2.26 KB, patch)
2016-01-24 17:52 UTC, Ben Woods
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Woods freebsd_committer 2016-01-24 16:53:18 UTC
When running a kernel with VIMAGE support, loading the ip_mroute kernel module will fail.

% sudo kldload -v ip_mroute
kldload: an error occurred while loading the module. Please check dmesg(8) for more details.
% dmesg
linker_load_file: Unsupported file type
% uname -a
FreeBSD sparticus.woods.am 11.0-CURRENT FreeBSD 11.0-CURRENT #1 r294463: Sun Jan 24 17:17:25 CET 2016     root@freenas.woods.am:/usr/obj/usr/src/sys/GENERIC-NODEBUG-VIMAGE  amd64


If I recompile the kernel with the same sources, with the only change being to disable VIMAGE (in this case using the GENERIC-NODEBUG kernel), then after reboot I am able to load the ip_mroute kernel option successfully.

Expected result: ip_mroute kernel module should be able to load regardless of whether VIMAGE support is enabled in the kernel or not.
Comment 1 Ben Woods freebsd_committer 2016-01-24 17:16:48 UTC
Some information provided by Marko Zec on the freebsd-net@freebsd.org mailing list:
https://lists.freebsd.org/pipermail/freebsd-net/2016-January/044447.html

In this particular case the problem is that ip_mroute demands more
space for "virtualized global" variables than what kernel linker has
put aside for each vnet.

Bumping VNET_MODMIN to 24 should circumvent the issue that Ben is
observing.  A more vnet-friendly fix would require refactoring
ip_mroute's arrays so that they get malloc()ed / free()d from SYSINIT
handlers instead of being declared "virtualized global".

Marko

===================================================================
--- vnet.c      (revision 294659)
+++ vnet.c      (working copy)
@@ -170,7 +170,7 @@
  * we want the virtualized global variable space to be page-sized, we
may
  * have more space than that in practice.
  */
-#define        VNET_MODMIN     8192
+#define        VNET_MODMIN     3 * 8192
 #define        VNET_SIZE       roundup2(VNET_BYTES, PAGE_SIZE)
 #define        VNET_MODSIZE    (VNET_SIZE - (VNET_BYTES - VNET_MODMIN))
Comment 2 Ben Woods freebsd_committer 2016-01-24 17:18:57 UTC
It is worth noting that building a kernel with VIMAGE and MROUTING both enabled seems to work fine. This problem only appears when multicast routing is not built into the kernel with the MROUTING option, but VIMAGE support is - in this case attempting to load the ip_mroute kernel module fails.
Comment 3 Marko Zec freebsd_committer 2016-01-24 17:24:17 UTC
Created attachment 166069 [details]
malloc() array space to unbreak kldloading on VNET kernels
Comment 4 Marko Zec freebsd_committer 2016-01-24 17:25:07 UTC
Please try the attached patch with both plain and VIMAGE kernels.

Marko
Comment 5 Ben Woods freebsd_committer 2016-01-24 17:52:18 UTC
Created attachment 166070 [details]
HEAD: malloc() array space to unbreak kldloading on VNET kernels

Thanks for your help Marko. Your patch didn't apply cleanly against head, so I have instead modified it to the attached. Can you please have a quick look and make sure it still looks ok?
Comment 6 Marko Zec freebsd_committer 2016-01-24 19:56:18 UTC
(In reply to Ben Woods from comment #5)

Oops, my patch was based on stable/10 branch.  Your version for head seems fine, pls. go ahead with testing...
Comment 7 Ben Woods freebsd_committer 2016-01-25 08:25:53 UTC
I have tested the attached patch against head, and can confirm it fixes the issue with the ip_mroute module not being able to load on a kernel with VIMAGE enabled.

On a kernel with VIMAGE enabled and this patch applied, I am successfully able to load and then unload the ip_mroute module.

On a GENERIC-NODEBUG kernel (without VIMAGE enabled) and this patch applied, I am successfully able to load and unload the ip_mroute module (no change from without the patch applied).

Note that I have not done exhaustive testing of the multicast routing functionality yet, but can confirm I have not noticed any other strange behaviour with the patch applied.
Comment 8 Ben Woods freebsd_committer 2016-01-27 22:51:51 UTC
I have just attempted to test multicast routing with this patch, but have experienced 2 strange issues:

1. Sometimes I found igmpproxy would fail to start, but if I then unloaded the ip_mroute kernel module, and reloaded it, then igmpproxy would successfully start.

2. Once I began using the multicast routing, my normal unicast network traffic would no longer work. When I unloaded the ip_mroute kernel module my unicast network traffic would begin working again.

I am in the process of building a generic kernel with no patchs and no VIMAGE, to determine if these 2 items are regressions introduced by this patch, or if they also exist in the unpatched generic kernel.
Comment 9 Ben Woods freebsd_committer 2016-01-28 06:22:40 UTC
(In reply to Ben Woods from comment #8)
I can confirm Issue #2 described above is NOT caused by this patch, and is an existing issue I am experiencing with multicast routing and IPFW.

Still investigating whether Issue #1 is related to this patch or not.
Comment 10 Ben Woods freebsd_committer 2016-03-19 16:34:55 UTC
Hi Marko,
What are your thoughts about this patch? Anything more required before committing it?
Thanks,
Ben
Comment 11 Ben Woods freebsd_committer 2016-05-06 22:25:08 UTC
Hi Marko / Bjoern,
Any chance we could consider committing this soon, to ensure it is fixed in time for the FreeBSD 11 code freeze in 1 month?
Regards,
Ben
Comment 12 Bjoern A. Zeeb freebsd_committer 2018-11-02 14:47:23 UTC
Hi Ben,

sorry this never made it.  I was looking for this change in HEAD the other day to get a reference for https://svnweb.freebsd.org/base?view=revision&revision=339930 and couldn't find it.  Only going through a lot of open PRs I now see that it was never committed.  I'll go and have a look even though it's probably no longer needed given the aforementioned change.

You didn't by any chance keep the patch up-to-date?
Comment 13 Bjoern A. Zeeb freebsd_committer 2019-11-19 13:39:21 UTC
*** Bug 242035 has been marked as a duplicate of this bug. ***
Comment 14 Bjoern A. Zeeb freebsd_committer 2019-11-19 13:55:04 UTC
I've updated the patch and put it into review at:

https://reviews.freebsd.org/D22443


You can download it from there to test (I know it compiles) by clicking "Download Raw Diff" on the right side menu.
Comment 15 commit-hook freebsd_committer 2019-11-19 15:39:37 UTC
A commit references this bug:

Author: bz
Date: Tue Nov 19 15:38:55 UTC 2019
New revision: 354857
URL: https://svnweb.freebsd.org/changeset/base/354857

Log:
  Reduce the vnet_set module size of ip_mroute to allow loading as a module.

  With VIMAGE kernels modules get special treatment as they need
  to also keep the original values and make copies for each instance.
  For that a few pages of vnet modspace are provided and the
  kernel-linker and the VNET framework know how to deal with things.
  When the modspace is (almost) full, other modules which would
  overflow the modspace cannot be loaded and kldload will fail.

  ip_mroute uses a lot of variable space, mostly be four big arrays:
  set_vnet 0000000000000510 vnet_entry_multicast_register_if
  set_vnet 0000000000000700 vnet_entry_viftable
  set_vnet 0000000000002000 vnet_entry_bw_meter_timers
  set_vnet 0000000000002800 vnet_entry_bw_upcalls

  Dynamically malloc the three big ones for each instance we need
  and free them again on vnet teardown (the 4th is an ifnet).
  That way they only need module space for a single pointer and
  allow a lot more modules using virtualized variables to be loaded
  on a VNET kernel.

  PR:		206583
  Reviewed by:	hselasky, kp
  MFC after:	3 weeks
  Differential Revision:	https://reviews.freebsd.org/D22443

Changes:
  head/sys/netinet/ip_mroute.c
Comment 16 commit-hook freebsd_committer 2020-01-11 00:08:39 UTC
A commit references this bug:

Author: bz
Date: Sat Jan 11 00:08:16 UTC 2020
New revision: 356621
URL: https://svnweb.freebsd.org/changeset/base/356621

Log:
  MFC r354857:

  Reduce the vnet_set module size of ip_mroute to allow loading as a module.

    With VIMAGE kernels modules get special treatment as they need
    to also keep the original values and make copies for each instance.
    For that a few pages of vnet modspace are provided and the
    kernel-linker and the VNET framework know how to deal with things.
    When the modspace is (almost) full, other modules which would
    overflow the modspace cannot be loaded and kldload will fail.

    ip_mroute uses a lot of variable space, mostly be four big arrays:
    set_vnet 0000000000000510 vnet_entry_multicast_register_if
    set_vnet 0000000000000700 vnet_entry_viftable
    set_vnet 0000000000002000 vnet_entry_bw_meter_timers
    set_vnet 0000000000002800 vnet_entry_bw_upcalls

    Dynamically malloc the three big ones for each instance we need
    and free them again on vnet teardown (the 4th is an ifnet).
    That way they only need module space for a single pointer and
    allow a lot more modules using virtualized variables to be loaded
    on a VNET kernel.

    PR:		206583

Changes:
_U  stable/12/
  stable/12/sys/netinet/ip_mroute.c
Comment 17 Bjoern A. Zeeb freebsd_committer 2020-01-11 01:23:45 UTC
Thank you for reporting.
Sorry this got lost and took so long to fix.
It's at least sorted in head and 12 now for the next 12 release.
Comment 18 Ben Woods freebsd_committer 2020-01-11 08:26:17 UTC
Thanks so much for following up and fixing it!
Comment 19 commit-hook freebsd_committer 2020-06-17 21:05:25 UTC
A commit references this bug:

Author: bz
Date: Wed Jun 17 21:04:39 UTC 2020
New revision: 362289
URL: https://svnweb.freebsd.org/changeset/base/362289

Log:
  When converting the static arrays to mallocarray() in r356621 I missed
  one place where we now need to multiply the size of the struct with the
  number of entries.  This lead to problems when restarting user space
  daemons, as the cleanup was never properly done, resulting in MRT_ADD_VIF
  EADDRINUSE.
  Properly zero all array elements to avoid this problem.

  PR:		246629, 206583
  Reported by:	(many)
  MFC after:	4 days
  Sponsored by:	Rubicon Communications, LLC (d/b/a "Netgate")

Changes:
  head/sys/netinet/ip_mroute.c
Comment 20 commit-hook freebsd_committer 2020-06-21 11:49:47 UTC
A commit references this bug:

Author: bz
Date: Sun Jun 21 11:48:55 UTC 2020
New revision: 362465
URL: https://svnweb.freebsd.org/changeset/base/362465

Log:
  MFC r362289:

    When converting the static arrays to mallocarray() in r356621 I missed
    one place where we now need to multiply the size of the struct with the
    number of entries.  This lead to problems when restarting user space
    daemons, as the cleanup was never properly done, resulting in MRT_ADD_VIF
    EADDRINUSE.
    Properly zero all array elements to avoid this problem.

  PR:		246629, 206583

Changes:
_U  stable/12/
  stable/12/sys/netinet/ip_mroute.c
Comment 21 commit-hook freebsd_committer 2020-06-21 22:09:50 UTC
A commit references this bug:

Author: bz
Date: Sun Jun 21 22:09:30 UTC 2020
New revision: 362472
URL: https://svnweb.freebsd.org/changeset/base/362472

Log:
  Rather than zeroing MAXVIFS times size of pointer [r362289] (still better than
  sizeof pointer before [r354857]), we need to zero MAXVIFS times the size of
  the struct.  All good things come in threes; I hope this is it on this one.

  PR:		246629, 206583
  Reported by:	kib
  MFC after:	ASAP

Changes:
  head/sys/netinet/ip_mroute.c
Comment 22 commit-hook freebsd_committer 2020-06-22 10:53:18 UTC
A commit references this bug:

Author: bz
Date: Mon Jun 22 10:52:31 UTC 2020
New revision: 362494
URL: https://svnweb.freebsd.org/changeset/base/362494

Log:
  MFC r362472:

    Rather than zeroing MAXVIFS times size of pointer [r362289] (still better than
    sizeof pointer before [r354857]), we need to zero MAXVIFS times the size of
    the struct.  All good things come in threes; I hope this is it on this one.

  PR:		246629, 206583
  Reported by:	kib

Changes:
  stable/12/sys/netinet/ip_mroute.c
Comment 23 Sergei Masharov 2020-07-31 07:07:48 UTC
Is it normal that I see the problem in the 12.1-RELEASE-p7 ?

# kldload ip_mroute
kldload: an error occurred while loading module ip_mroute. Please check dmesg(8) for more details.

# dmesg
link_elf_load_file: vnet module space is out of space; cannot allocate 0x55d8 for /boot/kernel/ip_mroute.ko
linker_load_file: /boot/kernel/ip_mroute.ko - unsupported file type


# kldstat
Id Refs Address                Size Name
 1   14 0xffffffff80200000  2448f20 kernel
 2    1 0xffffffff8264a000   3a99a8 zfs.ko
 3    2 0xffffffff829f4000     a5b8 opensolaris.ko
 5    1 0xffffffff82c21000     a210 if_lagg.ko
 6    1 0xffffffff82c2c000    32830 pf.ko
Comment 24 Bjoern A. Zeeb freebsd_committer 2020-07-31 09:50:26 UTC
(In reply to Sergei Masharov from comment #23)

This was only fixed after 12.1 RELEASE and there was no Errata Notice for it so it is fixed only in 12-STABLE or will be in the next upcoming 12.2 release ( https://www.freebsd.org/releases/12.2R/schedule.html )