From: Michael Ellerman <mpe@ellerman.id.au>
To: Christophe Leroy <christophe.leroy@csgroup.eu>,
"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>
Cc: "erhard_f@mailbox.org" <erhard_f@mailbox.org>,
"wangkefeng.wang@huawei.com" <wangkefeng.wang@huawei.com>,
"npiggin@gmail.com" <npiggin@gmail.com>
Subject: Re: [PATCH 5/6] powerpc/64: Only WARN if __pa()/__va() called with bad addresses
Date: Fri, 08 Apr 2022 14:01:50 +1000 [thread overview]
Message-ID: <87zgkw6x5d.fsf@mpe.ellerman.id.au> (raw)
In-Reply-To: <7381978d-26d1-4abb-e539-d28247a93d9b@csgroup.eu>
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 06/04/2022 à 16:58, Michael Ellerman a écrit :
>> We added checks to __pa() / __va() to ensure they're only called with
>> appropriate addresses. But using BUG_ON() is too strong, it means
>> virt_addr_valid() will BUG when DEBUG_VIRTUAL is enabled.
>>
>> Instead switch them to warnings, arm64 does the same.
>>
>> Fixes: 4dd7554a6456 ("powerpc/64: Add VIRTUAL_BUG_ON checks for __va and __pa addresses")
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>> arch/powerpc/include/asm/page.h | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
>> index f2c5c26869f1..40a27a56ee40 100644
>> --- a/arch/powerpc/include/asm/page.h
>> +++ b/arch/powerpc/include/asm/page.h
>> @@ -216,6 +216,12 @@ static inline bool pfn_valid(unsigned long pfn)
>> #define __pa(x) ((phys_addr_t)(unsigned long)(x) - VIRT_PHYS_OFFSET)
>> #else
>> #ifdef CONFIG_PPC64
>> +
>> +#ifdef CONFIG_DEBUG_VIRTUAL
>> +#define VIRTUAL_WARN_ON(x) WARN_ON(x)
>> +#else
>> +#define VIRTUAL_WARN_ON(x)
>> +#endif
>
> Could be:
>
> #define VIRTUAL_WARN_ON(x) WARN_ON(IS_ENABLED(CONFIG_DEBUG_VIRTUAL) && (x))
>
>> /*
>> * gcc miscompiles (unsigned long)(&static_var) - PAGE_OFFSET
>> * with -mcmodel=medium, so we use & and | instead of - and + on 64-bit.
>> @@ -223,13 +229,13 @@ static inline bool pfn_valid(unsigned long pfn)
>> */
>> #define __va(x) \
>> ({ \
>> - VIRTUAL_BUG_ON((unsigned long)(x) >= PAGE_OFFSET); \
>> + VIRTUAL_WARN_ON((unsigned long)(x) >= PAGE_OFFSET); \
>> (void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET); \
>> })
>>
>> #define __pa(x) \
>> ({ \
>> - VIRTUAL_BUG_ON((unsigned long)(x) < PAGE_OFFSET); \
>> + VIRTUAL_WARN_ON((unsigned long)(x) < PAGE_OFFSET); \
>> (unsigned long)(x) & 0x0fffffffffffffffUL; \
>> })
>>
>
> Isn't it dangerous to WARN (or BUG) here ? __pa() can be used very early
> during boot, like in prom_init.c
Yes. WARN is a bit less dangerous though :)
> Some other architectures have a __pa_nodebug(). The __pa() does the
> WARN() then calls __pa_nodebug(). Early users call __pa_nodebug() directly.
Yeah I saw that, we could go that way.
I think possibly the better option is for __pa() to have no checks,
instead the checks go in the higher level routines like virt_to_phys()
and phys_to_virt().
And then we can check uses of __pa() and any that are *not* early boot
or low level stuff can be converted to virt_to_phys().
cheers
next prev parent reply other threads:[~2022-04-08 4:02 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-06 14:57 [PATCH 1/6] powerpc: Fix virt_addr_valid() for 64-bit Book3E & 32-bit Michael Ellerman
2022-04-06 14:57 ` [PATCH 2/6] Revert "powerpc: Set max_mapnr correctly" Michael Ellerman
2022-04-06 14:57 ` [PATCH 3/6] powerpc/85xx: Fix virt_to_phys() off-by-one in smp_85xx_start_cpu() Michael Ellerman
2022-05-15 10:21 ` Michael Ellerman
2022-04-06 14:58 ` [PATCH 4/6] powerpc/vas: Fix __pa() handling in init_winctx_regs() Michael Ellerman
2022-04-06 14:58 ` [PATCH 5/6] powerpc/64: Only WARN if __pa()/__va() called with bad addresses Michael Ellerman
2022-04-06 15:18 ` Christophe Leroy
2022-04-08 4:01 ` Michael Ellerman [this message]
2022-04-06 14:58 ` [RFC PATCH 6/6] powerpc/mm: Add virt_addr_valid() checks Michael Ellerman
2022-04-10 12:27 ` [PATCH 1/6] powerpc: Fix virt_addr_valid() for 64-bit Book3E & 32-bit Michael Ellerman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87zgkw6x5d.fsf@mpe.ellerman.id.au \
--to=mpe@ellerman.id.au \
--cc=christophe.leroy@csgroup.eu \
--cc=erhard_f@mailbox.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=npiggin@gmail.com \
--cc=wangkefeng.wang@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).