diff mbox series

[01/16] s390/boot: fix section name escaping

Message ID 20190812215052.71840-1-ndesaulniers@google.com
State Superseded
Headers show
Series [01/16] s390/boot: fix section name escaping | expand

Commit Message

Nick Desaulniers Aug. 12, 2019, 9:50 p.m. UTC 
GCC unescapes escaped string section names while Clang does not. Because
__section uses the `#` stringification operator for the section name, it
doesn't need to be escaped.

This antipattern was found with:
$ grep -e __section\(\" -e __section__\(\" -r

Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

---
 arch/s390/boot/startup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.23.0.rc1.153.gdeed80330f-goog

Comments

Nick Desaulniers Aug. 12, 2019, 9:57 p.m. UTC | #1 
---------- Forwarded message ---------
From: Nick Desaulniers <ndesaulniers@google.com>

Date: Mon, Aug 12, 2019 at 2:53 PM
Subject: [PATCH 00/16] treewide: prefer __section from compiler_attributes.h
To: <akpm@linux-foundation.org>
Cc: <sedat.dilek@gmail.com>, <jpoimboe@redhat.com>, <yhs@fb.com>,
<miguel.ojeda.sandonis@gmail.com>,
<clang-built-linux@googlegroups.com>, Nick Desaulniers
<ndesaulniers@google.com>, Alexei Starovoitov <ast@kernel.org>, Daniel
Borkmann <daniel@iogearbox.net>, Martin KaFai Lau <kafai@fb.com>, Song
Liu <songliubraving@fb.com>, <netdev@vger.kernel.org>,
<bpf@vger.kernel.org>


GCC unescapes escaped string section names while Clang does not. Because
__section uses the `#` stringification operator for the section name, it
doesn't need to be escaped.

This fixes an Oops observed in distro's that use systemd and not
net.core.bpf_jit_enable=1, when their kernels are compiled with Clang.

Instead, we should:
1. Prefer __section(.section_name_no_quotes).
2. Only use __attribute__((__section(".section"))) when creating the
section name via C preprocessor (see the definition of __define_initcall
in arch/um/include/shared/init.h).

This antipattern was found with:
$ grep -e __section\(\" -e __section__\(\" -r

See the discussions in:
https://bugs.llvm.org/show_bug.cgi?id=42950
https://marc.info/?l=linux-netdev&m=156412960619946&w=2

Nick Desaulniers (16):
  s390/boot: fix section name escaping
  arc: prefer __section from compiler_attributes.h
  parisc: prefer __section from compiler_attributes.h
  um: prefer __section from compiler_attributes.h
  sh: prefer __section from compiler_attributes.h
  ia64: prefer __section from compiler_attributes.h
  arm: prefer __section from compiler_attributes.h
  mips: prefer __section from compiler_attributes.h
  sparc: prefer __section from compiler_attributes.h
  powerpc: prefer __section and __printf from compiler_attributes.h
  x86: prefer __section from compiler_attributes.h
  arm64: prefer __section from compiler_attributes.h
  include/asm-generic: prefer __section from compiler_attributes.h
  include/linux: prefer __section from compiler_attributes.h
  include/linux/compiler.h: remove unused KENTRY macro
  compiler_attributes.h: add note about __section

 arch/arc/include/asm/linkage.h        |  8 +++----
 arch/arc/include/asm/mach_desc.h      |  3 +--
 arch/arm/include/asm/cache.h          |  2 +-
 arch/arm/include/asm/mach/arch.h      |  4 ++--
 arch/arm/include/asm/setup.h          |  2 +-
 arch/arm64/include/asm/cache.h        |  2 +-
 arch/arm64/kernel/smp_spin_table.c    |  2 +-
 arch/ia64/include/asm/cache.h         |  2 +-
 arch/mips/include/asm/cache.h         |  2 +-
 arch/parisc/include/asm/cache.h       |  2 +-
 arch/parisc/include/asm/ldcw.h        |  2 +-
 arch/powerpc/boot/main.c              |  3 +--
 arch/powerpc/boot/ps3.c               |  6 ++----
 arch/powerpc/include/asm/cache.h      |  2 +-
 arch/powerpc/kernel/btext.c           |  2 +-
 arch/s390/boot/startup.c              |  2 +-
 arch/sh/include/asm/cache.h           |  2 +-
 arch/sparc/include/asm/cache.h        |  2 +-
 arch/sparc/kernel/btext.c             |  2 +-
 arch/um/kernel/um_arch.c              |  6 +++---
 arch/x86/include/asm/cache.h          |  2 +-
 arch/x86/include/asm/intel-mid.h      |  2 +-
 arch/x86/include/asm/iommu_table.h    |  5 ++---
 arch/x86/include/asm/irqflags.h       |  2 +-
 arch/x86/include/asm/mem_encrypt.h    |  2 +-
 arch/x86/kernel/cpu/cpu.h             |  3 +--
 include/asm-generic/error-injection.h |  2 +-
 include/asm-generic/kprobes.h         |  5 ++---
 include/linux/cache.h                 |  6 +++---
 include/linux/compiler.h              | 31 ++++-----------------------
 include/linux/compiler_attributes.h   | 10 +++++++++
 include/linux/cpu.h                   |  2 +-
 include/linux/export.h                |  2 +-
 include/linux/init_task.h             |  4 ++--
 include/linux/interrupt.h             |  5 ++---
 include/linux/sched/debug.h           |  2 +-
 include/linux/srcutree.h              |  2 +-
 37 files changed, 62 insertions(+), 83 deletions(-)

--
2.23.0.rc1.153.gdeed80330f-goog



-- 
Thanks,
~Nick Desaulniers
David Miller Aug. 12, 2019, 10:13 p.m. UTC | #2 
From: Nick Desaulniers <ndesaulniers@google.com>

Date: Mon, 12 Aug 2019 14:50:42 -0700

> Reported-by: Sedat Dilek <sedat.dilek@gmail.com>

> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>

> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>


Acked-by: David S. Miller <davem@davemloft.net>
Paul Burton Aug. 15, 2019, 9:38 a.m. UTC | #3 
Hi Nick,

On Mon, Aug 12, 2019 at 02:50:41PM -0700, Nick Desaulniers wrote:
> Reported-by: Sedat Dilek <sedat.dilek@gmail.com>

> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>

> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>


It would be good to add a commit message, even if it's just a line
repeating the subject & preferably describing the motivation.

> ---

>  arch/mips/include/asm/cache.h | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/arch/mips/include/asm/cache.h b/arch/mips/include/asm/cache.h

> index 8b14c2706aa5..af2d943580ee 100644

> --- a/arch/mips/include/asm/cache.h

> +++ b/arch/mips/include/asm/cache.h

> @@ -14,6 +14,6 @@

>  #define L1_CACHE_SHIFT		CONFIG_MIPS_L1_CACHE_SHIFT

>  #define L1_CACHE_BYTES		(1 << L1_CACHE_SHIFT)

>  

> -#define __read_mostly __attribute__((__section__(".data..read_mostly")))

> +#define __read_mostly __section(.data..read_mostly)

>  

>  #endif /* _ASM_CACHE_H */

> -- 

> 2.23.0.rc1.153.gdeed80330f-goog


I'm not copied on the rest of the series so I'm not sure what your
expectations are about where this should be applied. Let me know if
you'd prefer this to go through mips-next, otherwise:

    Acked-by: Paul Burton <paul.burton@mips.com>


Thanks,
    Paul
Thomas Gleixner Aug. 19, 2019, 10:31 a.m. UTC | #4 
Nick,

On Mon, 12 Aug 2019, Nick Desaulniers wrote:

-ECHANGELOG_EMPTY

While I think I know the reason for this change, it's still usefull to have
some explanaiton of WHY this is preferred in the change log.

Thanks,

	tglx
Sedat Dilek Aug. 19, 2019, 5:59 p.m. UTC | #5 
On Mon, Aug 12, 2019 at 11:52 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>

> Reported-by: Sedat Dilek <sedat.dilek@gmail.com>

> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>

> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>


Tested-by: Sedat Dilek <sedat.dilek@gmail.com> [ Linux v5.3-rc5 ]


Patchset "for-5.3/x86-section-name-escaping":

include/linux/compiler.h: remove unused KENTRY macro
include/linux: prefer __section from compiler_attributes.h
include/asm-generic: prefer __section from compiler_attributes.h
x86: prefer __section from compiler_attributes.h

Thanks.

- Sedat -

> ---

>  arch/x86/include/asm/cache.h       | 2 +-

>  arch/x86/include/asm/intel-mid.h   | 2 +-

>  arch/x86/include/asm/iommu_table.h | 5 ++---

>  arch/x86/include/asm/irqflags.h    | 2 +-

>  arch/x86/include/asm/mem_encrypt.h | 2 +-

>  arch/x86/kernel/cpu/cpu.h          | 3 +--

>  6 files changed, 7 insertions(+), 9 deletions(-)

>

> diff --git a/arch/x86/include/asm/cache.h b/arch/x86/include/asm/cache.h

> index abe08690a887..bb9f4bf4ec02 100644

> --- a/arch/x86/include/asm/cache.h

> +++ b/arch/x86/include/asm/cache.h

> @@ -8,7 +8,7 @@

>  #define L1_CACHE_SHIFT (CONFIG_X86_L1_CACHE_SHIFT)

>  #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)

>

> -#define __read_mostly __attribute__((__section__(".data..read_mostly")))

> +#define __read_mostly __section(.data..read_mostly)

>

>  #define INTERNODE_CACHE_SHIFT CONFIG_X86_INTERNODE_CACHE_SHIFT

>  #define INTERNODE_CACHE_BYTES (1 << INTERNODE_CACHE_SHIFT)

> diff --git a/arch/x86/include/asm/intel-mid.h b/arch/x86/include/asm/intel-mid.h

> index 8e5af119dc2d..f51f04aefe1b 100644

> --- a/arch/x86/include/asm/intel-mid.h

> +++ b/arch/x86/include/asm/intel-mid.h

> @@ -43,7 +43,7 @@ struct devs_id {

>

>  #define sfi_device(i)                                                          \

>         static const struct devs_id *const __intel_mid_sfi_##i##_dev __used     \

> -       __attribute__((__section__(".x86_intel_mid_dev.init"))) = &i

> +       __section(.x86_intel_mid_dev.init) = &i

>

>  /**

>  * struct mid_sd_board_info - template for SD device creation

> diff --git a/arch/x86/include/asm/iommu_table.h b/arch/x86/include/asm/iommu_table.h

> index 1fb3fd1a83c2..7d190710eb92 100644

> --- a/arch/x86/include/asm/iommu_table.h

> +++ b/arch/x86/include/asm/iommu_table.h

> @@ -50,9 +50,8 @@ struct iommu_table_entry {

>

>  #define __IOMMU_INIT(_detect, _depend, _early_init, _late_init, _finish)\

>         static const struct iommu_table_entry                           \

> -               __iommu_entry_##_detect __used                          \

> -       __attribute__ ((unused, __section__(".iommu_table"),            \

> -                       aligned((sizeof(void *)))))     \

> +               __iommu_entry_##_detect __used __section(.iommu_table)  \

> +               __aligned((sizeof(void *)))                             \

>         = {_detect, _depend, _early_init, _late_init,                   \

>            _finish ? IOMMU_FINISH_IF_DETECTED : 0}

>  /*

> diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h

> index 8a0e56e1dcc9..68db90bca813 100644

> --- a/arch/x86/include/asm/irqflags.h

> +++ b/arch/x86/include/asm/irqflags.h

> @@ -9,7 +9,7 @@

>  #include <asm/nospec-branch.h>

>

>  /* Provide __cpuidle; we can't safely include <linux/cpu.h> */

> -#define __cpuidle __attribute__((__section__(".cpuidle.text")))

> +#define __cpuidle __section(.cpuidle.text)

>

>  /*

>   * Interrupt control:

> diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h

> index 0c196c47d621..db2cd3709148 100644

> --- a/arch/x86/include/asm/mem_encrypt.h

> +++ b/arch/x86/include/asm/mem_encrypt.h

> @@ -50,7 +50,7 @@ void __init mem_encrypt_free_decrypted_mem(void);

>  bool sme_active(void);

>  bool sev_active(void);

>

> -#define __bss_decrypted __attribute__((__section__(".bss..decrypted")))

> +#define __bss_decrypted __section(.bss..decrypted)

>

>  #else  /* !CONFIG_AMD_MEM_ENCRYPT */

>

> diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h

> index c0e2407abdd6..7ff9dc41a603 100644

> --- a/arch/x86/kernel/cpu/cpu.h

> +++ b/arch/x86/kernel/cpu/cpu.h

> @@ -38,8 +38,7 @@ struct _tlb_table {

>

>  #define cpu_dev_register(cpu_devX) \

>         static const struct cpu_dev *const __cpu_dev_##cpu_devX __used \

> -       __attribute__((__section__(".x86_cpu_dev.init"))) = \

> -       &cpu_devX;

> +       __section(.x86_cpu_dev.init) = &cpu_devX;

>

>  extern const struct cpu_dev *const __x86_cpu_dev_start[],

>                             *const __x86_cpu_dev_end[];

> --

> 2.23.0.rc1.153.gdeed80330f-goog

>
Nick Desaulniers Aug. 27, 2019, 12:19 a.m. UTC | #6 
On Thu, Aug 15, 2019 at 2:38 AM Paul Burton <paul.burton@mips.com> wrote:
>

> Hi Nick,

>

> On Mon, Aug 12, 2019 at 02:50:41PM -0700, Nick Desaulniers wrote:

> > Reported-by: Sedat Dilek <sedat.dilek@gmail.com>

> > Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>

> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

>

> It would be good to add a commit message, even if it's just a line

> repeating the subject & preferably describing the motivation.

>

> > ---

> >  arch/mips/include/asm/cache.h | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> >

> > diff --git a/arch/mips/include/asm/cache.h b/arch/mips/include/asm/cache.h

> > index 8b14c2706aa5..af2d943580ee 100644

> > --- a/arch/mips/include/asm/cache.h

> > +++ b/arch/mips/include/asm/cache.h

> > @@ -14,6 +14,6 @@

> >  #define L1_CACHE_SHIFT               CONFIG_MIPS_L1_CACHE_SHIFT

> >  #define L1_CACHE_BYTES               (1 << L1_CACHE_SHIFT)

> >

> > -#define __read_mostly __attribute__((__section__(".data..read_mostly")))

> > +#define __read_mostly __section(.data..read_mostly)

> >

> >  #endif /* _ASM_CACHE_H */

> > --

> > 2.23.0.rc1.153.gdeed80330f-goog

>

> I'm not copied on the rest of the series so I'm not sure what your

> expectations are about where this should be applied. Let me know if

> you'd prefer this to go through mips-next, otherwise:

>

>     Acked-by: Paul Burton <paul.burton@mips.com>


Thanks Paul, going to send this up via Miguel's tree, if you don't
mind.  Updating my series now.  (Will probably avoid running
get_maintainer.pl on every patch...too hard to cc everyone on the
whole series).
-- 
Thanks,
~Nick Desaulniers
diff mbox series

Patch

diff --git a/arch/s390/boot/startup.c b/arch/s390/boot/startup.c
index 7b0d05414618..26493c4ff04b 100644
--- a/arch/s390/boot/startup.c
+++ b/arch/s390/boot/startup.c
@@ -46,7 +46,7 @@  struct diag_ops __bootdata_preserved(diag_dma_ops) = {
 	.diag0c = _diag0c_dma,
 	.diag308_reset = _diag308_reset_dma
 };
-static struct diag210 _diag210_tmp_dma __section(".dma.data");
+static struct diag210 _diag210_tmp_dma __section(.dma.data);
 struct diag210 *__bootdata_preserved(__diag210_tmp_dma) = &_diag210_tmp_dma;
 void _swsusp_reset_dma(void);
 unsigned long __bootdata_preserved(__swsusp_reset_dma) = __pa(_swsusp_reset_dma);