[v2] Makefile: install modules.builtin even if CONFIG_MODULES=n
diff mbox series

Message ID 3cd1a050fe692425352745672295033c@talpidae.net
State New
Headers show
Series
  • [v2] Makefile: install modules.builtin even if CONFIG_MODULES=n
Related show

Commit Message

Jonas Zeiger June 3, 2020, 1:34 p.m. UTC
Many applications check for available kernel features via:

  - /proc/modules (loaded modules, present if CONFIG_MODULES=y)
  - $(MODLIB)/modules.builtin (builtin modules)

They fail to detect features if the kernel was built with 
CONFIG_MODULES=n
and modules.builtin isn't installed.

Therefore, add the target "_builtin_inst_" and make "install" and
"modules_install" depend on it.

Tests results:

  - make install: kernel image is copied as before, modules.builtin 
copied
  - make modules_install: (CONFIG_MODULES=n) nothing is copied, exit 1

Signed-off-by: Jonas Zeiger <jonas.zeiger@talpidae.net>
---
  Makefile | 14 +++++++++++---
  1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Masahiro Yamada June 4, 2020, 12:33 a.m. UTC | #1
On Wed, Jun 3, 2020 at 10:34 PM Jonas Zeiger <jonas.zeiger@talpidae.net> wrote:
>
> Many applications check for available kernel features via:
>
>   - /proc/modules (loaded modules, present if CONFIG_MODULES=y)
>   - $(MODLIB)/modules.builtin (builtin modules)
>
> They fail to detect features if the kernel was built with
> CONFIG_MODULES=n
> and modules.builtin isn't installed.
>
> Therefore, add the target "_builtin_inst_" and make "install" and
> "modules_install" depend on it.
>
> Tests results:
>
>   - make install: kernel image is copied as before, modules.builtin
> copied
>   - make modules_install: (CONFIG_MODULES=n) nothing is copied, exit 1
>
> Signed-off-by: Jonas Zeiger <jonas.zeiger@talpidae.net>
> ---

Applied to linux-kbuild,
but this patch format is broken.

I manually fixed it up, but
please use 'git send-email' next time.




>   Makefile | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index a7bc91cbac8f..a160efd62897 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1315,6 +1315,16 @@ dt_binding_check: scripts_dtc
>   #
> ---------------------------------------------------------------------------
>   # Modules
>
> +# install modules.builtin regardless of CONFIG_MODULES
> +PHONY += _builtin_inst_
> +_builtin_inst_:
> +       @mkdir -p $(MODLIB)/
> +       @cp -f modules.builtin $(MODLIB)/
> +       @cp -f $(objtree)/modules.builtin.modinfo $(MODLIB)/
> +
> +PHONY += install
> +install: _builtin_inst_
> +
>   ifdef CONFIG_MODULES
>
>   # By default, build modules as well
> @@ -1344,7 +1354,7 @@ PHONY += modules_install
>   modules_install: _modinst_ _modinst_post
>
>   PHONY += _modinst_
> -_modinst_:
> +_modinst_: _builtin_inst_
>         @rm -rf $(MODLIB)/kernel
>         @rm -f $(MODLIB)/source
>         @mkdir -p $(MODLIB)/kernel
> @@ -1354,8 +1364,6 @@ _modinst_:
>                 ln -s $(CURDIR) $(MODLIB)/build ; \
>         fi
>         @sed 's:^:kernel/:' modules.order > $(MODLIB)/modules.order
> -       @cp -f modules.builtin $(MODLIB)/
> -       @cp -f $(objtree)/modules.builtin.modinfo $(MODLIB)/
>         $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modinst
>
>   # This depmod is only for convenience to give the initial
> --
> 2.26.2
>
Douglas Anderson June 9, 2020, 4:38 p.m. UTC | #2
Hi,

On Tue, Jun 3, 2020 at 9:33 AM Jonas Zeiger <jonas.zeiger@talpidae.net> wrote:
>
> Many applications check for available kernel features via:
>
>   - /proc/modules (loaded modules, present if CONFIG_MODULES=y)
>   - $(MODLIB)/modules.builtin (builtin modules)
>
> They fail to detect features if the kernel was built with
> CONFIG_MODULES=n
> and modules.builtin isn't installed.
>
> Therefore, add the target "_builtin_inst_" and make "install" and
> "modules_install" depend on it.
>
> Tests results:
>
>   - make install: kernel image is copied as before, modules.builtin
> copied
>   - make modules_install: (CONFIG_MODULES=n) nothing is copied, exit 1
>
> Signed-off-by: Jonas Zeiger <jonas.zeiger@talpidae.net>
> ---
>   Makefile | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)

Note that this change broke builds in the Chrome OS build system
because we require modules to be installed to a certain path and we
weren't passing "INSTALL_MOD_PATH" when we called "make install".

We can certainly fix our build system (I have a patch at
https://crrev.com/c/2237511 for it), but I do wonder if others will
hit the same issue.  Others might not have such a nice sandboxing
system so they might unknowingly try to install files to the build
computer's modules directory instead of their target.

-Doug
Guenter Roeck June 9, 2020, 5:31 p.m. UTC | #3
On Tue, Jun 9, 2020 at 9:38 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Jun 3, 2020 at 9:33 AM Jonas Zeiger <jonas.zeiger@talpidae.net> wrote:
> >
> > Many applications check for available kernel features via:
> >
> >   - /proc/modules (loaded modules, present if CONFIG_MODULES=y)
> >   - $(MODLIB)/modules.builtin (builtin modules)
> >
> > They fail to detect features if the kernel was built with
> > CONFIG_MODULES=n
> > and modules.builtin isn't installed.
> >
> > Therefore, add the target "_builtin_inst_" and make "install" and
> > "modules_install" depend on it.
> >
> > Tests results:
> >
> >   - make install: kernel image is copied as before, modules.builtin
> > copied
> >   - make modules_install: (CONFIG_MODULES=n) nothing is copied, exit 1
> >
> > Signed-off-by: Jonas Zeiger <jonas.zeiger@talpidae.net>
> > ---
> >   Makefile | 14 +++++++++++---
> >   1 file changed, 11 insertions(+), 3 deletions(-)
>
> Note that this change broke builds in the Chrome OS build system
> because we require modules to be installed to a certain path and we
> weren't passing "INSTALL_MOD_PATH" when we called "make install".
>
> We can certainly fix our build system (I have a patch at
> https://crrev.com/c/2237511 for it), but I do wonder if others will
> hit the same issue.  Others might not have such a nice sandboxing
> system so they might unknowingly try to install files to the build
> computer's modules directory instead of their target.
>

I am more concerned with people getting errors such as

mkdir: cannot create directory '/lib/modules/5.7.0+/': Permission denied

when running "make install", with no documentation or explanation that
or why INSTALL_MOD_PATH is now mandatory for non-root installations.
Even for root installations, it seems odd that "make install" now
installs module files; after all, this is what "make modules_install"
is for.

I can understand the use case for CONFIG_MODULES=n, but the impact and
changed behavior on systems with CONFIG_MODULES=y is quite unexpected.

Guenter
Masahiro Yamada June 12, 2020, 6:56 a.m. UTC | #4
On Wed, Jun 10, 2020 at 2:31 AM Guenter Roeck <groeck@google.com> wrote:
>
> On Tue, Jun 9, 2020 at 9:38 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Tue, Jun 3, 2020 at 9:33 AM Jonas Zeiger <jonas.zeiger@talpidae.net> wrote:
> > >
> > > Many applications check for available kernel features via:
> > >
> > >   - /proc/modules (loaded modules, present if CONFIG_MODULES=y)
> > >   - $(MODLIB)/modules.builtin (builtin modules)
> > >
> > > They fail to detect features if the kernel was built with
> > > CONFIG_MODULES=n
> > > and modules.builtin isn't installed.
> > >
> > > Therefore, add the target "_builtin_inst_" and make "install" and
> > > "modules_install" depend on it.
> > >
> > > Tests results:
> > >
> > >   - make install: kernel image is copied as before, modules.builtin
> > > copied
> > >   - make modules_install: (CONFIG_MODULES=n) nothing is copied, exit 1
> > >
> > > Signed-off-by: Jonas Zeiger <jonas.zeiger@talpidae.net>
> > > ---
> > >   Makefile | 14 +++++++++++---
> > >   1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > Note that this change broke builds in the Chrome OS build system
> > because we require modules to be installed to a certain path and we
> > weren't passing "INSTALL_MOD_PATH" when we called "make install".
> >
> > We can certainly fix our build system (I have a patch at
> > https://crrev.com/c/2237511 for it), but I do wonder if others will
> > hit the same issue.  Others might not have such a nice sandboxing
> > system so they might unknowingly try to install files to the build
> > computer's modules directory instead of their target.
> >
>
> I am more concerned with people getting errors such as
>
> mkdir: cannot create directory '/lib/modules/5.7.0+/': Permission denied
>
> when running "make install", with no documentation or explanation that
> or why INSTALL_MOD_PATH is now mandatory for non-root installations.
> Even for root installations, it seems odd that "make install" now
> installs module files; after all, this is what "make modules_install"
> is for.
>
> I can understand the use case for CONFIG_MODULES=n, but the impact and
> changed behavior on systems with CONFIG_MODULES=y is quite unexpected.
>
> Guenter


Sorry, I led this patch in a wrong way.

Maybe, we should allow 'make modules_install' for CONFIG_MODULES=n
as Jonas did in v1.


Another way might be to install it
in /boot/modules.builtin.(ver) when CONFIG_MODULES=n
but checking multiple locations would be inconvenient.
Guenter Roeck June 12, 2020, 1:31 p.m. UTC | #5
On Thu, Jun 11, 2020 at 11:57 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Wed, Jun 10, 2020 at 2:31 AM Guenter Roeck <groeck@google.com> wrote:
> >
> > On Tue, Jun 9, 2020 at 9:38 AM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Jun 3, 2020 at 9:33 AM Jonas Zeiger <jonas.zeiger@talpidae.net> wrote:
> > > >
> > > > Many applications check for available kernel features via:
> > > >
> > > >   - /proc/modules (loaded modules, present if CONFIG_MODULES=y)
> > > >   - $(MODLIB)/modules.builtin (builtin modules)
> > > >
> > > > They fail to detect features if the kernel was built with
> > > > CONFIG_MODULES=n
> > > > and modules.builtin isn't installed.
> > > >
> > > > Therefore, add the target "_builtin_inst_" and make "install" and
> > > > "modules_install" depend on it.
> > > >
> > > > Tests results:
> > > >
> > > >   - make install: kernel image is copied as before, modules.builtin
> > > > copied
> > > >   - make modules_install: (CONFIG_MODULES=n) nothing is copied, exit 1
> > > >
> > > > Signed-off-by: Jonas Zeiger <jonas.zeiger@talpidae.net>
> > > > ---
> > > >   Makefile | 14 +++++++++++---
> > > >   1 file changed, 11 insertions(+), 3 deletions(-)
> > >
> > > Note that this change broke builds in the Chrome OS build system
> > > because we require modules to be installed to a certain path and we
> > > weren't passing "INSTALL_MOD_PATH" when we called "make install".
> > >
> > > We can certainly fix our build system (I have a patch at
> > > https://crrev.com/c/2237511 for it), but I do wonder if others will
> > > hit the same issue.  Others might not have such a nice sandboxing
> > > system so they might unknowingly try to install files to the build
> > > computer's modules directory instead of their target.
> > >
> >
> > I am more concerned with people getting errors such as
> >
> > mkdir: cannot create directory '/lib/modules/5.7.0+/': Permission denied
> >
> > when running "make install", with no documentation or explanation that
> > or why INSTALL_MOD_PATH is now mandatory for non-root installations.
> > Even for root installations, it seems odd that "make install" now
> > installs module files; after all, this is what "make modules_install"
> > is for.
> >
> > I can understand the use case for CONFIG_MODULES=n, but the impact and
> > changed behavior on systems with CONFIG_MODULES=y is quite unexpected.
> >
> > Guenter
>
>
> Sorry, I led this patch in a wrong way.
>
> Maybe, we should allow 'make modules_install' for CONFIG_MODULES=n
> as Jonas did in v1.
>

Personally I think that would have been much better than the current
solution. It would specifically give people the choice to install
those files if they want to, without forcing others to do it (and to
do it in an unrelated context/command).

Thanks,
Guenter

>
> Another way might be to install it
> in /boot/modules.builtin.(ver) when CONFIG_MODULES=n
> but checking multiple locations would be inconvenient.
>
>
>
> --
> Best Regards
> Masahiro Yamada
Jonas Zeiger June 12, 2020, 3:35 p.m. UTC | #6
On 2020-06-12 08:56, Masahiro Yamada wrote:
> On Wed, Jun 10, 2020 at 2:31 AM Guenter Roeck <groeck@google.com> 
> wrote:
>> 
>> On Tue, Jun 9, 2020 at 9:38 AM Doug Anderson <dianders@chromium.org> 
>> wrote:
>> >
>> > Hi,
>> >
>> > On Tue, Jun 3, 2020 at 9:33 AM Jonas Zeiger <jonas.zeiger@talpidae.net> wrote:
>> > >
>> > > Many applications check for available kernel features via:
>> > >
>> > >   - /proc/modules (loaded modules, present if CONFIG_MODULES=y)
>> > >   - $(MODLIB)/modules.builtin (builtin modules)
>> > >
>> > > They fail to detect features if the kernel was built with
>> > > CONFIG_MODULES=n
>> > > and modules.builtin isn't installed.
>> > >
>> > > Therefore, add the target "_builtin_inst_" and make "install" and
>> > > "modules_install" depend on it.
>> > >
>> > > Tests results:
>> > >
>> > >   - make install: kernel image is copied as before, modules.builtin
>> > > copied
>> > >   - make modules_install: (CONFIG_MODULES=n) nothing is copied, exit 1
>> > >
>> > > Signed-off-by: Jonas Zeiger <jonas.zeiger@talpidae.net>
>> > > ---
>> > >   Makefile | 14 +++++++++++---
>> > >   1 file changed, 11 insertions(+), 3 deletions(-)
>> >
>> > Note that this change broke builds in the Chrome OS build system
>> > because we require modules to be installed to a certain path and we
>> > weren't passing "INSTALL_MOD_PATH" when we called "make install".
>> >
>> > We can certainly fix our build system (I have a patch at
>> > https://crrev.com/c/2237511 for it), but I do wonder if others will
>> > hit the same issue.  Others might not have such a nice sandboxing
>> > system so they might unknowingly try to install files to the build
>> > computer's modules directory instead of their target.
>> >
>> 
>> I am more concerned with people getting errors such as
>> 
>> mkdir: cannot create directory '/lib/modules/5.7.0+/': Permission 
>> denied
>> 
>> when running "make install", with no documentation or explanation that
>> or why INSTALL_MOD_PATH is now mandatory for non-root installations.
>> Even for root installations, it seems odd that "make install" now
>> installs module files; after all, this is what "make modules_install"
>> is for.
>> 
>> I can understand the use case for CONFIG_MODULES=n, but the impact and
>> changed behavior on systems with CONFIG_MODULES=y is quite unexpected.
>> 
>> Guenter
> 
> 
> Sorry, I led this patch in a wrong way.
> 
> Maybe, we should allow 'make modules_install' for CONFIG_MODULES=n
> as Jonas did in v1.
> 
> 
> Another way might be to install it
> in /boot/modules.builtin.(ver) when CONFIG_MODULES=n
> but checking multiple locations would be inconvenient.

I have noticed that my build system specified INSTALL_MOD_PATH for "make 
install", so the patch doesn't cause issues in my environment.

However, I should have noticed that the change is breaking some existing 
setups.

Masahiro, I still believe that the approach you favored (v2) makes more 
sense architecturally, but at this point it seems that v1 is more 
pragmatic.

Would you agree to revert v2 and apply v1 instead?

I will fix issues that may come up with v1, however unlikely.
Masahiro Yamada June 13, 2020, 2:58 a.m. UTC | #7
On Sat, Jun 13, 2020 at 12:35 AM Jonas Zeiger <jonas.zeiger@talpidae.net> wrote:
>
> On 2020-06-12 08:56, Masahiro Yamada wrote:
> > On Wed, Jun 10, 2020 at 2:31 AM Guenter Roeck <groeck@google.com>
> > wrote:
> >>
> >> On Tue, Jun 9, 2020 at 9:38 AM Doug Anderson <dianders@chromium.org>
> >> wrote:
> >> >
> >> > Hi,
> >> >
> >> > On Tue, Jun 3, 2020 at 9:33 AM Jonas Zeiger <jonas.zeiger@talpidae.net> wrote:
> >> > >
> >> > > Many applications check for available kernel features via:
> >> > >
> >> > >   - /proc/modules (loaded modules, present if CONFIG_MODULES=y)
> >> > >   - $(MODLIB)/modules.builtin (builtin modules)
> >> > >
> >> > > They fail to detect features if the kernel was built with
> >> > > CONFIG_MODULES=n
> >> > > and modules.builtin isn't installed.
> >> > >
> >> > > Therefore, add the target "_builtin_inst_" and make "install" and
> >> > > "modules_install" depend on it.
> >> > >
> >> > > Tests results:
> >> > >
> >> > >   - make install: kernel image is copied as before, modules.builtin
> >> > > copied
> >> > >   - make modules_install: (CONFIG_MODULES=n) nothing is copied, exit 1
> >> > >
> >> > > Signed-off-by: Jonas Zeiger <jonas.zeiger@talpidae.net>
> >> > > ---
> >> > >   Makefile | 14 +++++++++++---
> >> > >   1 file changed, 11 insertions(+), 3 deletions(-)
> >> >
> >> > Note that this change broke builds in the Chrome OS build system
> >> > because we require modules to be installed to a certain path and we
> >> > weren't passing "INSTALL_MOD_PATH" when we called "make install".
> >> >
> >> > We can certainly fix our build system (I have a patch at
> >> > https://crrev.com/c/2237511 for it), but I do wonder if others will
> >> > hit the same issue.  Others might not have such a nice sandboxing
> >> > system so they might unknowingly try to install files to the build
> >> > computer's modules directory instead of their target.
> >> >
> >>
> >> I am more concerned with people getting errors such as
> >>
> >> mkdir: cannot create directory '/lib/modules/5.7.0+/': Permission
> >> denied
> >>
> >> when running "make install", with no documentation or explanation that
> >> or why INSTALL_MOD_PATH is now mandatory for non-root installations.
> >> Even for root installations, it seems odd that "make install" now
> >> installs module files; after all, this is what "make modules_install"
> >> is for.
> >>
> >> I can understand the use case for CONFIG_MODULES=n, but the impact and
> >> changed behavior on systems with CONFIG_MODULES=y is quite unexpected.
> >>
> >> Guenter
> >
> >
> > Sorry, I led this patch in a wrong way.
> >
> > Maybe, we should allow 'make modules_install' for CONFIG_MODULES=n
> > as Jonas did in v1.
> >
> >
> > Another way might be to install it
> > in /boot/modules.builtin.(ver) when CONFIG_MODULES=n
> > but checking multiple locations would be inconvenient.
>
> I have noticed that my build system specified INSTALL_MOD_PATH for "make
> install", so the patch doesn't cause issues in my environment.
>
> However, I should have noticed that the change is breaking some existing
> setups.
>
> Masahiro, I still believe that the approach you favored (v2) makes more
> sense architecturally, but at this point it seems that v1 is more
> pragmatic.
>
> Would you agree to revert v2 and apply v1 instead?


Yes, I agree.

Thanks.


> I will fix issues that may come up with v1, however unlikely.
>

Patch
diff mbox series

diff --git a/Makefile b/Makefile
index a7bc91cbac8f..a160efd62897 100644
--- a/Makefile
+++ b/Makefile
@@ -1315,6 +1315,16 @@  dt_binding_check: scripts_dtc
  # 
---------------------------------------------------------------------------
  # Modules

+# install modules.builtin regardless of CONFIG_MODULES
+PHONY += _builtin_inst_
+_builtin_inst_:
+	@mkdir -p $(MODLIB)/
+	@cp -f modules.builtin $(MODLIB)/
+	@cp -f $(objtree)/modules.builtin.modinfo $(MODLIB)/
+
+PHONY += install
+install: _builtin_inst_
+
  ifdef CONFIG_MODULES

  # By default, build modules as well
@@ -1344,7 +1354,7 @@  PHONY += modules_install
  modules_install: _modinst_ _modinst_post

  PHONY += _modinst_
-_modinst_:
+_modinst_: _builtin_inst_
  	@rm -rf $(MODLIB)/kernel
  	@rm -f $(MODLIB)/source
  	@mkdir -p $(MODLIB)/kernel
@@ -1354,8 +1364,6 @@  _modinst_:
  		ln -s $(CURDIR) $(MODLIB)/build ; \
  	fi
  	@sed 's:^:kernel/:' modules.order > $(MODLIB)/modules.order
-	@cp -f modules.builtin $(MODLIB)/
-	@cp -f $(objtree)/modules.builtin.modinfo $(MODLIB)/
  	$(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modinst

  # This depmod is only for convenience to give the initial