linux-kernel - Re: [PATCH v2] asm-generic: remove a broken and needless ifdef conditional

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: <Ytq50u0nhkJ1UV0U@bombadil.infradead.org>
Date:   Fri, 22 Jul 2022 07:53:06 -0700
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Lukas Bulwahn <lukas.bulwahn@...il.com>
Cc:     Arnd Bergmann <arnd@...db.de>, linux-arch@...r.kernel.org,
        Palmer Dabbelt <palmer@...belt.com>,
        kernel-janitors@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] asm-generic: remove a broken and needless ifdef
 conditional

On Fri, Jul 22, 2022 at 01:07:11PM +0200, Lukas Bulwahn wrote:
> Commit 527701eda5f1 ("lib: Add a generic version of devmem_is_allowed()")
> introduces the config symbol GENERIC_LIB_DEVMEM_IS_ALLOWED, but then
> falsely refers to CONFIG_GENERIC_DEVMEM_IS_ALLOWED (note the missing LIB
> in the reference) in ./include/asm-generic/io.h.
> 
> Luckily, ./scripts/checkkconfigsymbols.py warns on non-existing configs:
> 
> GENERIC_DEVMEM_IS_ALLOWED
> Referencing files: include/asm-generic/io.h
> 
> The actual fix, though, is simply to not to make this function declaration
> dependent on any kernel config. For architectures that intend to use
> the generic version, the arch's 'select GENERIC_LIB_DEVMEM_IS_ALLOWED' will
> lead to picking the function definition, and for other architectures, this
> function is simply defined elsewhere.
> 
> The wrong '#ifndef' on a non-existing config symbol also always had the
> same effect (although more by mistake than by intent). So, there is no
> functional change.
> 
> Remove this broken and needless ifdef conditional.
> 
> Fixes: 527701eda5f1 ("lib: Add a generic version of devmem_is_allowed()")
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@...il.com>
> ---

There is only one architecture which defines this as a static inline.
Should that architecture include asm-generic/io.h, or if it already
does, it may end up with a compilation error.

So if arch/s390/include/asm/page.h can end up including asm-generic/io.h
or if any file including for s390 can include its arch page.h and
asm-generic/io.h then we'll still need the guard.

This may compile today for s390 because s390 may not use asm-generic/io.h.

So why not just keep the guard and correct this as intended?

  Luis

> v1: https://lore.kernel.org/all/20211006145859.9564-1-lukas.bulwahn@gmail.com/
> 
> Arnd, please pick this v2 for your asm-generic branch. 
> 
> 
>  include/asm-generic/io.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> index ce4c90601300..a68f8fbf423b 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -1221,9 +1221,7 @@ static inline void memcpy_toio(volatile void __iomem *addr, const void *buffer,
>  }
>  #endif
>  
> -#ifndef CONFIG_GENERIC_DEVMEM_IS_ALLOWED
>  extern int devmem_is_allowed(unsigned long pfn);
> -#endif
>  
>  #endif /* __KERNEL__ */
>  
> -- 
> 2.17.1
>