* Re: [PATCH 03/30] mm: remove CONFIG_HAVE_MEMBLOCK
From: Mike Rapoport @ 2018-09-27 4:50 UTC (permalink / raw)
To: Alexander Duyck
Cc: linux-mm, Andrew Morton, Catalin Marinas, chris, David Miller,
Geert Uytterhoeven, green.hu, Greg KH, gxt, Ingo Molnar, jejb,
jonas, Jonathan Corbet, lftan, msalter, Martin Schwidefsky,
mattst88, mpe, Michal Hocko, monstr, palmer, paul.burton, rkuo,
richard, dalias, Russell King - ARM Linux, fancer.lancer,
Thomas Gleixner, Tony Luck, vgupta, Yoshinori Sato, linux-alpha,
linux-arm-kernel, linux-c6x-dev, linux-hexagon, linux-ia64, LKML,
linux-m68k, linux-mips, linux-parisc,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), linux-riscv,
linux-s390, linux-sh, linux-snps-arc, linux-um, nios2-dev,
openrisc, sparclinux, uclinux-h8-devel
In-Reply-To: <CAKgT0UcC-GTtyPK9ynvj6r3YFqy8kE40iMJxzPowbNoXGf9iWg@mail.gmail.com>
On Wed, Sep 26, 2018 at 05:34:32PM -0700, Alexander Duyck wrote:
> On Wed, Sep 26, 2018 at 11:32 AM Mike Rapoport <rppt@linux.vnet.ibm.com> wrote:
> >
> > On Wed, Sep 26, 2018 at 09:58:41AM -0700, Alexander Duyck wrote:
> > > On Fri, Sep 14, 2018 at 5:11 AM Mike Rapoport <rppt@linux.vnet.ibm.com> wrote:
> > > >
> > > > All architecures use memblock for early memory management. There is no need
> > > > for the CONFIG_HAVE_MEMBLOCK configuration option.
> > > >
> > > > Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
> > >
> > > <snip>
> > >
> > > > diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> > > > index 5169205..4ae91fc 100644
> > > > --- a/include/linux/memblock.h
> > > > +++ b/include/linux/memblock.h
> > > > @@ -2,7 +2,6 @@
> > > > #define _LINUX_MEMBLOCK_H
> > > > #ifdef __KERNEL__
> > > >
> > > > -#ifdef CONFIG_HAVE_MEMBLOCK
> > > > /*
> > > > * Logical memory blocks.
> > > > *
> > > > @@ -460,7 +459,6 @@ static inline phys_addr_t memblock_alloc(phys_addr_t size, phys_addr_t align)
> > > > {
> > > > return 0;
> > > > }
> > > > -#endif /* CONFIG_HAVE_MEMBLOCK */
> > > >
> > > > #endif /* __KERNEL__ */
> > >
> > > There was an #else above this section and I believe it and the code
> > > after it needs to be stripped as well.
> >
> > Right, I've already sent the fix [1] and it's in mmots.
> >
> > [1] https://lkml.org/lkml/2018/9/19/416
> >
>
> Are you sure? The patch you reference appears to be for
> drivers/of/fdt.c, and the bit I pointed out here is in
> include/linux/memblock.h.
Ah, sorry. You are right, will fix. Thanks for spotting it!
> - Alex
>
--
Sincerely yours,
Mike.
^ permalink raw reply
* [PATCH] powerpc/tm: Reformat comments
From: Michael Neuling @ 2018-09-27 5:05 UTC (permalink / raw)
To: mpe; +Cc: linuxppc-dev, Michael Neuling
The comments in this file don't conform to the coding style so take
them to "Comment Formatting Re-Education Camp"
Suggested-by: Michael "Camp Drill Sargent" Ellerman <mpe@ellerman.id.au>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/kernel/tm.S | 49 +++++++++++++++++++++++++---------------
1 file changed, 31 insertions(+), 18 deletions(-)
diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S
index 50e5cff10d..c954aecd9b 100644
--- a/arch/powerpc/kernel/tm.S
+++ b/arch/powerpc/kernel/tm.S
@@ -92,7 +92,8 @@ _GLOBAL(tm_abort)
blr
EXPORT_SYMBOL_GPL(tm_abort);
-/* void tm_reclaim(struct thread_struct *thread,
+/*
+ * void tm_reclaim(struct thread_struct *thread,
* uint8_t cause)
*
* - Performs a full reclaim. This destroys outstanding
@@ -163,14 +164,16 @@ _GLOBAL(tm_reclaim)
*/
TRECLAIM(R4) /* Cause in r4 */
- /* ******************** GPRs ******************** */
- /* Stash the checkpointed r13 away in the scratch SPR and get the real
+ /*
+ * ******************** GPRs ********************
+ * Stash the checkpointed r13 away in the scratch SPR and get the real
* paca
*/
SET_SCRATCH0(r13)
GET_PACA(r13)
- /* Stash the checkpointed r1 away in paca tm_scratch and get the real
+ /*
+ * Stash the checkpointed r1 away in paca tm_scratch and get the real
* stack pointer back
*/
std r1, PACATMSCRATCH(r13)
@@ -195,7 +198,8 @@ _GLOBAL(tm_reclaim)
addi r7, r12, PT_CKPT_REGS /* Thread's ckpt_regs */
- /* Make r7 look like an exception frame so that we
+ /*
+ * Make r7 look like an exception frame so that we
* can use the neat GPRx(n) macros. r7 is NOT a pt_regs ptr!
*/
subi r7, r7, STACK_FRAME_OVERHEAD
@@ -223,7 +227,8 @@ _GLOBAL(tm_reclaim)
/* ******************** NIP ******************** */
mfspr r3, SPRN_TFHAR
std r3, _NIP(r7) /* Returns to failhandler */
- /* The checkpointed NIP is ignored when rescheduling/rechkpting,
+ /*
+ * The checkpointed NIP is ignored when rescheduling/rechkpting,
* but is used in signal return to 'wind back' to the abort handler.
*/
@@ -246,12 +251,14 @@ _GLOBAL(tm_reclaim)
std r3, THREAD_TM_TAR(r12)
std r4, THREAD_TM_DSCR(r12)
- /* MSR and flags: We don't change CRs, and we don't need to alter
+ /*
+ * MSR and flags: We don't change CRs, and we don't need to alter
* MSR.
*/
- /* ******************** FPR/VR/VSRs ************
+ /*
+ * ******************** FPR/VR/VSRs ************
* After reclaiming, capture the checkpointed FPRs/VRs.
*
* We enabled VEC/FP/VSX in the msr above, so we can execute these
@@ -277,7 +284,8 @@ _GLOBAL(tm_reclaim)
stfd fr0,FPSTATE_FPSCR(r7)
- /* TM regs, incl TEXASR -- these live in thread_struct. Note they've
+ /*
+ * TM regs, incl TEXASR -- these live in thread_struct. Note they've
* been updated by the treclaim, to explain to userland the failure
* cause (aborted).
*/
@@ -313,7 +321,7 @@ _GLOBAL(tm_reclaim)
blr
- /*
+ /*
* void __tm_recheckpoint(struct thread_struct *thread)
* - Restore the checkpointed register state saved by tm_reclaim
* when we switch_to a process.
@@ -329,7 +337,8 @@ _GLOBAL(__tm_recheckpoint)
std r2, STK_GOT(r1)
stdu r1, -TM_FRAME_SIZE(r1)
- /* We've a struct pt_regs at [r1+STACK_FRAME_OVERHEAD].
+ /*
+ * We've a struct pt_regs at [r1+STACK_FRAME_OVERHEAD].
* This is used for backing up the NVGPRs:
*/
SAVE_NVGPRS(r1)
@@ -338,7 +347,8 @@ _GLOBAL(__tm_recheckpoint)
addi r7, r3, PT_CKPT_REGS /* Thread's ckpt_regs */
- /* Make r7 look like an exception frame so that we
+ /*
+ * Make r7 look like an exception frame so that we
* can use the neat GPRx(n) macros. r7 is now NOT a pt_regs ptr!
*/
subi r7, r7, STACK_FRAME_OVERHEAD
@@ -407,14 +417,15 @@ restore_gprs:
REST_NVGPRS(r7) /* GPR14-31 */
- /* Load up PPR and DSCR here so we don't run with user values for long
- */
+ /* Load up PPR and DSCR here so we don't run with user values for long */
mtspr SPRN_DSCR, r5
mtspr SPRN_PPR, r6
- /* Do final sanity check on TEXASR to make sure FS is set. Do this
+ /*
+ * Do final sanity check on TEXASR to make sure FS is set. Do this
* here before we load up the userspace r1 so any bugs we hit will get
- * a call chain */
+ * a call chain
+ */
mfspr r5, SPRN_TEXASR
srdi r5, r5, 16
li r6, (TEXASR_FS)@h
@@ -422,7 +433,8 @@ restore_gprs:
1: tdeqi r6, 0
EMIT_BUG_ENTRY 1b,__FILE__,__LINE__,0
- /* Do final sanity check on MSR to make sure we are not transactional
+ /*
+ * Do final sanity check on MSR to make sure we are not transactional
* or suspended
*/
mfmsr r6
@@ -470,7 +482,8 @@ restore_gprs:
HMT_MEDIUM
- /* Our transactional state has now changed.
+ /*
+ * Our transactional state has now changed.
*
* Now just get out of here. Transactional (current) state will be
* updated once restore is called on the return path in the _switch-ed
--
2.17.1
^ permalink raw reply related
* Re: How to define some additional KBUILD_CFLAGS after building include/generated/asm-offsets.h ?
From: Masahiro Yamada @ 2018-09-27 5:24 UTC (permalink / raw)
To: Michael Ellerman
Cc: Christophe LEROY, linuxppc-dev, Linux Kbuild mailing list,
Michal Marek, Segher Boessenkool, LKML
In-Reply-To: <87bm8m5qbs.fsf@concordia.ellerman.id.au>
Hi.
2018-09-25 10:16 GMT+09:00 Michael Ellerman <mpe@ellerman.id.au>:
> Christophe LEROY <christophe.leroy@c-s.fr> writes:
>
>> Le 24/09/2018 =C3=A0 14:10, Michael Ellerman a =C3=A9crit :
>>> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>>>
>>>> I'm trying to implement TLS based stack protector in the Linux Kernel.
>>>> For that I need to give to GCC the offset at which it will find the
>>>> canary (register r2 is pointing to the current task struct).
>>>>
>>>> I have been able to do it with the below patch, but it only works when
>>>> include/generated/asm-offsets.h already exists from the start of the b=
uild.
>>>>
>>>> Is there a way to evaluate CANARY_OFFSET and add the stack-protector
>>>> flags to KBUILD_FLAGS only after include/generated/asm-offsets.h is bu=
ilt ?
>>>>
>>>> Or another way of add -mstack-protector-guard-offset=3Doffsetof(struct
>>>> task_struct, stack_canary) ?
>>>
>>> This seems to work, at least I see the value in CFLAGS:
>>>
>>> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
>>> index 07d9dce..39ee113 100644
>>> --- a/arch/powerpc/Makefile
>>> +++ b/arch/powerpc/Makefile
>>> @@ -404,6 +394,11 @@ archclean:
>>>
>>> archprepare: checkbin
>>>
>>> +prepare: stack_protector_prepare
>>> +
>>> +stack_protector_prepare: prepare0
>>> + $(eval KBUILD_CFLAGS +=3D -mstack-protector-guard-offset=3D$(shell=
awk '{if ($$2 =3D=3D "TSK_STACK_CANARY") print $$3;}' include/generated/as=
m-offsets.h))
>>> +
>>
>> Great, it works !
>> Thanks, I have sent v3 of the patches.
>
> Cool.
>
> It would be good to here from someone who knows Kbuild better than me if
> this is acceptable or just a gross hack :)
I am fine with this solution.
Thanks.
--=20
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v2 1/7] powerpc: rename setbits32/clrbits32 to setbits32_be/clrbits32_be
From: LABBE Corentin @ 2018-09-27 5:30 UTC (permalink / raw)
To: Christophe LEROY
Cc: Gilles.Muller, Julia.Lawall, agust, airlied, alexandre.torgue,
alistair, benh, carlo, davem, galak, joabreu, khilman,
maxime.ripard, michal.lkml, mpe, mporter, narmstrong,
nicolas.palix, oss, paulus, peppe.cavallaro, tj, vitb, wens,
netdev, linux-kernel, dri-devel, linux-ide, linux-amlogic,
linuxppc-dev, cocci, linux-arm-kernel
In-Reply-To: <eeaeff10-dcc9-ab50-5d54-4d727f12d584@c-s.fr>
On Tue, Sep 25, 2018 at 06:56:23AM +0200, Christophe LEROY wrote:
> Fix the patch title.
>
>
> Le 24/09/2018 à 21:04, Corentin Labbe a écrit :
> > Since setbits32/clrbits32 work on be32, it's better to remove ambiguity on
> > the used data type.
> >
> > Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> > ---
> > arch/powerpc/include/asm/fsl_lbc.h | 2 +-
> > arch/powerpc/include/asm/io.h | 5 +-
> > arch/powerpc/platforms/44x/canyonlands.c | 4 +-
> > arch/powerpc/platforms/4xx/gpio.c | 28 ++++-----
> > arch/powerpc/platforms/512x/pdm360ng.c | 6 +-
> > arch/powerpc/platforms/52xx/mpc52xx_common.c | 6 +-
> > arch/powerpc/platforms/52xx/mpc52xx_gpt.c | 10 ++--
> > arch/powerpc/platforms/82xx/ep8248e.c | 2 +-
> > arch/powerpc/platforms/82xx/km82xx.c | 6 +-
> > arch/powerpc/platforms/82xx/mpc8272_ads.c | 10 ++--
> > arch/powerpc/platforms/82xx/pq2.c | 2 +-
> > arch/powerpc/platforms/82xx/pq2ads-pci-pic.c | 4 +-
> > arch/powerpc/platforms/82xx/pq2fads.c | 10 ++--
> > arch/powerpc/platforms/83xx/km83xx.c | 6 +-
> > arch/powerpc/platforms/83xx/mpc836x_mds.c | 2 +-
> > arch/powerpc/platforms/85xx/mpc85xx_mds.c | 2 +-
> > arch/powerpc/platforms/85xx/mpc85xx_pm_ops.c | 4 +-
> > arch/powerpc/platforms/85xx/mpc85xx_rdb.c | 2 +-
> > arch/powerpc/platforms/85xx/p1022_ds.c | 4 +-
> > arch/powerpc/platforms/85xx/p1022_rdk.c | 4 +-
> > arch/powerpc/platforms/85xx/t1042rdb_diu.c | 4 +-
> > arch/powerpc/platforms/85xx/twr_p102x.c | 2 +-
> > arch/powerpc/platforms/86xx/mpc8610_hpcd.c | 4 +-
> > arch/powerpc/platforms/8xx/adder875.c | 2 +-
> > arch/powerpc/platforms/8xx/m8xx_setup.c | 4 +-
> > arch/powerpc/platforms/8xx/mpc86xads_setup.c | 4 +-
> > arch/powerpc/platforms/8xx/mpc885ads_setup.c | 28 ++++-----
> > arch/powerpc/platforms/embedded6xx/flipper-pic.c | 6 +-
> > arch/powerpc/platforms/embedded6xx/hlwd-pic.c | 8 +--
> > arch/powerpc/platforms/embedded6xx/wii.c | 10 ++--
> > arch/powerpc/sysdev/cpm1.c | 26 ++++-----
> > arch/powerpc/sysdev/cpm2.c | 16 ++---
> > arch/powerpc/sysdev/cpm_common.c | 4 +-
> > arch/powerpc/sysdev/fsl_85xx_l2ctlr.c | 8 +--
> > arch/powerpc/sysdev/fsl_lbc.c | 2 +-
> > arch/powerpc/sysdev/fsl_pci.c | 8 +--
> > arch/powerpc/sysdev/fsl_pmc.c | 2 +-
> > arch/powerpc/sysdev/fsl_rcpm.c | 74 ++++++++++++------------
> > arch/powerpc/sysdev/fsl_rio.c | 4 +-
> > arch/powerpc/sysdev/fsl_rmu.c | 8 +--
> > arch/powerpc/sysdev/mpic_timer.c | 12 ++--
> > 41 files changed, 178 insertions(+), 177 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/fsl_lbc.h b/arch/powerpc/include/asm/fsl_lbc.h
> > index c7240a024b96..4d6a56b48a28 100644
> > --- a/arch/powerpc/include/asm/fsl_lbc.h
> > +++ b/arch/powerpc/include/asm/fsl_lbc.h
> > @@ -276,7 +276,7 @@ static inline void fsl_upm_start_pattern(struct fsl_upm *upm, u8 pat_offset)
> > */
> > static inline void fsl_upm_end_pattern(struct fsl_upm *upm)
> > {
> > - clrbits32(upm->mxmr, MxMR_OP_RP);
> > + clrbits_be32(upm->mxmr, MxMR_OP_RP);
> >
> > while (in_be32(upm->mxmr) & MxMR_OP_RP)
> > cpu_relax();
> > diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
> > index e0331e754568..57486a1b9992 100644
> > --- a/arch/powerpc/include/asm/io.h
> > +++ b/arch/powerpc/include/asm/io.h
> > @@ -873,8 +873,8 @@ static inline void * bus_to_virt(unsigned long address)
> > #endif /* CONFIG_PPC32 */
> >
> > /* access ports */
> > -#define setbits32(_addr, _v) out_be32((_addr), in_be32(_addr) | (_v))
> > -#define clrbits32(_addr, _v) out_be32((_addr), in_be32(_addr) & ~(_v))
> > +#define setbits_be32(_addr, _v) out_be32((_addr), in_be32(_addr) | (_v))
> > +#define clrbits_be32(_addr, _v) out_be32((_addr), in_be32(_addr) & ~(_v))
> >
> > #define setbits16(_addr, _v) out_be16((_addr), in_be16(_addr) | (_v))
> > #define clrbits16(_addr, _v) out_be16((_addr), in_be16(_addr) & ~(_v))
> > @@ -904,6 +904,7 @@ static inline void * bus_to_virt(unsigned long address)
> > #define clrsetbits_le16(addr, clear, set) clrsetbits(le16, addr, clear, set)
> >
> > #define clrsetbits_8(addr, clear, set) clrsetbits(8, addr, clear, set)
> > +#define clrsetbits_be32(addr, clear, set) clrsetbits(be32, addr, clear, set)
>
> This one already exists a few lines above.
>
> >
> > #endif /* __KERNEL__ */
> >
> > diff --git a/arch/powerpc/platforms/44x/canyonlands.c b/arch/powerpc/platforms/44x/canyonlands.c
> > index 157f4ce46386..6aeb4ca64d09 100644
> > --- a/arch/powerpc/platforms/44x/canyonlands.c
> > +++ b/arch/powerpc/platforms/44x/canyonlands.c
> > @@ -113,8 +113,8 @@ static int __init ppc460ex_canyonlands_fixup(void)
> > * USB2HStop and gpio19 will be USB2DStop. For more details refer to
> > * table 34-7 of PPC460EX user manual.
> > */
> > - setbits32((vaddr + GPIO0_OSRH), 0x42000000);
> > - setbits32((vaddr + GPIO0_TSRH), 0x42000000);
> > + setbits_be32((vaddr + GPIO0_OSRH), 0x42000000);
> > + setbits_be32((vaddr + GPIO0_TSRH), 0x42000000);
> > err_gpio:
> > iounmap(vaddr);
> > err_bcsr:
> > diff --git a/arch/powerpc/platforms/4xx/gpio.c b/arch/powerpc/platforms/4xx/gpio.c
> > index 2238e369cde4..8436da0617fd 100644
> > --- a/arch/powerpc/platforms/4xx/gpio.c
> > +++ b/arch/powerpc/platforms/4xx/gpio.c
> > @@ -82,9 +82,9 @@ __ppc4xx_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
> > struct ppc4xx_gpio __iomem *regs = mm_gc->regs;
> >
> > if (val)
> > - setbits32(®s->or, GPIO_MASK(gpio));
> > + setbits_be32(®s->or, GPIO_MASK(gpio));
> > else
> > - clrbits32(®s->or, GPIO_MASK(gpio));
> > + clrbits_be32(®s->or, GPIO_MASK(gpio));
> > }
> >
> > static void
> > @@ -112,18 +112,18 @@ static int ppc4xx_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
> > spin_lock_irqsave(&chip->lock, flags);
> >
> > /* Disable open-drain function */
> > - clrbits32(®s->odr, GPIO_MASK(gpio));
> > + clrbits_be32(®s->odr, GPIO_MASK(gpio));
> >
> > /* Float the pin */
> > - clrbits32(®s->tcr, GPIO_MASK(gpio));
> > + clrbits_be32(®s->tcr, GPIO_MASK(gpio));
> >
> > /* Bits 0-15 use TSRL/OSRL, bits 16-31 use TSRH/OSRH */
> > if (gpio < 16) {
> > - clrbits32(®s->osrl, GPIO_MASK2(gpio));
> > - clrbits32(®s->tsrl, GPIO_MASK2(gpio));
> > + clrbits_be32(®s->osrl, GPIO_MASK2(gpio));
> > + clrbits_be32(®s->tsrl, GPIO_MASK2(gpio));
> > } else {
> > - clrbits32(®s->osrh, GPIO_MASK2(gpio));
> > - clrbits32(®s->tsrh, GPIO_MASK2(gpio));
> > + clrbits_be32(®s->osrh, GPIO_MASK2(gpio));
> > + clrbits_be32(®s->tsrh, GPIO_MASK2(gpio));
> > }
> >
> > spin_unlock_irqrestore(&chip->lock, flags);
> > @@ -145,18 +145,18 @@ ppc4xx_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
> > __ppc4xx_gpio_set(gc, gpio, val);
> >
> > /* Disable open-drain function */
> > - clrbits32(®s->odr, GPIO_MASK(gpio));
> > + clrbits_be32(®s->odr, GPIO_MASK(gpio));
> >
> > /* Drive the pin */
> > - setbits32(®s->tcr, GPIO_MASK(gpio));
> > + setbits_be32(®s->tcr, GPIO_MASK(gpio));
> >
> > /* Bits 0-15 use TSRL, bits 16-31 use TSRH */
> > if (gpio < 16) {
> > - clrbits32(®s->osrl, GPIO_MASK2(gpio));
> > - clrbits32(®s->tsrl, GPIO_MASK2(gpio));
> > + clrbits_be32(®s->osrl, GPIO_MASK2(gpio));
> > + clrbits_be32(®s->tsrl, GPIO_MASK2(gpio));
> > } else {
> > - clrbits32(®s->osrh, GPIO_MASK2(gpio));
> > - clrbits32(®s->tsrh, GPIO_MASK2(gpio));
> > + clrbits_be32(®s->osrh, GPIO_MASK2(gpio));
> > + clrbits_be32(®s->tsrh, GPIO_MASK2(gpio));
> > }
> >
> > spin_unlock_irqrestore(&chip->lock, flags);
> > diff --git a/arch/powerpc/platforms/512x/pdm360ng.c b/arch/powerpc/platforms/512x/pdm360ng.c
> > index dc81f05e0bce..06b95795267a 100644
> > --- a/arch/powerpc/platforms/512x/pdm360ng.c
> > +++ b/arch/powerpc/platforms/512x/pdm360ng.c
> > @@ -38,7 +38,7 @@ static int pdm360ng_get_pendown_state(void)
> >
> > reg = in_be32(pdm360ng_gpio_base + 0xc);
> > if (reg & 0x40)
> > - setbits32(pdm360ng_gpio_base + 0xc, 0x40);
> > + setbits_be32(pdm360ng_gpio_base + 0xc, 0x40);
> >
> > reg = in_be32(pdm360ng_gpio_base + 0x8);
> >
> > @@ -69,8 +69,8 @@ static int __init pdm360ng_penirq_init(void)
> > return -ENODEV;
> > }
> > out_be32(pdm360ng_gpio_base + 0xc, 0xffffffff);
> > - setbits32(pdm360ng_gpio_base + 0x18, 0x2000);
> > - setbits32(pdm360ng_gpio_base + 0x10, 0x40);
> > + setbits_be32(pdm360ng_gpio_base + 0x18, 0x2000);
> > + setbits_be32(pdm360ng_gpio_base + 0x10, 0x40);
> >
> > return 0;
> > }
> > diff --git a/arch/powerpc/platforms/52xx/mpc52xx_common.c b/arch/powerpc/platforms/52xx/mpc52xx_common.c
> > index 565e3a83dc9e..edfe619d67bf 100644
> > --- a/arch/powerpc/platforms/52xx/mpc52xx_common.c
> > +++ b/arch/powerpc/platforms/52xx/mpc52xx_common.c
> > @@ -314,13 +314,13 @@ int mpc5200_psc_ac97_gpio_reset(int psc_number)
> >
> > /* enable gpio pins for output */
> > setbits8(&wkup_gpio->wkup_gpioe, reset);
> > - setbits32(&simple_gpio->simple_gpioe, sync | out);
> > + setbits_be32(&simple_gpio->simple_gpioe, sync | out);
> >
> > setbits8(&wkup_gpio->wkup_ddr, reset);
> > - setbits32(&simple_gpio->simple_ddr, sync | out);
> > + setbits_be32(&simple_gpio->simple_ddr, sync | out);
> >
> > /* Assert cold reset */
> > - clrbits32(&simple_gpio->simple_dvo, sync | out);
> > + clrbits_be32(&simple_gpio->simple_dvo, sync | out);
> > clrbits8(&wkup_gpio->wkup_dvo, reset);
> >
> > /* wait for 1 us */
> > diff --git a/arch/powerpc/platforms/52xx/mpc52xx_gpt.c b/arch/powerpc/platforms/52xx/mpc52xx_gpt.c
> > index 17cf249b18ee..e9f4dec06077 100644
> > --- a/arch/powerpc/platforms/52xx/mpc52xx_gpt.c
> > +++ b/arch/powerpc/platforms/52xx/mpc52xx_gpt.c
> > @@ -142,7 +142,7 @@ static void mpc52xx_gpt_irq_unmask(struct irq_data *d)
> > unsigned long flags;
> >
> > raw_spin_lock_irqsave(&gpt->lock, flags);
> > - setbits32(&gpt->regs->mode, MPC52xx_GPT_MODE_IRQ_EN);
> > + setbits_be32(&gpt->regs->mode, MPC52xx_GPT_MODE_IRQ_EN);
> > raw_spin_unlock_irqrestore(&gpt->lock, flags);
> > }
> >
> > @@ -152,7 +152,7 @@ static void mpc52xx_gpt_irq_mask(struct irq_data *d)
> > unsigned long flags;
> >
> > raw_spin_lock_irqsave(&gpt->lock, flags);
> > - clrbits32(&gpt->regs->mode, MPC52xx_GPT_MODE_IRQ_EN);
> > + clrbits_be32(&gpt->regs->mode, MPC52xx_GPT_MODE_IRQ_EN);
> > raw_spin_unlock_irqrestore(&gpt->lock, flags);
> > }
> >
> > @@ -308,7 +308,7 @@ static int mpc52xx_gpt_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
> > dev_dbg(gpt->dev, "%s: gpio:%d\n", __func__, gpio);
> >
> > raw_spin_lock_irqsave(&gpt->lock, flags);
> > - clrbits32(&gpt->regs->mode, MPC52xx_GPT_MODE_GPIO_MASK);
> > + clrbits_be32(&gpt->regs->mode, MPC52xx_GPT_MODE_GPIO_MASK);
> > raw_spin_unlock_irqrestore(&gpt->lock, flags);
> >
> > return 0;
> > @@ -482,7 +482,7 @@ int mpc52xx_gpt_stop_timer(struct mpc52xx_gpt_priv *gpt)
> > return -EBUSY;
> > }
> >
> > - clrbits32(&gpt->regs->mode, MPC52xx_GPT_MODE_COUNTER_ENABLE);
> > + clrbits_be32(&gpt->regs->mode, MPC52xx_GPT_MODE_COUNTER_ENABLE);
> > raw_spin_unlock_irqrestore(&gpt->lock, flags);
> > return 0;
> > }
> > @@ -639,7 +639,7 @@ static int mpc52xx_wdt_release(struct inode *inode, struct file *file)
> > unsigned long flags;
> >
> > raw_spin_lock_irqsave(&gpt_wdt->lock, flags);
> > - clrbits32(&gpt_wdt->regs->mode,
> > + clrbits_be32(&gpt_wdt->regs->mode,
> > MPC52xx_GPT_MODE_COUNTER_ENABLE | MPC52xx_GPT_MODE_WDT_EN);
>
> The alignment needs to be fixed here (and all other places). The second
> line should start under the &
> Eventually use checkpatch to locate all places that need to be fixed.
> (checkpatch may even fix it for you)
>
Thanks, I will fix all reported problem
^ permalink raw reply
* Re: [PATCH v2 2/7] include: add setbits32/clrbits32/clrsetbits32/setbits64/clrbits64/clrsetbits64 in linux/setbits.h
From: LABBE Corentin @ 2018-09-27 5:35 UTC (permalink / raw)
To: Christophe LEROY
Cc: Gilles.Muller, Julia.Lawall, agust, airlied, alexandre.torgue,
alistair, benh, carlo, davem, galak, joabreu, khilman,
maxime.ripard, michal.lkml, mpe, mporter, narmstrong,
nicolas.palix, oss, paulus, peppe.cavallaro, tj, vitb, wens,
netdev, linux-kernel, dri-devel, linux-ide, linux-amlogic,
linuxppc-dev, cocci, linux-arm-kernel
In-Reply-To: <4a63152f-9eca-f7d9-8fe6-59caaab33666@c-s.fr>
On Tue, Sep 25, 2018 at 07:05:00AM +0200, Christophe LEROY wrote:
>
>
> Le 24/09/2018 à 21:04, Corentin Labbe a écrit :
> > This patch adds setbits32/clrbits32/clrsetbits32 and
> > setbits64/clrbits64/clrsetbits64 in linux/setbits.h header.
>
> Fix the patch subject and description.
>
> >
> > Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> > ---
> > include/linux/setbits.h | 88 +++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 88 insertions(+)
> > create mode 100644 include/linux/setbits.h
> >
> > diff --git a/include/linux/setbits.h b/include/linux/setbits.h
> > new file mode 100644
> > index 000000000000..6e7e257134ae
> > --- /dev/null
> > +++ b/include/linux/setbits.h
> > @@ -0,0 +1,88 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __LINUX_SETBITS_H
> > +#define __LINUX_SETBITS_H
> > +
> > +#include <linux/io.h>
> > +
> > +#define __setbits(readfunction, writefunction, addr, set) \
> > + writefunction((readfunction(addr) | (set)), addr)
>
> You don't need so long names for parameters in a 2 lines macro (See
> Linux Kernel Codying style §4 Naming).
>
> A single line macro would be feasible with only 3 chars names:
>
> #define __setbits(rfn, wfn, addr, set) wfn((rfn(addr) | (set)), addr)
>
Thanks I will fix all reported problem.
> > +#define __clrbits(readfunction, writefunction, addr, mask) \
> > + writefunction((readfunction(addr) & ~(mask)), addr)
> > +#define __clrsetbits(readfunction, writefunction, addr, mask, set) \
> > + writefunction(((readfunction(addr) & ~(mask)) | (set)), addr)
> > +#define __setclrbits(readfunction, writefunction, addr, mask, set) \
> > + writefunction(((readfunction(addr) | (set)) & ~(mask)), addr)
> > +
> > +#ifndef setbits_le32
> > +#define setbits_le32(addr, set) __setbits(readl, writel, addr, set)
> > +#endif
> > +#ifndef setbits_le32_relaxed
> > +#define setbits_le32_relaxed(addr, set) __setbits(readl_relaxed, writel_relaxed, \
> > + addr, set)
> > +#endif
> > +
> > +#ifndef clrbits_le32
> > +#define clrbits_le32(addr, mask) __clrbits(readl, writel, addr, mask)
> > +#endif
> > +#ifndef clrbits_le32_relaxed
> > +#define clrbits_le32_relaxed(addr, mask) __clrbits(readl_relaxed, writel_relaxed, \
> > + addr, mask)
> > +#endif
> > +
> > +#ifndef clrsetbits_le32
> > +#define clrsetbits_le32(addr, mask, set) __clrsetbits(readl, writel, addr, mask, set)
> > +#endif
> > +#ifndef clrsetbits_le32_relaxed
> > +#define clrsetbits_le32_relaxed(addr, mask, set) __clrsetbits(readl_relaxed, \
> > + writel_relaxed, \
> > + addr, mask, set)
> > +#endif
> > +
> > +#ifndef setclrbits_le32
> > +#define setclrbits_le32(addr, mask, set) __setclrbits(readl, writel, addr, mask, set)
> > +#endif
> > +#ifndef setclrbits_le32_relaxed
> > +#define setclrbits_le32_relaxed(addr, mask, set) __setclrbits(readl_relaxed, \
> > + writel_relaxed, \
> > + addr, mask, set)
> > +#endif
> > +
> > +/* We cannot use CONFIG_64BIT as some x86 drivers use non-atomicwriteq() */
> > +#if defined(writeq) && defined(readq)
>
> Take care. At least Alpha Arch defines it as a static inline without
> redefining it as a #define. (see arch/alpha/kernel/io.c)
In fact, it does in arch/alpha/include/asm/io.h along with a gentle comment.
But fixing their comment will be another interesting patch serie.
Regards
Corentin Labbe
^ permalink raw reply
* Re: [PATCH v4 1/2] powerpc/32: add stack protector support
From: Christophe LEROY @ 2018-09-27 6:20 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
linuxppc-dev, linux-kernel
In-Reply-To: <20180926191615.GO23155@gate.crashing.org>
Le 26/09/2018 à 21:16, Segher Boessenkool a écrit :
> On Wed, Sep 26, 2018 at 11:40:38AM +0000, Christophe Leroy wrote:
>> +static __always_inline void boot_init_stack_canary(void)
>> +{
>> + unsigned long canary;
>> +
>> + /* Try to get a semi random initial value. */
>> + get_random_bytes(&canary, sizeof(canary));
>> + canary ^= mftb();
>> + canary ^= LINUX_VERSION_CODE;
>> +
>> + current->stack_canary = canary;
>> +}
>
> I still think you should wait until there is entropy available. You
> haven't answered my questions about that (or I didn't see them): what
> does the kernel do in other similar cases?
>
> Looks great otherwise!
>
What do you mean by 'other similar cases' ? All arches have similar
boot_init_stack_canary(). x86 uses rdtsc() which is equivalent to our
mftb(). Most arches xor it with LINUX_VERSION_CODE.
The issue is that it is called very early in start_kernel(), however
they try to set some entropy anyway:
boot_cpu_init();
page_address_init();
pr_notice("%s", linux_banner);
setup_arch(&command_line);
/*
* Set up the the initial canary and entropy after arch
* and after adding latent and command line entropy.
*/
add_latent_entropy();
add_device_randomness(command_line, strlen(command_line));
boot_init_stack_canary();
Apparently, it is too early for calling wait_for_random_bytes(), see below.
However this is the canary for initial startup only. Only idle() still
uses this canary once the system is running. A new canary is set for any
new forked task.
Maybe should the idle canary be updated later once there is more entropy
? Today there is a new call to boot_init_stack_canary() in
cpu_startup_entry(), but it is enclosed inside #ifdef CONFIG_X86.
[ 0.000000] Unable to handle kernel paging request for data at
address 0x00000200
[ 0.000000] Faulting instruction address: 0xc0048e2c
[ 0.000000] Oops: Kernel access of bad area, sig: 11 [#1]
[ 0.000000] BE PREEMPT CMPC885
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted
4.19.0-rc4-s3k-dev-00498-g439f9fbadb38-dirty #6
[ 0.000000] NIP: c0048e2c LR: c05cf9ec CTR: c0018a68
[ 0.000000] REGS: c07e9e40 TRAP: 0300 Not tainted
(4.19.0-rc4-s3k-dev-00498-g439f9fbadb38-dirty)
[ 0.000000] MSR: 00001032 <ME,IR,DR,RI> CR: 28044222 XER: 00000000
[ 0.000000] DAR: 00000200 DSISR: c0000000
[ 0.000000] GPR00: c05cf9ec c07e9ef0 c078c560 00000000 00000000
00000001 c080a405 626f6f74
[ 0.000000] GPR08: 00001032 c07e8000 00000000 00000004 28044222
100c82b6 00000000 07ff9580
[ 0.000000] GPR16: 00000000 07ffb94c 00000000 00000000 00000000
00000000 00000000 00000000
[ 0.000000] GPR24: 00000000 07ff9580 c05cfb3c 00000000 c0790000
c07e8000 c0797740 00000000
[ 0.000000] NIP [c0048e2c] __schedule_bug+0x24/0x78
[ 0.000000] LR [c05cf9ec] __schedule+0x4f4/0x614
[ 0.000000] Call Trace:
[ 0.000000] [c07e9ef0] [07ffb94c] 0x7ffb94c (unreliable)
[ 0.000000] [c07e9f00] [c05cf9ec] __schedule+0x4f4/0x614
[ 0.000000] [c07e9f50] [c05cfb3c] schedule+0x30/0x5c
[ 0.000000] [c07e9f70] [c02bf15c] wait_for_random_bytes.part.4+0xa0/0xa8
[ 0.000000] [c07e9fb0] [c06fc9c8] start_kernel+0x98/0x46c
[ 0.000000] [c07e9ff0] [c00022cc] start_here+0x44/0x98
[ 0.000000] Instruction dump:
[ 0.000000] 419eff30 4bffff8c 4bfd6619 3d20c081 81298154 2f890000
4cbe0020 9421fff0
[ 0.000000] 7c0802a6 90010014 7c641b78 54290024 <80a40200> 80c90008
3c60c06a 388402fc
[ 0.000000] random: get_random_bytes called from
print_oops_end_marker+0x34/0x6c with crng_init=0
[ 0.000000] ---[ end trace 343b232a5e00519e ]---
Christophe
^ permalink raw reply
* Re: How to define some additional KBUILD_CFLAGS after building include/generated/asm-offsets.h ?
From: Michael Ellerman @ 2018-09-27 6:31 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Christophe LEROY, linuxppc-dev, Linux Kbuild mailing list,
Michal Marek, Segher Boessenkool, LKML
In-Reply-To: <CAK7LNAQA2JyK9aOp9QQF32bwsz2MitaZ5HrqSVjdEvRdCQpO3g@mail.gmail.com>
Masahiro Yamada <yamada.masahiro@socionext.com> writes:
> 2018-09-25 10:16 GMT+09:00 Michael Ellerman <mpe@ellerman.id.au>:
>> Christophe LEROY <christophe.leroy@c-s.fr> writes:
>>
>>> Le 24/09/2018 =C3=A0 14:10, Michael Ellerman a =C3=A9crit :
>>>> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>>>>
>>>>> I'm trying to implement TLS based stack protector in the Linux Kernel.
>>>>> For that I need to give to GCC the offset at which it will find the
>>>>> canary (register r2 is pointing to the current task struct).
>>>>>
>>>>> I have been able to do it with the below patch, but it only works when
>>>>> include/generated/asm-offsets.h already exists from the start of the =
build.
>>>>>
>>>>> Is there a way to evaluate CANARY_OFFSET and add the stack-protector
>>>>> flags to KBUILD_FLAGS only after include/generated/asm-offsets.h is b=
uilt ?
>>>>>
>>>>> Or another way of add -mstack-protector-guard-offset=3Doffsetof(struct
>>>>> task_struct, stack_canary) ?
>>>>
>>>> This seems to work, at least I see the value in CFLAGS:
>>>>
>>>> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
>>>> index 07d9dce..39ee113 100644
>>>> --- a/arch/powerpc/Makefile
>>>> +++ b/arch/powerpc/Makefile
>>>> @@ -404,6 +394,11 @@ archclean:
>>>>
>>>> archprepare: checkbin
>>>>
>>>> +prepare: stack_protector_prepare
>>>> +
>>>> +stack_protector_prepare: prepare0
>>>> + $(eval KBUILD_CFLAGS +=3D -mstack-protector-guard-offset=3D$(shel=
l awk '{if ($$2 =3D=3D "TSK_STACK_CANARY") print $$3;}' include/generated/a=
sm-offsets.h))
>>>> +
>>>
>>> Great, it works !
>>> Thanks, I have sent v3 of the patches.
>>
>> Cool.
>>
>> It would be good to here from someone who knows Kbuild better than me if
>> this is acceptable or just a gross hack :)
>
> I am fine with this solution.
Thanks.
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/tm: Avoid possible userspace r1 corruption on reclaim
From: Michael Ellerman @ 2018-09-27 6:41 UTC (permalink / raw)
To: Michael Neuling
Cc: linuxppc-dev, Nicholas Piggin, paulus, benh, Breno Leitao,
aneesh.kumar
In-Reply-To: <cc2f3d4526039cf71e4dac1c0a231e27d59cb4c0.camel@neuling.org>
Michael Neuling <mikey@neuling.org> writes:
> On Tue, 2018-09-25 at 22:00 +1000, Michael Ellerman wrote:
>> Michael Neuling <mikey@neuling.org> writes:
>> > Current we store the userspace r1 to PACATMSCRATCH before finally
>> > saving it to the thread struct.
>> >
>> > In theory an exception could be taken here (like a machine check or
>> > SLB miss) that could write PACATMSCRATCH and hence corrupt the
>> > userspace r1. The SLB fault currently doesn't touch PACATMSCRATCH, but
>> > others do.
>> >
>> > We've never actually seen this happen but it's theoretically
>> > possible. Either way, the code is fragile as it is.
>> >
>> > This patch saves r1 to the kernel stack (which can't fault) before we
>> > turn MSR[RI] back on. PACATMSCRATCH is still used but only with
>> > MSR[RI] off. We then copy r1 from the kernel stack to the thread
>> > struct once we have MSR[RI] back on.
>> >
>> > Suggested-by: Breno Leitao <leitao@debian.org>
>> > Signed-off-by: Michael Neuling <mikey@neuling.org>
>> > ---
>> > arch/powerpc/kernel/tm.S | 8 +++++++-
>> > 1 file changed, 7 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S
>> > index 701b0f5b09..8207816a1e 100644
>> > --- a/arch/powerpc/kernel/tm.S
>> > +++ b/arch/powerpc/kernel/tm.S
>> > @@ -178,6 +178,12 @@ _GLOBAL(tm_reclaim)
>> >
>> > std r11, GPR11(r1) /* Temporary stash */
>> >
>> > + /* Move r1 to kernel stack in case PACATMSCRATCH is used once
>> > + * we turn on RI
>> > + */
>>
>> I see we still need to send you to Comment Formatting Re-Education Camp.
>
> The rest of that file has they style, so I'm just keeping with that. I can
> submit a patch later to fix them all up.
Sure, but let's not add new ones.
And yeah fixing them all up in another patch would be good.
cheers
^ permalink raw reply
* [PATCH -next] PCI: hotplug: Use kmemdup rather than duplicating its implementation in pnv_php_add_devtree()
From: YueHaibing @ 2018-09-27 6:52 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Bjorn Helgaas
Cc: YueHaibing, linuxppc-dev, linux-pci, kernel-janitors
Use kmemdup rather than duplicating its implementation
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/pci/hotplug/pnv_php.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
index 5070620..ee54f5b 100644
--- a/drivers/pci/hotplug/pnv_php.c
+++ b/drivers/pci/hotplug/pnv_php.c
@@ -275,14 +275,13 @@ static int pnv_php_add_devtree(struct pnv_php_slot *php_slot)
goto free_fdt1;
}
- fdt = kzalloc(fdt_totalsize(fdt1), GFP_KERNEL);
+ fdt = kmemdup(fdt1, fdt_totalsize(fdt1), GFP_KERNEL);
if (!fdt) {
ret = -ENOMEM;
goto free_fdt1;
}
/* Unflatten device tree blob */
- memcpy(fdt, fdt1, fdt_totalsize(fdt1));
dt = of_fdt_unflatten_tree(fdt, php_slot->dn, NULL);
if (!dt) {
ret = -EINVAL;
^ permalink raw reply related
* Re: RESCIND Re: [v02] REPOST powerpc/migration: Handle unitialized timer reset
From: Michael Ellerman @ 2018-09-27 6:57 UTC (permalink / raw)
To: Michael Bringmann, Michael Ellerman, linuxppc-dev
Cc: Nathan Fontenot, minkim, Thomas Falcon, Tyrel Datwyler
In-Reply-To: <8816c05c-efc5-0b99-2de4-9cdf5480dd1a@linux.vnet.ibm.com>
Michael Bringmann <mwb@linux.vnet.ibm.com> writes:
> Hello, Michael:
> I am an idiot this morning. Not enough coffee.
Emailing when insufficiently caffeinated, that is dangerous :)
> I confused this posting with another. Please forget the request.
No worries.
> We do want to omit=20
>
> [PATCH v08 0/5] powerpc/hotplug: Update affinity for migrated CPUs
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-July/176301.html
>
> though.
OK, I've marked it 'rejected'.
cheers
^ permalink raw reply
* [PATCH v5 1/2] powerpc/32: add stack protector support
From: Christophe Leroy @ 2018-09-27 7:05 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-kernel, linuxppc-dev
This functionality was tentatively added in the past
(commit 6533b7c16ee5 ("powerpc: Initial stack protector
(-fstack-protector) support")) but had to be reverted
(commit f2574030b0e3 ("powerpc: Revert the initial stack
protector support") because of GCC implementing it differently
whether it had been built with libc support or not.
Now, GCC offers the possibility to manually set the
stack-protector mode (global or tls) regardless of libc support.
This time, the patch selects HAVE_STACKPROTECTOR only if
-mstack-protector-guard=tls is supported by GCC.
On PPC32, as register r2 points to current task_struct at
all time, the stack_canary located inside task_struct can be
used directly by using the following GCC options:
-mstack-protector-guard=tls
-mstack-protector-guard-reg=r2
-mstack-protector-guard-offset=offsetof(struct task_struct, stack_canary))
The protector is disabled for prom_init and bootx_init as
it is too early to handle it properly.
$ echo CORRUPT_STACK > /sys/kernel/debug/provoke-crash/DIRECT
[ 134.943666] Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: lkdtm_CORRUPT_STACK+0x64/0x64
[ 134.943666]
[ 134.955414] CPU: 0 PID: 283 Comm: sh Not tainted 4.18.0-s3k-dev-12143-ga3272be41209 #835
[ 134.963380] Call Trace:
[ 134.965860] [c6615d60] [c001f76c] panic+0x118/0x260 (unreliable)
[ 134.971775] [c6615dc0] [c001f654] panic+0x0/0x260
[ 134.976435] [c6615dd0] [c032c368] lkdtm_CORRUPT_STACK_STRONG+0x0/0x64
[ 134.982769] [c6615e00] [ffffffff] 0xffffffff
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v5: Using get_random_canary() and masking canary with CANARY_MASK
v4: disable stack protector in bootx_init
v3: the offset is now defined by a rule in the Makefile. No need anymore
to take stack_canary out of the randomised area of task_struct
arch/powerpc/Kconfig | 1 +
arch/powerpc/Makefile | 10 +++++++++
arch/powerpc/include/asm/stackprotector.h | 34 +++++++++++++++++++++++++++++++
arch/powerpc/kernel/Makefile | 2 ++
arch/powerpc/kernel/asm-offsets.c | 3 +++
arch/powerpc/platforms/powermac/Makefile | 1 +
6 files changed, 51 insertions(+)
create mode 100644 arch/powerpc/include/asm/stackprotector.h
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index a80669209155..3bcb05929931 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -180,6 +180,7 @@ config PPC
select HAVE_ARCH_SECCOMP_FILTER
select HAVE_ARCH_TRACEHOOK
select HAVE_CBPF_JIT if !PPC64
+ select HAVE_STACKPROTECTOR if $(cc-option,-mstack-protector-guard=tls) && PPC32
select HAVE_CONTEXT_TRACKING if PPC64
select HAVE_DEBUG_KMEMLEAK
select HAVE_DEBUG_STACKOVERFLOW
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 07d9dce7eda6..45b8eb4d8fe7 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -112,6 +112,9 @@ KBUILD_LDFLAGS += -m elf$(BITS)$(LDEMULATION)
KBUILD_ARFLAGS += --target=elf$(BITS)-$(GNUTARGET)
endif
+cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard=tls
+cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard-reg=r2
+
LDFLAGS_vmlinux-y := -Bstatic
LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) := -pie
LDFLAGS_vmlinux := $(LDFLAGS_vmlinux-y)
@@ -404,6 +407,13 @@ archclean:
archprepare: checkbin
+ifdef CONFIG_STACKPROTECTOR
+prepare: stack_protector_prepare
+
+stack_protector_prepare: prepare0
+ $(eval KBUILD_CFLAGS += -mstack-protector-guard-offset=$(shell awk '{if ($$2 == "TASK_CANARY") print $$3;}' include/generated/asm-offsets.h))
+endif
+
# Use the file '.tmp_gas_check' for binutils tests, as gas won't output
# to stdout and these checks are run even on install targets.
TOUT := .tmp_gas_check
diff --git a/arch/powerpc/include/asm/stackprotector.h b/arch/powerpc/include/asm/stackprotector.h
new file mode 100644
index 000000000000..d05d969c98c2
--- /dev/null
+++ b/arch/powerpc/include/asm/stackprotector.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * GCC stack protector support.
+ *
+ */
+
+#ifndef _ASM_STACKPROTECTOR_H
+#define _ASM_STACKPROTECTOR_H
+
+#include <linux/random.h>
+#include <linux/version.h>
+#include <asm/reg.h>
+#include <asm/current.h>
+
+/*
+ * Initialize the stackprotector canary value.
+ *
+ * NOTE: this must only be called from functions that never return,
+ * and it must always be inlined.
+ */
+static __always_inline void boot_init_stack_canary(void)
+{
+ unsigned long canary;
+
+ /* Try to get a semi random initial value. */
+ canary = get_random_canary();
+ canary ^= mftb();
+ canary ^= LINUX_VERSION_CODE;
+ canary &= CANARY_MASK;
+
+ current->stack_canary = canary;
+}
+
+#endif /* _ASM_STACKPROTECTOR_H */
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 1e64cfe22a83..85ffa488dfb5 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -20,6 +20,8 @@ CFLAGS_prom_init.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
CFLAGS_btext.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
CFLAGS_prom.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
+CFLAGS_prom_init.o += $(call cc-option, -fno-stack-protector)
+
ifdef CONFIG_FUNCTION_TRACER
# Do not trace early boot code
CFLAGS_REMOVE_cputable.o = $(CC_FLAGS_FTRACE)
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index ba9d0fc98730..a992f7a53cf3 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -79,6 +79,9 @@ int main(void)
{
OFFSET(THREAD, task_struct, thread);
OFFSET(MM, task_struct, mm);
+#ifdef CONFIG_STACKPROTECTOR
+ OFFSET(TASK_CANARY, task_struct, stack_canary);
+#endif
OFFSET(MMCONTEXTID, mm_struct, context.id);
#ifdef CONFIG_PPC64
DEFINE(SIGSEGV, SIGSEGV);
diff --git a/arch/powerpc/platforms/powermac/Makefile b/arch/powerpc/platforms/powermac/Makefile
index 561a67d65e4d..923bfb340433 100644
--- a/arch/powerpc/platforms/powermac/Makefile
+++ b/arch/powerpc/platforms/powermac/Makefile
@@ -1,5 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
CFLAGS_bootx_init.o += -fPIC
+CFLAGS_bootx_init.o += $(call cc-option, -fno-stack-protector)
ifdef CONFIG_FUNCTION_TRACER
# Do not trace early boot code
--
2.13.3
^ permalink raw reply related
* [PATCH v5 2/2] powerpc/64: add stack protector support
From: Christophe Leroy @ 2018-09-27 7:05 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-kernel, linuxppc-dev
In-Reply-To: <8e81b0647fea15e533a73ad4e9063c059fdfc6df.1537987712.git.christophe.leroy@c-s.fr>
On PPC64, as register r13 points to the paca_struct at all time,
this patch adds a copy of the canary there, which is copied at
task_switch.
That new canary is then used by using the following GCC options:
-mstack-protector-guard=tls
-mstack-protector-guard-reg=r13
-mstack-protector-guard-offset=offsetof(struct paca_struct, canary))
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/Kconfig | 2 +-
arch/powerpc/Makefile | 8 ++++++++
arch/powerpc/include/asm/paca.h | 3 +++
arch/powerpc/include/asm/stackprotector.h | 4 ++++
arch/powerpc/kernel/asm-offsets.c | 3 +++
arch/powerpc/kernel/entry_64.S | 4 ++++
6 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 3bcb05929931..602eea723624 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -180,7 +180,7 @@ config PPC
select HAVE_ARCH_SECCOMP_FILTER
select HAVE_ARCH_TRACEHOOK
select HAVE_CBPF_JIT if !PPC64
- select HAVE_STACKPROTECTOR if $(cc-option,-mstack-protector-guard=tls) && PPC32
+ select HAVE_STACKPROTECTOR if $(cc-option,-mstack-protector-guard=tls)
select HAVE_CONTEXT_TRACKING if PPC64
select HAVE_DEBUG_KMEMLEAK
select HAVE_DEBUG_STACKOVERFLOW
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 45b8eb4d8fe7..81552c7b46eb 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -113,7 +113,11 @@ KBUILD_ARFLAGS += --target=elf$(BITS)-$(GNUTARGET)
endif
cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard=tls
+ifdef CONFIG_PPC64
+cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard-reg=r13
+else
cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard-reg=r2
+endif
LDFLAGS_vmlinux-y := -Bstatic
LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) := -pie
@@ -411,8 +415,12 @@ ifdef CONFIG_STACKPROTECTOR
prepare: stack_protector_prepare
stack_protector_prepare: prepare0
+ifdef CONFIG_PPC64
+ $(eval KBUILD_CFLAGS += -mstack-protector-guard-offset=$(shell awk '{if ($$2 == "PACA_CANARY") print $$3;}' include/generated/asm-offsets.h))
+else
$(eval KBUILD_CFLAGS += -mstack-protector-guard-offset=$(shell awk '{if ($$2 == "TASK_CANARY") print $$3;}' include/generated/asm-offsets.h))
endif
+endif
# Use the file '.tmp_gas_check' for binutils tests, as gas won't output
# to stdout and these checks are run even on install targets.
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index 6d6b3706232c..98d883e58945 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -246,6 +246,9 @@ struct paca_struct {
struct slb_entry *mce_faulty_slbs;
u16 slb_save_cache_ptr;
#endif /* CONFIG_PPC_BOOK3S_64 */
+#ifdef CONFIG_STACKPROTECTOR
+ unsigned long canary;
+#endif
} ____cacheline_aligned;
extern struct paca_struct **paca_ptrs;
diff --git a/arch/powerpc/include/asm/stackprotector.h b/arch/powerpc/include/asm/stackprotector.h
index d05d969c98c2..1c8460e23583 100644
--- a/arch/powerpc/include/asm/stackprotector.h
+++ b/arch/powerpc/include/asm/stackprotector.h
@@ -11,6 +11,7 @@
#include <linux/version.h>
#include <asm/reg.h>
#include <asm/current.h>
+#include <asm/paca.h>
/*
* Initialize the stackprotector canary value.
@@ -29,6 +30,9 @@ static __always_inline void boot_init_stack_canary(void)
canary &= CANARY_MASK;
current->stack_canary = canary;
+#ifdef CONFIG_PPC64
+ get_paca()->canary = canary;
+#endif
}
#endif /* _ASM_STACKPROTECTOR_H */
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index a992f7a53cf3..773dee55b3f6 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -81,6 +81,9 @@ int main(void)
OFFSET(MM, task_struct, mm);
#ifdef CONFIG_STACKPROTECTOR
OFFSET(TASK_CANARY, task_struct, stack_canary);
+#ifdef CONFIG_PPC64
+ OFFSET(PACA_CANARY, paca_struct, canary);
+#endif
#endif
OFFSET(MMCONTEXTID, mm_struct, context.id);
#ifdef CONFIG_PPC64
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 77a888bfcb53..573fa879d785 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -624,6 +624,10 @@ _GLOBAL(_switch)
addi r6,r4,-THREAD /* Convert THREAD to 'current' */
std r6,PACACURRENT(r13) /* Set new 'current' */
+#if defined(CONFIG_STACKPROTECTOR)
+ ld r6, TASK_CANARY(r6)
+ std r6, PACA_CANARY(r13)
+#endif
ld r8,KSP(r4) /* new stack pointer */
#ifdef CONFIG_PPC_BOOK3S_64
--
2.13.3
^ permalink raw reply related
* Re: [PATCH v4 1/2] powerpc/32: add stack protector support
From: Segher Boessenkool @ 2018-09-27 7:45 UTC (permalink / raw)
To: Christophe LEROY
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
linuxppc-dev, linux-kernel
In-Reply-To: <220fbef8-429c-9485-d10e-c1eaa989918d@c-s.fr>
On Thu, Sep 27, 2018 at 08:20:00AM +0200, Christophe LEROY wrote:
> Le 26/09/2018 à 21:16, Segher Boessenkool a écrit :
> >On Wed, Sep 26, 2018 at 11:40:38AM +0000, Christophe Leroy wrote:
> >>+static __always_inline void boot_init_stack_canary(void)
> >>+{
> >>+ unsigned long canary;
> >>+
> >>+ /* Try to get a semi random initial value. */
> >>+ get_random_bytes(&canary, sizeof(canary));
> >>+ canary ^= mftb();
> >>+ canary ^= LINUX_VERSION_CODE;
> >>+
> >>+ current->stack_canary = canary;
> >>+}
> >
> >I still think you should wait until there is entropy available. You
> >haven't answered my questions about that (or I didn't see them): what
> >does the kernel do in other similar cases?
> >
> >Looks great otherwise!
>
> What do you mean by 'other similar cases' ? All arches have similar
> boot_init_stack_canary().
Yes, those, and other things that want entropy early.
> x86 uses rdtsc() which is equivalent to our
> mftb(). Most arches xor it with LINUX_VERSION_CODE.
>
> The issue is that it is called very early in start_kernel(), however
> they try to set some entropy anyway:
>
> boot_cpu_init();
> page_address_init();
> pr_notice("%s", linux_banner);
> setup_arch(&command_line);
> /*
> * Set up the the initial canary and entropy after arch
> * and after adding latent and command line entropy.
> */
> add_latent_entropy();
> add_device_randomness(command_line, strlen(command_line));
> boot_init_stack_canary();
>
> Apparently, it is too early for calling wait_for_random_bytes(), see below.
Hrm. Too early to call wait_event_interruptible? From there it went
into schedule(), which blew up. Well you say we have only one context
at this point, so that is not too surprising then :-)
> However this is the canary for initial startup only. Only idle() still
> uses this canary once the system is running. A new canary is set for any
> new forked task.
Ah, that makes things a lot better! Do those new tasks get a canary
from something with sufficient entropy though?
> Maybe should the idle canary be updated later once there is more entropy
That is tricky to do, but sure, if you can, that should help.
> ? Today there is a new call to boot_init_stack_canary() in
> cpu_startup_entry(), but it is enclosed inside #ifdef CONFIG_X86.
It needs to know the details of how ssp works on each platform.
Segher
^ permalink raw reply
* [PATCH 0/2] A few fixes to DTL buffer access over debugfs
From: Naveen N. Rao @ 2018-09-27 8:10 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
Two trivial fixes to DTL buffer access over debugfs when
CONFIG_VIRT_CPU_ACCOUNTING_NATIVE is not set.
- Naveen
Naveen N. Rao (2):
powerpc/pseries: Fix DTL buffer registration
powerpc/pseries: Fix how we iterate over the DTL entries
arch/powerpc/platforms/pseries/dtl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--
2.19.0
^ permalink raw reply
* [PATCH 1/2] powerpc/pseries: Fix DTL buffer registration
From: Naveen N. Rao @ 2018-09-27 8:10 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
In-Reply-To: <cover.1538035689.git.naveen.n.rao@linux.vnet.ibm.com>
When CONFIG_VIRT_CPU_ACCOUNTING_NATIVE is not set, we register the DTL
buffer for a cpu when the associated file under powerpc/dtl in debugfs
is opened. When doing so, we need to set the size of the buffer being
registered in the second u32 word of the buffer. This needs to be in big
endian, but we are not doing the conversion resulting in the below error
showing up in dmesg:
dtl_start: DTL registration for cpu 0 (hw 0) failed with -4
Fix this in the obvious manner.
Fixes: 7c105b63bd98 ("powerpc: Add CONFIG_CPU_LITTLE_ENDIAN kernel config option.")
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/platforms/pseries/dtl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/pseries/dtl.c b/arch/powerpc/platforms/pseries/dtl.c
index 18014cdeb590..c762689e0eb3 100644
--- a/arch/powerpc/platforms/pseries/dtl.c
+++ b/arch/powerpc/platforms/pseries/dtl.c
@@ -149,7 +149,7 @@ static int dtl_start(struct dtl *dtl)
/* Register our dtl buffer with the hypervisor. The HV expects the
* buffer size to be passed in the second word of the buffer */
- ((u32 *)dtl->buf)[1] = DISPATCH_LOG_BYTES;
+ ((u32 *)dtl->buf)[1] = cpu_to_be32(DISPATCH_LOG_BYTES);
hwcpu = get_hard_smp_processor_id(dtl->cpu);
addr = __pa(dtl->buf);
--
2.19.0
^ permalink raw reply related
* [PATCH 2/2] powerpc/pseries: Fix how we iterate over the DTL entries
From: Naveen N. Rao @ 2018-09-27 8:10 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
In-Reply-To: <cover.1538035689.git.naveen.n.rao@linux.vnet.ibm.com>
When CONFIG_VIRT_CPU_ACCOUNTING_NATIVE is not set, we look up dtl_idx in
the lppaca to determine the number of entries in the buffer. Since
lppaca is in big endian, we need to do an endian conversion before using
this in our calculation to determine the number of entries in the
buffer. Without this, we do not iterate over the existing entries in the
DTL buffer properly.
Fixes: 7c105b63bd98 ("powerpc: Add CONFIG_CPU_LITTLE_ENDIAN kernel config option.")
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/platforms/pseries/dtl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/pseries/dtl.c b/arch/powerpc/platforms/pseries/dtl.c
index c762689e0eb3..ef6595153642 100644
--- a/arch/powerpc/platforms/pseries/dtl.c
+++ b/arch/powerpc/platforms/pseries/dtl.c
@@ -184,7 +184,7 @@ static void dtl_stop(struct dtl *dtl)
static u64 dtl_current_index(struct dtl *dtl)
{
- return lppaca_of(dtl->cpu).dtl_idx;
+ return be64_to_cpu(lppaca_of(dtl->cpu).dtl_idx);
}
#endif /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE */
--
2.19.0
^ permalink raw reply related
* [PATCH v3 0/6] mm: online/offline_pages called w.o. mem_hotplug_lock
From: David Hildenbrand @ 2018-09-27 9:25 UTC (permalink / raw)
To: linux-mm
Cc: linux-kernel, linux-doc, linuxppc-dev, linux-acpi, xen-devel,
devel, David Hildenbrand, Andrew Morton, Balbir Singh,
Benjamin Herrenschmidt, Boris Ostrovsky, Dan Williams,
Greg Kroah-Hartman, Haiyang Zhang, Heiko Carstens, John Allen,
Jonathan Corbet, Joonsoo Kim, Juergen Gross, Kate Stewart,
K. Y. Srinivasan, Len Brown, Martin Schwidefsky,
Mathieu Malaterre, Michael Ellerman, Michael Neuling,
Michal Hocko, Nathan Fontenot, Oscar Salvador, Paul Mackerras,
Pavel Tatashin, Pavel Tatashin, Philippe Ombredanne,
Rafael J. Wysocki, Rafael J. Wysocki, Rashmica Gupta,
Stephen Hemminger, Thomas Gleixner, Vlastimil Babka,
YASUAKI ISHIMATSU
@Andrew, Only patch #5 changed (see change notes below). Thanks!
Reading through the code and studying how mem_hotplug_lock is to be used,
I noticed that there are two places where we can end up calling
device_online()/device_offline() - online_pages()/offline_pages() without
the mem_hotplug_lock. And there are other places where we call
device_online()/device_offline() without the device_hotplug_lock.
While e.g.
echo "online" > /sys/devices/system/memory/memory9/state
is fine, e.g.
echo 1 > /sys/devices/system/memory/memory9/online
Will not take the mem_hotplug_lock. However the device_lock() and
device_hotplug_lock.
E.g. via memory_probe_store(), we can end up calling
add_memory()->online_pages() without the device_hotplug_lock. So we can
have concurrent callers in online_pages(). We e.g. touch in online_pages()
basically unprotected zone->present_pages then.
Looks like there is a longer history to that (see Patch #2 for details),
and fixing it to work the way it was intended is not really possible. We
would e.g. have to take the mem_hotplug_lock in device/base/core.c, which
sounds wrong.
Summary: We had a lock inversion on mem_hotplug_lock and device_lock().
More details can be found in patch 3 and patch 6.
I propose the general rules (documentation added in patch 6):
1. add_memory/add_memory_resource() must only be called with
device_hotplug_lock.
2. remove_memory() must only be called with device_hotplug_lock. This is
already documented and holds for all callers.
3. device_online()/device_offline() must only be called with
device_hotplug_lock. This is already documented and true for now in core
code. Other callers (related to memory hotplug) have to be fixed up.
4. mem_hotplug_lock is taken inside of add_memory/remove_memory/
online_pages/offline_pages.
To me, this looks way cleaner than what we have right now (and easier to
verify). And looking at the documentation of remove_memory, using
lock_device_hotplug also for add_memory() feels natural.
v2 -> v3:
- Take device_hotplug_lock outside of loop in patch #5
- Added Ack to patch #5
v1 -> v2:
- Upstream changes in powerpc/powernv code required modifications to
patch #1, #4 and #5.
- Minor patch description changes.
- Added more locking details in patch #6.
- Added rb's
RFCv2 -> v1:
- Dropped an unnecessary _ref from remove_memory() in patch #1
- Minor patch description fixes.
- Added rb's
RFC -> RFCv2:
- Don't export device_hotplug_lock, provide proper remove_memory/add_memory
wrappers.
- Split up the patches a bit.
- Try to improve powernv memtrace locking
- Add some documentation for locking that matches my knowledg
David Hildenbrand (6):
mm/memory_hotplug: make remove_memory() take the device_hotplug_lock
mm/memory_hotplug: make add_memory() take the device_hotplug_lock
mm/memory_hotplug: fix online/offline_pages called w.o.
mem_hotplug_lock
powerpc/powernv: hold device_hotplug_lock when calling device_online()
powerpc/powernv: hold device_hotplug_lock when calling
memtrace_offline_pages()
memory-hotplug.txt: Add some details about locking internals
Documentation/memory-hotplug.txt | 42 ++++++++++++-
arch/powerpc/platforms/powernv/memtrace.c | 8 ++-
.../platforms/pseries/hotplug-memory.c | 8 +--
drivers/acpi/acpi_memhotplug.c | 4 +-
drivers/base/memory.c | 22 +++----
drivers/xen/balloon.c | 3 +
include/linux/memory_hotplug.h | 4 +-
mm/memory_hotplug.c | 59 +++++++++++++++----
8 files changed, 114 insertions(+), 36 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH v3 1/6] mm/memory_hotplug: make remove_memory() take the device_hotplug_lock
From: David Hildenbrand @ 2018-09-27 9:25 UTC (permalink / raw)
To: linux-mm
Cc: linux-kernel, linux-doc, linuxppc-dev, linux-acpi, xen-devel,
devel, David Hildenbrand, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Rafael J. Wysocki, Len Brown, Rashmica Gupta,
Michael Neuling, Balbir Singh, Nathan Fontenot, John Allen,
Andrew Morton, Michal Hocko, Dan Williams, Joonsoo Kim,
Vlastimil Babka, Pavel Tatashin, Greg Kroah-Hartman,
Oscar Salvador, YASUAKI ISHIMATSU, Mathieu Malaterre
In-Reply-To: <20180927092554.13567-1-david@redhat.com>
remove_memory() is exported right now but requires the
device_hotplug_lock, which is not exported. So let's provide a variant
that takes the lock and only export that one.
The lock is already held in
arch/powerpc/platforms/pseries/hotplug-memory.c
drivers/acpi/acpi_memhotplug.c
arch/powerpc/platforms/powernv/memtrace.c
Apart from that, there are not other users in the tree.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: Rashmica Gupta <rashmica.g@gmail.com>
Cc: Michael Neuling <mikey@neuling.org>
Cc: Balbir Singh <bsingharora@gmail.com>
Cc: Nathan Fontenot <nfont@linux.vnet.ibm.com>
Cc: John Allen <jallen@linux.vnet.ibm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Pavel Tatashin <pasha.tatashin@oracle.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: YASUAKI ISHIMATSU <yasu.isimatu@gmail.com>
Cc: Mathieu Malaterre <malat@debian.org>
Reviewed-by: Pavel Tatashin <pavel.tatashin@microsoft.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Rashmica Gupta <rashmica.g@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/powerpc/platforms/powernv/memtrace.c | 2 +-
arch/powerpc/platforms/pseries/hotplug-memory.c | 6 +++---
drivers/acpi/acpi_memhotplug.c | 2 +-
include/linux/memory_hotplug.h | 3 ++-
mm/memory_hotplug.c | 9 ++++++++-
5 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/memtrace.c b/arch/powerpc/platforms/powernv/memtrace.c
index a29fdf8a2e56..773623f6bfb1 100644
--- a/arch/powerpc/platforms/powernv/memtrace.c
+++ b/arch/powerpc/platforms/powernv/memtrace.c
@@ -121,7 +121,7 @@ static u64 memtrace_alloc_node(u32 nid, u64 size)
lock_device_hotplug();
end_pfn = base_pfn + nr_pages;
for (pfn = base_pfn; pfn < end_pfn; pfn += bytes>> PAGE_SHIFT) {
- remove_memory(nid, pfn << PAGE_SHIFT, bytes);
+ __remove_memory(nid, pfn << PAGE_SHIFT, bytes);
}
unlock_device_hotplug();
return base_pfn << PAGE_SHIFT;
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 9a15d39995e5..dd0264c43f3e 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -305,7 +305,7 @@ static int pseries_remove_memblock(unsigned long base, unsigned int memblock_siz
nid = memory_add_physaddr_to_nid(base);
for (i = 0; i < sections_per_block; i++) {
- remove_memory(nid, base, MIN_MEMORY_BLOCK_SIZE);
+ __remove_memory(nid, base, MIN_MEMORY_BLOCK_SIZE);
base += MIN_MEMORY_BLOCK_SIZE;
}
@@ -394,7 +394,7 @@ static int dlpar_remove_lmb(struct drmem_lmb *lmb)
block_sz = pseries_memory_block_size();
nid = memory_add_physaddr_to_nid(lmb->base_addr);
- remove_memory(nid, lmb->base_addr, block_sz);
+ __remove_memory(nid, lmb->base_addr, block_sz);
/* Update memory regions for memory remove */
memblock_remove(lmb->base_addr, block_sz);
@@ -681,7 +681,7 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
rc = dlpar_online_lmb(lmb);
if (rc) {
- remove_memory(nid, lmb->base_addr, block_sz);
+ __remove_memory(nid, lmb->base_addr, block_sz);
invalidate_lmb_associativity_index(lmb);
} else {
lmb->flags |= DRCONF_MEM_ASSIGNED;
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index 6b0d3ef7309c..811148415993 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -282,7 +282,7 @@ static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
nid = memory_add_physaddr_to_nid(info->start_addr);
acpi_unbind_memory_blocks(info);
- remove_memory(nid, info->start_addr, info->length);
+ __remove_memory(nid, info->start_addr, info->length);
list_del(&info->list);
kfree(info);
}
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 34a28227068d..1f096852f479 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -301,6 +301,7 @@ extern bool is_mem_section_removable(unsigned long pfn, unsigned long nr_pages);
extern void try_offline_node(int nid);
extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
extern void remove_memory(int nid, u64 start, u64 size);
+extern void __remove_memory(int nid, u64 start, u64 size);
#else
static inline bool is_mem_section_removable(unsigned long pfn,
@@ -317,6 +318,7 @@ static inline int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
}
static inline void remove_memory(int nid, u64 start, u64 size) {}
+static inline void __remove_memory(int nid, u64 start, u64 size) {}
#endif /* CONFIG_MEMORY_HOTREMOVE */
extern void __ref free_area_init_core_hotplug(int nid);
@@ -330,7 +332,6 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
unsigned long nr_pages, struct vmem_altmap *altmap);
extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
extern bool is_memblock_offlined(struct memory_block *mem);
-extern void remove_memory(int nid, u64 start, u64 size);
extern int sparse_add_one_section(struct pglist_data *pgdat,
unsigned long start_pfn, struct vmem_altmap *altmap);
extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 2698664bfd54..f6dbd5d8fffd 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1839,7 +1839,7 @@ EXPORT_SYMBOL(try_offline_node);
* and online/offline operations before this call, as required by
* try_offline_node().
*/
-void __ref remove_memory(int nid, u64 start, u64 size)
+void __ref __remove_memory(int nid, u64 start, u64 size)
{
int ret;
@@ -1868,5 +1868,12 @@ void __ref remove_memory(int nid, u64 start, u64 size)
mem_hotplug_done();
}
+
+void remove_memory(int nid, u64 start, u64 size)
+{
+ lock_device_hotplug();
+ __remove_memory(nid, start, size);
+ unlock_device_hotplug();
+}
EXPORT_SYMBOL_GPL(remove_memory);
#endif /* CONFIG_MEMORY_HOTREMOVE */
--
2.17.1
^ permalink raw reply related
* [PATCH v3 2/6] mm/memory_hotplug: make add_memory() take the device_hotplug_lock
From: David Hildenbrand @ 2018-09-27 9:25 UTC (permalink / raw)
To: linux-mm
Cc: linux-kernel, linux-doc, linuxppc-dev, linux-acpi, xen-devel,
devel, David Hildenbrand, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Rafael J. Wysocki, Len Brown,
Greg Kroah-Hartman, Boris Ostrovsky, Juergen Gross,
Nathan Fontenot, John Allen, Andrew Morton, Michal Hocko,
Dan Williams, Joonsoo Kim, Vlastimil Babka, Oscar Salvador,
Mathieu Malaterre, Pavel Tatashin, YASUAKI ISHIMATSU
In-Reply-To: <20180927092554.13567-1-david@redhat.com>
add_memory() currently does not take the device_hotplug_lock, however
is aleady called under the lock from
arch/powerpc/platforms/pseries/hotplug-memory.c
drivers/acpi/acpi_memhotplug.c
to synchronize against CPU hot-remove and similar.
In general, we should hold the device_hotplug_lock when adding memory
to synchronize against online/offline request (e.g. from user space) -
which already resulted in lock inversions due to device_lock() and
mem_hotplug_lock - see 30467e0b3be ("mm, hotplug: fix concurrent memory
hot-add deadlock"). add_memory()/add_memory_resource() will create memory
block devices, so this really feels like the right thing to do.
Holding the device_hotplug_lock makes sure that a memory block device
can really only be accessed (e.g. via .online/.state) from user space,
once the memory has been fully added to the system.
The lock is not held yet in
drivers/xen/balloon.c
arch/powerpc/platforms/powernv/memtrace.c
drivers/s390/char/sclp_cmd.c
drivers/hv/hv_balloon.c
So, let's either use the locked variants or take the lock.
Don't export add_memory_resource(), as it once was exported to be used
by XEN, which is never built as a module. If somebody requires it, we
also have to export a locked variant (as device_hotplug_lock is never
exported).
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Nathan Fontenot <nfont@linux.vnet.ibm.com>
Cc: John Allen <jallen@linux.vnet.ibm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: Mathieu Malaterre <malat@debian.org>
Cc: Pavel Tatashin <pavel.tatashin@microsoft.com>
Cc: YASUAKI ISHIMATSU <yasu.isimatu@gmail.com>
Reviewed-by: Pavel Tatashin <pavel.tatashin@microsoft.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Rashmica Gupta <rashmica.g@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
.../platforms/pseries/hotplug-memory.c | 2 +-
drivers/acpi/acpi_memhotplug.c | 2 +-
drivers/base/memory.c | 9 ++++++--
drivers/xen/balloon.c | 3 +++
include/linux/memory_hotplug.h | 1 +
mm/memory_hotplug.c | 22 ++++++++++++++++---
6 files changed, 32 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index dd0264c43f3e..d26a771d985e 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -673,7 +673,7 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
nid = memory_add_physaddr_to_nid(lmb->base_addr);
/* Add the memory */
- rc = add_memory(nid, lmb->base_addr, block_sz);
+ rc = __add_memory(nid, lmb->base_addr, block_sz);
if (rc) {
invalidate_lmb_associativity_index(lmb);
return rc;
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index 811148415993..8fe0960ea572 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -228,7 +228,7 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
if (node < 0)
node = memory_add_physaddr_to_nid(info->start_addr);
- result = add_memory(node, info->start_addr, info->length);
+ result = __add_memory(node, info->start_addr, info->length);
/*
* If the memory block has been used by the kernel, add_memory()
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 817320c7c4c1..40cac122ec73 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -519,15 +519,20 @@ memory_probe_store(struct device *dev, struct device_attribute *attr,
if (phys_addr & ((pages_per_block << PAGE_SHIFT) - 1))
return -EINVAL;
+ ret = lock_device_hotplug_sysfs();
+ if (ret)
+ goto out;
+
nid = memory_add_physaddr_to_nid(phys_addr);
- ret = add_memory(nid, phys_addr,
- MIN_MEMORY_BLOCK_SIZE * sections_per_block);
+ ret = __add_memory(nid, phys_addr,
+ MIN_MEMORY_BLOCK_SIZE * sections_per_block);
if (ret)
goto out;
ret = count;
out:
+ unlock_device_hotplug();
return ret;
}
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index a3f5cbfcd4a1..fdfc64f5acea 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -395,7 +395,10 @@ static enum bp_state reserve_additional_memory(void)
* callers drop the mutex before trying again.
*/
mutex_unlock(&balloon_mutex);
+ /* add_memory_resource() requires the device_hotplug lock */
+ lock_device_hotplug();
rc = add_memory_resource(nid, resource, memhp_auto_online);
+ unlock_device_hotplug();
mutex_lock(&balloon_mutex);
if (rc) {
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 1f096852f479..ffd9cd10fcf3 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -324,6 +324,7 @@ static inline void __remove_memory(int nid, u64 start, u64 size) {}
extern void __ref free_area_init_core_hotplug(int nid);
extern int walk_memory_range(unsigned long start_pfn, unsigned long end_pfn,
void *arg, int (*func)(struct memory_block *, void *));
+extern int __add_memory(int nid, u64 start, u64 size);
extern int add_memory(int nid, u64 start, u64 size);
extern int add_memory_resource(int nid, struct resource *resource, bool online);
extern int arch_add_memory(int nid, u64 start, u64 size,
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index f6dbd5d8fffd..affb03e0dfef 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1090,7 +1090,12 @@ static int online_memory_block(struct memory_block *mem, void *arg)
return device_online(&mem->dev);
}
-/* we are OK calling __meminit stuff here - we have CONFIG_MEMORY_HOTPLUG */
+/*
+ * NOTE: The caller must call lock_device_hotplug() to serialize hotplug
+ * and online/offline operations (triggered e.g. by sysfs).
+ *
+ * we are OK calling __meminit stuff here - we have CONFIG_MEMORY_HOTPLUG
+ */
int __ref add_memory_resource(int nid, struct resource *res, bool online)
{
u64 start, size;
@@ -1159,9 +1164,9 @@ int __ref add_memory_resource(int nid, struct resource *res, bool online)
mem_hotplug_done();
return ret;
}
-EXPORT_SYMBOL_GPL(add_memory_resource);
-int __ref add_memory(int nid, u64 start, u64 size)
+/* requires device_hotplug_lock, see add_memory_resource() */
+int __ref __add_memory(int nid, u64 start, u64 size)
{
struct resource *res;
int ret;
@@ -1175,6 +1180,17 @@ int __ref add_memory(int nid, u64 start, u64 size)
release_memory_resource(res);
return ret;
}
+
+int add_memory(int nid, u64 start, u64 size)
+{
+ int rc;
+
+ lock_device_hotplug();
+ rc = __add_memory(nid, start, size);
+ unlock_device_hotplug();
+
+ return rc;
+}
EXPORT_SYMBOL_GPL(add_memory);
#ifdef CONFIG_MEMORY_HOTREMOVE
--
2.17.1
^ permalink raw reply related
* [PATCH v3 3/6] mm/memory_hotplug: fix online/offline_pages called w.o. mem_hotplug_lock
From: David Hildenbrand @ 2018-09-27 9:25 UTC (permalink / raw)
To: linux-mm
Cc: linux-kernel, linux-doc, linuxppc-dev, linux-acpi, xen-devel,
devel, David Hildenbrand, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Rafael J. Wysocki, Len Brown,
Greg Kroah-Hartman, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, Martin Schwidefsky, Heiko Carstens,
Boris Ostrovsky, Juergen Gross, Rashmica Gupta, Michael Neuling,
Balbir Singh, Kate Stewart, Thomas Gleixner, Philippe Ombredanne,
Andrew Morton, Michal Hocko, Pavel Tatashin, Vlastimil Babka,
Dan Williams, Oscar Salvador, YASUAKI ISHIMATSU,
Mathieu Malaterre
In-Reply-To: <20180927092554.13567-1-david@redhat.com>
There seem to be some problems as result of 30467e0b3be ("mm, hotplug:
fix concurrent memory hot-add deadlock"), which tried to fix a possible
lock inversion reported and discussed in [1] due to the two locks
a) device_lock()
b) mem_hotplug_lock
While add_memory() first takes b), followed by a) during
bus_probe_device(), onlining of memory from user space first took a),
followed by b), exposing a possible deadlock.
In [1], and it was decided to not make use of device_hotplug_lock, but
rather to enforce a locking order.
The problems I spotted related to this:
1. Memory block device attributes: While .state first calls
mem_hotplug_begin() and the calls device_online() - which takes
device_lock() - .online does no longer call mem_hotplug_begin(), so
effectively calls online_pages() without mem_hotplug_lock.
2. device_online() should be called under device_hotplug_lock, however
onlining memory during add_memory() does not take care of that.
In addition, I think there is also something wrong about the locking in
3. arch/powerpc/platforms/powernv/memtrace.c calls offline_pages()
without locks. This was introduced after 30467e0b3be. And skimming over
the code, I assume it could need some more care in regards to locking
(e.g. device_online() called without device_hotplug_lock. This will
be addressed in the following patches.
Now that we hold the device_hotplug_lock when
- adding memory (e.g. via add_memory()/add_memory_resource())
- removing memory (e.g. via remove_memory())
- device_online()/device_offline()
We can move mem_hotplug_lock usage back into
online_pages()/offline_pages().
Why is mem_hotplug_lock still needed? Essentially to make
get_online_mems()/put_online_mems() be very fast (relying on
device_hotplug_lock would be very slow), and to serialize against
addition of memory that does not create memory block devices (hmm).
[1] http://driverdev.linuxdriverproject.org/pipermail/ driverdev-devel/
2015-February/065324.html
This patch is partly based on a patch by Vitaly Kuznetsov.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Rashmica Gupta <rashmica.g@gmail.com>
Cc: Michael Neuling <mikey@neuling.org>
Cc: Balbir Singh <bsingharora@gmail.com>
Cc: Kate Stewart <kstewart@linuxfoundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Philippe Ombredanne <pombredanne@nexb.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Pavel Tatashin <pavel.tatashin@microsoft.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: YASUAKI ISHIMATSU <yasu.isimatu@gmail.com>
Cc: Mathieu Malaterre <malat@debian.org>
Reviewed-by: Pavel Tatashin <pavel.tatashin@microsoft.com>
Reviewed-by: Rashmica Gupta <rashmica.g@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
drivers/base/memory.c | 13 +------------
mm/memory_hotplug.c | 28 ++++++++++++++++++++--------
2 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 40cac122ec73..0e5985682642 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -228,7 +228,6 @@ static bool pages_correctly_probed(unsigned long start_pfn)
/*
* MEMORY_HOTPLUG depends on SPARSEMEM in mm/Kconfig, so it is
* OK to have direct references to sparsemem variables in here.
- * Must already be protected by mem_hotplug_begin().
*/
static int
memory_block_action(unsigned long phys_index, unsigned long action, int online_type)
@@ -294,7 +293,6 @@ static int memory_subsys_online(struct device *dev)
if (mem->online_type < 0)
mem->online_type = MMOP_ONLINE_KEEP;
- /* Already under protection of mem_hotplug_begin() */
ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
/* clear online_type */
@@ -341,19 +339,11 @@ store_mem_state(struct device *dev,
goto err;
}
- /*
- * Memory hotplug needs to hold mem_hotplug_begin() for probe to find
- * the correct memory block to online before doing device_online(dev),
- * which will take dev->mutex. Take the lock early to prevent an
- * inversion, memory_subsys_online() callbacks will be implemented by
- * assuming it's already protected.
- */
- mem_hotplug_begin();
-
switch (online_type) {
case MMOP_ONLINE_KERNEL:
case MMOP_ONLINE_MOVABLE:
case MMOP_ONLINE_KEEP:
+ /* mem->online_type is protected by device_hotplug_lock */
mem->online_type = online_type;
ret = device_online(&mem->dev);
break;
@@ -364,7 +354,6 @@ store_mem_state(struct device *dev,
ret = -EINVAL; /* should never happen */
}
- mem_hotplug_done();
err:
unlock_device_hotplug();
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index affb03e0dfef..d4c7e42e46f3 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -860,7 +860,6 @@ static struct zone * __meminit move_pfn_range(int online_type, int nid,
return zone;
}
-/* Must be protected by mem_hotplug_begin() or a device_lock */
int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_type)
{
unsigned long flags;
@@ -872,6 +871,8 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
struct memory_notify arg;
struct memory_block *mem;
+ mem_hotplug_begin();
+
/*
* We can't use pfn_to_nid() because nid might be stored in struct page
* which is not yet initialized. Instead, we find nid from memory block.
@@ -936,6 +937,7 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
if (onlined_pages)
memory_notify(MEM_ONLINE, &arg);
+ mem_hotplug_done();
return 0;
failed_addition:
@@ -943,6 +945,7 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
(unsigned long long) pfn << PAGE_SHIFT,
(((unsigned long long) pfn + nr_pages) << PAGE_SHIFT) - 1);
memory_notify(MEM_CANCEL_ONLINE, &arg);
+ mem_hotplug_done();
return ret;
}
#endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
@@ -1147,20 +1150,20 @@ int __ref add_memory_resource(int nid, struct resource *res, bool online)
/* create new memmap entry */
firmware_map_add_hotplug(start, start + size, "System RAM");
+ /* device_online() will take the lock when calling online_pages() */
+ mem_hotplug_done();
+
/* online pages if requested */
if (online)
walk_memory_range(PFN_DOWN(start), PFN_UP(start + size - 1),
NULL, online_memory_block);
- goto out;
-
+ return ret;
error:
/* rollback pgdat allocation and others */
if (new_node)
rollback_node_hotadd(nid);
memblock_remove(start, size);
-
-out:
mem_hotplug_done();
return ret;
}
@@ -1588,10 +1591,16 @@ static int __ref __offline_pages(unsigned long start_pfn,
return -EINVAL;
if (!IS_ALIGNED(end_pfn, pageblock_nr_pages))
return -EINVAL;
+
+ mem_hotplug_begin();
+
/* This makes hotplug much easier...and readable.
we assume this for now. .*/
- if (!test_pages_in_a_zone(start_pfn, end_pfn, &valid_start, &valid_end))
+ if (!test_pages_in_a_zone(start_pfn, end_pfn, &valid_start,
+ &valid_end)) {
+ mem_hotplug_done();
return -EINVAL;
+ }
zone = page_zone(pfn_to_page(valid_start));
node = zone_to_nid(zone);
@@ -1600,8 +1609,10 @@ static int __ref __offline_pages(unsigned long start_pfn,
/* set above range as isolated */
ret = start_isolate_page_range(start_pfn, end_pfn,
MIGRATE_MOVABLE, true);
- if (ret)
+ if (ret) {
+ mem_hotplug_done();
return ret;
+ }
arg.start_pfn = start_pfn;
arg.nr_pages = nr_pages;
@@ -1672,6 +1683,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
writeback_set_ratelimit();
memory_notify(MEM_OFFLINE, &arg);
+ mem_hotplug_done();
return 0;
failed_removal:
@@ -1681,10 +1693,10 @@ static int __ref __offline_pages(unsigned long start_pfn,
memory_notify(MEM_CANCEL_OFFLINE, &arg);
/* pushback to free area */
undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
+ mem_hotplug_done();
return ret;
}
-/* Must be protected by mem_hotplug_begin() or a device_lock */
int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
{
return __offline_pages(start_pfn, start_pfn + nr_pages);
--
2.17.1
^ permalink raw reply related
* [PATCH v3 4/6] powerpc/powernv: hold device_hotplug_lock when calling device_online()
From: David Hildenbrand @ 2018-09-27 9:25 UTC (permalink / raw)
To: linux-mm
Cc: linux-kernel, linux-doc, linuxppc-dev, linux-acpi, xen-devel,
devel, David Hildenbrand, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Rashmica Gupta, Balbir Singh, Michael Neuling
In-Reply-To: <20180927092554.13567-1-david@redhat.com>
device_online() should be called with device_hotplug_lock() held.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Rashmica Gupta <rashmica.g@gmail.com>
Cc: Balbir Singh <bsingharora@gmail.com>
Cc: Michael Neuling <mikey@neuling.org>
Reviewed-by: Pavel Tatashin <pavel.tatashin@microsoft.com>
Reviewed-by: Rashmica Gupta <rashmica.g@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/powerpc/platforms/powernv/memtrace.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/platforms/powernv/memtrace.c b/arch/powerpc/platforms/powernv/memtrace.c
index 773623f6bfb1..fdd48f1a39f7 100644
--- a/arch/powerpc/platforms/powernv/memtrace.c
+++ b/arch/powerpc/platforms/powernv/memtrace.c
@@ -242,9 +242,11 @@ static int memtrace_online(void)
* we need to online the memory ourselves.
*/
if (!memhp_auto_online) {
+ lock_device_hotplug();
walk_memory_range(PFN_DOWN(ent->start),
PFN_UP(ent->start + ent->size - 1),
NULL, online_mem_block);
+ unlock_device_hotplug();
}
/*
--
2.17.1
^ permalink raw reply related
* [PATCH v3 5/6] powerpc/powernv: hold device_hotplug_lock when calling memtrace_offline_pages()
From: David Hildenbrand @ 2018-09-27 9:25 UTC (permalink / raw)
To: linux-mm
Cc: linux-kernel, linux-doc, linuxppc-dev, linux-acpi, xen-devel,
devel, David Hildenbrand, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Rashmica Gupta, Balbir Singh, Michael Neuling
In-Reply-To: <20180927092554.13567-1-david@redhat.com>
Let's perform all checking + offlining + removing under
device_hotplug_lock, so nobody can mess with these devices via
sysfs concurrently.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Rashmica Gupta <rashmica.g@gmail.com>
Cc: Balbir Singh <bsingharora@gmail.com>
Cc: Michael Neuling <mikey@neuling.org>
Reviewed-by: Pavel Tatashin <pavel.tatashin@microsoft.com>
Reviewed-by: Rashmica Gupta <rashmica.g@gmail.com>
Acked-by: Balbir Singh <bsingharora@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/powerpc/platforms/powernv/memtrace.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/powernv/memtrace.c b/arch/powerpc/platforms/powernv/memtrace.c
index fdd48f1a39f7..84d038ed3882 100644
--- a/arch/powerpc/platforms/powernv/memtrace.c
+++ b/arch/powerpc/platforms/powernv/memtrace.c
@@ -70,6 +70,7 @@ static int change_memblock_state(struct memory_block *mem, void *arg)
return 0;
}
+/* called with device_hotplug_lock held */
static bool memtrace_offline_pages(u32 nid, u64 start_pfn, u64 nr_pages)
{
u64 end_pfn = start_pfn + nr_pages - 1;
@@ -110,6 +111,7 @@ static u64 memtrace_alloc_node(u32 nid, u64 size)
/* Trace memory needs to be aligned to the size */
end_pfn = round_down(end_pfn - nr_pages, nr_pages);
+ lock_device_hotplug();
for (base_pfn = end_pfn; base_pfn > start_pfn; base_pfn -= nr_pages) {
if (memtrace_offline_pages(nid, base_pfn, nr_pages) == true) {
/*
@@ -118,7 +120,6 @@ static u64 memtrace_alloc_node(u32 nid, u64 size)
* we never try to remove memory that spans two iomem
* resources.
*/
- lock_device_hotplug();
end_pfn = base_pfn + nr_pages;
for (pfn = base_pfn; pfn < end_pfn; pfn += bytes>> PAGE_SHIFT) {
__remove_memory(nid, pfn << PAGE_SHIFT, bytes);
@@ -127,6 +128,7 @@ static u64 memtrace_alloc_node(u32 nid, u64 size)
return base_pfn << PAGE_SHIFT;
}
}
+ unlock_device_hotplug();
return 0;
}
--
2.17.1
^ permalink raw reply related
* [PATCH v3 6/6] memory-hotplug.txt: Add some details about locking internals
From: David Hildenbrand @ 2018-09-27 9:25 UTC (permalink / raw)
To: linux-mm
Cc: linux-kernel, linux-doc, linuxppc-dev, linux-acpi, xen-devel,
devel, David Hildenbrand, Jonathan Corbet, Michal Hocko,
Andrew Morton
In-Reply-To: <20180927092554.13567-1-david@redhat.com>
Let's document the magic a bit, especially why device_hotplug_lock is
required when adding/removing memory and how it all play together with
requests to online/offline memory from user space.
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Reviewed-by: Pavel Tatashin <pavel.tatashin@microsoft.com>
Reviewed-by: Rashmica Gupta <rashmica.g@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
Documentation/memory-hotplug.txt | 42 +++++++++++++++++++++++++++++++-
1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/Documentation/memory-hotplug.txt b/Documentation/memory-hotplug.txt
index 7f49ebf3ddb2..ce4faa5530fa 100644
--- a/Documentation/memory-hotplug.txt
+++ b/Documentation/memory-hotplug.txt
@@ -3,7 +3,7 @@ Memory Hotplug
==============
:Created: Jul 28 2007
-:Updated: Add description of notifier of memory hotplug: Oct 11 2007
+:Updated: Add some details about locking internals: Aug 20 2018
This document is about memory hotplug including how-to-use and current status.
Because Memory Hotplug is still under development, contents of this text will
@@ -495,6 +495,46 @@ further processing of the notification queue.
NOTIFY_STOP stops further processing of the notification queue.
+
+Locking Internals
+=================
+
+When adding/removing memory that uses memory block devices (i.e. ordinary RAM),
+the device_hotplug_lock should be held to:
+
+- synchronize against online/offline requests (e.g. via sysfs). This way, memory
+ block devices can only be accessed (.online/.state attributes) by user
+ space once memory has been fully added. And when removing memory, we
+ know nobody is in critical sections.
+- synchronize against CPU hotplug and similar (e.g. relevant for ACPI and PPC)
+
+Especially, there is a possible lock inversion that is avoided using
+device_hotplug_lock when adding memory and user space tries to online that
+memory faster than expected:
+
+- device_online() will first take the device_lock(), followed by
+ mem_hotplug_lock
+- add_memory_resource() will first take the mem_hotplug_lock, followed by
+ the device_lock() (while creating the devices, during bus_add_device()).
+
+As the device is visible to user space before taking the device_lock(), this
+can result in a lock inversion.
+
+onlining/offlining of memory should be done via device_online()/
+device_offline() - to make sure it is properly synchronized to actions
+via sysfs. Holding device_hotplug_lock is advised (to e.g. protect online_type)
+
+When adding/removing/onlining/offlining memory or adding/removing
+heterogeneous/device memory, we should always hold the mem_hotplug_lock in
+write mode to serialise memory hotplug (e.g. access to global/zone
+variables).
+
+In addition, mem_hotplug_lock (in contrast to device_hotplug_lock) in read
+mode allows for a quite efficient get_online_mems/put_online_mems
+implementation, so code accessing memory can protect from that memory
+vanishing.
+
+
Future Work
===========
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] kdb: use correct pointer when 'btc' calls 'btt'
From: Michael Ellerman @ 2018-09-27 11:09 UTC (permalink / raw)
To: Christophe LEROY, Daniel Thompson
Cc: Jason Wessel, linux-kernel, linuxppc-dev, kgdb-bugreport, stable,
me
In-Reply-To: <760ed3ff-43d9-75e5-b36c-aaa7871ffe50@c-s.fr>
Christophe LEROY <christophe.leroy@c-s.fr> writes:
> Le 26/09/2018 =C3=A0 13:11, Daniel Thompson a =C3=A9crit=C2=A0:
>> On 16/09/2018 20:06, Daniel Thompson wrote:
>>> On Fri, Sep 14, 2018 at 12:35:44PM +0000, Christophe Leroy wrote:
>>>> On a powerpc 8xx, 'btc' fails as follows:
>>>> Entering kdb (current=3D0x(ptrval), pid 282) due to Keyboard Entry
...
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>> Cc: <stable@vger.kernel.org> # 4.15+
>>>
>>> Would a Fixes: be better here?
>>> Fixes: ad67b74d2469d9b82 ("printk: hash addresses printed with %p")
>>=20
>> Christophe, When you add the Fixes: could you also add my
>>=20
>> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
>
> Ok, thanks for the review, but do I have to do anything really ?
>
> The Fixes: and now your Reviewed-by: appear automatically in patchwork=20
> (https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=3D65715),=
=20
> so I believe they'll be automatically included when Jason or someone=20
> else takes the patch, no ?
patchwork won't add the Fixes tag from the reply, it needs to be in the
original mail.
See:
https://github.com/getpatchwork/patchwork/issues/151
cheers
^ permalink raw reply
* Re: [PATCH] kdb: use correct pointer when 'btc' calls 'btt'
From: Christophe LEROY @ 2018-09-27 11:30 UTC (permalink / raw)
To: Michael Ellerman, Daniel Thompson
Cc: Jason Wessel, linux-kernel, linuxppc-dev, kgdb-bugreport, stable,
me
In-Reply-To: <877ej7urh3.fsf@concordia.ellerman.id.au>
Le 27/09/2018 à 13:09, Michael Ellerman a écrit :
> Christophe LEROY <christophe.leroy@c-s.fr> writes:
>> Le 26/09/2018 à 13:11, Daniel Thompson a écrit :
>>> On 16/09/2018 20:06, Daniel Thompson wrote:
>>>> On Fri, Sep 14, 2018 at 12:35:44PM +0000, Christophe Leroy wrote:
>>>>> On a powerpc 8xx, 'btc' fails as follows:
>>>>> Entering kdb (current=0x(ptrval), pid 282) due to Keyboard Entry
> ...
>>>>>
>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>>> Cc: <stable@vger.kernel.org> # 4.15+
>>>>
>>>> Would a Fixes: be better here?
>>>> Fixes: ad67b74d2469d9b82 ("printk: hash addresses printed with %p")
>>>
>>> Christophe, When you add the Fixes: could you also add my
>>>
>>> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
>>
>> Ok, thanks for the review, but do I have to do anything really ?
>>
>> The Fixes: and now your Reviewed-by: appear automatically in patchwork
>> (https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=65715),
>> so I believe they'll be automatically included when Jason or someone
>> else takes the patch, no ?
>
> patchwork won't add the Fixes tag from the reply, it needs to be in the
> original mail.
>
> See:
> https://github.com/getpatchwork/patchwork/issues/151
>
Ok, so it accounts it and adds a '1' in the F column in the patches
list, but won't take it into account.
Then I'll send a v2 with revised commit text.
Christophe
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox