| Message ID | 20200629061840.4065483-9-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Warn on orphan section placement | expand |
On Sun, Jun 28, 2020 at 11:18 PM Kees Cook <keescook@chromium.org> wrote: > > Fix a case of needless quotes in __section(), which Clang doesn't like. > > Acked-by: Will Deacon <will@kernel.org> > Signed-off-by: Kees Cook <keescook@chromium.org> Yep, I remember bugs from this. Probably should scan the kernel for other instances of this. +Joe for checkpatch.pl validation. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > --- > arch/arm64/mm/mmu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > index 1df25f26571d..dce024ea6084 100644 > --- a/arch/arm64/mm/mmu.c > +++ b/arch/arm64/mm/mmu.c > @@ -42,7 +42,7 @@ > u64 idmap_t0sz = TCR_T0SZ(VA_BITS); > u64 idmap_ptrs_per_pgd = PTRS_PER_PGD; > > -u64 __section(".mmuoff.data.write") vabits_actual; > +u64 __section(.mmuoff.data.write) vabits_actual; > EXPORT_SYMBOL(vabits_actual); > > u64 kimage_voffset __ro_after_init; > -- > 2.25.1 >
On Mon, 2020-06-29 at 12:53 -0700, Nick Desaulniers wrote: > On Sun, Jun 28, 2020 at 11:18 PM Kees Cook <keescook@chromium.org> wrote: > > Fix a case of needless quotes in __section(), which Clang doesn't like. > > > > Acked-by: Will Deacon <will@kernel.org> > > Signed-off-by: Kees Cook <keescook@chromium.org> > > Yep, I remember bugs from this. Probably should scan the kernel for > other instances of this. +Joe for checkpatch.pl validation. > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> $ git grep -P -n '__section\s*\(\s*\"' arch/arm64/mm/mmu.c:45:u64 __section(".mmuoff.data.write") vabits_actual; include/linux/compiler.h:211: __section("___kentry" "+" #sym ) \ include/linux/export.h:133: static int __ksym_marker_##sym[0] __section(".discard.ksym") __used include/linux/srcutree.h:127: __section("___srcu_struct_ptrs") = &name My recollection is I submitted a patch to _add_ quotes https://lore.kernel.org/patchwork/patch/1125785/
On Mon, Jun 29, 2020 at 12:53:47PM -0700, Nick Desaulniers wrote: > On Sun, Jun 28, 2020 at 11:18 PM Kees Cook <keescook@chromium.org> wrote: > > > > Fix a case of needless quotes in __section(), which Clang doesn't like. > > > > Acked-by: Will Deacon <will@kernel.org> > > Signed-off-by: Kees Cook <keescook@chromium.org> > > Yep, I remember bugs from this. Probably should scan the kernel for > other instances of this. +Joe for checkpatch.pl validation. I think the others are safe because they're in macros: $ git grep -4 '__section("' include/linux/compiler.h-# define KENTRY(sym) \ include/linux/compiler.h- extern typeof(sym) sym; \ include/linux/compiler.h- static const unsigned long __kentry_##sym \ include/linux/compiler.h- __used \ include/linux/compiler.h: __section("___kentry" "+" #sym ) \ include/linux/compiler.h- = (unsigned long)&sym; -- include/linux/export.h-#define __ksym_marker(sym) \ include/linux/export.h: static int __ksym_marker_##sym[0] __section(".discard.ksym") __used -- include/linux/srcutree.h-# define __DEFINE_SRCU(name, is_static) \ include/linux/srcutree.h- is_static struct srcu_struct name; \ include/linux/srcutree.h- struct srcu_struct * const __srcu_struct_##name \ include/linux/srcutree.h: __section("___srcu_struct_ptrs") = &name > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Thanks!
On Mon, Jun 29, 2020 at 1:03 PM Joe Perches <joe@perches.com> wrote: > > On Mon, 2020-06-29 at 12:53 -0700, Nick Desaulniers wrote: > > On Sun, Jun 28, 2020 at 11:18 PM Kees Cook <keescook@chromium.org> wrote: > > > Fix a case of needless quotes in __section(), which Clang doesn't like. > > > > > > Acked-by: Will Deacon <will@kernel.org> > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > > Yep, I remember bugs from this. Probably should scan the kernel for > > other instances of this. +Joe for checkpatch.pl validation. > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > $ git grep -P -n '__section\s*\(\s*\"' > arch/arm64/mm/mmu.c:45:u64 __section(".mmuoff.data.write") vabits_actual; > include/linux/compiler.h:211: __section("___kentry" "+" #sym ) \ > include/linux/export.h:133: static int __ksym_marker_##sym[0] __section(".discard.ksym") __used > include/linux/srcutree.h:127: __section("___srcu_struct_ptrs") = &name > > My recollection is I submitted a patch > to _add_ quotes > > https://lore.kernel.org/patchwork/patch/1125785/ Hey, yeah! Did you end up sending v2?
On Mon, Jun 29, 2020 at 01:04:31PM -0700, Kees Cook wrote: > On Mon, Jun 29, 2020 at 12:53:47PM -0700, Nick Desaulniers wrote: > > On Sun, Jun 28, 2020 at 11:18 PM Kees Cook <keescook@chromium.org> wrote: > > > > > > Fix a case of needless quotes in __section(), which Clang doesn't like. > > > > > > Acked-by: Will Deacon <will@kernel.org> > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > > Yep, I remember bugs from this. Probably should scan the kernel for > > other instances of this. +Joe for checkpatch.pl validation. > > I think the others are safe because they're in macros: Why does that make it safe -- the commit msg is a bit sparse, but I assume the problem is that it generates __attribute__((__section__("\".foo\""))) from __section(".foo") after preprocessing, and clang keeps the quotes in the section name when generating assembly, while gcc appears to strip them off. It does that even if nested in another macro, no? > > $ git grep -4 '__section("' > include/linux/compiler.h-# define KENTRY(sym) \ Am I missing something, or is KENTRY unused in the tree? > include/linux/compiler.h- extern typeof(sym) sym; \ > include/linux/compiler.h- static const unsigned long __kentry_##sym \ > include/linux/compiler.h- __used \ > include/linux/compiler.h: __section("___kentry" "+" #sym ) \ > include/linux/compiler.h- = (unsigned long)&sym; > -- > include/linux/export.h-#define __ksym_marker(sym) \ > include/linux/export.h: static int __ksym_marker_##sym[0] __section(".discard.ksym") __used > -- > include/linux/srcutree.h-# define __DEFINE_SRCU(name, is_static) \ > include/linux/srcutree.h- is_static struct srcu_struct name; \ > include/linux/srcutree.h- struct srcu_struct * const __srcu_struct_##name \ > include/linux/srcutree.h: __section("___srcu_struct_ptrs") = &name > > > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > Thanks! > > -- > Kees Cook
On Mon, Jun 29, 2020 at 04:54:48PM -0400, Arvind Sankar wrote: > On Mon, Jun 29, 2020 at 01:04:31PM -0700, Kees Cook wrote: > > On Mon, Jun 29, 2020 at 12:53:47PM -0700, Nick Desaulniers wrote: > > > On Sun, Jun 28, 2020 at 11:18 PM Kees Cook <keescook@chromium.org> wrote: > > > > > > > > Fix a case of needless quotes in __section(), which Clang doesn't like. > > > > > > > > Acked-by: Will Deacon <will@kernel.org> > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > > > > Yep, I remember bugs from this. Probably should scan the kernel for > > > other instances of this. +Joe for checkpatch.pl validation. > > > > I think the others are safe because they're in macros: > > Why does that make it safe -- the commit msg is a bit sparse, but I > assume the problem is that it generates > __attribute__((__section__("\".foo\""))) > from > __section(".foo") > after preprocessing, and clang keeps the quotes in the section name when > generating assembly, while gcc appears to strip them off. > > It does that even if nested in another macro, no? Yep, I can see things like: [25] ".discard.ksym" PROGBITS 0000000000000000 0000217c 0000000000000000 0000000000000000 WA 0 0 4 Doesn't seem to cause a build error, but that can't be good. > > > > > $ git grep -4 '__section("' > > include/linux/compiler.h-# define KENTRY(sym) \ > > Am I missing something, or is KENTRY unused in the tree? > > > include/linux/compiler.h- extern typeof(sym) sym; \ > > include/linux/compiler.h- static const unsigned long __kentry_##sym \ > > include/linux/compiler.h- __used \ > > include/linux/compiler.h: __section("___kentry" "+" #sym ) \ > > include/linux/compiler.h- = (unsigned long)&sym; > > -- > > include/linux/export.h-#define __ksym_marker(sym) \ > > include/linux/export.h: static int __ksym_marker_##sym[0] __section(".discard.ksym") __used > > -- > > include/linux/srcutree.h-# define __DEFINE_SRCU(name, is_static) \ > > include/linux/srcutree.h- is_static struct srcu_struct name; \ > > include/linux/srcutree.h- struct srcu_struct * const __srcu_struct_##name \ > > include/linux/srcutree.h: __section("___srcu_struct_ptrs") = &name > > > > > > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > > > Thanks! > > > > -- > > Kees Cook
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index 1df25f26571d..dce024ea6084 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -42,7 +42,7 @@ u64 idmap_t0sz = TCR_T0SZ(VA_BITS); u64 idmap_ptrs_per_pgd = PTRS_PER_PGD; -u64 __section(".mmuoff.data.write") vabits_actual; +u64 __section(.mmuoff.data.write) vabits_actual; EXPORT_SYMBOL(vabits_actual); u64 kimage_voffset __ro_after_init;