* [PATCH -next] powerpc/mm: check base flags in ioremap_prot
@ 2021-09-03 9:03 ` Nanyong Sun
0 siblings, 0 replies; 6+ messages in thread
From: Nanyong Sun @ 2021-09-03 9:03 UTC (permalink / raw)
To: mpe, benh, paulus, akpm, npiggin, christophe.leroy
Cc: linuxppc-dev, linux-kernel
Some drivers who call ioremap_prot without setting base flags like
ioremap_prot(addr, len, 0) may work well before
commit 56f3c1413f5c ("powerpc/mm: properly set PAGE_KERNEL flags in
ioremap()"), but now they will get a virtual address "successfully"
from ioremap_prot and badly fault on memory access later because that
patch also dropped the hack adding of base flags for ioremap_prot.
So return NULL and throw a warning if the caller of ioremap_prot did
not set base flags properly. Why not just hack adding PAGE_KERNEL flags
in the ioremap_prot, because most scenarios can be covered by high level
functions like ioremap(), ioremap_coherent(), ioremap_cache()...
so it is better to keep max flexibility for this low level api.
Signed-off-by: Nanyong Sun <sunnanyong@huawei.com>
---
arch/powerpc/mm/ioremap.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/powerpc/mm/ioremap.c b/arch/powerpc/mm/ioremap.c
index 57342154d2b0..b7eda0f0d04d 100644
--- a/arch/powerpc/mm/ioremap.c
+++ b/arch/powerpc/mm/ioremap.c
@@ -46,6 +46,10 @@ void __iomem *ioremap_prot(phys_addr_t addr, unsigned long size, unsigned long f
pte_t pte = __pte(flags);
void *caller = __builtin_return_address(0);
+ /* The caller should set base page flags properly */
+ if (WARN_ON((flags & _PAGE_PRESENT) == 0))
+ return NULL;
+
/* writeable implies dirty for kernel addresses */
if (pte_write(pte))
pte = pte_mkdirty(pte);
--
2.18.0.huawei.25
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH -next] powerpc/mm: check base flags in ioremap_prot
@ 2021-09-03 9:03 ` Nanyong Sun
0 siblings, 0 replies; 6+ messages in thread
From: Nanyong Sun @ 2021-09-03 9:03 UTC (permalink / raw)
To: mpe, benh, paulus, akpm, npiggin, christophe.leroy
Cc: linuxppc-dev, linux-kernel
Some drivers who call ioremap_prot without setting base flags like
ioremap_prot(addr, len, 0) may work well before
commit 56f3c1413f5c ("powerpc/mm: properly set PAGE_KERNEL flags in
ioremap()"), but now they will get a virtual address "successfully"
from ioremap_prot and badly fault on memory access later because that
patch also dropped the hack adding of base flags for ioremap_prot.
So return NULL and throw a warning if the caller of ioremap_prot did
not set base flags properly. Why not just hack adding PAGE_KERNEL flags
in the ioremap_prot, because most scenarios can be covered by high level
functions like ioremap(), ioremap_coherent(), ioremap_cache()...
so it is better to keep max flexibility for this low level api.
Signed-off-by: Nanyong Sun <sunnanyong@huawei.com>
---
arch/powerpc/mm/ioremap.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/powerpc/mm/ioremap.c b/arch/powerpc/mm/ioremap.c
index 57342154d2b0..b7eda0f0d04d 100644
--- a/arch/powerpc/mm/ioremap.c
+++ b/arch/powerpc/mm/ioremap.c
@@ -46,6 +46,10 @@ void __iomem *ioremap_prot(phys_addr_t addr, unsigned long size, unsigned long f
pte_t pte = __pte(flags);
void *caller = __builtin_return_address(0);
+ /* The caller should set base page flags properly */
+ if (WARN_ON((flags & _PAGE_PRESENT) == 0))
+ return NULL;
+
/* writeable implies dirty for kernel addresses */
if (pte_write(pte))
pte = pte_mkdirty(pte);
--
2.18.0.huawei.25
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH -next] powerpc/mm: check base flags in ioremap_prot
2021-09-03 9:03 ` Nanyong Sun
(?)
@ 2021-09-03 9:16 ` Christophe Leroy
2021-09-04 6:38 ` Nanyong Sun
-1 siblings, 1 reply; 6+ messages in thread
From: Christophe Leroy @ 2021-09-03 9:16 UTC (permalink / raw)
To: Nanyong Sun, mpe, benh, paulus, akpm, npiggin, christophe.leroy
Cc: linuxppc-dev, linux-kernel
Le 03/09/2021 à 11:03, Nanyong Sun a écrit :
> Some drivers who call ioremap_prot without setting base flags like
> ioremap_prot(addr, len, 0) may work well before
> commit 56f3c1413f5c ("powerpc/mm: properly set PAGE_KERNEL flags in
> ioremap()"), but now they will get a virtual address "successfully"
> from ioremap_prot and badly fault on memory access later because that
> patch also dropped the hack adding of base flags for ioremap_prot.
>
> So return NULL and throw a warning if the caller of ioremap_prot did
> not set base flags properly. Why not just hack adding PAGE_KERNEL flags
> in the ioremap_prot, because most scenarios can be covered by high level
> functions like ioremap(), ioremap_coherent(), ioremap_cache()...
> so it is better to keep max flexibility for this low level api.
As far as I can see, there is no user of this fonction that sets flags to 0 in the kernel tree.
Did you find any ? If you did, I think it is better to fix the caller.
Christophe
>
> Signed-off-by: Nanyong Sun <sunnanyong@huawei.com>
> ---
> arch/powerpc/mm/ioremap.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/powerpc/mm/ioremap.c b/arch/powerpc/mm/ioremap.c
> index 57342154d2b0..b7eda0f0d04d 100644
> --- a/arch/powerpc/mm/ioremap.c
> +++ b/arch/powerpc/mm/ioremap.c
> @@ -46,6 +46,10 @@ void __iomem *ioremap_prot(phys_addr_t addr, unsigned long size, unsigned long f
> pte_t pte = __pte(flags);
> void *caller = __builtin_return_address(0);
>
> + /* The caller should set base page flags properly */
> + if (WARN_ON((flags & _PAGE_PRESENT) == 0))
This probably doesn't work for some plateforms like book3s/64. You should use helpers like
pte_present().
See the comment at
https://elixir.bootlin.com/linux/v5.14/source/arch/powerpc/include/asm/book3s/64/pgtable.h#L591
> + return NULL;
> +
> /* writeable implies dirty for kernel addresses */
> if (pte_write(pte))
> pte = pte_mkdirty(pte);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] powerpc/mm: check base flags in ioremap_prot
2021-09-03 9:16 ` Christophe Leroy
@ 2021-09-04 6:38 ` Nanyong Sun
2021-09-04 11:20 ` Michael Ellerman
0 siblings, 1 reply; 6+ messages in thread
From: Nanyong Sun @ 2021-09-04 6:38 UTC (permalink / raw)
To: Christophe Leroy, mpe, benh, paulus, akpm, npiggin, christophe.leroy
Cc: wangkefeng 00584194, linuxppc-dev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4962 bytes --]
On 2021/9/3 17:16, Christophe Leroy wrote:
>
>
> Le 03/09/2021 à 11:03, Nanyong Sun a écrit :
>> Some drivers who call ioremap_prot without setting base flags like
>> ioremap_prot(addr, len, 0) may work well before
>> commit 56f3c1413f5c ("powerpc/mm: properly set PAGE_KERNEL flags in
>> ioremap()"), but now they will get a virtual address "successfully"
>> from ioremap_prot and badly fault on memory access later because that
>> patch also dropped the hack adding of base flags for ioremap_prot.
>>
>> So return NULL and throw a warning if the caller of ioremap_prot did
>> not set base flags properly. Why not just hack adding PAGE_KERNEL flags
>> in the ioremap_prot, because most scenarios can be covered by high level
>> functions like ioremap(), ioremap_coherent(), ioremap_cache()...
>> so it is better to keep max flexibility for this low level api.
>
> As far as I can see, there is no user of this fonction that sets flags
> to 0 in the kernel tree.
>
> Did you find any ? If you did, I think it is better to fix the caller.
>
> Christophe
>
I see some vendor's drivers which are not on upstream has used
ioremap_prot like
ioremap_prot(addr,len, _PAGE_NO_CACHE | _PAGE_GUARDED) or
ioremap_prot(addr,len, 0), and they worked well on old kernel versions
before commit
56f3c1413f5c ("powerpc/mm: properly set PAGE_KERNEL flags in ioremap()").
Actually, in the commit( git show 56f3c1413f5c ), you can see that in old
kernel versions, the implementations of ioremap_xxx just set flags as
_PAGE_xxx or 0,
Code examples of the commit:
In arch/powerpc/mm/pgtable_32.c
ioremap(phys_addr_t addr, unsigned long size)
{
- return __ioremap_caller(addr, size, _PAGE_NO_CACHE | _PAGE_GUARDED,
- __builtin_return_address(0));
+ unsigned long flags = pgprot_val(pgprot_noncached(PAGE_KERNEL));
+
+ return __ioremap_caller(addr, size, flags,
__builtin_return_address(0));
}
In arch/powerpc/mm/pgtable_64.c
void __iomem * ioremap(phys_addr_t addr, unsigned long size)
{
- unsigned long flags = pgprot_val(pgprot_noncached(__pgprot(0)));
+ unsigned long flags = pgprot_val(pgprot_noncached(PAGE_KERNEL));
void *caller = __builtin_return_address(0);
They rely on the low level functions to add base flags.
So, these driver codes like 'ioremap_prot(addr,len, _PAGE_NO_CACHE) '
in old kernel version is**not very improper.
Ofcourse, when porting new kernel versions, they need to change because the
api implementation has changed, but it's difficult for driver developer
to find out what
happend and how to change, because they still get a virtual address
"successfully"
from ioremap_prot without base flags and then page fault on memory
access later.
So, it is necessary to check and report base flags missing in
ioremap_prot() timely.
Secondly, the commit 56f3c1413f5c ("powerpc/mm: properly set PAGE_KERNEL
flags in ioremap()") delete the hack adding of PAGE_KERNEL flags in low
level
implementation and add flags properly for all ioremap_xx() APIs except
ioreamp_prot,
for ioreamp_prot, it not only loss the hack adding, but also loss the
basic flags check
which is necessary.
So we need add this basic check for this API.
Nanyong
>>
>> Signed-off-by: Nanyong Sun <sunnanyong@huawei.com>
>> ---
>> arch/powerpc/mm/ioremap.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/powerpc/mm/ioremap.c b/arch/powerpc/mm/ioremap.c
>> index 57342154d2b0..b7eda0f0d04d 100644
>> --- a/arch/powerpc/mm/ioremap.c
>> +++ b/arch/powerpc/mm/ioremap.c
>> @@ -46,6 +46,10 @@ void __iomem *ioremap_prot(phys_addr_t addr,
>> unsigned long size, unsigned long f
>> pte_t pte = __pte(flags);
>> void *caller = __builtin_return_address(0);
>> + /* The caller should set base page flags properly */
>> + if (WARN_ON((flags & _PAGE_PRESENT) == 0))
>
> This probably doesn't work for some plateforms like book3s/64. You
> should use helpers like pte_present().
>
> See the comment at
> https://elixir.bootlin.com/linux/v5.14/source/arch/powerpc/include/asm/book3s/64/pgtable.h#L591
>
I'm afraid that pte_present() is not ok for book3s/64, because it also
check _PAGE_PTE which will be set in the bottom
half of ioremap, so it would always return fail because the caller of
ioremap_prot wouldn't set _PAGE_PTE. I think it's ok that
not check _PAGE_INVALID here because we intend to create a new valid PTE
here.
And I think check _PAGE_PRESENT is ok because in kernel version before
commit 56f3c1413f5c , the function __ioremap_at()
and __ioremap_caller() used _PAGE_PRESENT to check base flags, book3s/64
was also present by then.
Nanyong
>> + return NULL;
>> +
>> /* writeable implies dirty for kernel addresses */
>> if (pte_write(pte))
>> pte = pte_mkdirty(pte);
>>
> .
[-- Attachment #2: Type: text/html, Size: 7409 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] powerpc/mm: check base flags in ioremap_prot
2021-09-04 6:38 ` Nanyong Sun
@ 2021-09-04 11:20 ` Michael Ellerman
0 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2021-09-04 11:20 UTC (permalink / raw)
To: Nanyong Sun, Christophe Leroy, benh, paulus, akpm, npiggin,
christophe.leroy
Cc: linuxppc-dev, linux-kernel, wangkefeng 00584194
Nanyong Sun <sunnanyong@huawei.com> writes:
> On 2021/9/3 17:16, Christophe Leroy wrote:
>> Le 03/09/2021 à 11:03, Nanyong Sun a écrit :
>>> Some drivers who call ioremap_prot without setting base flags like
>>> ioremap_prot(addr, len, 0) may work well before
>>> commit 56f3c1413f5c ("powerpc/mm: properly set PAGE_KERNEL flags in
>>> ioremap()"), but now they will get a virtual address "successfully"
>>> from ioremap_prot and badly fault on memory access later because that
>>> patch also dropped the hack adding of base flags for ioremap_prot.
>>>
>>> So return NULL and throw a warning if the caller of ioremap_prot did
>>> not set base flags properly. Why not just hack adding PAGE_KERNEL flags
>>> in the ioremap_prot, because most scenarios can be covered by high level
>>> functions like ioremap(), ioremap_coherent(), ioremap_cache()...
>>> so it is better to keep max flexibility for this low level api.
>>
>> As far as I can see, there is no user of this fonction that sets flags
>> to 0 in the kernel tree.
>>
>> Did you find any ? If you did, I think it is better to fix the caller.
>>
>> Christophe
>>
> I see some vendor's drivers which are not on upstream ...
Sorry, but we don't carry extraneous checks in upstream for the sake of
out-of-tree drivers.
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] powerpc/mm: check base flags in ioremap_prot
@ 2021-09-04 11:20 ` Michael Ellerman
0 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2021-09-04 11:20 UTC (permalink / raw)
To: Nanyong Sun, Christophe Leroy, benh, paulus, akpm, npiggin,
christophe.leroy
Cc: wangkefeng 00584194, linuxppc-dev, linux-kernel
Nanyong Sun <sunnanyong@huawei.com> writes:
> On 2021/9/3 17:16, Christophe Leroy wrote:
>> Le 03/09/2021 à 11:03, Nanyong Sun a écrit :
>>> Some drivers who call ioremap_prot without setting base flags like
>>> ioremap_prot(addr, len, 0) may work well before
>>> commit 56f3c1413f5c ("powerpc/mm: properly set PAGE_KERNEL flags in
>>> ioremap()"), but now they will get a virtual address "successfully"
>>> from ioremap_prot and badly fault on memory access later because that
>>> patch also dropped the hack adding of base flags for ioremap_prot.
>>>
>>> So return NULL and throw a warning if the caller of ioremap_prot did
>>> not set base flags properly. Why not just hack adding PAGE_KERNEL flags
>>> in the ioremap_prot, because most scenarios can be covered by high level
>>> functions like ioremap(), ioremap_coherent(), ioremap_cache()...
>>> so it is better to keep max flexibility for this low level api.
>>
>> As far as I can see, there is no user of this fonction that sets flags
>> to 0 in the kernel tree.
>>
>> Did you find any ? If you did, I think it is better to fix the caller.
>>
>> Christophe
>>
> I see some vendor's drivers which are not on upstream ...
Sorry, but we don't carry extraneous checks in upstream for the sake of
out-of-tree drivers.
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-04 11:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-03 9:03 [PATCH -next] powerpc/mm: check base flags in ioremap_prot Nanyong Sun
2021-09-03 9:03 ` Nanyong Sun
2021-09-03 9:16 ` Christophe Leroy
2021-09-04 6:38 ` Nanyong Sun
2021-09-04 11:20 ` Michael Ellerman
2021-09-04 11:20 ` Michael Ellerman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.