Re: [PATCH v2 00/15] sysctl: Remove sentinel elements from drivers
Le 02/10/2023 à 10:55, Joel Granados via B4 Relay a écrit : > From: Joel Granados > > What? > These commits remove the sentinel element (last empty element) from the > sysctl arrays of all the files under the "drivers/" directory that use a > sysctl array for registration. The merging of the preparation patches > (in https://lore.kernel.org/all/zo5yx5jfoggi%2f...@bombadil.infradead.org/) > to mainline allows us to just remove sentinel elements without changing > behavior (more info here [1]). > > These commits are part of a bigger set (here > https://github.com/Joelgranados/linux/tree/tag/sysctl_remove_empty_elem_V4) > that remove the ctl_table sentinel. Make the review process easier by > chunking the commits into manageable pieces. Each chunk can be reviewed > separately without noise from parallel sets. > > Now that the architecture chunk has been mostly reviewed [6], we send > the "drivers/" directory. Once this one is done, it will be follwed by > "fs/*", "kernel/*", "net/*" and miscellaneous. The final set will remove > the unneeded check for ->procname == NULL. > > Why? > By removing the sysctl sentinel elements we avoid kernel bloat as > ctl_table arrays get moved out of kernel/sysctl.c into their own > respective subsystems. This move was started long ago to avoid merge > conflicts; the sentinel removal bit came after Mathew Wilcox suggested > it to avoid bloating the kernel by one element as arrays moved out. This > patchset will reduce the overall build time size of the kernel and run > time memory bloat by about ~64 bytes per declared ctl_table array. I > have consolidated some links that shed light on the history of this > effort [2]. > > Testing: > * Ran sysctl selftests (./tools/testing/selftests/sysctl/sysctl.sh) > * Ran this through 0-day with no errors or warnings > > Size saving after removing all sentinels: >These are the bytes that we save after removing all the sentinels >(this plus all the other chunks). I included them to get an idea of >how much memory we are talking about. > * bloat-o-meter: > - The "yesall" configuration results save 9158 bytes > > https://lore.kernel.org/all/20230621091000.424843-1-j.grana...@samsung.com/ > - The "tiny" config + CONFIG_SYSCTL save 1215 bytes > > https://lore.kernel.org/all/20230809105006.1198165-1-j.grana...@samsung.com/ > * memory usage: > In memory savings are measured to be 7296 bytes. (here is how to > measure [3]) > > Size saving after this patchset: > * bloat-o-meter > - The "yesall" config saves 2432 bytes [4] > - The "tiny" config saves 64 bytes [5] > * memory usage: > In this case there were no bytes saved because I do not have any > of the drivers in the patch. To measure it comment the printk in > `new_dir` and uncomment the if conditional in `new_links` [3]. > > --- > Changes in v2: > - Left the dangling comma in the ctl_table arrays. > - Link to v1: > https://lore.kernel.org/r/20230928-jag-sysctl_remove_empty_elem_drivers-v1-0-e59120fca...@samsung.com > > Comments/feedback greatly appreciated Same problem on powerpc CI tests, all boot target failed, most of them with similar OOPS, see https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20231002-jag-sysctl_remove_empty_elem_drivers-v2-15-02dd0d46f...@samsung.com/ What is strange is that I pushed your series into my github account, and got no failure, see https://github.com/chleroy/linux/actions/runs/6378951278 Christophe > > Best > > Joel > > [1] > We are able to remove a sentinel table without behavioral change by > introducing a table_size argument in the same place where procname is > checked for NULL. The idea is for it to keep stopping when it hits > ->procname == NULL, while the sentinel is still present. And when the > sentinel is removed, it will stop on the table_size. You can go to > (https://lore.kernel.org/all/20230809105006.1198165-1-j.grana...@samsung.com/) > for more information. > > [2] > Links Related to the ctl_table sentinel removal: > * Good summary from Luis sent with the "pull request" for the >preparation patches. >https://lore.kernel.org/all/zo5yx5jfoggi%2f...@bombadil.infradead.org/ > * Another very good summary from Luis. >https://lore.kernel.org/all/zmfizkfkvxuft...@bombadil.infradead.org/ > * This is a patch set that replaces register_sysctl_table with register_sysctl >https://lore.kernel.org/all/20230302204612.782387-1-mcg...@kernel.org/ > * Patch set to deprecate register_sysctl_paths() >https://lore.kernel.org/all/20230302202826.776286-1-mcg...@kernel.org/ > * Here there is an explicit expectation for the removal of the sentinel > element. >https://lore.kernel.org/all/20230321130908.6972-1-frank...@vivo.com > * The "ARRAY_SIZE" approach was mentioned (proposed?) in this thread >https://lore.kernel.org/all/20220220060626.15885-1-tangm...@uniontech.com > > [3] > T
Re: [PATCH 00/15] sysctl: Remove sentinel elements from drivers
Le 02/10/2023 à 10:47, Joel Granados a écrit : > On Thu, Sep 28, 2023 at 04:31:30PM +0000, Christophe Leroy wrote: > I followed this trace and proc_handler is correctly defined in tty_table > (struct ctl_table) in drivers/tty/tty_io.c:tty_init and there is not > path that changes these values. > Additionally, we then fail trying to print instead of continuing with > the initialization. My conjecture is that this might be due to something > different than tht sysctl register call. > > Does this happen consistenly or is this just a one off issue? Don't know. > > To what branch are these patches being applied to? As far as I understand from https://github.com/linuxppc/linux-snowpatch/commits/snowpatch/375319, it's being applied on https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=d774975 > > I'm going to post my V2 and keep working on this issue if it pops up > again. > Christophe
Re: [PATCH 04/15] tty: Remove now superfluous sentinel element from ctl_table array
Le 02/10/2023 à 10:17, Jiri Slaby a écrit :
> On 28. 09. 23, 15:21, Joel Granados via B4 Relay wrote:
>> From: Joel Granados
>>
>> This commit comes at the tail end of a greater effort to remove the
>> empty elements at the end of the ctl_table arrays (sentinels) which
>> will reduce the overall build time size of the kernel and run time
>> memory bloat by ~64 bytes per sentinel (further information Link :
>> https://lore.kernel.org/all/zo5yx5jfoggi%2f...@bombadil.infradead.org/)
>>
>> Remove sentinel from tty_table
>>
>> Signed-off-by: Joel Granados
>> ---
>> drivers/tty/tty_io.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>> index 8a94e5a43c6d..2f925dc54a20 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b/drivers/tty/tty_io.c
>> @@ -3607,8 +3607,7 @@ static struct ctl_table tty_table[] = {
>> .proc_handler = proc_dointvec,
>> .extra1 = SYSCTL_ZERO,
>> .extra2 = SYSCTL_ONE,
>> - },
>> - { }
>> + }
>
> Why to remove the comma? One would need to add one when adding a new entry?
Does it make any difference at all ?
In one case you have:
@
something old,
},
+ {
+ something new,
+ },
}
In the other case you have:
@
something old,
+ },
+ {
+ something new,
}
}
Christophe
Re: [PATCH 00/15] sysctl: Remove sentinel elements from drivers
Le 28/09/2023 à 15:21, Joel Granados via B4 Relay a écrit : > From: Joel Granados Automatic test fails on powerpc, see https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20230928-jag-sysctl_remove_empty_elem_drivers-v1-15-e59120fca...@samsung.com/ Kernel attempted to read user page (1a111316) - exploit attempt? (uid: 0) BUG: Unable to handle kernel data access on read at 0x1a111316 Faulting instruction address: 0xc0545338 Oops: Kernel access of bad area, sig: 11 [#1] BE PAGE_SIZE=4K PowerPC 44x Platform Modules linked in: CPU: 0 PID: 1 Comm: swapper Not tainted 6.5.0-rc6-gdef13277bacb #1 Hardware name: amcc,bamboo 440GR Rev. B 0x422218d3 PowerPC 44x Platform NIP: c0545338 LR: c0548468 CTR: REGS: c084fae0 TRAP: 0300 Not tainted (6.5.0-rc6-gdef13277bacb) MSR: 00021000 CR: 84004288 XER: DEAR: 1a111316 ESR: GPR00: c0548468 c084fbd0 c0888000 c084fc99 c084fc7c 1a110316 000a GPR08: c084fd18 1a111316 04ff 22000282 c00027c0 GPR16: c004 c003d544 0001 c003eb2c 096023d4 GPR24: c0636502 c0636502 c084fc74 c0588510 c084fc68 c084fc7c c084fc99 0002 NIP [c0545338] string+0x78/0x148 LR [c0548468] vsnprintf+0x3d8/0x824 Call Trace: [c084fbd0] [c084fc7c] 0xc084fc7c (unreliable) [c084fbe0] [c0548468] vsnprintf+0x3d8/0x824 [c084fc30] [c0072dec] vprintk_store+0x17c/0x4c8 [c084fcc0] [c007322c] vprintk_emit+0xf4/0x2a0 [c084fd00] [c0073d04] _printk+0x60/0x88 [c084fd40] [c01ab63c] sysctl_err+0x78/0xa4 [c084fd80] [c01ab404] __register_sysctl_table+0x6a0/0x6c4 [c084fde0] [c06a585c] __register_sysctl_init+0x30/0x78 [c084fe00] [c06a8cc8] tty_init+0x44/0x168 [c084fe30] [c00023c4] do_one_initcall+0x64/0x2a0 [c084fea0] [c068f060] kernel_init_freeable+0x184/0x230 [c084fee0] [c00027e4] kernel_init+0x24/0x124 [c084ff00] [c000f1fc] ret_from_kernel_user_thread+0x14/0x1c --- interrupt: 0 at 0x0 NIP: LR: CTR: REGS: c084ff10 TRAP: Not tainted (6.5.0-rc6-gdef13277bacb) MSR: <> CR: XER: GPR00: GPR08: GPR16: GPR24: NIP [] 0x0 LR [] 0x0 --- interrupt: 0 Code: 91610008 90e1000c 4bffd0b5 80010014 38210010 7c0803a6 4e800020 409d0008 9923 38630001 38840001 4240ffd0 <7d2a20ae> 7f851840 5528063e 2c08 ---[ end trace ]--- note: swapper[1] exited with irqs disabled Kernel panic - not syncing: Attempted to kill init! exitcode=0x000b > > What? > These commits remove the sentinel element (last empty element) from the > sysctl arrays of all the files under the "drivers/" directory that use a > sysctl array for registration. The merging of the preparation patches > (in https://lore.kernel.org/all/zo5yx5jfoggi%2f...@bombadil.infradead.org/) > to mainline allows us to just remove sentinel elements without changing > behavior (more info here [1]). > > These commits are part of a bigger set (here > https://github.com/Joelgranados/linux/tree/tag/sysctl_remove_empty_elem_V4) > that remove the ctl_table sentinel. Make the review process easier by > chunking the commits into manageable pieces. Each chunk can be reviewed > separately without noise from parallel sets. > > Now that the architecture chunk has been mostly reviewed [6], we send > the "drivers/" directory. Once this one is done, it will be follwed by > "fs/*", "kernel/*", "net/*" and miscellaneous. The final set will remove > the unneeded check for ->procname == NULL. > > Why? > By removing the sysctl sentinel elements we avoid kernel bloat as > ctl_table arrays get moved out of kernel/sysctl.c into their own > respective subsystems. This move was started long ago to avoid merge > conflicts; the sentinel removal bit came after Mathew Wilcox suggested > it to avoid bloating the kernel by one element as arrays moved out. This > patchset will reduce the overall build time size of the kernel and run > time memory bloat by about ~64 bytes per declared ctl_table array. I > have consolidated some links that shed light on the history of this > effort [2]. > > Testing: > * Ran sysctl selftests (./tools/testing/selftests/sysctl/sysctl.sh) > * Ran this through 0-day with no errors or warnings > > Size saving after removing all sentinels: >These are the bytes that we save after removing all the sentinels >(this plus all the other chunks). I included them to get an idea of >how much memory we are talking about. > * bloat-o-meter: > - The "yesall" configuration results save 9158 bytes > > https://lore.kernel.org/all/20230621091000.424843-1-j.grana...@samsung.com/ > - The "tiny" config + CONFIG_SYSCTL save 1215 bytes > > https://lore.
Re: [PATCH v7 13/41] mm: Make pte_mkwrite() take a VMA
Le 27/02/2023 à 23:29, Rick Edgecombe a écrit :
> The x86 Control-flow Enforcement Technology (CET) feature includes a new
> type of memory called shadow stack. This shadow stack memory has some
> unusual properties, which requires some core mm changes to function
> properly.
>
> One of these unusual properties is that shadow stack memory is writable,
> but only in limited ways. These limits are applied via a specific PTE
> bit combination. Nevertheless, the memory is writable, and core mm code
> will need to apply the writable permissions in the typical paths that
> call pte_mkwrite().
>
> In addition to VM_WRITE, the shadow stack VMA's will have a flag denoting
> that they are special shadow stack flavor of writable memory. So make
> pte_mkwrite() take a VMA, so that the x86 implementation of it can know to
> create regular writable memory or shadow stack memory.
>
> Apply the same changes for pmd_mkwrite() and huge_pte_mkwrite().
I'm not sure it is a good idea to add a second argument to
pte_mkwrite(). All pte_mk() only take a pte and nothing else.
I think you should do the same as commit d9ed9faac283 ("mm: add new
arch_make_huge_pte() method for tile support")
Christophe
>
> No functional change.
>
> Cc: linux-...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Cc: linux-al...@vger.kernel.org
> Cc: linux-snps-...@lists.infradead.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-c...@vger.kernel.org
> Cc: linux-hexa...@vger.kernel.org
> Cc: linux-i...@vger.kernel.org
> Cc: loonga...@lists.linux.dev
> Cc: linux-m...@lists.linux-m68k.org
> Cc: Michal Simek
> Cc: Dinh Nguyen
> Cc: linux-m...@vger.kernel.org
> Cc: linux-openr...@vger.kernel.org
> Cc: linux-par...@vger.kernel.org
> Cc: linuxppc-...@lists.ozlabs.org
> Cc: linux-ri...@lists.infradead.org
> Cc: linux-s...@vger.kernel.org
> Cc: linux...@vger.kernel.org
> Cc: sparcli...@vger.kernel.org
> Cc: linux...@lists.infradead.org
> Cc: xen-devel@lists.xenproject.org
> Cc: linux-a...@vger.kernel.org
> Cc: linux...@kvack.org
> Tested-by: Pengfei Xu
> Tested-by: John Allen
> Tested-by: Kees Cook
> Acked-by: Mike Rapoport (IBM)
> Acked-by: Michael Ellerman
> Acked-by: David Hildenbrand
> Reviewed-by: Kees Cook
> Suggested-by: David Hildenbrand
> Signed-off-by: Rick Edgecombe
>
> ---
> Hi Non-x86 Arch’s,
>
> x86 has a feature that allows for the creation of a special type of
> writable memory (shadow stack) that is only writable in limited specific
> ways. Previously, changes were proposed to core MM code to teach it to
> decide when to create normally writable memory or the special shadow stack
> writable memory, but David Hildenbrand suggested[0] to change
> pXX_mkwrite() to take a VMA, so awareness of shadow stack memory can be
> moved into x86 code.
>
> Since pXX_mkwrite() is defined in every arch, it requires some tree-wide
> changes. So that is why you are seeing some patches out of a big x86
> series pop up in your arch mailing list. There is no functional change.
> After this refactor, the shadow stack series goes on to use the arch
> helpers to push shadow stack memory details inside arch/x86.
>
> Testing was just 0-day build testing.
>
> Hopefully that is enough context. Thanks!
>
> [0]
> https://lore.kernel.org/lkml/0e29a2d0-08d8-bcd6-ff26-4bea0e403...@redhat.com/#t
>
> v6:
> - New patch
> ---
> Documentation/mm/arch_pgtable_helpers.rst| 9 ++---
> arch/alpha/include/asm/pgtable.h | 6 +-
> arch/arc/include/asm/hugepage.h | 2 +-
> arch/arc/include/asm/pgtable-bits-arcv2.h| 7 ++-
> arch/arm/include/asm/pgtable-3level.h| 7 ++-
> arch/arm/include/asm/pgtable.h | 2 +-
> arch/arm64/include/asm/pgtable.h | 4 ++--
> arch/csky/include/asm/pgtable.h | 2 +-
> arch/hexagon/include/asm/pgtable.h | 2 +-
> arch/ia64/include/asm/pgtable.h | 2 +-
> arch/loongarch/include/asm/pgtable.h | 4 ++--
> arch/m68k/include/asm/mcf_pgtable.h | 2 +-
> arch/m68k/include/asm/motorola_pgtable.h | 6 +-
> arch/m68k/include/asm/sun3_pgtable.h | 6 +-
> arch/microblaze/include/asm/pgtable.h| 2 +-
> arch/mips/include/asm/pgtable.h | 6 +++---
> arch/nios2/include/asm/pgtable.h | 2 +-
> arch/openrisc/include/asm/pgtable.h | 2 +-
> arch/parisc/include/asm/pgtable.h| 6 +-
> arch/powerpc/include/asm/book3s/32/pgtable.h | 2 +-
> arch/powerpc/include/asm/book3s/64/pgtable.h | 4 ++--
> arch/powerpc/include/asm/nohash/32/pgtable.h | 2 +-
> arch/powerpc/include/asm/nohash/32/pte-8xx.h | 2 +-
> arch/powerpc/include/asm/nohash/64/pgtable.h | 2 +-
> arch/riscv/include/asm/pgtable.h | 6 +++---
> arch/s390/include/asm/hugetlb.h | 4 ++--
> arch/s390/include/asm/pgtable.h | 4 ++--
> arch/sh/include/asm/pgtable_32.h
Re: [PATCH 08/11] swiotlb: make the swiotlb_init interface more useful
Le 27/02/2022 à 15:30, Christoph Hellwig a écrit : > Pass a bool to pass if swiotlb needs to be enabled based on the > addressing needs and replace the verbose argument with a set of > flags, including one to force enable bounce buffering. > > Note that this patch removes the possibility to force xen-swiotlb > use using swiotlb=force on the command line on x86 (arm and arm64 > never supported that), but this interface will be restored shortly. > > Signed-off-by: Christoph Hellwig > --- > arch/arm/mm/init.c | 6 + > arch/arm64/mm/init.c | 6 + > arch/ia64/mm/init.c| 4 +-- > arch/mips/cavium-octeon/dma-octeon.c | 2 +- > arch/mips/loongson64/dma.c | 2 +- > arch/mips/sibyte/common/dma.c | 2 +- > arch/powerpc/include/asm/swiotlb.h | 1 + > arch/powerpc/mm/mem.c | 3 ++- arch/powerpc/mm/mem.o:(.toc+0x0): undefined reference to `ppc_swiotlb_flags' make[1]: *** [vmlinux] Error 1 /linux/Makefile:1155: recipe for target 'vmlinux' failed > arch/powerpc/platforms/pseries/setup.c | 3 --- > arch/riscv/mm/init.c | 8 +- > arch/s390/mm/init.c| 3 +-- > arch/x86/kernel/cpu/mshyperv.c | 8 -- > arch/x86/kernel/pci-dma.c | 15 ++- > arch/x86/mm/mem_encrypt_amd.c | 3 --- > drivers/xen/swiotlb-xen.c | 4 +-- > include/linux/swiotlb.h| 15 ++- > include/trace/events/swiotlb.h | 29 - > kernel/dma/swiotlb.c | 35 ++ > 18 files changed, 56 insertions(+), 93 deletions(-) >
Re: [PATCH 3/3] memblock: cleanup memblock_free interface
Le 23/09/2021 à 14:01, Mike Rapoport a écrit : On Thu, Sep 23, 2021 at 11:47:48AM +0200, Christophe Leroy wrote: Le 23/09/2021 à 09:43, Mike Rapoport a écrit : From: Mike Rapoport For ages memblock_free() interface dealt with physical addresses even despite the existence of memblock_alloc_xx() functions that return a virtual pointer. Introduce memblock_phys_free() for freeing physical ranges and repurpose memblock_free() to free virtual pointers to make the following pairing abundantly clear: int memblock_phys_free(phys_addr_t base, phys_addr_t size); phys_addr_t memblock_phys_alloc(phys_addr_t base, phys_addr_t size); void *memblock_alloc(phys_addr_t size, phys_addr_t align); void memblock_free(void *ptr, size_t size); Replace intermediate memblock_free_ptr() with memblock_free() and drop unnecessary aliases memblock_free_early() and memblock_free_early_nid(). Suggested-by: Linus Torvalds Signed-off-by: Mike Rapoport --- diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c index 1a04e5bdf655..37826d8c4f74 100644 --- a/arch/s390/kernel/smp.c +++ b/arch/s390/kernel/smp.c @@ -723,7 +723,7 @@ void __init smp_save_dump_cpus(void) /* Get the CPU registers */ smp_save_cpu_regs(sa, addr, is_boot_cpu, page); } - memblock_free(page, PAGE_SIZE); + memblock_phys_free(page, PAGE_SIZE); diag_amode31_ops.diag308_reset(); pcpu_set_smt(0); } @@ -880,7 +880,7 @@ void __init smp_detect_cpus(void) /* Add CPUs present at boot */ __smp_rescan_cpus(info, true); - memblock_free_early((unsigned long)info, sizeof(*info)); + memblock_free(info, sizeof(*info)); } /* I'm a bit lost. IIUC memblock_free_early() and memblock_free() where identical. Yes, they were, but all calls to memblock_free_early() were using __pa(vaddr) because they had a virtual address at hand. I'm still not following. In the above memblock_free_early() was taking (unsigned long)info . Was it a bug ? It looks odd to hide bug fixes in such a big patch, should that bug fix go in patch 2 ? In the first hunk memblock_free() gets replaced by memblock_phys_free() In the second hunk memblock_free_early() gets replaced by memblock_free() In the first hunk the memory is allocated with memblock_phys_alloc() and we have a physical range to free. In the second hunk the memory is allocated with memblock_alloc() and we are freeing a virtual pointer. I think it would be easier to follow if you could split it in several patches: It was an explicit request from Linus to make it a single commit: but the actual commit can and should be just a single commit that just fixes 'memblock_free()' to have sane interfaces. I don't feel strongly about splitting it (except my laziness really objects), but I don't think doing the conversion in several steps worth the churn. The commit is quite big (55 files changed, approx 100 lines modified). If done in the right order the change should be minimal. It is rather not-easy to follow and review when a function that was existing (namely memblock_free() ) disappears and re-appears in the same commit but to do something different. You do: - memblock_free() ==> memblock_phys_free() - memblock_free_ptr() ==> memblock_free() At least you could split in two patches, the advantage would be that between first and second patch memblock() doesn't exist anymore so you can check you really don't have anymore user. - First patch: Create memblock_phys_free() and change all relevant memblock_free() to memblock_phys_free() - Or change memblock_free() to memblock_phys_free() and make memblock_free() an alias of it. - Second patch: Make memblock_free_ptr() become memblock_free() and change all remaining callers to the new semantics (IIUC memblock_free(__pa(ptr)) becomes memblock_free(ptr) and make memblock_free_ptr() an alias of memblock_free() - Fourth patch: Replace and drop memblock_free_ptr() - Fifth patch: Drop memblock_free_early() and memblock_free_early_nid() (All users should have been upgraded to memblock_free_phys() in patch 1 or memblock_free() in patch 2) Christophe
Re: [PATCH 3/3] memblock: cleanup memblock_free interface
Le 23/09/2021 à 09:43, Mike Rapoport a écrit : From: Mike Rapoport For ages memblock_free() interface dealt with physical addresses even despite the existence of memblock_alloc_xx() functions that return a virtual pointer. Introduce memblock_phys_free() for freeing physical ranges and repurpose memblock_free() to free virtual pointers to make the following pairing abundantly clear: int memblock_phys_free(phys_addr_t base, phys_addr_t size); phys_addr_t memblock_phys_alloc(phys_addr_t base, phys_addr_t size); void *memblock_alloc(phys_addr_t size, phys_addr_t align); void memblock_free(void *ptr, size_t size); Replace intermediate memblock_free_ptr() with memblock_free() and drop unnecessary aliases memblock_free_early() and memblock_free_early_nid(). Suggested-by: Linus Torvalds Signed-off-by: Mike Rapoport --- diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c index 1a04e5bdf655..37826d8c4f74 100644 --- a/arch/s390/kernel/smp.c +++ b/arch/s390/kernel/smp.c @@ -723,7 +723,7 @@ void __init smp_save_dump_cpus(void) /* Get the CPU registers */ smp_save_cpu_regs(sa, addr, is_boot_cpu, page); } - memblock_free(page, PAGE_SIZE); + memblock_phys_free(page, PAGE_SIZE); diag_amode31_ops.diag308_reset(); pcpu_set_smt(0); } @@ -880,7 +880,7 @@ void __init smp_detect_cpus(void) /* Add CPUs present at boot */ __smp_rescan_cpus(info, true); - memblock_free_early((unsigned long)info, sizeof(*info)); + memblock_free(info, sizeof(*info)); } /* I'm a bit lost. IIUC memblock_free_early() and memblock_free() where identical. In the first hunk memblock_free() gets replaced by memblock_phys_free() In the second hunk memblock_free_early() gets replaced by memblock_free() I think it would be easier to follow if you could split it in several patches: - First patch: Create memblock_phys_free() and change all relevant memblock_free() to memblock_phys_free() - Or change memblock_free() to memblock_phys_free() and make memblock_free() an alias of it. - Second patch: Make memblock_free_ptr() become memblock_free() and change all remaining callers to the new semantics (IIUC memblock_free(__pa(ptr)) becomes memblock_free(ptr) and make memblock_free_ptr() an alias of memblock_free() - Fourth patch: Replace and drop memblock_free_ptr() - Fifth patch: Drop memblock_free_early() and memblock_free_early_nid() (All users should have been upgraded to memblock_free_phys() in patch 1 or memblock_free() in patch 2) Christophe
Re: [PATCH 8/8] xen/hvc: replace BUG_ON() with negative return value
Le 13/05/2021 à 12:03, Juergen Gross a écrit :
Xen frontends shouldn't BUG() in case of illegal data received from
their backends. So replace the BUG_ON()s when reading illegal data from
the ring page with negative return values.
Signed-off-by: Juergen Gross
---
drivers/tty/hvc/hvc_xen.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index 92c9a476defc..30d7ffb1e04c 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -86,6 +86,11 @@ static int __write_console(struct xencons_info *xencons,
cons = intf->out_cons;
prod = intf->out_prod;
mb(); /* update queue values before going on */
+
+ if (WARN_ONCE((prod - cons) > sizeof(intf->out),
+ "Illegal ring page indices"))
+ return -EINVAL;
+
BUG_ON((prod - cons) > sizeof(intf->out));
Why keep the BUG_ON() ?
while ((sent < len) && ((prod - cons) < sizeof(intf->out)))
@@ -114,7 +119,10 @@ static int domU_write_console(uint32_t vtermno, const char
*data, int len)
*/
while (len) {
int sent = __write_console(cons, data, len);
-
+
+ if (sent < 0)
+ return sent;
+
data += sent;
len -= sent;
@@ -138,7 +146,10 @@ static int domU_read_console(uint32_t vtermno, char *buf, int len)
cons = intf->in_cons;
prod = intf->in_prod;
mb(); /* get pointers before reading ring */
- BUG_ON((prod - cons) > sizeof(intf->in));
+
+ if (WARN_ONCE((prod - cons) > sizeof(intf->in),
+ "Illegal ring page indices"))
+ return -EINVAL;
while (cons != prod && recv < len)
buf[recv++] = intf->in[MASK_XENCONS_IDX(cons++, intf->in)];
Re: [PATCH v2 0/7] Rid W=1 warnings in Ethernet
Le 15/01/2021 à 12:18, Lee Jones a écrit : On Thu, 14 Jan 2021, Lee Jones wrote: On Thu, 14 Jan 2021, Jakub Kicinski wrote: On Thu, 14 Jan 2021 08:33:49 + Lee Jones wrote: On Wed, 13 Jan 2021, Jakub Kicinski wrote: On Wed, 13 Jan 2021 16:41:16 + Lee Jones wrote: Resending the stragglers again. This set is part of a larger effort attempting to clean-up W=1 kernel builds, which are currently overwhelmingly riddled with niggly little warnings. v2: - Squashed IBM patches - Fixed real issue in SMSC - Added Andrew's Reviewed-by tags on remainder Does not apply, please rebase on net-next/master. These are based on Tuesday's next/master. What's next/master? I'm not sure if this is a joke, or not? :) next/master == Linux Next. The daily merged repo where all of the *-next branches end up to ensure interoperability. It's also the branch that is most heavily tested by the auto-builders to ensure the vast majority of issues are ironed out before hitting Mainline. This is net-next: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/ Looks like net-next gets merged into next/master: commit 452958f1f3d1c8980a8414f9c37c8c6de24c7d32 Merge: 1eabba209a17a f50e2f9f79164 Author: Stephen Rothwell Date: Thu Jan 14 10:35:40 2021 +1100 Merge remote-tracking branch 'net-next/master' So I'm not sure what it's conflicting with. Do you have patches in net-next that didn't make it into next/master for some reason? I'll try to rebase again tomorrow. Hopefully I am able to reproduce your issue by then. Okay so my development branch rebased again with no issue. Rebasing is not same as patches application. I also took the liberty to checkout net-next and cherry-pick the patches [0], which again didn't cause a problem. Also normal, cherry-picking is not the same as applying a patch series. I'm not sure what else to suggest. Is your local copy up-to-date? I guess so, I have the same problem as Jakub, see below. I had to use 'git am -3' to apply you series. As you can see, git falls back to 3 way merge for patch 1, which means your series is close to but not fully in sync with net-next. [root@localhost linux-powerpc]# git remote -v net-next https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git (fetch) net-next https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git (push) [root@localhost linux-powerpc]# git checkout net-next/master -b net-next Switched to a new branch 'net-next' [root@localhost linux-powerpc]# git am /root/Downloads/Rid-W-1-warnings-in-Ethernet.patch Applying: net: ethernet: smsc: smc91x: Fix function name in kernel-doc header error: patch failed: drivers/net/ethernet/smsc/smc91x.c:2192 error: drivers/net/ethernet/smsc/smc91x.c: patch does not apply Patch failed at 0001 net: ethernet: smsc: smc91x: Fix function name in kernel-doc header hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". [root@localhost linux-powerpc]# git am --abort [root@localhost linux-powerpc]# git am -3 /root/Downloads/Rid-W-1-warnings-in-Ethernet.patch Applying: net: ethernet: smsc: smc91x: Fix function name in kernel-doc header Using index info to reconstruct a base tree... M drivers/net/ethernet/smsc/smc91x.c Falling back to patching base and 3-way merge... Auto-merging drivers/net/ethernet/smsc/smc91x.c Applying: net: xen-netback: xenbus: Demote nonconformant kernel-doc headers Applying: net: ethernet: ti: am65-cpsw-qos: Demote non-conformant function header Applying: net: ethernet: ti: am65-cpts: Document am65_cpts_rx_enable()'s 'en' parameter Applying: net: ethernet: ibm: ibmvnic: Fix some kernel-doc misdemeanours Applying: net: ethernet: toshiba: ps3_gelic_net: Fix some kernel-doc misdemeanours Applying: net: ethernet: toshiba: spider_net: Document a whole bunch of function parameters Christophe
Re: [Xen-devel] [PATCH v2 19/21] treewide: add checks for the return value of memblock_alloc*()
Le 31/01/2019 à 07:44, Christophe Leroy a écrit :
Le 31/01/2019 à 07:41, Mike Rapoport a écrit :
On Thu, Jan 31, 2019 at 07:07:46AM +0100, Christophe Leroy wrote:
Le 21/01/2019 à 09:04, Mike Rapoport a écrit :
Add check for the return value of memblock_alloc*() functions and call
panic() in case of error.
The panic message repeats the one used by panicing memblock
allocators with
adjustment of parameters to include only relevant ones.
The replacement was mostly automated with semantic patches like the one
below with manual massaging of format strings.
@@
expression ptr, size, align;
@@
ptr = memblock_alloc(size, align);
+ if (!ptr)
+ panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__,
size, align);
Signed-off-by: Mike Rapoport
Reviewed-by: Guo Ren # c-sky
Acked-by: Paul Burton # MIPS
Acked-by: Heiko Carstens # s390
Reviewed-by: Juergen Gross # Xen
---
[...]
diff --git a/mm/sparse.c b/mm/sparse.c
index 7ea5dc6..ad94242 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
[...]
@@ -425,6 +436,10 @@ static void __init sparse_buffer_init(unsigned
long size, int nid)
memblock_alloc_try_nid_raw(size, PAGE_SIZE,
__pa(MAX_DMA_ADDRESS),
MEMBLOCK_ALLOC_ACCESSIBLE, nid);
+ if (!sparsemap_buf)
+ panic("%s: Failed to allocate %lu bytes align=0x%lx nid=%d
from=%lx\n",
+ __func__, size, PAGE_SIZE, nid, __pa(MAX_DMA_ADDRESS));
+
memblock_alloc_try_nid_raw() does not panic (help explicitly says:
Does not
zero allocated memory, does not panic if request cannot be satisfied.).
"Does not panic" does not mean it always succeeds.
I agree, but at least here you are changing the behaviour by making it
panic explicitly. Are we sure there are not cases where the system could
just continue functionning ? Maybe a WARN_ON() would be enough there ?
Looking more in details, it looks like everything is done to live with
sparsemap_buf NULL, all functions using it check it so having it NULL
shouldn't imply a panic I believe, see code below.
static void *sparsemap_buf __meminitdata;
static void *sparsemap_buf_end __meminitdata;
static void __init sparse_buffer_init(unsigned long size, int nid)
{
WARN_ON(sparsemap_buf); /* forgot to call sparse_buffer_fini()? */
sparsemap_buf =
memblock_alloc_try_nid_raw(size, PAGE_SIZE,
__pa(MAX_DMA_ADDRESS),
MEMBLOCK_ALLOC_ACCESSIBLE, nid);
sparsemap_buf_end = sparsemap_buf + size;
}
static void __init sparse_buffer_fini(void)
{
unsigned long size = sparsemap_buf_end - sparsemap_buf;
if (sparsemap_buf && size > 0)
memblock_free_early(__pa(sparsemap_buf), size);
sparsemap_buf = NULL;
}
void * __meminit sparse_buffer_alloc(unsigned long size)
{
void *ptr = NULL;
if (sparsemap_buf) {
ptr = PTR_ALIGN(sparsemap_buf, size);
if (ptr + size > sparsemap_buf_end)
ptr = NULL;
else
sparsemap_buf = ptr + size;
}
return ptr;
}
Christophe
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 19/21] treewide: add checks for the return value of memblock_alloc*()
Le 31/01/2019 à 07:41, Mike Rapoport a écrit :
On Thu, Jan 31, 2019 at 07:07:46AM +0100, Christophe Leroy wrote:
Le 21/01/2019 à 09:04, Mike Rapoport a écrit :
Add check for the return value of memblock_alloc*() functions and call
panic() in case of error.
The panic message repeats the one used by panicing memblock allocators with
adjustment of parameters to include only relevant ones.
The replacement was mostly automated with semantic patches like the one
below with manual massaging of format strings.
@@
expression ptr, size, align;
@@
ptr = memblock_alloc(size, align);
+ if (!ptr)
+ panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__,
size, align);
Signed-off-by: Mike Rapoport
Reviewed-by: Guo Ren # c-sky
Acked-by: Paul Burton# MIPS
Acked-by: Heiko Carstens # s390
Reviewed-by: Juergen Gross # Xen
---
[...]
diff --git a/mm/sparse.c b/mm/sparse.c
index 7ea5dc6..ad94242 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
[...]
@@ -425,6 +436,10 @@ static void __init sparse_buffer_init(unsigned long size,
int nid)
memblock_alloc_try_nid_raw(size, PAGE_SIZE,
__pa(MAX_DMA_ADDRESS),
MEMBLOCK_ALLOC_ACCESSIBLE, nid);
+ if (!sparsemap_buf)
+ panic("%s: Failed to allocate %lu bytes align=0x%lx nid=%d
from=%lx\n",
+ __func__, size, PAGE_SIZE, nid, __pa(MAX_DMA_ADDRESS));
+
memblock_alloc_try_nid_raw() does not panic (help explicitly says: Does not
zero allocated memory, does not panic if request cannot be satisfied.).
"Does not panic" does not mean it always succeeds.
I agree, but at least here you are changing the behaviour by making it
panic explicitly. Are we sure there are not cases where the system could
just continue functionning ? Maybe a WARN_ON() would be enough there ?
Christophe
Stephen Rothwell reports a boot failure due to this change.
Please see my reply on that thread.
Christophe
sparsemap_buf_end = sparsemap_buf + size;
}
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 19/21] treewide: add checks for the return value of memblock_alloc*()
Le 21/01/2019 à 09:04, Mike Rapoport a écrit :
Add check for the return value of memblock_alloc*() functions and call
panic() in case of error.
The panic message repeats the one used by panicing memblock allocators with
adjustment of parameters to include only relevant ones.
The replacement was mostly automated with semantic patches like the one
below with manual massaging of format strings.
@@
expression ptr, size, align;
@@
ptr = memblock_alloc(size, align);
+ if (!ptr)
+ panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__,
size, align);
Signed-off-by: Mike Rapoport
Reviewed-by: Guo Ren # c-sky
Acked-by: Paul Burton# MIPS
Acked-by: Heiko Carstens # s390
Reviewed-by: Juergen Gross # Xen
---
[...]
diff --git a/mm/sparse.c b/mm/sparse.c
index 7ea5dc6..ad94242 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
[...]
@@ -425,6 +436,10 @@ static void __init sparse_buffer_init(unsigned long size,
int nid)
memblock_alloc_try_nid_raw(size, PAGE_SIZE,
__pa(MAX_DMA_ADDRESS),
MEMBLOCK_ALLOC_ACCESSIBLE, nid);
+ if (!sparsemap_buf)
+ panic("%s: Failed to allocate %lu bytes align=0x%lx nid=%d
from=%lx\n",
+ __func__, size, PAGE_SIZE, nid, __pa(MAX_DMA_ADDRESS));
+
memblock_alloc_try_nid_raw() does not panic (help explicitly says: Does
not zero allocated memory, does not panic if request cannot be satisfied.).
Stephen Rothwell reports a boot failure due to this change.
Christophe
sparsemap_buf_end = sparsemap_buf + size;
}
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v3] idle/x86: remove the call to boot_init_stack_canary() from cpu_startup_entry()
commit d7880812b359 ("idle: Add the stack canary init to
cpu_startup_entry()") added the call to boot_init_stack_canary()
in cpu_startup_entry() in an #ifdef CONFIG_X86 statement, with
the intention to remove that #ifdef later.
While implementing stack protector for powerpc, it has been
observed that calling boot_init_stack_canary() is also needed
for powerpc which uses per task (TLS) stack canary like the X86.
However, calling boot_init_stack_canary() would break arches
using global stack canary (ARM, SH, MIPS and XTENSA).
Instead of modifying the #ifdef CONFIG_X86 in an
#if defined(CONFIG_X86) || defined(CONFIG_PPC), powerpc
implemented the call to boot_init_stack_canary() in the function
calling cpu_startup_entry()
On x86, we have two functions calling cpu_startup_entry():
- start_secondary()
- cpu_bringup_and_idle()
start_secondary() already calls boot_init_stack_canary().
This patch adds the call to boot_init_stack_canary() in
cpu_bringup_and_idle() and removes it from cpu_startup_entry()
Reviewed-by: Juergen Gross
Signed-off-by: Christophe Leroy
---
v3: Fixed linux/stackprotector.h inclusion
v2: Revised commit log (#if defined had been droped by 'git commit')
arch/x86/xen/smp_pv.c | 2 ++
kernel/sched/idle.c | 15 ---
kernel/sched/sched.h | 1 -
3 files changed, 2 insertions(+), 16 deletions(-)
diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index e3b18ad49889..145506f9fdbe 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
#include
#include
@@ -88,6 +89,7 @@ static void cpu_bringup(void)
asmlinkage __visible void cpu_bringup_and_idle(void)
{
cpu_bringup();
+ boot_init_stack_canary();
cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
}
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 16f84142f2f4..f5516bae0c1b 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -347,21 +347,6 @@ EXPORT_SYMBOL_GPL(play_idle);
void cpu_startup_entry(enum cpuhp_state state)
{
- /*
-* This #ifdef needs to die, but it's too late in the cycle to
-* make this generic (ARM and SH have never invoked the canary
-* init for the non boot CPUs!). Will be fixed in 3.11
-*/
-#ifdef CONFIG_X86
- /*
-* If we're the non-boot CPU, nothing set the stack canary up
-* for us. The boot CPU already has it initialized but no harm
-* in doing it again. This is a good place for updating it, as
-* we wont ever return from this function (so the invalid
-* canaries already on the stack wont ever trigger).
-*/
- boot_init_stack_canary();
-#endif
arch_cpu_idle_prepare();
cpuhp_online_idle(state);
while (1)
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 455fa330de04..75a718ef771d 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -56,7 +56,6 @@
#include
#include
#include
-#include
#include
#include
#include
--
2.17.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] idle/x86: remove the call to boot_init_stack_canary() from cpu_startup_entry()
Le 19/10/2018 à 12:01, Juergen Gross a écrit :
On 19/10/2018 11:29, Christophe Leroy wrote:
commit d7880812b359 ("idle: Add the stack canary init to
cpu_startup_entry()") added the call to boot_init_stack_canary()
in cpu_startup_entry() in an #ifdef CONFIG_X86 statement, with
the intention to remove that #ifdef later.
While implementing stack protector for powerpc, it has been
observed that calling boot_init_stack_canary() is also needed
for powerpc which uses per task (TLS) stack canary like the X86.
However, calling boot_init_stack_canary() would break arches
using global stack canary (ARM, SH, MIPS and XTENSA).
Instead of adding modifying the #ifdef in a
implemented the call to boot_init_stack_canary() in the function
calling cpu_startup_entry()
I can't parse this sentence.
Oops, git commit took the #if for a comment and droped it. Fixed in v2.
On x86, we have two functions calling cpu_startup_entry():
- start_secondary()
- cpu_bringup_and_idle()
start_secondary() already calls boot_init_stack_canary().
This patch adds the call to boot_init_stack_canary() in
cpu_bringup_and_idle() and removes it from cpu_startup_entry()
Signed-off-by: Christophe Leroy
With the commit message made understandable you can add my
Reviewed-by: Juergen Gross
Thanks
Christophe
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] idle/x86: remove the call to boot_init_stack_canary() from cpu_startup_entry()
commit d7880812b359 ("idle: Add the stack canary init to
cpu_startup_entry()") added the call to boot_init_stack_canary()
in cpu_startup_entry() in an #ifdef CONFIG_X86 statement, with
the intention to remove that #ifdef later.
While implementing stack protector for powerpc, it has been
observed that calling boot_init_stack_canary() is also needed
for powerpc which uses per task (TLS) stack canary like the X86.
However, calling boot_init_stack_canary() would break arches
using global stack canary (ARM, SH, MIPS and XTENSA).
Instead of modifying the #ifdef CONFIG_X86 in an
#if defined(CONFIG_X86) || defined(CONFIG_PPC), powerpc
implemented the call to boot_init_stack_canary() in the function
calling cpu_startup_entry()
On x86, we have two functions calling cpu_startup_entry():
- start_secondary()
- cpu_bringup_and_idle()
start_secondary() already calls boot_init_stack_canary().
This patch adds the call to boot_init_stack_canary() in
cpu_bringup_and_idle() and removes it from cpu_startup_entry()
Reviewed-by: Juergen Gross
Signed-off-by: Christophe Leroy
---
v2: Revised commit log (#if defined had been droped by 'git commit')
arch/x86/xen/smp_pv.c | 1 +
kernel/sched/idle.c | 15 ---
2 files changed, 1 insertion(+), 15 deletions(-)
diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index e3b18ad49889..0e05e8e23998 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -88,6 +88,7 @@ static void cpu_bringup(void)
asmlinkage __visible void cpu_bringup_and_idle(void)
{
cpu_bringup();
+ boot_init_stack_canary();
cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
}
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 16f84142f2f4..f5516bae0c1b 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -347,21 +347,6 @@ EXPORT_SYMBOL_GPL(play_idle);
void cpu_startup_entry(enum cpuhp_state state)
{
- /*
-* This #ifdef needs to die, but it's too late in the cycle to
-* make this generic (ARM and SH have never invoked the canary
-* init for the non boot CPUs!). Will be fixed in 3.11
-*/
-#ifdef CONFIG_X86
- /*
-* If we're the non-boot CPU, nothing set the stack canary up
-* for us. The boot CPU already has it initialized but no harm
-* in doing it again. This is a good place for updating it, as
-* we wont ever return from this function (so the invalid
-* canaries already on the stack wont ever trigger).
-*/
- boot_init_stack_canary();
-#endif
arch_cpu_idle_prepare();
cpuhp_online_idle(state);
while (1)
--
2.13.3
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] idle/x86: remove the call to boot_init_stack_canary() from cpu_startup_entry()
commit d7880812b359 ("idle: Add the stack canary init to
cpu_startup_entry()") added the call to boot_init_stack_canary()
in cpu_startup_entry() in an #ifdef CONFIG_X86 statement, with
the intention to remove that #ifdef later.
While implementing stack protector for powerpc, it has been
observed that calling boot_init_stack_canary() is also needed
for powerpc which uses per task (TLS) stack canary like the X86.
However, calling boot_init_stack_canary() would break arches
using global stack canary (ARM, SH, MIPS and XTENSA).
Instead of adding modifying the #ifdef in a
implemented the call to boot_init_stack_canary() in the function
calling cpu_startup_entry()
On x86, we have two functions calling cpu_startup_entry():
- start_secondary()
- cpu_bringup_and_idle()
start_secondary() already calls boot_init_stack_canary().
This patch adds the call to boot_init_stack_canary() in
cpu_bringup_and_idle() and removes it from cpu_startup_entry()
Signed-off-by: Christophe Leroy
---
arch/x86/xen/smp_pv.c | 1 +
kernel/sched/idle.c | 15 ---
2 files changed, 1 insertion(+), 15 deletions(-)
diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index e3b18ad49889..0e05e8e23998 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -88,6 +88,7 @@ static void cpu_bringup(void)
asmlinkage __visible void cpu_bringup_and_idle(void)
{
cpu_bringup();
+ boot_init_stack_canary();
cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
}
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 16f84142f2f4..f5516bae0c1b 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -347,21 +347,6 @@ EXPORT_SYMBOL_GPL(play_idle);
void cpu_startup_entry(enum cpuhp_state state)
{
- /*
-* This #ifdef needs to die, but it's too late in the cycle to
-* make this generic (ARM and SH have never invoked the canary
-* init for the non boot CPUs!). Will be fixed in 3.11
-*/
-#ifdef CONFIG_X86
- /*
-* If we're the non-boot CPU, nothing set the stack canary up
-* for us. The boot CPU already has it initialized but no harm
-* in doing it again. This is a good place for updating it, as
-* we wont ever return from this function (so the invalid
-* canaries already on the stack wont ever trigger).
-*/
- boot_init_stack_canary();
-#endif
arch_cpu_idle_prepare();
cpuhp_online_idle(state);
while (1)
--
2.13.3
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
