linux-kernel - Re: [PATCH RFC V2] purgatory: fix up declarations

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] [day] [month] [year] [list]
Message-ID: <20170107162232.GA20320@osadl.at>
Date:   Sat, 7 Jan 2017 16:22:32 +0000
From:   Nicholas Mc Guire <der.herr@...r.at>
To:     Dave Young <dyoung@...hat.com>
Cc:     Vivek Goyal <vgoyal@...hat.com>,
        Nicholas Mc Guire <hofrat@...dl.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org, Baoquan He <bhe@...hat.com>
Subject: Re: [PATCH RFC V2] purgatory: fix up declarations

On Wed, Jan 04, 2017 at 02:16:20PM +0800, Dave Young wrote:
> Vivek, thanks for ccing me..
> 
> On 01/03/17 at 04:34pm, Nicholas Mc Guire wrote:
> > On Tue, Jan 03, 2017 at 10:38:14AM -0500, Vivek Goyal wrote:
> > > On Fri, Dec 23, 2016 at 12:43:07PM +0100, Nicholas Mc Guire wrote:
> > > > Add the missing declarations of basic purgatory functions and variables
> > > > used with kexec_purgatory_get_set_symbol() to allow a clean build.
> > > > 
> > > > Fixes: commit 8fc5b4d4121c ("purgatory: core purgatory functionality")
> > > > Signed-off-by: Nicholas Mc Guire <hofrat@...dl.org>
> > > > ---
> > > > 
> > > > V2: after kbuild test robot <lkp@...el.com> reported a build failure
> > > >     removed incorrect declaration of copy_backup_region which is static
> > > >     anyway.
> > > > 
> > > > sparse complained about:
> > > >   CHECK   arch/x86/purgatory/purgatory.c
> > > > arch/x86/purgatory/purgatory.c:21:15: warning: symbol 'backup_dest' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:22:15: warning: symbol 'backup_src' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:23:15: warning: symbol 'backup_sz' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:25:4: warning: symbol 'sha256_digest' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:27:19: warning: symbol 'sha_regions' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:42:5: warning: symbol 'verify_sha256_digest' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:61:6: warning: symbol 'purgatory' was not declared. Should it be static?
> > > >   CC      arch/x86/purgatory/purgatory.o
> > > > 
> > > > Numerous sparse messages regarding functions not being declared, these
> > > > functions are resolved via kexec_purgatory_get_set_symbol() and not
> > > > directly called anywhere.
> > > 
> > > Hi Nicholas,
> > > 
> > > So purgatory() is a separate piece of small binary which does not link
> > > against kernel. And we don't want these symbols static as kernel
> > > obtains the values of these symbols and modifies binary in place on
> > > the fly. I am assuming if we make them static, then we will lose this
> > > capability to be able to read elf headers and be able to modify value
> > > of these symbols.
> > 
> > I don´t understand why this would be lost - the symbols are not being
> > used by kernel code other than kexec code it self -  in what way
> > would declaring them extern change there handling ? 
> > kexec_purgatory_find_symbol is using the elf header to resolve the 
> > symbol location and declaring it extern should not change that in any 
> > way - am I overlooking something ?
> > 
> > > 
> > > Now question is how to supress warnings from sparse. If just declaring
> > > them extern in header file and including that header file in some other
> > > .c file make the sparse warning go away, so be it. Atleast we need
> > > to make explicit comment that this is being done just to take care
> > > of sparse warning.
> > > 
> > > I am not very happy with the solution though. In future it will make
> > > people scratch their head that why are we including this header file
> > > and why some symbols are being declared extern. So if there is another
> > > way to tell sparse to not worry about it, would be even better.
> > >
> > 
> > The assumtion was that these changes would be side-effect free - if they are
> > not then this is probably the wrong path to go - the intent is to remove 
> > the sparse warnings only.
> 
> Another way is do not include the header file, but declare them in the c
> file just for avoiding the sparse warnings with some comments to explain
> it.
>
that would make sparse happy as you suggest but now checkpatch is fussing.

...
WARNING: externs should be avoided in .c files
#62: FILE: arch/x86/purgatory/purgatory.c:27:
+extern unsigned long backup_src;

WARNING: externs should be avoided in .c files
#63: FILE: arch/x86/purgatory/purgatory.c:28:
+extern unsigned long backup_sz;
...

unfortunately it seems that both of these tools do not permit marking 
something as a false positive for this case (__force in sparse will
not work here). At the same time I do think that would not be a very
clean solution ither.

So the alternative solution is to create arch/x86/purgatory.h and put it
all into there - V3 containting that solution just sent out. 

thx!
hofrat