* [Bug 205099] KASAN hit at raid6_pq: BUG: Unable to handle kernel data access at 0x00f0fd0d
From: bugzilla-daemon @ 2021-11-26 9:14 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-205099-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=205099
--- Comment #38 from Christophe Leroy (christophe.leroy@csgroup.eu) ---
Looks like only x86 are arm implement this vmalloc= parameter:
[chleroy@PO20335 linux-powerpc]$ git grep 'early_param("vmalloc"' arch/
arch/arm/mm/mmu.c:early_param("vmalloc", early_vmalloc);
arch/x86/mm/pgtable_32.c:early_param("vmalloc", parse_vmalloc);
However, your vmalloc area has a size of 65M:
Kernel virtual memory layout:
* 0xf6000000..0xfec00000 : kasan shadow mem
* 0xf5bbf000..0xf5fff000 : fixmap
* 0xf5400000..0xf5800000 : highmem PTEs
* 0xf5115000..0xf5400000 : early ioremap
* 0xf1000000..0xf5110000 : vmalloc & ioremap
* 0xb0000000..0xc0000000 : modules
Memory: 1928984K/2097152K available (22288K kernel code, 2616K rwdata, 4868K
rodata, 1408K init, 8981K bss, 168168K reserved, 0K cma-reserved, 1310720K
highmem)
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [Bug 205099] KASAN hit at raid6_pq: BUG: Unable to handle kernel data access at 0x00f0fd0d
From: bugzilla-daemon @ 2021-11-26 9:20 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-205099-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=205099
--- Comment #39 from Christophe Leroy (christophe.leroy@csgroup.eu) ---
Can you retry with CONFIG_LOWMEM_SIZE=0x28000000 or
CONFIG_LOWMEM_SIZE=0x20000000 ?
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [PATCH 1/3] powerpc/code-patching: work around code patching verification in patching tests
From: Nicholas Piggin @ 2021-11-26 10:27 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <f9e53405-b5b9-15d1-eaf9-0616a5b87424@csgroup.eu>
Excerpts from Christophe Leroy's message of November 26, 2021 4:34 pm:
>
>
> Le 26/11/2021 à 04:22, Nicholas Piggin a écrit :
>> Code patching tests patch the stack and (non-module) vmalloc space now,
>> which falls afoul of the new address check.
>>
>> The stack patching can easily be fixed, but the vmalloc patching is more
>> difficult. For now, add an ugly workaround to skip the check while the
>> test code is running.
>
> This really looks hacky.
>
> To skip the test, you can call do_patch_instruction() instead of calling
> patch_instruction().
And make a do_patch_branch function. I thought about it, and thought
this is sligtly easier.
Thanks,
Nick
^ permalink raw reply
* [PATCH] powerpc/ptdump: Fix display a BAT's size unit
From: Christophe Leroy @ 2021-11-26 10:30 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
We have wrong units on BAT's sizes (G instead of M, M instead of ...)
---[ Instruction Block Address Translation ]---
0: 0xc0000000-0xc03fffff 0x00000000 4G Kernel x m
1: 0xc0400000-0xc05fffff 0x00400000 2G Kernel x m
2: 0xc0600000-0xc06fffff 0x00600000 1G Kernel x m
3: 0xc0700000-0xc077ffff 0x00700000 512M Kernel x m
4: 0xc0780000-0xc079ffff 0x00780000 128M Kernel x m
5: 0xc07a0000-0xc07bffff 0x007a0000 128M Kernel x m
6: -
7: -
This is because pt_dump_size() expects a size in Kbytes but
bat_show_603() gives the size in bytes.
To avoid risk of confusion, change pt_dump_size() to take bytes.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/mm/ptdump/ptdump.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/mm/ptdump/ptdump.c b/arch/powerpc/mm/ptdump/ptdump.c
index bf251191e78d..031956d0ee84 100644
--- a/arch/powerpc/mm/ptdump/ptdump.c
+++ b/arch/powerpc/mm/ptdump/ptdump.c
@@ -123,7 +123,7 @@ static struct ptdump_range ptdump_range[] __ro_after_init = {
void pt_dump_size(struct seq_file *m, unsigned long size)
{
- static const char units[] = "KMGTPE";
+ static const char units[] = " KMGTPE";
const char *unit = units;
/* Work out what appropriate unit to use */
@@ -176,7 +176,7 @@ static void dump_addr(struct pg_state *st, unsigned long addr)
pt_dump_seq_printf(st->seq, REG "-" REG " ", st->start_address, addr - 1);
pt_dump_seq_printf(st->seq, " " REG " ", st->start_pa);
- pt_dump_size(st->seq, (addr - st->start_address) >> 10);
+ pt_dump_size(st->seq, addr - st->start_address);
}
static void note_prot_wx(struct pg_state *st, unsigned long addr)
--
2.33.1
^ permalink raw reply related
* Re: [PATCH] powerpc/pseries/vas: Don't print an error when VAS is unavailable
From: Nicholas Piggin @ 2021-11-26 10:31 UTC (permalink / raw)
To: Cédric Le Goater, linuxppc-dev; +Cc: Haren Myneni
In-Reply-To: <43d21c1a-9122-d698-2229-e56c77a91313@kaod.org>
Excerpts from Cédric Le Goater's message of November 26, 2021 5:13 pm:
> On 11/26/21 06:21, Nicholas Piggin wrote:
>> KVM does not support VAS so guests always print a useless error on boot
>>
>> vas: HCALL(398) error -2, query_type 0, result buffer 0x57f2000
>>
>> Change this to only print the message if the error is not H_FUNCTION.
>
>
> Just being curious, why is it even called since "ibm,compression" should
> not be exposed in the DT ?
It looks like vas does not test for it. I guess in theory there can be
other functions than compression implemented as an accelerator. Maybe
that's why?
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 1/3] powerpc/code-patching: work around code patching verification in patching tests
From: Christophe Leroy @ 2021-11-26 10:39 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Sachin Sant
In-Reply-To: <1637922388.sidg5s6in4.astroid@bobo.none>
Le 26/11/2021 à 11:27, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of November 26, 2021 4:34 pm:
>>
>>
>> Le 26/11/2021 à 04:22, Nicholas Piggin a écrit :
>>> Code patching tests patch the stack and (non-module) vmalloc space now,
>>> which falls afoul of the new address check.
>>>
>>> The stack patching can easily be fixed, but the vmalloc patching is more
>>> difficult. For now, add an ugly workaround to skip the check while the
>>> test code is running.
>>
>> This really looks hacky.
>>
>> To skip the test, you can call do_patch_instruction() instead of calling
>> patch_instruction().
>
> And make a do_patch_branch function. I thought about it, and thought
> this is sligtly easier.
>
Anyway, as reported by Sachin the ftrace code also trips in the new
verification. So I have submitted a patch to revert to the previous
level of verification.
Then we can fix all this properly without going through a temporary hack
and activate the verification again once every caller is fixed.
I was not able to reproduce Sachin's problem on PPC32. Could it be
specific to PPC64 ?
Christophe
^ permalink raw reply
* Re: [PATCH] powerpc/64s/radix: Fix unmapping huge vmaps when CONFIG_HUGETLB_PAGE=n
From: Nicholas Piggin @ 2021-11-26 10:55 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev
In-Reply-To: <87tufzzbwh.fsf@linkitivity.dja.id.au>
Excerpts from Daniel Axtens's message of November 26, 2021 4:09 pm:
> Hi,
>
>> pmd_huge is defined out to false when HUGETLB_PAGE is not configured,
>> but the vmap code still installs huge PMDs. This leads to errors
>> encountering bad PMDs when vunmapping because it is not seen as a
>> huge PTE, and the bad PMD check catches it. The end result may not
>> be much more serious than some bad pmd warning messages, because the
>> pmd_none_or_clear_bad() does what we wanted and clears the huge PTE
>> anyway.
>
> Huh. So vmap seems to key off arch_vmap_p?d_supported which checks for
> radix and HAVE_ARCH_HUGE_VMAP.
>
>> Fix this by checking pmd_is_leaf(), which checks for a PTE regardless
>> of config options. The whole huge/large/leaf stuff is a tangled mess
>> but that's kernel-wide and not something we can improve much in
>> arch/powerpc code.
>
> I guess I'm a bit late to the party here because p?d_is_leaf was added
> in 2019 in commit d6eacedd1f0e ("powerpc/book3s: Use config independent
> helpers for page table walk") but why wouldn't we just make pmd_huge()
> not config dependent?
I guess so it constant folds code if hugetlbfs is not configured
(and maybe so !huge kernels would correctly print a bad PMD warning if
they got huge PMD in user mappings).
>
> Also, looking at that commit, there are a few places that might still
> throw warnings, e.g. find_linux_pte, find_current_mm_pte, pud_page which
> seem like they might still throw warnings if they were to encounter a
> huge vmap page:
>
> struct page *pud_page(pud_t pud)
> {
> if (pud_is_leaf(pud)) {
> VM_WARN_ON(!pud_huge(pud));
Oh, hmm. That is used in vmalloc.c so maybe that warning should be
removed as a false positive. Good catch.
> Do these functions need special treatment for huge vmappings()?
find_linux_pte etc could be called for vmaps. I'm not sure I see a
problem in that function.
Thanks,
Nick
>
> Apart from those questions, the patch itself makes sense to me and I can
> follow how it would fix a problem.
>
> Reviewed-by: Daniel Axtens <dja@axtens.net>
>
> Kind regards,
> Daniel
>
^ permalink raw reply
* [Bug 205099] KASAN hit at raid6_pq: BUG: Unable to handle kernel data access at 0x00f0fd0d
From: bugzilla-daemon @ 2021-11-26 11:33 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-205099-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=205099
--- Comment #40 from Christophe Leroy (christophe.leroy@csgroup.eu) ---
Would also be great if you can activate CONFIG_PTDUMP_DEBUGFS and provide the
content of /sys/kernel/debug/kernel_page_tables
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [PATCH] powerpc/32s: Allocate one 256k IBAT instead of two consecutives 128k IBATs
From: Christophe Leroy @ 2021-11-26 12:40 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
Today we have the following IBATs allocated:
---[ Instruction Block Address Translation ]---
0: 0xc0000000-0xc03fffff 0x00000000 4M Kernel x m
1: 0xc0400000-0xc05fffff 0x00400000 2M Kernel x m
2: 0xc0600000-0xc06fffff 0x00600000 1M Kernel x m
3: 0xc0700000-0xc077ffff 0x00700000 512K Kernel x m
4: 0xc0780000-0xc079ffff 0x00780000 128K Kernel x m
5: 0xc07a0000-0xc07bffff 0x007a0000 128K Kernel x m
6: -
7: -
The two 128K should be a single 256K instead.
When _etext is not aligned to 128Kbytes, the system will allocate
all necessary BATs to the lower 128Kbytes boundary, then allocate
an additional 128Kbytes BAT for the remaining block.
Instead, align the top to 128Kbytes so that the function directly
allocates a 256Mbytes last block:
---[ Instruction Block Address Translation ]---
0: 0xc0000000-0xc03fffff 0x00000000 4M Kernel x m
1: 0xc0400000-0xc05fffff 0x00400000 2M Kernel x m
2: 0xc0600000-0xc06fffff 0x00600000 1M Kernel x m
3: 0xc0700000-0xc077ffff 0x00700000 512K Kernel x m
4: 0xc0780000-0xc07bffff 0x00780000 256K Kernel x m
5: -
6: -
7: -
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/mm/book3s32/mmu.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/mm/book3s32/mmu.c b/arch/powerpc/mm/book3s32/mmu.c
index 27061583a010..33ab63d56435 100644
--- a/arch/powerpc/mm/book3s32/mmu.c
+++ b/arch/powerpc/mm/book3s32/mmu.c
@@ -196,18 +196,17 @@ void mmu_mark_initmem_nx(void)
int nb = mmu_has_feature(MMU_FTR_USE_HIGH_BATS) ? 8 : 4;
int i;
unsigned long base = (unsigned long)_stext - PAGE_OFFSET;
- unsigned long top = (unsigned long)_etext - PAGE_OFFSET;
+ unsigned long top = ALIGN((unsigned long)_etext - PAGE_OFFSET, SZ_128K);
unsigned long border = (unsigned long)__init_begin - PAGE_OFFSET;
unsigned long size;
- for (i = 0; i < nb - 1 && base < top && top - base > (128 << 10);) {
+ for (i = 0; i < nb - 1 && base < top;) {
size = block_size(base, top);
setibat(i++, PAGE_OFFSET + base, base, size, PAGE_KERNEL_TEXT);
base += size;
}
if (base < top) {
size = block_size(base, top);
- size = max(size, 128UL << 10);
if ((top - base) > size) {
size <<= 1;
if (strict_kernel_rwx_enabled() && base + size > border)
--
2.33.1
^ permalink raw reply related
* Re: [PATCH] powerpc: mm: radix_tlb: rearrange the if-else block
From: Christophe Leroy @ 2021-11-26 13:43 UTC (permalink / raw)
To: Anders Roxell, benh, paulus
Cc: Arnd Bergmann, llvm, ndesaulniers, linux-kernel, nathan,
linuxppc-dev
In-Reply-To: <20211125154406.470082-1-anders.roxell@linaro.org>
Le 25/11/2021 à 16:44, Anders Roxell a écrit :
> Clang warns:
>
> arch/powerpc/mm/book3s64/radix_tlb.c:1191:23: error: variable 'hstart' is uninitialized when used here [-Werror,-Wuninitialized]
> __tlbiel_va_range(hstart, hend, pid,
> ^~~~~~
> arch/powerpc/mm/book3s64/radix_tlb.c:1175:23: note: initialize the variable 'hstart' to silence this warning
> unsigned long hstart, hend;
> ^
> = 0
> arch/powerpc/mm/book3s64/radix_tlb.c:1191:31: error: variable 'hend' is uninitialized when used here [-Werror,-Wuninitialized]
> __tlbiel_va_range(hstart, hend, pid,
> ^~~~
> arch/powerpc/mm/book3s64/radix_tlb.c:1175:29: note: initialize the variable 'hend' to silence this warning
> unsigned long hstart, hend;
> ^
> = 0
> 2 errors generated.
>
> Rework the if-else to pull the 'IS_ENABLE(CONFIG_TRANSPARENT_HUGEPAGE)'
> check one level up, this will silent the warnings. That will also
> simplify the 'else' path. Clang is getting confused with these warnings,
> but the warnings is a false-positive.
But you are duplicating a significant part of the code by doing that,
and duplicated code generaly leads to bugs.
And we already have redundant stuff between FLUSH_TYPE_LOCAL leg and
cputlb_use_tlbie() leg.
Can't you fix CLANG instead :) ?
Or just add an else to the IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) that
sets hstart and hend to 0 ?
Or just put hstart and hend calculation outside the IS_ENABLED() ? After
all GCC should drop the calculation when not used.
>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> ---
> arch/powerpc/mm/book3s64/radix_tlb.c | 31 +++++++++++++++++++++-------
> 1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/mm/book3s64/radix_tlb.c b/arch/powerpc/mm/book3s64/radix_tlb.c
> index 7724af19ed7e..e494a45ce1b4 100644
> --- a/arch/powerpc/mm/book3s64/radix_tlb.c
> +++ b/arch/powerpc/mm/book3s64/radix_tlb.c
> @@ -1170,16 +1170,14 @@ static inline void __radix__flush_tlb_range(struct mm_struct *mm,
> _tlbiel_pid_multicast(mm, pid, RIC_FLUSH_ALL);
> }
> }
> - } else {
> + } else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
> bool hflush = false;
> unsigned long hstart, hend;
>
> - if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
> - hstart = (start + PMD_SIZE - 1) & PMD_MASK;
> - hend = end & PMD_MASK;
> - if (hstart < hend)
> - hflush = true;
> - }
> + hstart = (start + PMD_SIZE - 1) & PMD_MASK;
> + hend = end & PMD_MASK;
> + if (hstart < hend)
> + hflush = true;
>
> if (type == FLUSH_TYPE_LOCAL) {
> asm volatile("ptesync": : :"memory");
> @@ -1207,6 +1205,25 @@ static inline void __radix__flush_tlb_range(struct mm_struct *mm,
> _tlbiel_va_range_multicast(mm,
> hstart, hend, pid, PMD_SIZE, MMU_PAGE_2M, flush_pwc);
> }
> + } else {
> +
> + if (type == FLUSH_TYPE_LOCAL) {
> + asm volatile("ptesync" : : : "memory");
> + if (flush_pwc)
> + /* For PWC, only one flush is needed */
> + __tlbiel_pid(pid, 0, RIC_FLUSH_PWC);
> + __tlbiel_va_range(start, end, pid, page_size, mmu_virtual_psize);
> + ppc_after_tlbiel_barrier();
> + } else if (cputlb_use_tlbie()) {
> + asm volatile("ptesync" : : : "memory");
> + if (flush_pwc)
> + __tlbie_pid(pid, RIC_FLUSH_PWC);
> + __tlbie_va_range(start, end, pid, page_size, mmu_virtual_psize);
> + asm volatile("eieio; tlbsync; ptesync" : : : "memory");
> + } else {
> + _tlbiel_va_range_multicast(mm,
> + start, end, pid, page_size, mmu_virtual_psize, flush_pwc);
> + }
> }
> out:
> preempt_enable();
>
^ permalink raw reply
* [powerpc:merge] BUILD SUCCESS 2dbc3a3e8fc1ea24589150a874cd37904898286a
From: kernel test robot @ 2021-11-26 13:47 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git merge
branch HEAD: 2dbc3a3e8fc1ea24589150a874cd37904898286a Automatic merge of 'next' into merge (2021-11-25 21:55)
elapsed time: 1569m
configs tested: 54
configs skipped: 3
The following configs have been built successfully.
More configs may be tested in the coming days.
gcc tested configs:
arm defconfig
arm allyesconfig
arm allmodconfig
arm64 allyesconfig
arm64 defconfig
i386 randconfig-c001-20211125
ia64 defconfig
ia64 allmodconfig
ia64 allyesconfig
m68k allmodconfig
m68k defconfig
m68k allyesconfig
nios2 defconfig
arc allyesconfig
nds32 allnoconfig
csky defconfig
alpha defconfig
nds32 defconfig
alpha allyesconfig
nios2 allyesconfig
arc defconfig
sh allmodconfig
h8300 allyesconfig
xtensa allyesconfig
parisc defconfig
s390 allyesconfig
s390 allmodconfig
parisc allyesconfig
s390 defconfig
i386 allyesconfig
sparc allyesconfig
sparc defconfig
i386 defconfig
i386 debian-10.3
mips allyesconfig
mips allmodconfig
powerpc allyesconfig
powerpc allmodconfig
powerpc allnoconfig
riscv nommu_k210_defconfig
riscv allyesconfig
riscv nommu_virt_defconfig
riscv allnoconfig
riscv defconfig
riscv rv32_defconfig
riscv allmodconfig
um i386_defconfig
um x86_64_defconfig
x86_64 allyesconfig
x86_64 defconfig
x86_64 rhel-8.3
x86_64 kexec
x86_64 rhel-8.3-func
x86_64 rhel-8.3-kselftests
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply
* Re: [PATCH] powerpc: mm: radix_tlb: rearrange the if-else block
From: Arnd Bergmann @ 2021-11-26 13:59 UTC (permalink / raw)
To: Christophe Leroy
Cc: Anders Roxell, Arnd Bergmann, llvm, Nick Desaulniers,
Linux Kernel Mailing List, Nathan Chancellor, Paul Mackerras,
linuxppc-dev
In-Reply-To: <6b1e51a8-2f4d-2024-df90-a35c926d7a30@csgroup.eu>
On Fri, Nov 26, 2021 at 2:43 PM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
> Le 25/11/2021 à 16:44, Anders Roxell a écrit :
> Can't you fix CLANG instead :) ?
>
> Or just add an else to the IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) that
> sets hstart and hend to 0 ?
That doesn't sound any less risky than duplicating the code, it can lead to
incorrect changes just as easily if a patch ends up actually flushing at the
wrong address, and the compiler fails to complain because of the bogus
initialization.
> Or just put hstart and hend calculation outside the IS_ENABLED() ? After
> all GCC should drop the calculation when not used.
I like this one. I'm still unsure how clang can get so confused about whether
the variables are initialized or not, usually it handles this much better than
gcc. My best guess is that one of the memory clobbers makes it conclude
that 'hflush' can be true when it gets written to by an inline asm.
Arnd
^ permalink raw reply
* Re: [PATCH] powerpc: mm: radix_tlb: rearrange the if-else block
From: Nathan Chancellor @ 2021-11-26 15:46 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Anders Roxell, llvm, Nick Desaulniers, Linux Kernel Mailing List,
Paul Mackerras, linuxppc-dev
In-Reply-To: <CAK8P3a0n_n+PnfYmAdS9923yheLqYXRp8=65hKf9abLCRAX8ig@mail.gmail.com>
On Fri, Nov 26, 2021 at 02:59:29PM +0100, Arnd Bergmann wrote:
> On Fri, Nov 26, 2021 at 2:43 PM Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
> > Le 25/11/2021 à 16:44, Anders Roxell a écrit :
> > Can't you fix CLANG instead :) ?
> >
> > Or just add an else to the IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) that
> > sets hstart and hend to 0 ?
>
> That doesn't sound any less risky than duplicating the code, it can lead to
> incorrect changes just as easily if a patch ends up actually flushing at the
> wrong address, and the compiler fails to complain because of the bogus
> initialization.
>
> > Or just put hstart and hend calculation outside the IS_ENABLED() ? After
> > all GCC should drop the calculation when not used.
>
> I like this one. I'm still unsure how clang can get so confused about whether
> the variables are initialized or not, usually it handles this much better than
> gcc. My best guess is that one of the memory clobbers makes it conclude
> that 'hflush' can be true when it gets written to by an inline asm.
As far as I am aware, clang's analysis does not evaluate variables when
generating a control flow graph and using that for static analysis:
https://godbolt.org/z/PdGxoq9j7
Based on the control flow graph, it knows that hstart and hend are
uninitialized because IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) gets
expanded to 0 by the preprocessor but it does not seem like it can piece
together that hflush's value of false is only changed to true under the
now 'if (0) {' branch, meaning that all the calls to __tlbiel_va_range()
never get evaluated. That may or may not be easy to fix in clang but we
run into issues like this so infrequently.
At any rate, the below diff works for me.
Cheers,
Nathan
diff --git a/arch/powerpc/mm/book3s64/radix_tlb.c b/arch/powerpc/mm/book3s64/radix_tlb.c
index 7724af19ed7e..156a631df976 100644
--- a/arch/powerpc/mm/book3s64/radix_tlb.c
+++ b/arch/powerpc/mm/book3s64/radix_tlb.c
@@ -1174,12 +1174,10 @@ static inline void __radix__flush_tlb_range(struct mm_struct *mm,
bool hflush = false;
unsigned long hstart, hend;
- if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
- hstart = (start + PMD_SIZE - 1) & PMD_MASK;
- hend = end & PMD_MASK;
- if (hstart < hend)
- hflush = true;
- }
+ hstart = (start + PMD_SIZE - 1) & PMD_MASK;
+ hend = end & PMD_MASK;
+ if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && hstart < hend)
+ hflush = true;
if (type == FLUSH_TYPE_LOCAL) {
asm volatile("ptesync": : :"memory");
^ permalink raw reply related
* Re: [PATCH] w1: Misuse of get_user()/put_user() reported by sparse
From: Greg Kroah-Hartman @ 2021-11-26 16:00 UTC (permalink / raw)
To: Christophe Leroy
Cc: Evgeniy Polyakov, linuxppc-dev, linux-kernel, kernel test robot
In-Reply-To: <2163689da6544c289254b3c69848acc36db998f5.1637313047.git.christophe.leroy@csgroup.eu>
On Fri, Nov 19, 2021 at 10:15:09AM +0100, Christophe Leroy wrote:
> sparse warnings: (new ones prefixed by >>)
> >> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected char [noderef] __user *_pu_addr @@ got char *buf @@
> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: expected char [noderef] __user *_pu_addr
> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: got char *buf
> >> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected char const [noderef] __user *_gu_addr @@ got char const *buf @@
> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: expected char const [noderef] __user *_gu_addr
> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: got char const *buf
>
> The buffer buf is a failsafe buffer in kernel space, it's not user
> memory hence doesn't deserve the use of get_user() or put_user().
>
> Access 'buf' content directly.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Link: https://lore.kernel.org/lkml/202111190526.K5vb7NWC-lkp@intel.com/T/
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> drivers/w1/slaves/w1_ds28e04.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/w1/slaves/w1_ds28e04.c b/drivers/w1/slaves/w1_ds28e04.c
> index e4f336111edc..d75bb16fb7a1 100644
> --- a/drivers/w1/slaves/w1_ds28e04.c
> +++ b/drivers/w1/slaves/w1_ds28e04.c
> @@ -339,10 +339,7 @@ static BIN_ATTR_RW(pio, 1);
> static ssize_t crccheck_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> - if (put_user(w1_enable_crccheck + 0x30, buf))
> - return -EFAULT;
> -
> - return sizeof(w1_enable_crccheck);
> + return sprintf(buf, "%d", w1_enable_crccheck);
This should be sysfs_emit(), right?
> }
>
> static ssize_t crccheck_store(struct device *dev, struct device_attribute *attr,
> @@ -353,11 +350,8 @@ static ssize_t crccheck_store(struct device *dev, struct device_attribute *attr,
> if (count != 1 || !buf)
> return -EINVAL;
>
> - if (get_user(val, buf))
> - return -EFAULT;
> -
> /* convert to decimal */
> - val = val - 0x30;
> + val = *buf - 0x30;
Why not use a proper function that can parse a string and turn it into a
number?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] w1: Misuse of get_user()/put_user() reported by sparse
From: Christophe Leroy @ 2021-11-26 16:10 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Evgeniy Polyakov, linuxppc-dev, linux-kernel, kernel test robot
In-Reply-To: <YaEEqeKyWPfUP7vM@kroah.com>
Le 26/11/2021 à 17:00, Greg Kroah-Hartman a écrit :
> On Fri, Nov 19, 2021 at 10:15:09AM +0100, Christophe Leroy wrote:
>> sparse warnings: (new ones prefixed by >>)
>>>> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected char [noderef] __user *_pu_addr @@ got char *buf @@
>> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: expected char [noderef] __user *_pu_addr
>> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: got char *buf
>>>> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected char const [noderef] __user *_gu_addr @@ got char const *buf @@
>> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: expected char const [noderef] __user *_gu_addr
>> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: got char const *buf
>>
>> The buffer buf is a failsafe buffer in kernel space, it's not user
>> memory hence doesn't deserve the use of get_user() or put_user().
>>
>> Access 'buf' content directly.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Link: https://lore.kernel.org/lkml/202111190526.K5vb7NWC-lkp@intel.com/T/
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> drivers/w1/slaves/w1_ds28e04.c | 10 ++--------
>> 1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/w1/slaves/w1_ds28e04.c b/drivers/w1/slaves/w1_ds28e04.c
>> index e4f336111edc..d75bb16fb7a1 100644
>> --- a/drivers/w1/slaves/w1_ds28e04.c
>> +++ b/drivers/w1/slaves/w1_ds28e04.c
>> @@ -339,10 +339,7 @@ static BIN_ATTR_RW(pio, 1);
>> static ssize_t crccheck_show(struct device *dev, struct device_attribute *attr,
>> char *buf)
>> {
>> - if (put_user(w1_enable_crccheck + 0x30, buf))
>> - return -EFAULT;
>> -
>> - return sizeof(w1_enable_crccheck);
>> + return sprintf(buf, "%d", w1_enable_crccheck);
>
> This should be sysfs_emit(), right?
Ok
>
>> }
>>
>> static ssize_t crccheck_store(struct device *dev, struct device_attribute *attr,
>> @@ -353,11 +350,8 @@ static ssize_t crccheck_store(struct device *dev, struct device_attribute *attr,
>> if (count != 1 || !buf)
>> return -EINVAL;
>>
>> - if (get_user(val, buf))
>> - return -EFAULT;
>> -
>> /* convert to decimal */
>> - val = val - 0x30;
>> + val = *buf - 0x30;
>
> Why not use a proper function that can parse a string and turn it into a
> number?
I wanted to keep the change minimal. But I can also replace it with some
scanf.
But don't we have any generic function to read and store a bool after all ?
Thanks
Christophe
^ permalink raw reply
* Re: [PATCH] w1: Misuse of get_user()/put_user() reported by sparse
From: Greg Kroah-Hartman @ 2021-11-26 16:24 UTC (permalink / raw)
To: Christophe Leroy
Cc: Evgeniy Polyakov, linuxppc-dev, linux-kernel, kernel test robot
In-Reply-To: <39ec092d-428c-7fce-7291-a88a4caf8b9f@csgroup.eu>
On Fri, Nov 26, 2021 at 05:10:46PM +0100, Christophe Leroy wrote:
>
>
> Le 26/11/2021 à 17:00, Greg Kroah-Hartman a écrit :
> > On Fri, Nov 19, 2021 at 10:15:09AM +0100, Christophe Leroy wrote:
> > > sparse warnings: (new ones prefixed by >>)
> > > > > drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected char [noderef] __user *_pu_addr @@ got char *buf @@
> > > drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: expected char [noderef] __user *_pu_addr
> > > drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: got char *buf
> > > > > drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected char const [noderef] __user *_gu_addr @@ got char const *buf @@
> > > drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: expected char const [noderef] __user *_gu_addr
> > > drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: got char const *buf
> > >
> > > The buffer buf is a failsafe buffer in kernel space, it's not user
> > > memory hence doesn't deserve the use of get_user() or put_user().
> > >
> > > Access 'buf' content directly.
> > >
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Link: https://lore.kernel.org/lkml/202111190526.K5vb7NWC-lkp@intel.com/T/
> > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> > > ---
> > > drivers/w1/slaves/w1_ds28e04.c | 10 ++--------
> > > 1 file changed, 2 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/w1/slaves/w1_ds28e04.c b/drivers/w1/slaves/w1_ds28e04.c
> > > index e4f336111edc..d75bb16fb7a1 100644
> > > --- a/drivers/w1/slaves/w1_ds28e04.c
> > > +++ b/drivers/w1/slaves/w1_ds28e04.c
> > > @@ -339,10 +339,7 @@ static BIN_ATTR_RW(pio, 1);
> > > static ssize_t crccheck_show(struct device *dev, struct device_attribute *attr,
> > > char *buf)
> > > {
> > > - if (put_user(w1_enable_crccheck + 0x30, buf))
> > > - return -EFAULT;
> > > -
> > > - return sizeof(w1_enable_crccheck);
> > > + return sprintf(buf, "%d", w1_enable_crccheck);
> >
> > This should be sysfs_emit(), right?
>
> Ok
>
> >
> > > }
> > > static ssize_t crccheck_store(struct device *dev, struct device_attribute *attr,
> > > @@ -353,11 +350,8 @@ static ssize_t crccheck_store(struct device *dev, struct device_attribute *attr,
> > > if (count != 1 || !buf)
> > > return -EINVAL;
> > > - if (get_user(val, buf))
> > > - return -EFAULT;
> > > -
> > > /* convert to decimal */
> > > - val = val - 0x30;
> > > + val = *buf - 0x30;
> >
> > Why not use a proper function that can parse a string and turn it into a
> > number?
>
> I wanted to keep the change minimal. But I can also replace it with some
> scanf.
>
> But don't we have any generic function to read and store a bool after all ?
Yes we do, please use kstrtobool().
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] powerpc: mm: radix_tlb: rearrange the if-else block
From: Christophe Leroy @ 2021-11-26 16:25 UTC (permalink / raw)
To: Nathan Chancellor, Arnd Bergmann
Cc: Anders Roxell, llvm, Nick Desaulniers, Linux Kernel Mailing List,
Paul Mackerras, linuxppc-dev
In-Reply-To: <YaEBTbjGyUBmISGK@archlinux-ax161>
Le 26/11/2021 à 16:46, Nathan Chancellor a écrit :
> On Fri, Nov 26, 2021 at 02:59:29PM +0100, Arnd Bergmann wrote:
>> On Fri, Nov 26, 2021 at 2:43 PM Christophe Leroy
>> <christophe.leroy@csgroup.eu> wrote:
>>> Le 25/11/2021 à 16:44, Anders Roxell a écrit :
>>> Can't you fix CLANG instead :) ?
>>>
>>> Or just add an else to the IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) that
>>> sets hstart and hend to 0 ?
>>
>> That doesn't sound any less risky than duplicating the code, it can lead to
>> incorrect changes just as easily if a patch ends up actually flushing at the
>> wrong address, and the compiler fails to complain because of the bogus
>> initialization.
>>
>>> Or just put hstart and hend calculation outside the IS_ENABLED() ? After
>>> all GCC should drop the calculation when not used.
>>
>> I like this one. I'm still unsure how clang can get so confused about whether
>> the variables are initialized or not, usually it handles this much better than
>> gcc. My best guess is that one of the memory clobbers makes it conclude
>> that 'hflush' can be true when it gets written to by an inline asm.
>
> As far as I am aware, clang's analysis does not evaluate variables when
> generating a control flow graph and using that for static analysis:
>
> https://godbolt.org/z/PdGxoq9j7
>
> Based on the control flow graph, it knows that hstart and hend are
> uninitialized because IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) gets
> expanded to 0 by the preprocessor but it does not seem like it can piece
> together that hflush's value of false is only changed to true under the
> now 'if (0) {' branch, meaning that all the calls to __tlbiel_va_range()
> never get evaluated. That may or may not be easy to fix in clang but we
> run into issues like this so infrequently.
>
> At any rate, the below diff works for me.
>
> Cheers,
> Nathan
>
> diff --git a/arch/powerpc/mm/book3s64/radix_tlb.c b/arch/powerpc/mm/book3s64/radix_tlb.c
> index 7724af19ed7e..156a631df976 100644
> --- a/arch/powerpc/mm/book3s64/radix_tlb.c
> +++ b/arch/powerpc/mm/book3s64/radix_tlb.c
> @@ -1174,12 +1174,10 @@ static inline void __radix__flush_tlb_range(struct mm_struct *mm,
> bool hflush = false;
> unsigned long hstart, hend;
>
> - if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
> - hstart = (start + PMD_SIZE - 1) & PMD_MASK;
> - hend = end & PMD_MASK;
> - if (hstart < hend)
> - hflush = true;
> - }
> + hstart = (start + PMD_SIZE - 1) & PMD_MASK;
> + hend = end & PMD_MASK;
> + if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && hstart < hend)
> + hflush = true;
Yes I like that much better.
Maybe even better with
hflush = IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && hstart < hend;
(And remove default false value at declaration).
>
> if (type == FLUSH_TYPE_LOCAL) {
> asm volatile("ptesync": : :"memory");
>
^ permalink raw reply
* [PATCH v2] w1: Misuse of get_user()/put_user() reported by sparse
From: Christophe Leroy @ 2021-11-26 16:47 UTC (permalink / raw)
To: Evgeniy Polyakov, Greg Kroah-Hartman
Cc: linuxppc-dev, linux-kernel, kernel test robot
sparse warnings: (new ones prefixed by >>)
>> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected char [noderef] __user *_pu_addr @@ got char *buf @@
drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: expected char [noderef] __user *_pu_addr
drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: got char *buf
>> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected char const [noderef] __user *_gu_addr @@ got char const *buf @@
drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: expected char const [noderef] __user *_gu_addr
drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: got char const *buf
The buffer buf is a failsafe buffer in kernel space, it's not user
memory hence doesn't deserve the use of get_user() or put_user().
Access 'buf' content directly.
Reported-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/lkml/202111190526.K5vb7NWC-lkp@intel.com/T/
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: Use sysfs_emit() and kstrtobool()
---
drivers/w1/slaves/w1_ds28e04.c | 25 +++----------------------
1 file changed, 3 insertions(+), 22 deletions(-)
diff --git a/drivers/w1/slaves/w1_ds28e04.c b/drivers/w1/slaves/w1_ds28e04.c
index e4f336111edc..98f80f412cfd 100644
--- a/drivers/w1/slaves/w1_ds28e04.c
+++ b/drivers/w1/slaves/w1_ds28e04.c
@@ -32,7 +32,7 @@ static int w1_strong_pullup = 1;
module_param_named(strong_pullup, w1_strong_pullup, int, 0);
/* enable/disable CRC checking on DS28E04-100 memory accesses */
-static char w1_enable_crccheck = 1;
+static bool w1_enable_crccheck = true;
#define W1_EEPROM_SIZE 512
#define W1_PAGE_COUNT 16
@@ -339,32 +339,13 @@ static BIN_ATTR_RW(pio, 1);
static ssize_t crccheck_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
- if (put_user(w1_enable_crccheck + 0x30, buf))
- return -EFAULT;
-
- return sizeof(w1_enable_crccheck);
+ return sysfs_emit(buf, "%d\n", w1_enable_crccheck);
}
static ssize_t crccheck_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
- char val;
-
- if (count != 1 || !buf)
- return -EINVAL;
-
- if (get_user(val, buf))
- return -EFAULT;
-
- /* convert to decimal */
- val = val - 0x30;
- if (val != 0 && val != 1)
- return -EINVAL;
-
- /* set the new value */
- w1_enable_crccheck = val;
-
- return sizeof(w1_enable_crccheck);
+ return kstrtobool(buf, &w1_enable_crccheck) ? : count;
}
static DEVICE_ATTR_RW(crccheck);
--
2.33.1
^ permalink raw reply related
* Re: [PATCH v2] w1: Misuse of get_user()/put_user() reported by sparse
From: Greg Kroah-Hartman @ 2021-11-26 16:54 UTC (permalink / raw)
To: Christophe Leroy
Cc: Evgeniy Polyakov, linuxppc-dev, linux-kernel, kernel test robot
In-Reply-To: <926b572075a26835f4e39d05710cd1b75fd4d5a4.1637945194.git.christophe.leroy@csgroup.eu>
On Fri, Nov 26, 2021 at 05:47:58PM +0100, Christophe Leroy wrote:
> sparse warnings: (new ones prefixed by >>)
> >> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected char [noderef] __user *_pu_addr @@ got char *buf @@
> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: expected char [noderef] __user *_pu_addr
> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: got char *buf
> >> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected char const [noderef] __user *_gu_addr @@ got char const *buf @@
> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: expected char const [noderef] __user *_gu_addr
> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: got char const *buf
>
> The buffer buf is a failsafe buffer in kernel space, it's not user
> memory hence doesn't deserve the use of get_user() or put_user().
>
> Access 'buf' content directly.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Link: https://lore.kernel.org/lkml/202111190526.K5vb7NWC-lkp@intel.com/T/
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> v2: Use sysfs_emit() and kstrtobool()
> ---
> drivers/w1/slaves/w1_ds28e04.c | 25 +++----------------------
> 1 file changed, 3 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/w1/slaves/w1_ds28e04.c b/drivers/w1/slaves/w1_ds28e04.c
> index e4f336111edc..98f80f412cfd 100644
> --- a/drivers/w1/slaves/w1_ds28e04.c
> +++ b/drivers/w1/slaves/w1_ds28e04.c
> @@ -32,7 +32,7 @@ static int w1_strong_pullup = 1;
> module_param_named(strong_pullup, w1_strong_pullup, int, 0);
>
> /* enable/disable CRC checking on DS28E04-100 memory accesses */
> -static char w1_enable_crccheck = 1;
> +static bool w1_enable_crccheck = true;
>
> #define W1_EEPROM_SIZE 512
> #define W1_PAGE_COUNT 16
> @@ -339,32 +339,13 @@ static BIN_ATTR_RW(pio, 1);
> static ssize_t crccheck_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> - if (put_user(w1_enable_crccheck + 0x30, buf))
> - return -EFAULT;
> -
> - return sizeof(w1_enable_crccheck);
> + return sysfs_emit(buf, "%d\n", w1_enable_crccheck);
> }
>
> static ssize_t crccheck_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - char val;
> -
> - if (count != 1 || !buf)
> - return -EINVAL;
> -
> - if (get_user(val, buf))
> - return -EFAULT;
> -
> - /* convert to decimal */
> - val = val - 0x30;
> - if (val != 0 && val != 1)
> - return -EINVAL;
> -
> - /* set the new value */
> - w1_enable_crccheck = val;
> -
> - return sizeof(w1_enable_crccheck);
> + return kstrtobool(buf, &w1_enable_crccheck) ? : count;
Please spell this line out, using ? : is unreadable at times.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v2] w1: Misuse of get_user()/put_user() reported by sparse
From: Christophe Leroy @ 2021-11-26 16:57 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Evgeniy Polyakov, linuxppc-dev, linux-kernel, kernel test robot
In-Reply-To: <YaERVtyYpJ+BTQ/f@kroah.com>
Le 26/11/2021 à 17:54, Greg Kroah-Hartman a écrit :
> On Fri, Nov 26, 2021 at 05:47:58PM +0100, Christophe Leroy wrote:
>> sparse warnings: (new ones prefixed by >>)
>>>> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected char [noderef] __user *_pu_addr @@ got char *buf @@
>> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: expected char [noderef] __user *_pu_addr
>> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: got char *buf
>>>> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected char const [noderef] __user *_gu_addr @@ got char const *buf @@
>> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: expected char const [noderef] __user *_gu_addr
>> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: got char const *buf
>>
>> The buffer buf is a failsafe buffer in kernel space, it's not user
>> memory hence doesn't deserve the use of get_user() or put_user().
>>
>> Access 'buf' content directly.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Link: https://lore.kernel.org/lkml/202111190526.K5vb7NWC-lkp@intel.com/T/
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> v2: Use sysfs_emit() and kstrtobool()
>> ---
>> drivers/w1/slaves/w1_ds28e04.c | 25 +++----------------------
>> 1 file changed, 3 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/w1/slaves/w1_ds28e04.c b/drivers/w1/slaves/w1_ds28e04.c
>> index e4f336111edc..98f80f412cfd 100644
>> --- a/drivers/w1/slaves/w1_ds28e04.c
>> +++ b/drivers/w1/slaves/w1_ds28e04.c
>> @@ -32,7 +32,7 @@ static int w1_strong_pullup = 1;
>> module_param_named(strong_pullup, w1_strong_pullup, int, 0);
>>
>> /* enable/disable CRC checking on DS28E04-100 memory accesses */
>> -static char w1_enable_crccheck = 1;
>> +static bool w1_enable_crccheck = true;
>>
>> #define W1_EEPROM_SIZE 512
>> #define W1_PAGE_COUNT 16
>> @@ -339,32 +339,13 @@ static BIN_ATTR_RW(pio, 1);
>> static ssize_t crccheck_show(struct device *dev, struct device_attribute *attr,
>> char *buf)
>> {
>> - if (put_user(w1_enable_crccheck + 0x30, buf))
>> - return -EFAULT;
>> -
>> - return sizeof(w1_enable_crccheck);
>> + return sysfs_emit(buf, "%d\n", w1_enable_crccheck);
>> }
>>
>> static ssize_t crccheck_store(struct device *dev, struct device_attribute *attr,
>> const char *buf, size_t count)
>> {
>> - char val;
>> -
>> - if (count != 1 || !buf)
>> - return -EINVAL;
>> -
>> - if (get_user(val, buf))
>> - return -EFAULT;
>> -
>> - /* convert to decimal */
>> - val = val - 0x30;
>> - if (val != 0 && val != 1)
>> - return -EINVAL;
>> -
>> - /* set the new value */
>> - w1_enable_crccheck = val;
>> -
>> - return sizeof(w1_enable_crccheck);
>> + return kstrtobool(buf, &w1_enable_crccheck) ? : count;
>
> Please spell this line out, using ? : is unreadable at times.
>
You prefer something like:
int err = kstrtobool(buf, &w1_enable_crccheck);
return err ? err : count;
Or
int err = kstrtobool(buf, &w1_enable_crccheck);
if (err)
return err;
return count;
?
^ permalink raw reply
* [PATCH v3] w1: Misuse of get_user()/put_user() reported by sparse
From: Christophe Leroy @ 2021-11-26 17:06 UTC (permalink / raw)
To: Evgeniy Polyakov, Greg Kroah-Hartman
Cc: linuxppc-dev, linux-kernel, kernel test robot
sparse warnings: (new ones prefixed by >>)
>> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected char [noderef] __user *_pu_addr @@ got char *buf @@
drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: expected char [noderef] __user *_pu_addr
drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: got char *buf
>> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected char const [noderef] __user *_gu_addr @@ got char const *buf @@
drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: expected char const [noderef] __user *_gu_addr
drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: got char const *buf
The buffer buf is a failsafe buffer in kernel space, it's not user
memory hence doesn't deserve the use of get_user() or put_user().
Access 'buf' content directly.
Reported-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/lkml/202111190526.K5vb7NWC-lkp@intel.com/T/
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v3: Rewrite crccheck_store() more userfriendly
v2: Use sysfs_emit() and kstrtobool()
---
drivers/w1/slaves/w1_ds28e04.c | 26 ++++++--------------------
1 file changed, 6 insertions(+), 20 deletions(-)
diff --git a/drivers/w1/slaves/w1_ds28e04.c b/drivers/w1/slaves/w1_ds28e04.c
index e4f336111edc..6cef6e2edb89 100644
--- a/drivers/w1/slaves/w1_ds28e04.c
+++ b/drivers/w1/slaves/w1_ds28e04.c
@@ -32,7 +32,7 @@ static int w1_strong_pullup = 1;
module_param_named(strong_pullup, w1_strong_pullup, int, 0);
/* enable/disable CRC checking on DS28E04-100 memory accesses */
-static char w1_enable_crccheck = 1;
+static bool w1_enable_crccheck = true;
#define W1_EEPROM_SIZE 512
#define W1_PAGE_COUNT 16
@@ -339,32 +339,18 @@ static BIN_ATTR_RW(pio, 1);
static ssize_t crccheck_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
- if (put_user(w1_enable_crccheck + 0x30, buf))
- return -EFAULT;
-
- return sizeof(w1_enable_crccheck);
+ return sysfs_emit(buf, "%d\n", w1_enable_crccheck);
}
static ssize_t crccheck_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
- char val;
-
- if (count != 1 || !buf)
- return -EINVAL;
+ int err = kstrtobool(buf, &w1_enable_crccheck);
- if (get_user(val, buf))
- return -EFAULT;
+ if (err)
+ return err;
- /* convert to decimal */
- val = val - 0x30;
- if (val != 0 && val != 1)
- return -EINVAL;
-
- /* set the new value */
- w1_enable_crccheck = val;
-
- return sizeof(w1_enable_crccheck);
+ return count;
}
static DEVICE_ATTR_RW(crccheck);
--
2.33.1
^ permalink raw reply related
* Re: [PATCH v2] w1: Misuse of get_user()/put_user() reported by sparse
From: Greg Kroah-Hartman @ 2021-11-26 17:02 UTC (permalink / raw)
To: Christophe Leroy
Cc: Evgeniy Polyakov, linuxppc-dev, linux-kernel, kernel test robot
In-Reply-To: <8e5493ac-dd05-9bad-c9ae-169114e0fdcf@csgroup.eu>
On Fri, Nov 26, 2021 at 05:57:58PM +0100, Christophe Leroy wrote:
>
>
> Le 26/11/2021 à 17:54, Greg Kroah-Hartman a écrit :
> > On Fri, Nov 26, 2021 at 05:47:58PM +0100, Christophe Leroy wrote:
> > > sparse warnings: (new ones prefixed by >>)
> > > > > drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected char [noderef] __user *_pu_addr @@ got char *buf @@
> > > drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: expected char [noderef] __user *_pu_addr
> > > drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: got char *buf
> > > > > drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected char const [noderef] __user *_gu_addr @@ got char const *buf @@
> > > drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: expected char const [noderef] __user *_gu_addr
> > > drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: got char const *buf
> > >
> > > The buffer buf is a failsafe buffer in kernel space, it's not user
> > > memory hence doesn't deserve the use of get_user() or put_user().
> > >
> > > Access 'buf' content directly.
> > >
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Link: https://lore.kernel.org/lkml/202111190526.K5vb7NWC-lkp@intel.com/T/
> > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> > > ---
> > > v2: Use sysfs_emit() and kstrtobool()
> > > ---
> > > drivers/w1/slaves/w1_ds28e04.c | 25 +++----------------------
> > > 1 file changed, 3 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/w1/slaves/w1_ds28e04.c b/drivers/w1/slaves/w1_ds28e04.c
> > > index e4f336111edc..98f80f412cfd 100644
> > > --- a/drivers/w1/slaves/w1_ds28e04.c
> > > +++ b/drivers/w1/slaves/w1_ds28e04.c
> > > @@ -32,7 +32,7 @@ static int w1_strong_pullup = 1;
> > > module_param_named(strong_pullup, w1_strong_pullup, int, 0);
> > > /* enable/disable CRC checking on DS28E04-100 memory accesses */
> > > -static char w1_enable_crccheck = 1;
> > > +static bool w1_enable_crccheck = true;
> > > #define W1_EEPROM_SIZE 512
> > > #define W1_PAGE_COUNT 16
> > > @@ -339,32 +339,13 @@ static BIN_ATTR_RW(pio, 1);
> > > static ssize_t crccheck_show(struct device *dev, struct device_attribute *attr,
> > > char *buf)
> > > {
> > > - if (put_user(w1_enable_crccheck + 0x30, buf))
> > > - return -EFAULT;
> > > -
> > > - return sizeof(w1_enable_crccheck);
> > > + return sysfs_emit(buf, "%d\n", w1_enable_crccheck);
> > > }
> > > static ssize_t crccheck_store(struct device *dev, struct device_attribute *attr,
> > > const char *buf, size_t count)
> > > {
> > > - char val;
> > > -
> > > - if (count != 1 || !buf)
> > > - return -EINVAL;
> > > -
> > > - if (get_user(val, buf))
> > > - return -EFAULT;
> > > -
> > > - /* convert to decimal */
> > > - val = val - 0x30;
> > > - if (val != 0 && val != 1)
> > > - return -EINVAL;
> > > -
> > > - /* set the new value */
> > > - w1_enable_crccheck = val;
> > > -
> > > - return sizeof(w1_enable_crccheck);
> > > + return kstrtobool(buf, &w1_enable_crccheck) ? : count;
> >
> > Please spell this line out, using ? : is unreadable at times.
> >
>
> You prefer something like:
>
> int err = kstrtobool(buf, &w1_enable_crccheck);
>
> return err ? err : count;
>
>
> Or
>
> int err = kstrtobool(buf, &w1_enable_crccheck);
>
> if (err)
> return err;
>
> return count;
This one. Write code for people to read first, compiler second.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v5 1/3] powerpc/bitops: Use immediate operand when possible
From: LEROY Christophe @ 2021-11-26 17:27 UTC (permalink / raw)
To: Michael Ellerman
Cc: Paul Mackerras, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <e6f815d9181bab09df3b350af51149437863e9f9.1632236981.git.christophe.leroy@csgroup.eu>
Hi Michael,
Any chance to get this series merged this cycle ?
Thanks
Christophe
Le 21/09/2021 à 17:09, Christophe Leroy a écrit :
> Today we get the following code generation for bitops like
> set or clear bit:
>
> c0009fe0: 39 40 08 00 li r10,2048
> c0009fe4: 7c e0 40 28 lwarx r7,0,r8
> c0009fe8: 7c e7 53 78 or r7,r7,r10
> c0009fec: 7c e0 41 2d stwcx. r7,0,r8
>
> c000d568: 39 00 18 00 li r8,6144
> c000d56c: 7c c0 38 28 lwarx r6,0,r7
> c000d570: 7c c6 40 78 andc r6,r6,r8
> c000d574: 7c c0 39 2d stwcx. r6,0,r7
>
> Most set bits are constant on lower 16 bits, so it can easily
> be replaced by the "immediate" version of the operation. Allow
> GCC to choose between the normal or immediate form.
>
> For clear bits, on 32 bits 'rlwinm' can be used instead of 'andc' for
> when all bits to be cleared are consecutive.
>
> On 64 bits we don't have any equivalent single operation for clearing,
> single bits or a few bits, we'd need two 'rldicl' so it is not
> worth it, the li/andc sequence is doing the same.
>
> With this patch we get:
>
> c0009fe0: 7d 00 50 28 lwarx r8,0,r10
> c0009fe4: 61 08 08 00 ori r8,r8,2048
> c0009fe8: 7d 00 51 2d stwcx. r8,0,r10
>
> c000d558: 7c e0 40 28 lwarx r7,0,r8
> c000d55c: 54 e7 05 64 rlwinm r7,r7,0,21,18
> c000d560: 7c e0 41 2d stwcx. r7,0,r8
>
> On pmac32_defconfig, it reduces the text by approx 10 kbytes.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org>
> ---
> v5: Fixed the argument of is_rlwinm_mask_valid() in test_and_clear_bits()
>
> v4: Rebased
>
> v3:
> - Using the mask validation proposed by Segher
>
> v2:
> - Use "n" instead of "i" as constraint for the rlwinm mask
> - Improve mask verification to handle more than single bit masks
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/include/asm/bitops.h | 89 ++++++++++++++++++++++++++++---
> 1 file changed, 81 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/bitops.h b/arch/powerpc/include/asm/bitops.h
> index 11847b6a244e..a05d8c62cbea 100644
> --- a/arch/powerpc/include/asm/bitops.h
> +++ b/arch/powerpc/include/asm/bitops.h
> @@ -71,19 +71,61 @@ static inline void fn(unsigned long mask, \
> __asm__ __volatile__ ( \
> prefix \
> "1:" PPC_LLARX "%0,0,%3,0\n" \
> - stringify_in_c(op) "%0,%0,%2\n" \
> + #op "%I2 %0,%0,%2\n" \
> PPC_STLCX "%0,0,%3\n" \
> "bne- 1b\n" \
> : "=&r" (old), "+m" (*p) \
> - : "r" (mask), "r" (p) \
> + : "rK" (mask), "r" (p) \
> : "cc", "memory"); \
> }
>
> DEFINE_BITOP(set_bits, or, "")
> -DEFINE_BITOP(clear_bits, andc, "")
> -DEFINE_BITOP(clear_bits_unlock, andc, PPC_RELEASE_BARRIER)
> DEFINE_BITOP(change_bits, xor, "")
>
> +static __always_inline bool is_rlwinm_mask_valid(unsigned long x)
> +{
> + if (!x)
> + return false;
> + if (x & 1)
> + x = ~x; // make the mask non-wrapping
> + x += x & -x; // adding the low set bit results in at most one bit set
> +
> + return !(x & (x - 1));
> +}
> +
> +#define DEFINE_CLROP(fn, prefix) \
> +static inline void fn(unsigned long mask, volatile unsigned long *_p) \
> +{ \
> + unsigned long old; \
> + unsigned long *p = (unsigned long *)_p; \
> + \
> + if (IS_ENABLED(CONFIG_PPC32) && \
> + __builtin_constant_p(mask) && is_rlwinm_mask_valid(~mask)) {\
> + asm volatile ( \
> + prefix \
> + "1:" "lwarx %0,0,%3\n" \
> + "rlwinm %0,%0,0,%2\n" \
> + "stwcx. %0,0,%3\n" \
> + "bne- 1b\n" \
> + : "=&r" (old), "+m" (*p) \
> + : "n" (~mask), "r" (p) \
> + : "cc", "memory"); \
> + } else { \
> + asm volatile ( \
> + prefix \
> + "1:" PPC_LLARX "%0,0,%3,0\n" \
> + "andc %0,%0,%2\n" \
> + PPC_STLCX "%0,0,%3\n" \
> + "bne- 1b\n" \
> + : "=&r" (old), "+m" (*p) \
> + : "r" (mask), "r" (p) \
> + : "cc", "memory"); \
> + } \
> +}
> +
> +DEFINE_CLROP(clear_bits, "")
> +DEFINE_CLROP(clear_bits_unlock, PPC_RELEASE_BARRIER)
> +
> static inline void arch_set_bit(int nr, volatile unsigned long *addr)
> {
> set_bits(BIT_MASK(nr), addr + BIT_WORD(nr));
> @@ -116,12 +158,12 @@ static inline unsigned long fn( \
> __asm__ __volatile__ ( \
> prefix \
> "1:" PPC_LLARX "%0,0,%3,%4\n" \
> - stringify_in_c(op) "%1,%0,%2\n" \
> + #op "%I2 %1,%0,%2\n" \
> PPC_STLCX "%1,0,%3\n" \
> "bne- 1b\n" \
> postfix \
> : "=&r" (old), "=&r" (t) \
> - : "r" (mask), "r" (p), "i" (IS_ENABLED(CONFIG_PPC64) ? eh : 0) \
> + : "rK" (mask), "r" (p), "i" (IS_ENABLED(CONFIG_PPC64) ? eh : 0) \
> : "cc", "memory"); \
> return (old & mask); \
> }
> @@ -130,11 +172,42 @@ DEFINE_TESTOP(test_and_set_bits, or, PPC_ATOMIC_ENTRY_BARRIER,
> PPC_ATOMIC_EXIT_BARRIER, 0)
> DEFINE_TESTOP(test_and_set_bits_lock, or, "",
> PPC_ACQUIRE_BARRIER, 1)
> -DEFINE_TESTOP(test_and_clear_bits, andc, PPC_ATOMIC_ENTRY_BARRIER,
> - PPC_ATOMIC_EXIT_BARRIER, 0)
> DEFINE_TESTOP(test_and_change_bits, xor, PPC_ATOMIC_ENTRY_BARRIER,
> PPC_ATOMIC_EXIT_BARRIER, 0)
>
> +static inline unsigned long test_and_clear_bits(unsigned long mask, volatile unsigned long *_p)
> +{
> + unsigned long old, t;
> + unsigned long *p = (unsigned long *)_p;
> +
> + if (IS_ENABLED(CONFIG_PPC32) &&
> + __builtin_constant_p(mask) && is_rlwinm_mask_valid(~mask)) {
> + asm volatile (
> + PPC_ATOMIC_ENTRY_BARRIER
> + "1:" "lwarx %0,0,%3\n"
> + "rlwinm %1,%0,0,%2\n"
> + "stwcx. %1,0,%3\n"
> + "bne- 1b\n"
> + PPC_ATOMIC_EXIT_BARRIER
> + : "=&r" (old), "=&r" (t)
> + : "n" (~mask), "r" (p)
> + : "cc", "memory");
> + } else {
> + asm volatile (
> + PPC_ATOMIC_ENTRY_BARRIER
> + "1:" PPC_LLARX "%0,0,%3,0\n"
> + "andc %1,%0,%2\n"
> + PPC_STLCX "%1,0,%3\n"
> + "bne- 1b\n"
> + PPC_ATOMIC_EXIT_BARRIER
> + : "=&r" (old), "=&r" (t)
> + : "r" (mask), "r" (p)
> + : "cc", "memory");
> + }
> +
> + return (old & mask);
> +}
> +
> static inline int arch_test_and_set_bit(unsigned long nr,
> volatile unsigned long *addr)
> {
>
^ permalink raw reply
* Re: [PATCH] powerpc/mm: Use refcount_t for refcount
From: Christophe Leroy @ 2021-11-26 17:55 UTC (permalink / raw)
To: Michael Ellerman, Chuhong Yuan; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <87y302segw.fsf@concordia.ellerman.id.au>
Le 09/08/2019 à 14:36, Michael Ellerman a écrit :
> Chuhong Yuan <hslester96@gmail.com> writes:
>> Reference counters are preferred to use refcount_t instead of
>> atomic_t.
>> This is because the implementation of refcount_t can prevent
>> overflows and detect possible use-after-free.
>> So convert atomic_t ref counters to refcount_t.
>>
>> Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
>
> Thanks.
>
> We don't have a fast implementation of refcount_t, so I'm worried this
> could cause a measurable performance regression.
Fast implementations have been removed by commit
https://github.com/linuxppc/linux/commit/fb041bb7c0a918b95c6889fc965cdc4a75b4c0ca
It's now considered that the generic implementation is good enough for
everybody.
However, this series doesn't apply anymore and needs rebase:
Applying: powerpc/mm: Use refcount_t for refcount
Using index info to reconstruct a base tree...
M arch/powerpc/mm/book3s64/mmu_context.c
M arch/powerpc/mm/book3s64/pgtable.c
M arch/powerpc/mm/pgtable-frag.c
M include/linux/mm_types.h
Falling back to patching base and 3-way merge...
Auto-merging include/linux/mm_types.h
CONFLICT (content): Merge conflict in include/linux/mm_types.h
Auto-merging arch/powerpc/mm/pgtable-frag.c
CONFLICT (content): Merge conflict in arch/powerpc/mm/pgtable-frag.c
Auto-merging arch/powerpc/mm/book3s64/pgtable.c
CONFLICT (content): Merge conflict in arch/powerpc/mm/book3s64/pgtable.c
Auto-merging arch/powerpc/mm/book3s64/mmu_context.c
Patch failed at 0001 powerpc/mm: Use refcount_t for refcount
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Thanks
Christophe
>
> Did you benchmark it at all?
>
> cheers
>
>> diff --git a/arch/powerpc/mm/book3s64/mmu_context.c b/arch/powerpc/mm/book3s64/mmu_context.c
>> index 2d0cb5ba9a47..f836fd5a6abc 100644
>> --- a/arch/powerpc/mm/book3s64/mmu_context.c
>> +++ b/arch/powerpc/mm/book3s64/mmu_context.c
>> @@ -231,7 +231,7 @@ static void pmd_frag_destroy(void *pmd_frag)
>> /* drop all the pending references */
>> count = ((unsigned long)pmd_frag & ~PAGE_MASK) >> PMD_FRAG_SIZE_SHIFT;
>> /* We allow PTE_FRAG_NR fragments from a PTE page */
>> - if (atomic_sub_and_test(PMD_FRAG_NR - count, &page->pt_frag_refcount)) {
>> + if (refcount_sub_and_test(PMD_FRAG_NR - count, &page->pt_frag_refcount)) {
>> pgtable_pmd_page_dtor(page);
>> __free_page(page);
>> }
>> diff --git a/arch/powerpc/mm/book3s64/pgtable.c b/arch/powerpc/mm/book3s64/pgtable.c
>> index 7d0e0d0d22c4..40056896ce4e 100644
>> --- a/arch/powerpc/mm/book3s64/pgtable.c
>> +++ b/arch/powerpc/mm/book3s64/pgtable.c
>> @@ -277,7 +277,7 @@ static pmd_t *__alloc_for_pmdcache(struct mm_struct *mm)
>> return NULL;
>> }
>>
>> - atomic_set(&page->pt_frag_refcount, 1);
>> + refcount_set(&page->pt_frag_refcount, 1);
>>
>> ret = page_address(page);
>> /*
>> @@ -294,7 +294,7 @@ static pmd_t *__alloc_for_pmdcache(struct mm_struct *mm)
>> * count.
>> */
>> if (likely(!mm->context.pmd_frag)) {
>> - atomic_set(&page->pt_frag_refcount, PMD_FRAG_NR);
>> + refcount_set(&page->pt_frag_refcount, PMD_FRAG_NR);
>> mm->context.pmd_frag = ret + PMD_FRAG_SIZE;
>> }
>> spin_unlock(&mm->page_table_lock);
>> @@ -317,8 +317,7 @@ void pmd_fragment_free(unsigned long *pmd)
>> {
>> struct page *page = virt_to_page(pmd);
>>
>> - BUG_ON(atomic_read(&page->pt_frag_refcount) <= 0);
>> - if (atomic_dec_and_test(&page->pt_frag_refcount)) {
>> + if (refcount_dec_and_test(&page->pt_frag_refcount)) {
>> pgtable_pmd_page_dtor(page);
>> __free_page(page);
>> }
>> diff --git a/arch/powerpc/mm/pgtable-frag.c b/arch/powerpc/mm/pgtable-frag.c
>> index a7b05214760c..4ef8231b677f 100644
>> --- a/arch/powerpc/mm/pgtable-frag.c
>> +++ b/arch/powerpc/mm/pgtable-frag.c
>> @@ -24,7 +24,7 @@ void pte_frag_destroy(void *pte_frag)
>> /* drop all the pending references */
>> count = ((unsigned long)pte_frag & ~PAGE_MASK) >> PTE_FRAG_SIZE_SHIFT;
>> /* We allow PTE_FRAG_NR fragments from a PTE page */
>> - if (atomic_sub_and_test(PTE_FRAG_NR - count, &page->pt_frag_refcount)) {
>> + if (refcount_sub_and_test(PTE_FRAG_NR - count, &page->pt_frag_refcount)) {
>> pgtable_page_dtor(page);
>> __free_page(page);
>> }
>> @@ -71,7 +71,7 @@ static pte_t *__alloc_for_ptecache(struct mm_struct *mm, int kernel)
>> return NULL;
>> }
>>
>> - atomic_set(&page->pt_frag_refcount, 1);
>> + refcount_set(&page->pt_frag_refcount, 1);
>>
>> ret = page_address(page);
>> /*
>> @@ -87,7 +87,7 @@ static pte_t *__alloc_for_ptecache(struct mm_struct *mm, int kernel)
>> * count.
>> */
>> if (likely(!pte_frag_get(&mm->context))) {
>> - atomic_set(&page->pt_frag_refcount, PTE_FRAG_NR);
>> + refcount_set(&page->pt_frag_refcount, PTE_FRAG_NR);
>> pte_frag_set(&mm->context, ret + PTE_FRAG_SIZE);
>> }
>> spin_unlock(&mm->page_table_lock);
>> @@ -110,8 +110,7 @@ void pte_fragment_free(unsigned long *table, int kernel)
>> {
>> struct page *page = virt_to_page(table);
>>
>> - BUG_ON(atomic_read(&page->pt_frag_refcount) <= 0);
>> - if (atomic_dec_and_test(&page->pt_frag_refcount)) {
>> + if (refcount_dec_and_test(&page->pt_frag_refcount)) {
>> if (!kernel)
>> pgtable_page_dtor(page);
>> __free_page(page);
>> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
>> index 3a37a89eb7a7..7fe23a3faf95 100644
>> --- a/include/linux/mm_types.h
>> +++ b/include/linux/mm_types.h
>> @@ -14,6 +14,7 @@
>> #include <linux/uprobes.h>
>> #include <linux/page-flags-layout.h>
>> #include <linux/workqueue.h>
>> +#include <linux/refcount.h>
>>
>> #include <asm/mmu.h>
>>
>> @@ -147,7 +148,7 @@ struct page {
>> unsigned long _pt_pad_2; /* mapping */
>> union {
>> struct mm_struct *pt_mm; /* x86 pgds only */
>> - atomic_t pt_frag_refcount; /* powerpc */
>> + refcount_t pt_frag_refcount; /* powerpc */
>> };
>> #if ALLOC_SPLIT_PTLOCKS
>> spinlock_t *ptl;
>> --
>> 2.20.1
^ permalink raw reply
* Re: [PATCH] powerpc/code-patching: Relax verification of patchability
From: Sachin Sant @ 2021-11-26 18:00 UTC (permalink / raw)
To: Christophe Leroy
Cc: Stephen Rothwell, linux-kernel, Nicholas Piggin, Paul Mackerras,
linuxppc-dev
In-Reply-To: <68d7d57675e0963fe5e2c4b84b0cb2390c78638c.1637912333.git.christophe.leroy@csgroup.eu>
> Running code patching self-tests ...
> patch_instruction() called on invalid text address 0xe1011e58 from test_code_patching+0x34/0xd6c
>
> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Fixes: 8b8a8f0ab3f5 ("powerpc/code-patching: Improve verification of patchability")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/lib/code-patching.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
This fixes the problem for me.
Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
Thanks
-Sachin
^ 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