linux-kernel - Re: [PATCH v1] setlocalversion: use ${objtree}/include/config/auto.conf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAABy=s1t0rLJdcXbeoE8E2Dz9zBA8F3Cf0tQPGrM2hgUx=G_8g@mail.gmail.com>
Date: Wed, 12 Mar 2025 12:13:29 -0700
From: "Hong, Yifan" <elsk@...gle.com>
To: Masahiro Yamada <masahiroy@...nel.org>
Cc: Rasmus Villemoes <linux@...musvillemoes.dk>, kernel-team@...roid.com, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] setlocalversion: use ${objtree}/include/config/auto.conf

On Wed, Mar 12, 2025 at 12:31 AM Masahiro Yamada <masahiroy@...nel.org> wrote:
>
> On Wed, Mar 12, 2025 at 11:12 AM HONG Yifan <elsk@...gle.com> wrote:
> >
> > setlocalversion reads include/config/auto.conf, which is located below
> > $(objtree) with commit 214c0eea43b2 ("kbuild: add $(objtree)/ prefix to
> > some in-kernel build artifacts").
> >
> > Hence, the setlocalversion script needs to use
> > $(objtree)/include/config/auto.conf as well.
> >
> > Note that $(objtree) is not necessarily `.` when O (aka KBUILD_OUTPUT)
> > is set, because of commit 13b25489b6f8 ("kbuild: change working
> > directory to external module directory with M=").
>
> Is this a real issue?
> If so, please attach some commands to reproduce an issue.
>
> setlocalversion is invoked only at line 1238 of the top-level Makefile,
> within the check "ifeq ($(KBUILD_EXTMOD),)"
> So, it is never called with external module builds.

Thanks Masahiro. You are right; this is not a real issue as the code
is right now.

In our case, we have this issue because we have
https://lore.kernel.org/all/20210121213641.3477522-1-willmcvicker@google.com/
("[PATCH v6] modules: introduce the MODULE_SCMVERSION config")
in our tree to support the SCM version for modules. The patch was not
accepted so
we applied this patch locally. Hence, technically this patch should also only be
applied locally by us. The paragraph that says "Note that $(objtree) is not
necessarily `.`..." is not correct.

Still, I think it makes sense to be consistent with other places that mentions
include/config/auto.conf.

Should I update the commit message and send another patch? Or would you
like to reject this change?

>
>
>
>
>
>
>
> > Signed-off-by: HONG Yifan <elsk@...gle.com>
> > ---
> > Implementation note: Should I test -z ${objtree} before using it? Otherwise it
> > looks at /include/config/auto.conf which is wrong.
> > Or should I fall back to `.` if objtree is not found in the environment
> > variables?
> > I could also `exit 1` if $objtree is not set. Please let me know what you think.
> >
> >  scripts/setlocalversion | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/setlocalversion b/scripts/setlocalversion
> > index 28169d7e143b..88f54eb5a7c2 100755
> > --- a/scripts/setlocalversion
> > +++ b/scripts/setlocalversion
> > @@ -186,7 +186,7 @@ if ${no_local}; then
> >         exit 0
> >  fi
> >
> > -if ! test -e include/config/auto.conf; then
> > +if ! test -e ${objtree}/include/config/auto.conf; then
> >         echo "Error: kernelrelease not valid - run 'make prepare' to update it" >&2
> >         exit 1
> >  fi
> > --
> > 2.49.0.rc0.332.g42c0ae87b1-goog
> >
>
>
> --
> Best Regards
> Masahiro Yamada