* Re: [RFC PATCH v2 1/3] mm/gup: fix gup_fast with dynamic page table folding
From: Christian Borntraeger @ 2020-09-08 12:09 UTC (permalink / raw)
To: Christophe Leroy, Gerald Schaefer, Jason Gunthorpe, John Hubbard
Cc: Peter Zijlstra, Catalin Marinas, Dave Hansen, linux-mm,
Paul Mackerras, linux-sparc, Alexander Gordeev, Claudio Imbrenda,
Will Deacon, linux-arch, linux-s390, Vasily Gorbik,
Richard Weinberger, linux-x86, Russell King, Ingo Molnar,
Andrey Ryabinin, Jeff Dike, Arnd Bergmann, Heiko Carstens,
linux-um, Borislav Petkov, Andy Lutomirski, Thomas Gleixner,
linux-arm, Linus Torvalds, LKML, Andrew Morton, linux-power,
Mike Rapoport
In-Reply-To: <82fbe8f9-f199-5fc2-4168-eb43ad0b0346@csgroup.eu>
On 08.09.20 07:06, Christophe Leroy wrote:
>
>
> Le 07/09/2020 à 20:00, Gerald Schaefer a écrit :
>> From: Alexander Gordeev <agordeev@linux.ibm.com>
>>
>> Commit 1a42010cdc26 ("s390/mm: convert to the generic get_user_pages_fast
>> code") introduced a subtle but severe bug on s390 with gup_fast, due to
>> dynamic page table folding.
>>
>> The question "What would it require for the generic code to work for s390"
>> has already been discussed here
>> https://lkml.kernel.org/r/20190418100218.0a4afd51@mschwideX1
>> and ended with a promising approach here
>> https://lkml.kernel.org/r/20190419153307.4f2911b5@mschwideX1
>> which in the end unfortunately didn't quite work completely.
>>
>> We tried to mimic static level folding by changing pgd_offset to always
>> calculate top level page table offset, and do nothing in folded pXd_offset.
>> What has been overlooked is that PxD_SIZE/MASK and thus pXd_addr_end do
>> not reflect this dynamic behaviour, and still act like static 5-level
>> page tables.
>>
>
> [...]
>
>>
>> Fix this by introducing new pXd_addr_end_folded helpers, which take an
>> additional pXd entry value parameter, that can be used on s390
>> to determine the correct page table level and return corresponding
>> end / boundary. With that, the pointer iteration will always
>> happen in gup_pgd_range for s390. No change for other architectures
>> introduced.
>
> Not sure pXd_addr_end_folded() is the best understandable name, allthough I don't have any alternative suggestion at the moment.
> Maybe could be something like pXd_addr_end_fixup() as it will disappear in the next patch, or pXd_addr_end_gup() ?
>
> Also, if it happens to be acceptable to get patch 2 in stable, I think you should switch patch 1 and patch 2 to avoid the step through pXd_addr_end_folded()
given that this fixes a data corruption issue, wouldnt it be the best to go forward
with this patch ASAP and then handle the other patches on top with all the time that
we need?
>
>
>>
>> Fixes: 1a42010cdc26 ("s390/mm: convert to the generic get_user_pages_fast code")
>> Cc: <stable@vger.kernel.org> # 5.2+
>> Reviewed-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
>> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
>> Signed-off-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
>> ---
>> arch/s390/include/asm/pgtable.h | 42 +++++++++++++++++++++++++++++++++
>> include/linux/pgtable.h | 16 +++++++++++++
>> mm/gup.c | 8 +++----
>> 3 files changed, 62 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
>> index 7eb01a5459cd..027206e4959d 100644
>> --- a/arch/s390/include/asm/pgtable.h
>> +++ b/arch/s390/include/asm/pgtable.h
>> @@ -512,6 +512,48 @@ static inline bool mm_pmd_folded(struct mm_struct *mm)
>> }
>> #define mm_pmd_folded(mm) mm_pmd_folded(mm)
>> +/*
>> + * With dynamic page table levels on s390, the static pXd_addr_end() functions
>> + * will not return corresponding dynamic boundaries. This is no problem as long
>> + * as only pXd pointers are passed down during page table walk, because
>> + * pXd_offset() will simply return the given pointer for folded levels, and the
>> + * pointer iteration over a range simply happens at the correct page table
>> + * level.
>> + * It is however a problem with gup_fast, or other places walking the page
>> + * tables w/o locks using READ_ONCE(), and passing down the pXd values instead
>> + * of pointers. In this case, the pointer given to pXd_offset() is a pointer to
>> + * a stack variable, which cannot be used for pointer iteration at the correct
>> + * level. Instead, the iteration then has to happen by going up to pgd level
>> + * again. To allow this, provide pXd_addr_end_folded() functions with an
>> + * additional pXd value parameter, which can be used on s390 to determine the
>> + * folding level and return the corresponding boundary.
>> + */
>> +static inline unsigned long rste_addr_end_folded(unsigned long rste, unsigned long addr, unsigned long end)
>
> What does 'rste' stands for ?
>
> Isn't this line a bit long ?
this is region/segment table entry according to the architecture.
On our platform we do have the pagetables with a different format that
next levels (segment table -> 1MB granularity, region 3rd table -> 2 GB
granularity, region 2nd table -> 4TB granularity, region 1st table -> 8 PB
granularity. ST,R3,R2,R1 have the same format and are thus often called
crste (combined region and segment table entry).
^ permalink raw reply
* Re: [PATCH kernel] powerpc/dma: Fix dma_map_ops::get_required_mask
From: Alexey Kardashevskiy @ 2020-09-08 12:06 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Oliver O'Halloran, linuxppc-dev, Cédric Le Goater
In-Reply-To: <20200908054416.GA13585@lst.de>
On 08/09/2020 15:44, Christoph Hellwig wrote:
> On Tue, Sep 08, 2020 at 11:51:06AM +1000, Alexey Kardashevskiy wrote:
>> What is dma_get_required_mask() for anyway? What "requires" what here?
>
> Yes, it is a really odd API. It comes from classic old PCI where
> 64-bit addressing required an additional bus cycle, and various devices
> had different addressing schemes, with the smaller addresses beeing
> more efficient. So this allows the driver to request the "required"
> addressing mode to address all memory. "preferred" might be a better
> name as we'll bounce buffer if it isn't met. I also don't really see
> why a driver would ever want to use it for a modern PCIe device.
a-ha, this makes more sense, thanks. Then I guess we need to revert that
one bit from yours f1565c24b596, do not we?
--
Alexey
^ permalink raw reply
* Re: [PATCH kernel] powerpc/dma: Fix dma_map_ops::get_required_mask
From: Cédric Le Goater @ 2020-09-08 11:45 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev
Cc: Oliver O'Halloran, Christoph Hellwig
In-Reply-To: <20200908015106.79661-1-aik@ozlabs.ru>
On 9/8/20 3:51 AM, Alexey Kardashevskiy wrote:
> There are 2 problems with it:
> 1. "<" vs expected "<<"
> 2. the shift number is an IOMMU page number mask, not an address mask
> as the IOMMU page shift is missing.
>
> This did not hit us before f1565c24b596 ("powerpc: use the generic
> dma_ops_bypass mode") because we had there additional code to handle
> bypass mask so this chunk (almost?) never executed. However there
> were reports that aacraid does not work with "iommu=nobypass".
> After f1565c24b596, aacraid (and probably others which call
> dma_get_required_mask() before setting the mask) was unable to
> enable 64bit DMA and fall back to using IOMMU which was known not to work,
> one of the problems is double free of an IOMMU page.
>
> This fixes DMA for aacraid, both with and without "iommu=nobypass"
> in the kernel command line. Verified with "stress-ng -d 4".
>
> Fixes: f1565c24b596 ("powerpc: use the generic dma_ops_bypass mode")
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
The boston system looks solid with this patch.
Tested-by: Cédric Le Goater <clg@kaod.org>
Thanks a lot !
C.
> ---
>
> The original code came Jun 24 2011:
> 6a5c7be5e484 ("powerpc: Override dma_get_required_mask by platform hook and ops")
>
>
> What is dma_get_required_mask() for anyway? What "requires" what here?
>
> Even though it works for now (due to huge - >4GB - default DMA window),
> I am still not convinced we do not want this chunk here
> (this is what f1565c24b596 removed):
>
> if (dev_is_pci(dev)) {
> u64 bypass_mask = dma_direct_get_required_mask(dev);
>
> if (dma_iommu_bypass_supported(dev, bypass_mask))
> return bypass_mask;
> }
> ---
> arch/powerpc/kernel/dma-iommu.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/dma-iommu.c b/arch/powerpc/kernel/dma-iommu.c
> index 569fecd7b5b2..9053fc9d20c7 100644
> --- a/arch/powerpc/kernel/dma-iommu.c
> +++ b/arch/powerpc/kernel/dma-iommu.c
> @@ -120,7 +120,8 @@ u64 dma_iommu_get_required_mask(struct device *dev)
> if (!tbl)
> return 0;
>
> - mask = 1ULL < (fls_long(tbl->it_offset + tbl->it_size) - 1);
> + mask = 1ULL << (fls_long(tbl->it_offset + tbl->it_size) +
> + tbl->it_page_shift - 1);
> mask += mask - 1;
>
> return mask;
>
^ permalink raw reply
* Re: [PATCH v1 5/5] powerpc/fault: Perform exception fixup in do_page_fault()
From: Nicholas Piggin @ 2020-09-08 9:13 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <5748c8f5cf0a9b3686169e2c7709107e6aaec408.1596734105.git.christophe.leroy@csgroup.eu>
Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:
> Exception fixup doesn't require the heady full regs saving,
heavy
> do it from do_page_fault() directly.
>
> For that, split bad_page_fault() in two parts.
>
> As bad_page_fault() can also be called from other places than
> handle_page_fault(), it will still perform exception fixup and
> fallback on __bad_page_fault().
>
> handle_page_fault() directly calls __bad_page_fault() as the
> exception fixup will now be done by do_page_fault()
Looks good. We can probably get rid of bad_page_fault completely after
this too.
Hmm, the alignment exception might(?) hit user copies if the user points
it to CI memory. Then you could race and the memory gets unmapped. In
that case the exception table check might be better to be explicit there
with comments.
The first call in do_hash_fault is not required (copy user will never
be in nmi context). The second one and the one in slb_fault could be
made explicit too. Anyway for now this is fine.
Thanks,
Nick
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/kernel/entry_32.S | 2 +-
> arch/powerpc/kernel/exceptions-64e.S | 2 +-
> arch/powerpc/kernel/exceptions-64s.S | 2 +-
> arch/powerpc/mm/fault.c | 33 ++++++++++++++++++++--------
> 4 files changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
> index f4d0af8e1136..c198786591f9 100644
> --- a/arch/powerpc/kernel/entry_32.S
> +++ b/arch/powerpc/kernel/entry_32.S
> @@ -678,7 +678,7 @@ handle_page_fault:
> mr r5,r3
> addi r3,r1,STACK_FRAME_OVERHEAD
> lwz r4,_DAR(r1)
> - bl bad_page_fault
> + bl __bad_page_fault
> b ret_from_except_full
>
> #ifdef CONFIG_PPC_BOOK3S_32
> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
> index d9ed79415100..dd9161ea5da8 100644
> --- a/arch/powerpc/kernel/exceptions-64e.S
> +++ b/arch/powerpc/kernel/exceptions-64e.S
> @@ -1024,7 +1024,7 @@ storage_fault_common:
> mr r5,r3
> addi r3,r1,STACK_FRAME_OVERHEAD
> ld r4,_DAR(r1)
> - bl bad_page_fault
> + bl __bad_page_fault
> b ret_from_except
>
> /*
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index f7d748b88705..2cb3bcfb896d 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -3254,7 +3254,7 @@ handle_page_fault:
> mr r5,r3
> addi r3,r1,STACK_FRAME_OVERHEAD
> ld r4,_DAR(r1)
> - bl bad_page_fault
> + bl __bad_page_fault
> b interrupt_return
>
> /* We have a data breakpoint exception - handle it */
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index edde169ba3a6..bd6e397eb84a 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -542,10 +542,20 @@ NOKPROBE_SYMBOL(__do_page_fault);
> int do_page_fault(struct pt_regs *regs, unsigned long address,
> unsigned long error_code)
> {
> + const struct exception_table_entry *entry;
> enum ctx_state prev_state = exception_enter();
> int rc = __do_page_fault(regs, address, error_code);
> exception_exit(prev_state);
> - return rc;
> + if (likely(!rc))
> + return 0;
> +
> + entry = search_exception_tables(regs->nip);
> + if (unlikely(!entry))
> + return rc;
> +
> + instruction_pointer_set(regs, extable_fixup(entry));
> +
> + return 0;
> }
> NOKPROBE_SYMBOL(do_page_fault);
>
> @@ -554,17 +564,10 @@ NOKPROBE_SYMBOL(do_page_fault);
> * It is called from the DSI and ISI handlers in head.S and from some
> * of the procedures in traps.c.
> */
> -void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
> +void __bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
> {
> - const struct exception_table_entry *entry;
> int is_write = page_fault_is_write(regs->dsisr);
>
> - /* Are we prepared to handle this fault? */
> - if ((entry = search_exception_tables(regs->nip)) != NULL) {
> - regs->nip = extable_fixup(entry);
> - return;
> - }
> -
> /* kernel has accessed a bad area */
>
> switch (TRAP(regs)) {
> @@ -598,3 +601,15 @@ void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
>
> die("Kernel access of bad area", regs, sig);
> }
> +
> +void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
> +{
> + const struct exception_table_entry *entry;
> +
> + /* Are we prepared to handle this fault? */
> + entry = search_exception_tables(instruction_pointer(regs));
> + if (entry)
> + instruction_pointer_set(regs, extable_fixup(entry));
> + else
> + __bad_page_fault(regs, address, sig);
> +}
> --
> 2.25.0
>
>
^ permalink raw reply
* Re: [PATCH v1 1/5] powerpc/mm: sanity_check_fault() should work for all, not only BOOK3S
From: Christophe Leroy @ 2020-09-08 8:56 UTC (permalink / raw)
To: Nicholas Piggin, Benjamin Herrenschmidt, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1599554359.m174sr2fhg.astroid@bobo.none>
Le 08/09/2020 à 10:43, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:
>> The verification and message introduced by commit 374f3f5979f9
>> ("powerpc/mm/hash: Handle user access of kernel address gracefully")
>> applies to all platforms, it should not be limited to BOOK3S.
>>
>> Make the BOOK3S version of sanity_check_fault() the one for all,
>> and bail out earlier if not BOOK3S.
>>
>> Fixes: 374f3f5979f9 ("powerpc/mm/hash: Handle user access of kernel address gracefully")
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> arch/powerpc/mm/fault.c | 8 +++-----
>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index 925a7231abb3..2efa34d7e644 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -303,7 +303,6 @@ static inline void cmo_account_page_fault(void)
>> static inline void cmo_account_page_fault(void) { }
>> #endif /* CONFIG_PPC_SMLPAR */
>>
>> -#ifdef CONFIG_PPC_BOOK3S
>> static void sanity_check_fault(bool is_write, bool is_user,
>> unsigned long error_code, unsigned long address)
>> {
>> @@ -320,6 +319,9 @@ static void sanity_check_fault(bool is_write, bool is_user,
>> return;
>> }
>>
>> + if (!IS_ENABLED(CONFIG_PPC_BOOK3S))
>> + return;
>
> Seems okay. Why is address == -1 special though? I guess it's because
> it may not be an exploit kernel reference but a buggy pointer underflow?
> In that case -1 doesn't seem like it would catch very much. Would it be
> better to test for high bit set for example ((long)address < 0) ?
See
https://github.com/linuxppc/linux/commit/0f9aee0cb9da7db7d96f63cfa2dc5e4f1bffeb87#diff-f9658f412252f3bb3093e0a95b37f3ac
-1 is what mmap() returns on error, if the app uses that as a pointer
that's a programming error not an exploit.
Euh .. If you test (long)address < 0, then the entire kernel falls into
that range as usually it goes from 0xc0000000 to 0xffffffff
But we could skip the top page entirely, anyway it is never mapped.
>
> Anyway for your patch
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Thanks
Christophe
^ permalink raw reply
* Re: [PATCH v1 4/5] powerpc/fault: Avoid heavy search_exception_tables() verification
From: Nicholas Piggin @ 2020-09-08 8:54 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <b07bac7a882c69deb9e6c8f234a68b3022f29072.1596734105.git.christophe.leroy@csgroup.eu>
Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:
> search_exception_tables() is an heavy operation, we have to avoid it.
> When KUAP is selected, we'll know the fault has been blocked by KUAP.
> Otherwise, it behaves just as if the address was already in the TLBs
> and no fault was generated.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Sorry I missed reviewing this. Yes, we discussed this and decided
that it's not effective I think (and KUAP solves it properly).
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/mm/fault.c | 20 +++++---------------
> 1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 525e0c2b5406..edde169ba3a6 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -214,24 +214,14 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
> if (address >= TASK_SIZE)
> return true;
>
> - if (!is_exec && (error_code & DSISR_PROTFAULT) &&
> - !search_exception_tables(regs->nip)) {
> + // Read/write fault blocked by KUAP is bad, it can never succeed.
> + if (bad_kuap_fault(regs, address, is_write)) {
> pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
> - address,
> - from_kuid(&init_user_ns, current_uid()));
> - }
> -
> - // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
> - if (!search_exception_tables(regs->nip))
> - return true;
> -
> - // Read/write fault in a valid region (the exception table search passed
> - // above), but blocked by KUAP is bad, it can never succeed.
> - if (bad_kuap_fault(regs, address, is_write))
> + address, from_kuid(&init_user_ns, current_uid()));
> return true;
> + }
>
> - // What's left? Kernel fault on user in well defined regions (extable
> - // matched), and allowed by KUAP in the faulting context.
> + // What's left? Kernel fault on user and allowed by KUAP in the faulting context.
> return false;
> }
>
> --
> 2.25.0
>
>
^ permalink raw reply
* Re: [RFC PATCH 02/12] powerpc: remove arguments from interrupt handler functions
From: Christophe Leroy @ 2020-09-08 8:50 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev
In-Reply-To: <a4eac9b0-da50-de8a-439e-173da7c20252@csgroup.eu>
Le 08/09/2020 à 10:29, Christophe Leroy a écrit :
>
>
> Le 08/09/2020 à 09:48, Nicholas Piggin a écrit :
>> Excerpts from Christophe Leroy's message of September 7, 2020 9:34 pm:
>>> On Mon, 2020-09-07 at 11:20 +0200, Christophe Leroy wrote:
>>>>
>>>> Le 05/09/2020 à 19:43, Nicholas Piggin a écrit :
>>>>> Make interrupt handlers all just take the pt_regs * argument and load
>>>>> DAR/DSISR etc from that. Make those that return a value return long.
>>>>
>>>> I like this, it will likely simplify a bit the VMAP_STACK mess.
>>>>
>>>> Not sure it is that easy. My board is stuck after the start of init.
>>>>
>>>>
>>>> On the 8xx, on Instruction TLB Error exception, we do
>>>>
>>>> andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1
>>>> bits */
>>>>
>>>> On book3s/32, on ISI exception we do:
>>>> andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1
>>>> bits */
>>>>
>>>> On 40x and bookE, on ISI exception we do:
>>>> li r5,0 /* Pass zero as arg3 */
>>>>
>>>>
>>>> And regs->dsisr will just contain nothing
>>>>
>>>> So it means we should at least write back r5 into regs->dsisr from
>>>> there
>>>> ? The performance impact should be minimal as we already write _DAR so
>>>> the cache line should already be in the cache.
>>>>
>>>> A hacky 'stw r5, _DSISR(r1)' in handle_page_fault() does the trick,
>>>> allthough we don't want to do it for both ISI and DSI at the end, so
>>>> you'll have to do it in every head_xxx.S
>>>
>>> To get you series build and work, I did the following hacks:
>>
>> Great, thanks for this.
>>
>>> diff --git a/arch/powerpc/include/asm/interrupt.h
>>> b/arch/powerpc/include/asm/interrupt.h
>>> index acfcc7d5779b..c11045d3113a 100644
>>> --- a/arch/powerpc/include/asm/interrupt.h
>>> +++ b/arch/powerpc/include/asm/interrupt.h
>>> @@ -93,7 +93,9 @@ static inline void interrupt_nmi_exit_prepare(struct
>>> pt_regs *regs, struct inter
>>> {
>>> nmi_exit();
>>> +#ifdef CONFIG_PPC64
>>> this_cpu_set_ftrace_enabled(state->ftrace_enabled);
>>> +#endif
>>
>> This seems okay, not a hack.
>>
>>> #ifdef CONFIG_PPC_BOOK3S_64
>>> /* Check we didn't change the pending interrupt mask. */
>>> diff --git a/arch/powerpc/kernel/entry_32.S
>>> b/arch/powerpc/kernel/entry_32.S
>>> index f4d0af8e1136..66f7adbe1076 100644
>>> --- a/arch/powerpc/kernel/entry_32.S
>>> +++ b/arch/powerpc/kernel/entry_32.S
>>> @@ -663,6 +663,7 @@ ppc_swapcontext:
>>> */
>>> .globl handle_page_fault
>>> handle_page_fault:
>>> + stw r5,_DSISR(r1)
>>> addi r3,r1,STACK_FRAME_OVERHEAD
>>> #ifdef CONFIG_PPC_BOOK3S_32
>>> andis. r0,r5,DSISR_DABRMATCH@h
>>
>> Is this what you want to do for 32, or do you want to seperate
>> ISI and DSI sides?
>>
>
> No I think we want to separate ISI and DSI sides.
>
> And I think the specific filtering done in ISI could be done in
> do_page_fault() in C. Ok, it would make a special handling for is_exec
> but there are already several places where the behaviour differs based
> on is_exec.
> The only thing we need to keep at ASM level is the DABR stuff for
> calling do_break() in handle_page_fault(), because it is used to decide
> whether full regs are saved or not. But it could be a test done earlier
> in the prolog and the result being kept in one of the CR bits.
Looking at it once more, I'm wondering whether we really need a full regs.
Before commit
https://github.com/linuxppc/linux/commit/d300627c6a53693fb01479b59b0cdd293761b1fa#diff-f9658f412252f3bb3093e0a95b37f3ac
do_break() was called from do_page_fault() without a full regs set.
Christophe
^ permalink raw reply
* Re: [PATCH v1 3/5] powerpc/fault: Reorder tests in bad_kernel_fault()
From: Nicholas Piggin @ 2020-09-08 8:46 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <ef827b73770a7b155079393f8d8430e10a99ec94.1596734104.git.christophe.leroy@csgroup.eu>
Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:
> Check address earlier to simplify the following test.
Good logic reduction.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/mm/fault.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 9ef9ee244f72..525e0c2b5406 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -210,17 +210,17 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
> return true;
> }
>
> - if (!is_exec && address < TASK_SIZE && (error_code & DSISR_PROTFAULT) &&
> + // Kernel fault on kernel address is bad
> + if (address >= TASK_SIZE)
> + return true;
> +
> + if (!is_exec && (error_code & DSISR_PROTFAULT) &&
> !search_exception_tables(regs->nip)) {
> pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
> address,
> from_kuid(&init_user_ns, current_uid()));
> }
>
> - // Kernel fault on kernel address is bad
> - if (address >= TASK_SIZE)
> - return true;
> -
> // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
> if (!search_exception_tables(regs->nip))
> return true;
> --
> 2.25.0
>
>
^ permalink raw reply
* Re: [PATCH v1 2/5] powerpc/fault: Unnest definition of page_fault_is_write() and page_fault_is_bad()
From: Nicholas Piggin @ 2020-09-08 8:44 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <4cd127f8988b7b5d3a9b24b67dbad81fef3aee7f.1596734104.git.christophe.leroy@csgroup.eu>
Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:
> To make it more readable, separate page_fault_is_write() and page_fault_is_bad()
> to avoir several levels of #ifdefs
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/mm/fault.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 2efa34d7e644..9ef9ee244f72 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -363,17 +363,19 @@ static void sanity_check_fault(bool is_write, bool is_user,
> */
> #if (defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
> #define page_fault_is_write(__err) ((__err) & ESR_DST)
> -#define page_fault_is_bad(__err) (0)
> #else
> #define page_fault_is_write(__err) ((__err) & DSISR_ISSTORE)
> -#if defined(CONFIG_PPC_8xx)
> +#endif
> +
> +#if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
> +#define page_fault_is_bad(__err) (0)
> +#elif defined(CONFIG_PPC_8xx)
> #define page_fault_is_bad(__err) ((__err) & DSISR_NOEXEC_OR_G)
> #elif defined(CONFIG_PPC64)
> #define page_fault_is_bad(__err) ((__err) & DSISR_BAD_FAULT_64S)
> #else
> #define page_fault_is_bad(__err) ((__err) & DSISR_BAD_FAULT_32S)
> #endif
> -#endif
>
> /*
> * For 600- and 800-family processors, the error_code parameter is DSISR
> --
> 2.25.0
>
>
^ permalink raw reply
* Re: [PATCH v1 1/5] powerpc/mm: sanity_check_fault() should work for all, not only BOOK3S
From: Nicholas Piggin @ 2020-09-08 8:43 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <7baae4086cbb9ffb08c933b065ff7d29dbc03dd6.1596734104.git.christophe.leroy@csgroup.eu>
Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:
> The verification and message introduced by commit 374f3f5979f9
> ("powerpc/mm/hash: Handle user access of kernel address gracefully")
> applies to all platforms, it should not be limited to BOOK3S.
>
> Make the BOOK3S version of sanity_check_fault() the one for all,
> and bail out earlier if not BOOK3S.
>
> Fixes: 374f3f5979f9 ("powerpc/mm/hash: Handle user access of kernel address gracefully")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/mm/fault.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 925a7231abb3..2efa34d7e644 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -303,7 +303,6 @@ static inline void cmo_account_page_fault(void)
> static inline void cmo_account_page_fault(void) { }
> #endif /* CONFIG_PPC_SMLPAR */
>
> -#ifdef CONFIG_PPC_BOOK3S
> static void sanity_check_fault(bool is_write, bool is_user,
> unsigned long error_code, unsigned long address)
> {
> @@ -320,6 +319,9 @@ static void sanity_check_fault(bool is_write, bool is_user,
> return;
> }
>
> + if (!IS_ENABLED(CONFIG_PPC_BOOK3S))
> + return;
Seems okay. Why is address == -1 special though? I guess it's because
it may not be an exploit kernel reference but a buggy pointer underflow?
In that case -1 doesn't seem like it would catch very much. Would it be
better to test for high bit set for example ((long)address < 0) ?
Anyway for your patch
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> +
> /*
> * For hash translation mode, we should never get a
> * PROTFAULT. Any update to pte to reduce access will result in us
> @@ -354,10 +356,6 @@ static void sanity_check_fault(bool is_write, bool is_user,
>
> WARN_ON_ONCE(error_code & DSISR_PROTFAULT);
> }
> -#else
> -static void sanity_check_fault(bool is_write, bool is_user,
> - unsigned long error_code, unsigned long address) { }
> -#endif /* CONFIG_PPC_BOOK3S */
>
> /*
> * Define the correct "is_write" bit in error_code based
> --
> 2.25.0
>
>
^ permalink raw reply
* Re: [RFC PATCH 02/12] powerpc: remove arguments from interrupt handler functions
From: Christophe Leroy @ 2020-09-08 8:29 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev
In-Reply-To: <1599551224.3zoap14y55.astroid@bobo.none>
Le 08/09/2020 à 09:48, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of September 7, 2020 9:34 pm:
>> On Mon, 2020-09-07 at 11:20 +0200, Christophe Leroy wrote:
>>>
>>> Le 05/09/2020 à 19:43, Nicholas Piggin a écrit :
>>>> Make interrupt handlers all just take the pt_regs * argument and load
>>>> DAR/DSISR etc from that. Make those that return a value return long.
>>>
>>> I like this, it will likely simplify a bit the VMAP_STACK mess.
>>>
>>> Not sure it is that easy. My board is stuck after the start of init.
>>>
>>>
>>> On the 8xx, on Instruction TLB Error exception, we do
>>>
>>> andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 bits */
>>>
>>> On book3s/32, on ISI exception we do:
>>> andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 bits */
>>>
>>> On 40x and bookE, on ISI exception we do:
>>> li r5,0 /* Pass zero as arg3 */
>>>
>>>
>>> And regs->dsisr will just contain nothing
>>>
>>> So it means we should at least write back r5 into regs->dsisr from there
>>> ? The performance impact should be minimal as we already write _DAR so
>>> the cache line should already be in the cache.
>>>
>>> A hacky 'stw r5, _DSISR(r1)' in handle_page_fault() does the trick,
>>> allthough we don't want to do it for both ISI and DSI at the end, so
>>> you'll have to do it in every head_xxx.S
>>
>> To get you series build and work, I did the following hacks:
>
> Great, thanks for this.
>
>> diff --git a/arch/powerpc/include/asm/interrupt.h
>> b/arch/powerpc/include/asm/interrupt.h
>> index acfcc7d5779b..c11045d3113a 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -93,7 +93,9 @@ static inline void interrupt_nmi_exit_prepare(struct
>> pt_regs *regs, struct inter
>> {
>> nmi_exit();
>>
>> +#ifdef CONFIG_PPC64
>> this_cpu_set_ftrace_enabled(state->ftrace_enabled);
>> +#endif
>
> This seems okay, not a hack.
>
>> #ifdef CONFIG_PPC_BOOK3S_64
>> /* Check we didn't change the pending interrupt mask. */
>> diff --git a/arch/powerpc/kernel/entry_32.S
>> b/arch/powerpc/kernel/entry_32.S
>> index f4d0af8e1136..66f7adbe1076 100644
>> --- a/arch/powerpc/kernel/entry_32.S
>> +++ b/arch/powerpc/kernel/entry_32.S
>> @@ -663,6 +663,7 @@ ppc_swapcontext:
>> */
>> .globl handle_page_fault
>> handle_page_fault:
>> + stw r5,_DSISR(r1)
>> addi r3,r1,STACK_FRAME_OVERHEAD
>> #ifdef CONFIG_PPC_BOOK3S_32
>> andis. r0,r5,DSISR_DABRMATCH@h
>
> Is this what you want to do for 32, or do you want to seperate
> ISI and DSI sides?
>
No I think we want to separate ISI and DSI sides.
And I think the specific filtering done in ISI could be done in
do_page_fault() in C. Ok, it would make a special handling for is_exec
but there are already several places where the behaviour differs based
on is_exec.
The only thing we need to keep at ASM level is the DABR stuff for
calling do_break() in handle_page_fault(), because it is used to decide
whether full regs are saved or not. But it could be a test done earlier
in the prolog and the result being kept in one of the CR bits.
That way we can avoid reloading dsisr and dar from the stack, especially
for VMAP stack as they are saved quite early in the prolog.
Or we can take them out of the thread struct and save them in the stack
a little latter in the prolog, but then we have to keep the RI bit a bit
cleared longer.
While we are playing with do_page_fault(), did you have a look at the
series I sent
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/7baae4086cbb9ffb08c933b065ff7d29dbc03dd6.1596734104.git.christophe.leroy@csgroup.eu/
Christophe
^ permalink raw reply
* Re: [RFC PATCH v2 2/3] mm: make pXd_addr_end() functions page-table entry aware
From: Christophe Leroy @ 2020-09-08 8:16 UTC (permalink / raw)
To: Alexander Gordeev, Michael Ellerman
Cc: Peter Zijlstra, Catalin Marinas, Dave Hansen, linux-mm,
Paul Mackerras, linux-sparc, Claudio Imbrenda, Will Deacon,
linux-arch, linux-s390, Vasily Gorbik, Christian Borntraeger,
Richard Weinberger, linux-x86, Russell King, Jason Gunthorpe,
Ingo Molnar, Andrey Ryabinin, Gerald Schaefer, Jeff Dike,
Arnd Bergmann, John Hubbard, Heiko Carstens, linux-um,
Borislav Petkov, Andy Lutomirski, Thomas Gleixner, linux-arm,
Linus Torvalds, LKML, Andrew Morton, linux-power, Mike Rapoport
In-Reply-To: <20200908074638.GA19099@oc3871087118.ibm.com>
Le 08/09/2020 à 09:46, Alexander Gordeev a écrit :
> On Tue, Sep 08, 2020 at 07:14:38AM +0200, Christophe Leroy wrote:
>> You forgot arch/powerpc/mm/book3s64/subpage_prot.c it seems.
>
> Yes, and also two more sources :/
> arch/powerpc/mm/kasan/8xx.c
> arch/powerpc/mm/kasan/kasan_init_32.c
>
> But these two are not quite obvious wrt pgd_addr_end() used
> while traversing pmds. Could you please clarify a bit?
>
>
> diff --git a/arch/powerpc/mm/kasan/8xx.c b/arch/powerpc/mm/kasan/8xx.c
> index 2784224..89c5053 100644
> --- a/arch/powerpc/mm/kasan/8xx.c
> +++ b/arch/powerpc/mm/kasan/8xx.c
> @@ -15,8 +15,8 @@
> for (k_cur = k_start; k_cur != k_end; k_cur = k_next, pmd += 2, block += SZ_8M) {
> pte_basic_t *new;
>
> - k_next = pgd_addr_end(k_cur, k_end);
> - k_next = pgd_addr_end(k_next, k_end);
> + k_next = pmd_addr_end(k_cur, k_end);
> + k_next = pmd_addr_end(k_next, k_end);
No, I don't think so.
On powerpc32 we have only two levels, so pgd and pmd are more or less
the same.
But pmd_addr_end() as defined in include/asm-generic/pgtable-nopmd.h is
a no-op, so I don't think it will work.
It is likely that this function should iterate on pgd, then you get pmd
= pmd_offset(pud_offset(p4d_offset(pgd)));
> if ((void *)pmd_page_vaddr(*pmd) != kasan_early_shadow_pte)
> continue;
>
> diff --git a/arch/powerpc/mm/kasan/kasan_init_32.c b/arch/powerpc/mm/kasan/kasan_init_32.c
> index fb29404..3f7d6dc6 100644
> --- a/arch/powerpc/mm/kasan/kasan_init_32.c
> +++ b/arch/powerpc/mm/kasan/kasan_init_32.c
> @@ -38,7 +38,7 @@ int __init kasan_init_shadow_page_tables(unsigned long k_start, unsigned long k_
> for (k_cur = k_start; k_cur != k_end; k_cur = k_next, pmd++) {
> pte_t *new;
>
> - k_next = pgd_addr_end(k_cur, k_end);
> + k_next = pmd_addr_end(k_cur, k_end);
Same here I get, iterate on pgd then get pmd =
pmd_offset(pud_offset(p4d_offset(pgd)));
> if ((void *)pmd_page_vaddr(*pmd) != kasan_early_shadow_pte)
> continue;
>
> @@ -196,7 +196,7 @@ void __init kasan_early_init(void)
> kasan_populate_pte(kasan_early_shadow_pte, PAGE_KERNEL);
>
> do {
> - next = pgd_addr_end(addr, end);
> + next = pmd_addr_end(addr, end);
> pmd_populate_kernel(&init_mm, pmd, kasan_early_shadow_pte);
> } while (pmd++, addr = next, addr != end);
>
>
Christophe
^ permalink raw reply
* Re: [RFC PATCH 02/12] powerpc: remove arguments from interrupt handler functions
From: Nicholas Piggin @ 2020-09-08 7:48 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev
In-Reply-To: <1599478457.27656.1.camel@po17688vm.idsi0.si.c-s.fr>
Excerpts from Christophe Leroy's message of September 7, 2020 9:34 pm:
> On Mon, 2020-09-07 at 11:20 +0200, Christophe Leroy wrote:
>>
>> Le 05/09/2020 à 19:43, Nicholas Piggin a écrit :
>> > Make interrupt handlers all just take the pt_regs * argument and load
>> > DAR/DSISR etc from that. Make those that return a value return long.
>>
>> I like this, it will likely simplify a bit the VMAP_STACK mess.
>>
>> Not sure it is that easy. My board is stuck after the start of init.
>>
>>
>> On the 8xx, on Instruction TLB Error exception, we do
>>
>> andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 bits */
>>
>> On book3s/32, on ISI exception we do:
>> andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 bits */
>>
>> On 40x and bookE, on ISI exception we do:
>> li r5,0 /* Pass zero as arg3 */
>>
>>
>> And regs->dsisr will just contain nothing
>>
>> So it means we should at least write back r5 into regs->dsisr from there
>> ? The performance impact should be minimal as we already write _DAR so
>> the cache line should already be in the cache.
>>
>> A hacky 'stw r5, _DSISR(r1)' in handle_page_fault() does the trick,
>> allthough we don't want to do it for both ISI and DSI at the end, so
>> you'll have to do it in every head_xxx.S
>
> To get you series build and work, I did the following hacks:
Great, thanks for this.
> diff --git a/arch/powerpc/include/asm/interrupt.h
> b/arch/powerpc/include/asm/interrupt.h
> index acfcc7d5779b..c11045d3113a 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -93,7 +93,9 @@ static inline void interrupt_nmi_exit_prepare(struct
> pt_regs *regs, struct inter
> {
> nmi_exit();
>
> +#ifdef CONFIG_PPC64
> this_cpu_set_ftrace_enabled(state->ftrace_enabled);
> +#endif
This seems okay, not a hack.
> #ifdef CONFIG_PPC_BOOK3S_64
> /* Check we didn't change the pending interrupt mask. */
> diff --git a/arch/powerpc/kernel/entry_32.S
> b/arch/powerpc/kernel/entry_32.S
> index f4d0af8e1136..66f7adbe1076 100644
> --- a/arch/powerpc/kernel/entry_32.S
> +++ b/arch/powerpc/kernel/entry_32.S
> @@ -663,6 +663,7 @@ ppc_swapcontext:
> */
> .globl handle_page_fault
> handle_page_fault:
> + stw r5,_DSISR(r1)
> addi r3,r1,STACK_FRAME_OVERHEAD
> #ifdef CONFIG_PPC_BOOK3S_32
> andis. r0,r5,DSISR_DABRMATCH@h
Is this what you want to do for 32, or do you want to seperate
ISI and DSI sides?
Thanks,
Nick
^ permalink raw reply
* Re: [RFC PATCH v2 2/3] mm: make pXd_addr_end() functions page-table entry aware
From: Alexander Gordeev @ 2020-09-08 7:46 UTC (permalink / raw)
To: Christophe Leroy, Michael Ellerman
Cc: Peter Zijlstra, Catalin Marinas, Dave Hansen, linux-mm,
Paul Mackerras, linux-sparc, Claudio Imbrenda, Will Deacon,
linux-arch, linux-s390, Vasily Gorbik, Christian Borntraeger,
Richard Weinberger, linux-x86, Russell King, Jason Gunthorpe,
Ingo Molnar, Andrey Ryabinin, Gerald Schaefer, Jeff Dike,
Arnd Bergmann, John Hubbard, Heiko Carstens, linux-um,
Borislav Petkov, Andy Lutomirski, Thomas Gleixner, linux-arm,
Linus Torvalds, LKML, Andrew Morton, linux-power, Mike Rapoport
In-Reply-To: <31dfb3ed-a0cc-3024-d389-ab9bd19e881f@csgroup.eu>
On Tue, Sep 08, 2020 at 07:14:38AM +0200, Christophe Leroy wrote:
> You forgot arch/powerpc/mm/book3s64/subpage_prot.c it seems.
Yes, and also two more sources :/
arch/powerpc/mm/kasan/8xx.c
arch/powerpc/mm/kasan/kasan_init_32.c
But these two are not quite obvious wrt pgd_addr_end() used
while traversing pmds. Could you please clarify a bit?
diff --git a/arch/powerpc/mm/kasan/8xx.c b/arch/powerpc/mm/kasan/8xx.c
index 2784224..89c5053 100644
--- a/arch/powerpc/mm/kasan/8xx.c
+++ b/arch/powerpc/mm/kasan/8xx.c
@@ -15,8 +15,8 @@
for (k_cur = k_start; k_cur != k_end; k_cur = k_next, pmd += 2, block += SZ_8M) {
pte_basic_t *new;
- k_next = pgd_addr_end(k_cur, k_end);
- k_next = pgd_addr_end(k_next, k_end);
+ k_next = pmd_addr_end(k_cur, k_end);
+ k_next = pmd_addr_end(k_next, k_end);
if ((void *)pmd_page_vaddr(*pmd) != kasan_early_shadow_pte)
continue;
diff --git a/arch/powerpc/mm/kasan/kasan_init_32.c b/arch/powerpc/mm/kasan/kasan_init_32.c
index fb29404..3f7d6dc6 100644
--- a/arch/powerpc/mm/kasan/kasan_init_32.c
+++ b/arch/powerpc/mm/kasan/kasan_init_32.c
@@ -38,7 +38,7 @@ int __init kasan_init_shadow_page_tables(unsigned long k_start, unsigned long k_
for (k_cur = k_start; k_cur != k_end; k_cur = k_next, pmd++) {
pte_t *new;
- k_next = pgd_addr_end(k_cur, k_end);
+ k_next = pmd_addr_end(k_cur, k_end);
if ((void *)pmd_page_vaddr(*pmd) != kasan_early_shadow_pte)
continue;
@@ -196,7 +196,7 @@ void __init kasan_early_init(void)
kasan_populate_pte(kasan_early_shadow_pte, PAGE_KERNEL);
do {
- next = pgd_addr_end(addr, end);
+ next = pmd_addr_end(addr, end);
pmd_populate_kernel(&init_mm, pmd, kasan_early_shadow_pte);
} while (pmd++, addr = next, addr != end);
> Christophe
^ permalink raw reply related
* Re: [RFC PATCH 02/12] powerpc: remove arguments from interrupt handler functions
From: Nicholas Piggin @ 2020-09-08 7:46 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev
In-Reply-To: <e34fead9-a356-3ae6-aa33-544380230bd5@csgroup.eu>
Excerpts from Christophe Leroy's message of September 7, 2020 7:20 pm:
>
>
> Le 05/09/2020 à 19:43, Nicholas Piggin a écrit :
>> Make interrupt handlers all just take the pt_regs * argument and load
>> DAR/DSISR etc from that. Make those that return a value return long.
>
> I like this, it will likely simplify a bit the VMAP_STACK mess.
>
> Not sure it is that easy. My board is stuck after the start of init.
>
>
> On the 8xx, on Instruction TLB Error exception, we do
>
> andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 bits */
>
> On book3s/32, on ISI exception we do:
> andis. r5,r9,DSISR_SRR1_MATCH_32S@h /* Filter relevant SRR1 bits */
>
> On 40x and bookE, on ISI exception we do:
> li r5,0 /* Pass zero as arg3 */
>
>
> And regs->dsisr will just contain nothing
>
> So it means we should at least write back r5 into regs->dsisr from there
> ? The performance impact should be minimal as we already write _DAR so
> the cache line should already be in the cache.
Yes, I think that would be required. Sorry I didn't look closely at
32 bit.
> A hacky 'stw r5, _DSISR(r1)' in handle_page_fault() does the trick,
> allthough we don't want to do it for both ISI and DSI at the end, so
> you'll have to do it in every head_xxx.S
>
>
> While you are at it, it would probably also make sense to do remove the
> address param of bad_page_fault(), there is no point in loading back
> regs->dar in handle_page_fault() and machine_check_8xx() and
> alignment_exception(), just read regs->dar in bad_page_fault()
>
> The case of do_break() should also be looked at.
Yeah that's valid, I didn't do that because bad_page_fault was also
being called from asm, but an incremental patch should be quite easy.
> Why changing return code from int to long ?
Oh it's to make the next patch work without any changes to function
prototypes. Some handlers are returning int, others long. There is
no reason not to just return long AFAIKS so that's what I changed to.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] selftests/powerpc: Skip PROT_SAO test in guests/LPARS
From: Michael Ellerman @ 2020-09-08 7:22 UTC (permalink / raw)
To: Sachin Sant; +Cc: linuxppc-dev
In-Reply-To: <5E12FA66-4147-46E6-9D8A-358FFAFDC3FB@linux.vnet.ibm.com>
Sachin Sant <sachinp@linux.vnet.ibm.com> writes:
>> On 01-Sep-2020, at 6:16 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>>
>> In commit 9b725a90a8f1 ("powerpc/64s: Disallow PROT_SAO in LPARs by
>> default") PROT_SAO was disabled in guests/LPARs by default. So skip
>> the test if we are running in a guest to avoid a spurious failure.
>>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> —
>
> Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
>
> With the fix test is skipped while running in a guest
>
> # ./prot_sao
> test: prot-sao
> tags: git_version:unknown
> [SKIP] Test skipped on line 25
> skip: prot-sao
> #
Thanks. Sorry I missed adding your Tested-by tag.
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/boot/dts: Fix dtc "pciex" warnings
From: Michael Ellerman @ 2020-09-08 7:11 UTC (permalink / raw)
To: Christian Lamparter, linuxppc-dev
Cc: sfr, Chris Blake,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
In-Reply-To: <d2652e63-b136-a805-fd6d-00584b64c772@gmail.com>
Christian Lamparter <chunkeey@gmail.com> writes:
> On 2020-06-23 15:03, Michael Ellerman wrote:
>> With CONFIG_OF_ALL_DTBS=y, as set by eg. allmodconfig, we see lots of
>> warnings about our dts files, such as:
>>
>> arch/powerpc/boot/dts/glacier.dts:492.26-532.5:
>> Warning (pci_bridge): /plb/pciex@d00000000: node name is not "pci"
>> or "pcie"
>>
>> The node name should not particularly matter, it's just a name, and
>> AFAICS there's no kernel code that cares whether nodes are *named*
>> "pciex" or "pcie". So shutup these warnings by converting to the name
>> dtc wants.
>>
>> As always there's some risk this could break something obscure that
>> does rely on the name, in which case we can revert.
>
> Hmm, I noticed this when I was looking up why nobody commented
> on my series of adding more devices to the APM82181/bluestone series:
>
> <https://lore.kernel.org/linuxppc-dev/cover.1598124791.git.chunkeey@gmail.com/>
> (I'll post a v3 "soonish".)
>
>
> Unfortunately yes. This patch will break uboot code in Meraki MX60(W) / MX60.
>
> > https://github.com/riptidewave93/meraki-uboot/blob/mx60w-20180413/board/amcc/bluestone/bluestone.c#L1178
>
> | if (!pci_available()) {
> | fdt_find_and_setprop(blob, "/plb/pciex@d00000000", "status",
> | "disabled", sizeof("disabled"), 1);
> | }
>
>
> Backstory: There are two version of the Meraki MX60. The MX60
> and the MX60W. The difference is that the MX60W has a populated
> mini-pcie slot on the PCB for a >W<ireless card.
>
> That said, this is not earth shattering.
I'm happy to revert that hunk if you think any one is actually booting
mainline on those.
cheers
> (In theory, this can also cause problems for the bluestone and canyonlands
> dev boards that have the option to be configured as either dual sata or
> pcie+sata.... But this is probably not a problem for customer boards)
>
> OT: Please note that the plb, opb and ebc node paths (/plb/opb/ebc) are
> hardcoded too :(. Amending the proper unit-addresses will lead to no-longer
> working DTBs as the "ranges" are missing.
>
> Cheers,
> Christian
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>>
>> diff --git a/arch/powerpc/boot/dts/bluestone.dts b/arch/powerpc/boot/dts/bluestone.dts
>> index cc965a1816b6..aa1ae94cd776 100644
>> --- a/arch/powerpc/boot/dts/bluestone.dts
>> +++ b/arch/powerpc/boot/dts/bluestone.dts
>> @@ -325,7 +325,7 @@ EMAC0: ethernet@ef600c00 {
>> };
>> };
>>
>> - PCIE0: pciex@d00000000 {
>> + PCIE0: pcie@d00000000 {
>> device_type = "pci";
>> #interrupt-cells = <1>;
>> #size-cells = <2>;
^ permalink raw reply
* Re: [PATCH v2] kbuild: preprocess module linker script
From: Geert Uytterhoeven @ 2020-09-08 6:59 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-ia64@vger.kernel.org, Catalin Marinas, Paul Mackerras,
linux-riscv, Will Deacon, Anton Ivanov, Linux-Arch,
Richard Weinberger, Russell King, Fenghua Yu, Albert Ou,
Arnd Bergmann, linux-kbuild, Jeff Dike, linux-um, linux-m68k,
Tony Luck, Paul Walmsley, Linux ARM, Michal Marek,
Linux Kernel Mailing List, Palmer Dabbelt, Jessica Yu,
linuxppc-dev
In-Reply-To: <20200908042708.2511528-1-masahiroy@kernel.org>
On Tue, Sep 8, 2020 at 6:29 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> There was a request to preprocess the module linker script like we
> do for the vmlinux one. (https://lkml.org/lkml/2020/8/21/512)
>
> The difference between vmlinux.lds and module.lds is that the latter
> is needed for external module builds, thus must be cleaned up by
> 'make mrproper' instead of 'make clean'. Also, it must be created
> by 'make modules_prepare'.
>
> You cannot put it in arch/$(SRCARCH)/kernel/, which is cleaned up by
> 'make clean'. I moved arch/$(SRCARCH)/kernel/module.lds to
> arch/$(SRCARCH)/include/asm/module.lds.h, which is included from
> scripts/module.lds.S.
>
> scripts/module.lds is fine because 'make clean' keeps all the
> build artifacts under scripts/.
>
> You can add arch-specific sections in <asm/module.lds.h>.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> Tested-by: Jessica Yu <jeyu@kernel.org>
> Acked-by: Will Deacon <will@kernel.org>
> arch/m68k/Makefile | 1 -
> .../{kernel/module.lds => include/asm/module.lds.h} | 0
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH] mm: check for memory's node later during boot
From: Laurent Dufour @ 2020-09-08 6:56 UTC (permalink / raw)
To: Andrew Morton
Cc: nathanl, Rafael J. Wysocki, Greg Kroah-Hartman, cheloha,
linux-kernel, linux-mm, linuxppc-dev
In-Reply-To: <20200903143523.02e163bf06be3b48bac7f967@linux-foundation.org>
Le 03/09/2020 à 23:35, Andrew Morton a écrit :
> On Wed, 2 Sep 2020 11:09:11 +0200 Laurent Dufour <ldufour@linux.ibm.com> wrote:
>
>> register_mem_sect_under_nodem() is checking the memory block's node id only
>> if the system state is "SYSTEM_BOOTING". On PowerPC, the memory blocks are
>> registered while the system state is "SYSTEM_SCHEDULING", the one before
>> SYSTEM_RUNNING.
>>
>> The consequence on PowerPC guest with interleaved memory node's ranges is
>> that some memory block could be assigned to multiple nodes on sysfs. This
>> lately prevents some memory hot-plug and hot-unplug to succeed because
>> links are remaining. Such a panic is then displayed:
>>
>> ------------[ cut here ]------------
>> kernel BUG at /Users/laurent/src/linux-ppc/mm/memory_hotplug.c:1084!
>> Oops: Exception in kernel mode, sig: 5 [#1]
>> LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
>> Modules linked in: rpadlpar_io rpaphp pseries_rng rng_core vmx_crypto gf128mul binfmt_misc ip_tables x_tables xfs libcrc32c crc32c_vpmsum autofs4
>> CPU: 8 PID: 10256 Comm: drmgr Not tainted 5.9.0-rc1+ #25
>> NIP: c000000000403f34 LR: c000000000403f2c CTR: 0000000000000000
>> REGS: c0000004876e3660 TRAP: 0700 Not tainted (5.9.0-rc1+)
>> MSR: 800000000282b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 24000448 XER: 20040000
>> CFAR: c000000000846d20 IRQMASK: 0
>> GPR00: c000000000403f2c c0000004876e38f0 c0000000012f6f00 ffffffffffffffef
>> GPR04: 0000000000000227 c0000004805ae680 0000000000000000 00000004886f0000
>> GPR08: 0000000000000226 0000000000000003 0000000000000002 fffffffffffffffd
>> GPR12: 0000000088000484 c00000001ec96280 0000000000000000 0000000000000000
>> GPR16: 0000000000000000 0000000000000000 0000000000000004 0000000000000003
>> GPR20: c00000047814ffe0 c0000007ffff7c08 0000000000000010 c0000000013332c8
>> GPR24: 0000000000000000 c0000000011f6cc0 0000000000000000 0000000000000000
>> GPR28: ffffffffffffffef 0000000000000001 0000000150000000 0000000010000000
>> NIP [c000000000403f34] add_memory_resource+0x244/0x340
>> LR [c000000000403f2c] add_memory_resource+0x23c/0x340
>> Call Trace:
>> [c0000004876e38f0] [c000000000403f2c] add_memory_resource+0x23c/0x340 (unreliable)
>> [c0000004876e39c0] [c00000000040408c] __add_memory+0x5c/0xf0
>> [c0000004876e39f0] [c0000000000e2b94] dlpar_add_lmb+0x1b4/0x500
>> [c0000004876e3ad0] [c0000000000e3888] dlpar_memory+0x1f8/0xb80
>> [c0000004876e3b60] [c0000000000dc0d0] handle_dlpar_errorlog+0xc0/0x190
>> [c0000004876e3bd0] [c0000000000dc398] dlpar_store+0x198/0x4a0
>> [c0000004876e3c90] [c00000000072e630] kobj_attr_store+0x30/0x50
>> [c0000004876e3cb0] [c00000000051f954] sysfs_kf_write+0x64/0x90
>> [c0000004876e3cd0] [c00000000051ee40] kernfs_fop_write+0x1b0/0x290
>> [c0000004876e3d20] [c000000000438dd8] vfs_write+0xe8/0x290
>> [c0000004876e3d70] [c0000000004391ac] ksys_write+0xdc/0x130
>> [c0000004876e3dc0] [c000000000034e40] system_call_exception+0x160/0x270
>> [c0000004876e3e20] [c00000000000d740] system_call_common+0xf0/0x27c
>> Instruction dump:
>> 48442e35 60000000 0b030000 3cbe0001 7fa3eb78 7bc48402 38a5fffe 7ca5fa14
>> 78a58402 48442db1 60000000 7c7c1b78 <0b030000> 7f23cb78 4bda371d 60000000
>> ---[ end trace 562fd6c109cd0fb2 ]---
>>
>> To prevent this multiple links, make the node checking done for states
>> prior to SYSTEM_RUNNING.
>
> Did you consider adding a cc:stable to this fix?
I should have, but now I've to review the fix based on David's comment.
^ permalink raw reply
* Re: [PATCH kernel] powerpc/dma: Fix dma_map_ops::get_required_mask
From: Michael Ellerman @ 2020-09-08 6:45 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev
Cc: Alexey Kardashevskiy, Oliver O'Halloran, Christoph Hellwig,
Cédric Le Goater
In-Reply-To: <20200908015106.79661-1-aik@ozlabs.ru>
Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> There are 2 problems with it:
> 1. "<" vs expected "<<"
> 2. the shift number is an IOMMU page number mask, not an address mask
> as the IOMMU page shift is missing.
>
> This did not hit us before f1565c24b596 ("powerpc: use the generic
> dma_ops_bypass mode") because we had there additional code to handle
> bypass mask so this chunk (almost?) never executed. However there
> were reports that aacraid does not work with "iommu=nobypass".
> After f1565c24b596, aacraid (and probably others which call
> dma_get_required_mask() before setting the mask) was unable to
> enable 64bit DMA and fall back to using IOMMU which was known not to work,
> one of the problems is double free of an IOMMU page.
>
> This fixes DMA for aacraid, both with and without "iommu=nobypass"
> in the kernel command line. Verified with "stress-ng -d 4".
>
> Fixes: f1565c24b596 ("powerpc: use the generic dma_ops_bypass mode")
I think it'd be better to point the Fixes tag at 6a5c7be5e484, which
originally introduced the bug, even if we didn't notice it until
f1565c24b596 exposed it (or made it more likely).
cheers
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>
> The original code came Jun 24 2011:
> 6a5c7be5e484 ("powerpc: Override dma_get_required_mask by platform hook and ops")
>
>
> What is dma_get_required_mask() for anyway? What "requires" what here?
>
> Even though it works for now (due to huge - >4GB - default DMA window),
> I am still not convinced we do not want this chunk here
> (this is what f1565c24b596 removed):
>
> if (dev_is_pci(dev)) {
> u64 bypass_mask = dma_direct_get_required_mask(dev);
>
> if (dma_iommu_bypass_supported(dev, bypass_mask))
> return bypass_mask;
> }
> ---
> arch/powerpc/kernel/dma-iommu.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/dma-iommu.c b/arch/powerpc/kernel/dma-iommu.c
> index 569fecd7b5b2..9053fc9d20c7 100644
> --- a/arch/powerpc/kernel/dma-iommu.c
> +++ b/arch/powerpc/kernel/dma-iommu.c
> @@ -120,7 +120,8 @@ u64 dma_iommu_get_required_mask(struct device *dev)
> if (!tbl)
> return 0;
>
> - mask = 1ULL < (fls_long(tbl->it_offset + tbl->it_size) - 1);
> + mask = 1ULL << (fls_long(tbl->it_offset + tbl->it_size) +
> + tbl->it_page_shift - 1);
> mask += mask - 1;
>
> return mask;
> --
> 2.17.1
^ permalink raw reply
* Re: [PATCH kernel] powerpc/dma: Fix dma_map_ops::get_required_mask
From: Christoph Hellwig @ 2020-09-08 5:44 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Oliver O'Halloran, linuxppc-dev, Christoph Hellwig,
Cédric Le Goater
In-Reply-To: <20200908015106.79661-1-aik@ozlabs.ru>
On Tue, Sep 08, 2020 at 11:51:06AM +1000, Alexey Kardashevskiy wrote:
> What is dma_get_required_mask() for anyway? What "requires" what here?
Yes, it is a really odd API. It comes from classic old PCI where
64-bit addressing required an additional bus cycle, and various devices
had different addressing schemes, with the smaller addresses beeing
more efficient. So this allows the driver to request the "required"
addressing mode to address all memory. "preferred" might be a better
name as we'll bounce buffer if it isn't met. I also don't really see
why a driver would ever want to use it for a modern PCIe device.
^ permalink raw reply
* Re: [RFC PATCH v2 0/3] mm/gup: fix gup_fast with dynamic page table folding
From: Christophe Leroy @ 2020-09-08 5:22 UTC (permalink / raw)
To: Mike Rapoport, Gerald Schaefer
Cc: Peter Zijlstra, Catalin Marinas, Dave Hansen, linux-mm,
Paul Mackerras, linux-sparc, Alexander Gordeev, Claudio Imbrenda,
Will Deacon, linux-arch, linux-s390, Vasily Gorbik,
Jason Gunthorpe, Richard Weinberger, linux-x86, Russell King,
Christian Borntraeger, Ingo Molnar, Andrey Ryabinin, Jeff Dike,
Arnd Bergmann, John Hubbard, Heiko Carstens, linux-um,
Borislav Petkov, Andy Lutomirski, Thomas Gleixner, linux-arm,
Linus Torvalds, LKML, Andrew Morton, linux-power
In-Reply-To: <20200907201256.GC1976319@kernel.org>
Le 07/09/2020 à 22:12, Mike Rapoport a écrit :
> On Mon, Sep 07, 2020 at 08:00:55PM +0200, Gerald Schaefer wrote:
>> This is v2 of an RFC previously discussed here:
>> https://lore.kernel.org/lkml/20200828140314.8556-1-gerald.schaefer@linux.ibm.com/
>>
>> Patch 1 is a fix for a regression in gup_fast on s390, after our conversion
>> to common gup_fast code. It will introduce special helper functions
>> pXd_addr_end_folded(), which have to be used in places where pagetable walk
>> is done w/o lock and with READ_ONCE, so currently only in gup_fast.
>>
>> Patch 2 is an attempt to make that more generic, i.e. change pXd_addr_end()
>> themselves by adding an extra pXd value parameter. That was suggested by
>> Jason during v1 discussion, because he is already thinking of some other
>> places where he might want to switch to the READ_ONCE logic for pagetable
>> walks. In general, that would be the cleanest / safest solution, but there
>> is some impact on other architectures and common code, hence the new and
>> greatly enlarged recipient list.
>>
>> Patch 3 is a "nice to have" add-on, which makes pXd_addr_end() inline
>> functions instead of #defines, so that we get some type checking for the
>> new pXd value parameter.
>>
>> Not sure about Fixes/stable tags for the generic solution. Only patch 1
>> fixes a real bug on s390, and has Fixes/stable tags. Patches 2 + 3 might
>> still be nice to have in stable, to ease future backports, but I guess
>> "nice to have" does not really qualify for stable backports.
>
> I also think that adding pXd parameter to pXd_addr_end() is a cleaner
> way and with this patch 1 is not really required. I would even merge
> patches 2 and 3 into a single patch and use only it as the fix.
Why not merging patches 2 and 3, but I would keep patch 1 separate but
after the generic changes, so that we first do the generic changes, then
we do the specific S390 use of it.
Christophe
^ permalink raw reply
* Re: [RFC PATCH v2 3/3] mm: make generic pXd_addr_end() macros inline functions
From: Christophe Leroy @ 2020-09-08 5:19 UTC (permalink / raw)
To: Gerald Schaefer, Jason Gunthorpe, John Hubbard
Cc: Peter Zijlstra, Catalin Marinas, Dave Hansen, linux-mm,
Paul Mackerras, linux-sparc, Alexander Gordeev, Claudio Imbrenda,
Will Deacon, linux-arch, linux-s390, Vasily Gorbik,
Richard Weinberger, linux-x86, Russell King,
Christian Borntraeger, Ingo Molnar, Andrey Ryabinin, Jeff Dike,
Arnd Bergmann, Heiko Carstens, linux-um, Borislav Petkov,
Andy Lutomirski, Thomas Gleixner, linux-arm, Linus Torvalds, LKML,
Andrew Morton, linux-power, Mike Rapoport
In-Reply-To: <20200907180058.64880-4-gerald.schaefer@linux.ibm.com>
Le 07/09/2020 à 20:00, Gerald Schaefer a écrit :
> From: Alexander Gordeev <agordeev@linux.ibm.com>
>
> Since pXd_addr_end() macros take pXd page-table entry as a
> parameter it makes sense to check the entry type on compile.
> Even though most archs do not make use of page-table entries
> in pXd_addr_end() calls, checking the type in traversal code
> paths could help to avoid subtle bugs.
>
> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> Signed-off-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
> ---
> include/linux/pgtable.h | 36 ++++++++++++++++++++----------------
> 1 file changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index 67ebc22cf83d..d9e7d16c2263 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -656,31 +656,35 @@ static inline int arch_unmap_one(struct mm_struct *mm,
> */
>
> #ifndef pgd_addr_end
> -#define pgd_addr_end(pgd, addr, end) \
> -({ unsigned long __boundary = ((addr) + PGDIR_SIZE) & PGDIR_MASK; \
> - (__boundary - 1 < (end) - 1)? __boundary: (end); \
> -})
> +#define pgd_addr_end pgd_addr_end
I think that #define is pointless, usually there is no such #define for
the default case.
> +static inline unsigned long pgd_addr_end(pgd_t pgd, unsigned long addr, unsigned long end)
> +{ unsigned long __boundary = (addr + PGDIR_SIZE) & PGDIR_MASK;
> + return (__boundary - 1 < end - 1) ? __boundary : end;
> +}
Please use the standard layout, ie entry { and exit } alone on their
line, and space between local vars declaration and the rest.
Also remove the leading __ in front of var names as it's not needed once
it is not macros anymore.
f_name()
{
some_local_var;
do_something();
}
> #endif
>
> #ifndef p4d_addr_end
> -#define p4d_addr_end(p4d, addr, end) \
> -({ unsigned long __boundary = ((addr) + P4D_SIZE) & P4D_MASK; \
> - (__boundary - 1 < (end) - 1)? __boundary: (end); \
> -})
> +#define p4d_addr_end p4d_addr_end
> +static inline unsigned long p4d_addr_end(p4d_t p4d, unsigned long addr, unsigned long end)
> +{ unsigned long __boundary = (addr + P4D_SIZE) & P4D_MASK;
> + return (__boundary - 1 < end - 1) ? __boundary : end;
> +}
> #endif
>
> #ifndef pud_addr_end
> -#define pud_addr_end(pud, addr, end) \
> -({ unsigned long __boundary = ((addr) + PUD_SIZE) & PUD_MASK; \
> - (__boundary - 1 < (end) - 1)? __boundary: (end); \
> -})
> +#define pud_addr_end pud_addr_end
> +static inline unsigned long pud_addr_end(pud_t pud, unsigned long addr, unsigned long end)
> +{ unsigned long __boundary = (addr + PUD_SIZE) & PUD_MASK;
> + return (__boundary - 1 < end - 1) ? __boundary : end;
> +}
> #endif
>
> #ifndef pmd_addr_end
> -#define pmd_addr_end(pmd, addr, end) \
> -({ unsigned long __boundary = ((addr) + PMD_SIZE) & PMD_MASK; \
> - (__boundary - 1 < (end) - 1)? __boundary: (end); \
> -})
> +#define pmd_addr_end pmd_addr_end
> +static inline unsigned long pmd_addr_end(pmd_t pmd, unsigned long addr, unsigned long end)
> +{ unsigned long __boundary = (addr + PMD_SIZE) & PMD_MASK;
> + return (__boundary - 1 < end - 1) ? __boundary : end;
> +}
> #endif
>
> /*
>
^ permalink raw reply
* Re: [RFC PATCH v2 2/3] mm: make pXd_addr_end() functions page-table entry aware
From: Christophe Leroy @ 2020-09-08 5:14 UTC (permalink / raw)
To: Gerald Schaefer, Jason Gunthorpe, John Hubbard
Cc: Peter Zijlstra, Catalin Marinas, Dave Hansen, linux-mm,
Paul Mackerras, linux-sparc, Alexander Gordeev, Claudio Imbrenda,
Will Deacon, linux-arch, linux-s390, Vasily Gorbik,
Richard Weinberger, linux-x86, Russell King,
Christian Borntraeger, Ingo Molnar, Andrey Ryabinin, Jeff Dike,
Arnd Bergmann, Heiko Carstens, linux-um, Borislav Petkov,
Andy Lutomirski, Thomas Gleixner, linux-arm, Linus Torvalds, LKML,
Andrew Morton, linux-power, Mike Rapoport
In-Reply-To: <20200907180058.64880-3-gerald.schaefer@linux.ibm.com>
Le 07/09/2020 à 20:00, Gerald Schaefer a écrit :
> From: Alexander Gordeev <agordeev@linux.ibm.com>
>
> Unlike all other page-table abstractions pXd_addr_end() do not take
> into account a particular table entry in which context the functions
> are called. On architectures with dynamic page-tables folding that
> might lead to lack of necessary information that is difficult to
> obtain other than from the table entry itself. That already led to
> a subtle memory corruption issue on s390.
>
> By letting pXd_addr_end() functions know about the page-table entry
> we allow archs not only make extra checks, but also optimizations.
>
> As result of this change the pXd_addr_end_folded() functions used
> in gup_fast traversal code become unnecessary and get replaced with
> universal pXd_addr_end() variants.
>
> The arch-specific updates not only add dereferencing of page-table
> entry pointers, but also small changes to the code flow to make those
> dereferences possible, at least for x86 and powerpc. Also for arm64,
> but in way that should not have any impact.
>
[...]
>
> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> Signed-off-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
> ---
> arch/arm/include/asm/pgtable-2level.h | 2 +-
> arch/arm/mm/idmap.c | 6 ++--
> arch/arm/mm/mmu.c | 8 ++---
> arch/arm64/kernel/hibernate.c | 16 ++++++----
> arch/arm64/kvm/mmu.c | 16 +++++-----
> arch/arm64/mm/kasan_init.c | 8 ++---
> arch/arm64/mm/mmu.c | 25 +++++++--------
> arch/powerpc/mm/book3s64/radix_pgtable.c | 7 ++---
> arch/powerpc/mm/hugetlbpage.c | 6 ++--
You forgot arch/powerpc/mm/book3s64/subpage_prot.c it seems.
> arch/s390/include/asm/pgtable.h | 8 ++---
> arch/s390/mm/page-states.c | 8 ++---
> arch/s390/mm/pageattr.c | 8 ++---
> arch/s390/mm/vmem.c | 8 ++---
> arch/sparc/mm/hugetlbpage.c | 6 ++--
> arch/um/kernel/tlb.c | 8 ++---
> arch/x86/mm/init_64.c | 15 ++++-----
> arch/x86/mm/kasan_init_64.c | 16 +++++-----
> include/asm-generic/pgtable-nop4d.h | 2 +-
> include/asm-generic/pgtable-nopmd.h | 2 +-
> include/asm-generic/pgtable-nopud.h | 2 +-
> include/linux/pgtable.h | 26 ++++-----------
> mm/gup.c | 8 ++---
> mm/ioremap.c | 8 ++---
> mm/kasan/init.c | 17 +++++-----
> mm/madvise.c | 4 +--
> mm/memory.c | 40 ++++++++++++------------
> mm/mlock.c | 18 ++++++++---
> mm/mprotect.c | 8 ++---
> mm/pagewalk.c | 8 ++---
> mm/swapfile.c | 8 ++---
> mm/vmalloc.c | 16 +++++-----
> 31 files changed, 165 insertions(+), 173 deletions(-)
Christophe
^ permalink raw reply
* Re: [RFC PATCH v2 1/3] mm/gup: fix gup_fast with dynamic page table folding
From: Christophe Leroy @ 2020-09-08 5:06 UTC (permalink / raw)
To: Gerald Schaefer, Jason Gunthorpe, John Hubbard
Cc: Peter Zijlstra, Catalin Marinas, Dave Hansen, linux-mm,
Paul Mackerras, linux-sparc, Alexander Gordeev, Claudio Imbrenda,
Will Deacon, linux-arch, linux-s390, Vasily Gorbik,
Richard Weinberger, linux-x86, Russell King,
Christian Borntraeger, Ingo Molnar, Andrey Ryabinin, Jeff Dike,
Arnd Bergmann, Heiko Carstens, linux-um, Borislav Petkov,
Andy Lutomirski, Thomas Gleixner, linux-arm, Linus Torvalds, LKML,
Andrew Morton, linux-power, Mike Rapoport
In-Reply-To: <20200907180058.64880-2-gerald.schaefer@linux.ibm.com>
Le 07/09/2020 à 20:00, Gerald Schaefer a écrit :
> From: Alexander Gordeev <agordeev@linux.ibm.com>
>
> Commit 1a42010cdc26 ("s390/mm: convert to the generic get_user_pages_fast
> code") introduced a subtle but severe bug on s390 with gup_fast, due to
> dynamic page table folding.
>
> The question "What would it require for the generic code to work for s390"
> has already been discussed here
> https://lkml.kernel.org/r/20190418100218.0a4afd51@mschwideX1
> and ended with a promising approach here
> https://lkml.kernel.org/r/20190419153307.4f2911b5@mschwideX1
> which in the end unfortunately didn't quite work completely.
>
> We tried to mimic static level folding by changing pgd_offset to always
> calculate top level page table offset, and do nothing in folded pXd_offset.
> What has been overlooked is that PxD_SIZE/MASK and thus pXd_addr_end do
> not reflect this dynamic behaviour, and still act like static 5-level
> page tables.
>
[...]
>
> Fix this by introducing new pXd_addr_end_folded helpers, which take an
> additional pXd entry value parameter, that can be used on s390
> to determine the correct page table level and return corresponding
> end / boundary. With that, the pointer iteration will always
> happen in gup_pgd_range for s390. No change for other architectures
> introduced.
Not sure pXd_addr_end_folded() is the best understandable name,
allthough I don't have any alternative suggestion at the moment.
Maybe could be something like pXd_addr_end_fixup() as it will disappear
in the next patch, or pXd_addr_end_gup() ?
Also, if it happens to be acceptable to get patch 2 in stable, I think
you should switch patch 1 and patch 2 to avoid the step through
pXd_addr_end_folded()
>
> Fixes: 1a42010cdc26 ("s390/mm: convert to the generic get_user_pages_fast code")
> Cc: <stable@vger.kernel.org> # 5.2+
> Reviewed-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> Signed-off-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
> ---
> arch/s390/include/asm/pgtable.h | 42 +++++++++++++++++++++++++++++++++
> include/linux/pgtable.h | 16 +++++++++++++
> mm/gup.c | 8 +++----
> 3 files changed, 62 insertions(+), 4 deletions(-)
>
> diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
> index 7eb01a5459cd..027206e4959d 100644
> --- a/arch/s390/include/asm/pgtable.h
> +++ b/arch/s390/include/asm/pgtable.h
> @@ -512,6 +512,48 @@ static inline bool mm_pmd_folded(struct mm_struct *mm)
> }
> #define mm_pmd_folded(mm) mm_pmd_folded(mm)
>
> +/*
> + * With dynamic page table levels on s390, the static pXd_addr_end() functions
> + * will not return corresponding dynamic boundaries. This is no problem as long
> + * as only pXd pointers are passed down during page table walk, because
> + * pXd_offset() will simply return the given pointer for folded levels, and the
> + * pointer iteration over a range simply happens at the correct page table
> + * level.
> + * It is however a problem with gup_fast, or other places walking the page
> + * tables w/o locks using READ_ONCE(), and passing down the pXd values instead
> + * of pointers. In this case, the pointer given to pXd_offset() is a pointer to
> + * a stack variable, which cannot be used for pointer iteration at the correct
> + * level. Instead, the iteration then has to happen by going up to pgd level
> + * again. To allow this, provide pXd_addr_end_folded() functions with an
> + * additional pXd value parameter, which can be used on s390 to determine the
> + * folding level and return the corresponding boundary.
> + */
> +static inline unsigned long rste_addr_end_folded(unsigned long rste, unsigned long addr, unsigned long end)
What does 'rste' stands for ?
Isn't this line a bit long ?
> +{
> + unsigned long type = (rste & _REGION_ENTRY_TYPE_MASK) >> 2;
> + unsigned long size = 1UL << (_SEGMENT_SHIFT + type * 11);
> + unsigned long boundary = (addr + size) & ~(size - 1);
> +
> + /*
> + * FIXME The below check is for internal testing only, to be removed
> + */
> + VM_BUG_ON(type < (_REGION_ENTRY_TYPE_R3 >> 2));
> +
> + return (boundary - 1) < (end - 1) ? boundary : end;
> +}
> +
> +#define pgd_addr_end_folded pgd_addr_end_folded
> +static inline unsigned long pgd_addr_end_folded(pgd_t pgd, unsigned long addr, unsigned long end)
> +{
> + return rste_addr_end_folded(pgd_val(pgd), addr, end);
> +}
> +
> +#define p4d_addr_end_folded p4d_addr_end_folded
> +static inline unsigned long p4d_addr_end_folded(p4d_t p4d, unsigned long addr, unsigned long end)
> +{
> + return rste_addr_end_folded(p4d_val(p4d), addr, end);
> +}
> +
> static inline int mm_has_pgste(struct mm_struct *mm)
> {
> #ifdef CONFIG_PGSTE
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index e8cbc2e795d5..981c4c2a31fe 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -681,6 +681,22 @@ static inline int arch_unmap_one(struct mm_struct *mm,
> })
> #endif
>
> +#ifndef pgd_addr_end_folded
> +#define pgd_addr_end_folded(pgd, addr, end) pgd_addr_end(addr, end)
> +#endif
> +
> +#ifndef p4d_addr_end_folded
> +#define p4d_addr_end_folded(p4d, addr, end) p4d_addr_end(addr, end)
> +#endif
> +
> +#ifndef pud_addr_end_folded
> +#define pud_addr_end_folded(pud, addr, end) pud_addr_end(addr, end)
> +#endif
> +
> +#ifndef pmd_addr_end_folded
> +#define pmd_addr_end_folded(pmd, addr, end) pmd_addr_end(addr, end)
> +#endif
> +
> /*
> * When walking page tables, we usually want to skip any p?d_none entries;
> * and any p?d_bad entries - reporting the error before resetting to none.
> diff --git a/mm/gup.c b/mm/gup.c
> index bd883a112724..ba4aace5d0f4 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2521,7 +2521,7 @@ static int gup_pmd_range(pud_t pud, unsigned long addr, unsigned long end,
> do {
> pmd_t pmd = READ_ONCE(*pmdp);
>
> - next = pmd_addr_end(addr, end);
> + next = pmd_addr_end_folded(pmd, addr, end);
> if (!pmd_present(pmd))
> return 0;
>
> @@ -2564,7 +2564,7 @@ static int gup_pud_range(p4d_t p4d, unsigned long addr, unsigned long end,
> do {
> pud_t pud = READ_ONCE(*pudp);
>
> - next = pud_addr_end(addr, end);
> + next = pud_addr_end_folded(pud, addr, end);
> if (unlikely(!pud_present(pud)))
> return 0;
> if (unlikely(pud_huge(pud))) {
> @@ -2592,7 +2592,7 @@ static int gup_p4d_range(pgd_t pgd, unsigned long addr, unsigned long end,
> do {
> p4d_t p4d = READ_ONCE(*p4dp);
>
> - next = p4d_addr_end(addr, end);
> + next = p4d_addr_end_folded(p4d, addr, end);
> if (p4d_none(p4d))
> return 0;
> BUILD_BUG_ON(p4d_huge(p4d));
> @@ -2617,7 +2617,7 @@ static void gup_pgd_range(unsigned long addr, unsigned long end,
> do {
> pgd_t pgd = READ_ONCE(*pgdp);
>
> - next = pgd_addr_end(addr, end);
> + next = pgd_addr_end_folded(pgd, addr, end);
> if (pgd_none(pgd))
> return;
> if (unlikely(pgd_huge(pgd))) {
>
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