[GIT] kbuild changes for v3.11-rc1

Commit Message

Michal Marek July 11, 2013, 1:54 p.m. UTC

On Wed, Jul 10, 2013 at 07:11:53PM -0700, Linus Torvalds wrote:
> You need to start being more careful. And I would seriously suggest
> you start doing some explicit testing for this. You can do things like
> "find . -user root", and if that shows a single file in the kernel
> tree after a "make [modules_]install", then there's a problem.

Good idea. I added this after the *install calls in my test script.


> Commit d2aae8477cd00325bb7c7c7e95be488088900c48 is broken. It causes
> root to re-write "include/config/kernel.release".

Yeah. It also reveals another bug that we rewrite the kernel.release
file each time. This patch should fix it, but please do not apply it
yet:

From f522eef24216f229c82b6bbb55f326814db2bac0 Mon Sep 17 00:00:00 2001
From: Michal Marek <[e-mail]>
Date: Thu, 11 Jul 2013 15:34:51 +0200
Subject: [PATCH] kbuild: Do not overwrite include/config/kernel.release
 needlessly

Use filechk to detect if the content changed or not.



> There is no excuse for this. That commit is shit. There's no way in
> hell that "make modules_install" should ever rebuild anything, so
> adding that kind of dependency is fundamentally wrong and broken.

We also run dozens of gcc checks during every make invocation, including
make install or make help, so there is some room for improvement. But at
least, these do not write to the source or object tree.

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Linus Torvalds July 11, 2013, 5:16 p.m. UTC | #1

On Thu, Jul 11, 2013 at 6:54 AM, Michal Marek <[e-mail]> wrote:
>
> Yeah. It also reveals another bug that we rewrite the kernel.release
> file each time. This patch should fix it, but please do not apply it
> yet:

So I think not rewriting the file is a good thing, but in this case it
would actually have hidden the real bug.

At least for most people. For me, it would have rewritten the file
anyway, because as a normal user I have

  core.abbrev=12

in my ~/.gitconfig, but when running it as root, it will take the
abbrev setting from the git defaults (which I think is 7). So the
content actually *changes* if root generates the version as opposed to
my normal user account.

Of course, that's arguably something we could fix in our localversion
script (by just forcing a particular abbreviation length), but I
actually like how a plain "git describe" (which will use the
user-specified SHA1 abbreviations) matches "uname -r" (which will
obviously take the version from the kernel version file at
build-time). So I'm not convinced that it is wrong for us to just use
the format that the user specified in their gitconfig, even if it does
result in "odd" situations like this where "make kernelrelease" will
then potentially give different output for different users.

> We also run dozens of gcc checks during every make invocation, including
> make install or make help, so there is some room for improvement. But at
> least, these do not write to the source or object tree.

It would be good to try to minimize these kinds of things, for the
simple reason that I'm not at all sure that we necessarily get all the
temporary file security issues right.

Running as little as possible as root is good practice for a very real
reason: there can be very subtle security issues. Let's not worry
about it *too* much, but keeping it in mind is good.

At least for the gcc checking scripts I looked at, we seem to be doing
things safely (ie using pipes instead of temp-files in /tmp).

              Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Robert Richter July 12, 2013, 10:57 a.m. UTC | #2

On 11.07.13 10:16:18, Linus Torvalds wrote:
> On Thu, Jul 11, 2013 at 6:54 AM, Michal Marek <[e-mail]> wrote:
> >
> > Yeah. It also reveals another bug that we rewrite the kernel.release
> > file each time.

The odd thing I have in a specific configuration is that depmod is
missing the kernelrelease when I only build install rules, though I
had built everything before and kernelrelease should be there:

 $ make <makeflags>
 $ make <makeflags> modules_install zinstall
 ...
   DEPMOD  
 Usage: .../scripts/depmod.sh /sbin/depmod <kernelrelease>

Note that makeflags include the -j option, INSTALL_MOD_PATH and
INSTALL_PATH variables set, so no root perms required in this case.

I agree, the fix to add a dependency to the kernelrelease file is
wrong here since it rewrites the object tree, which should be readonly
for install rules.

I could reproduce the error above in my setup and will look at it
again, hopefully fixing it more carefully.

Appologize the trouble this made.

Thanks,

-Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Makefile b/Makefile
index 4e3575c..3aa232f 100644
--- a/Makefile
+++ b/Makefile
@@ -794,10 +794,13 @@  PHONY += $(vmlinux-dirs)
 $(vmlinux-dirs): prepare scripts
 	$(Q)$(MAKE) $(build)=$@
 
+define filechk_kernel.release
+	echo "$(KERNELVERSION)$$($(CONFIG_SHELL) $(srctree)/scripts/setlocalversion $(srctree))"
+endef
+
 # Store (new) KERNELRELEASE string in include/config/kernel.release
 include/config/kernel.release: include/config/auto.conf FORCE
-	$(Q)rm -f $@
-	$(Q)echo "$(KERNELVERSION)$$($(CONFIG_SHELL) $(srctree)/scripts/setlocalversion $(srctree))" > $@
+	$(call filechk,kernel.release)
 
 
 # Things we need to do before we recursively start building the kernel