[PATCH] powerpc: Use printk instead of WARN in change_memory_attr

[PATCH] powerpc: Use printk instead of WARN in change_memory_attr

[Ritesh Harjani (IBM)]

Use pr_warn_once instead of WARN_ON_ONCE as discussed here [1]
for printing possible use of set_memory_* on linear map on Hash.

[1]: https://lore.kernel.org/all/877cc2fpi2.fsf@mail.lhotse/#t

Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 arch/powerpc/mm/pageattr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
index ac22bf28086f..c8c2d664c6f3 100644
--- a/arch/powerpc/mm/pageattr.c
+++ b/arch/powerpc/mm/pageattr.c
@@ -94,8 +94,11 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
 	if (!radix_enabled()) {
 		int region = get_region_id(addr);

-		if (WARN_ON_ONCE(region != VMALLOC_REGION_ID && region != IO_REGION_ID))
+		if (region != VMALLOC_REGION_ID && region != IO_REGION_ID) {
+			pr_warn_once("%s: possible use of set_memory_* on linear map on Hash from (%ps)\n",
+					__func__, __builtin_return_address(0));
 			return -EINVAL;
+		}
 	}
 #endif

--
2.39.2

Re: [PATCH] powerpc: Use printk instead of WARN in change_memory_attr

[Christophe Leroy]

Le 27/08/2024 à 11:12, Ritesh Harjani (IBM) a écrit :
> [Vous ne recevez pas souvent de courriers de ritesh.list@gmail.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> Use pr_warn_once instead of WARN_ON_ONCE as discussed here [1]
> for printing possible use of set_memory_* on linear map on Hash.
> 
> [1]: https://lore.kernel.org/all/877cc2fpi2.fsf@mail.lhotse/#t
> 
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> ---
>   arch/powerpc/mm/pageattr.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
> index ac22bf28086f..c8c2d664c6f3 100644
> --- a/arch/powerpc/mm/pageattr.c
> +++ b/arch/powerpc/mm/pageattr.c
> @@ -94,8 +94,11 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
>          if (!radix_enabled()) {
>                  int region = get_region_id(addr);
> 
> -               if (WARN_ON_ONCE(region != VMALLOC_REGION_ID && region != IO_REGION_ID))
> +               if (region != VMALLOC_REGION_ID && region != IO_REGION_ID) {
> +                       pr_warn_once("%s: possible use of set_memory_* on linear map on Hash from (%ps)\n",
> +                                       __func__, __builtin_return_address(0));

Is it really only linear map ?

What about "possible user of set_memory_* outside of vmalloc or io region.

Maybe a show_stack() would also be worth it ?


But in principle I think it would be better to keep the WARN_ONCE until 
we can add __must_check to set_memory_xxx() functions to be sure all 
callers check the result, as mandated by 
https://github.com/KSPP/linux/issues/7

Christophe

Re: [PATCH] powerpc: Use printk instead of WARN in change_memory_attr

[Ritesh Harjani]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:

> Le 27/08/2024 à 11:12, Ritesh Harjani (IBM) a écrit :
>> [Vous ne recevez pas souvent de courriers de ritesh.list@gmail.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>> 
>> Use pr_warn_once instead of WARN_ON_ONCE as discussed here [1]
>> for printing possible use of set_memory_* on linear map on Hash.
>> 
>> [1]: https://lore.kernel.org/all/877cc2fpi2.fsf@mail.lhotse/#t
>> 
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> ---
>>   arch/powerpc/mm/pageattr.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
>> index ac22bf28086f..c8c2d664c6f3 100644
>> --- a/arch/powerpc/mm/pageattr.c
>> +++ b/arch/powerpc/mm/pageattr.c
>> @@ -94,8 +94,11 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
>>          if (!radix_enabled()) {
>>                  int region = get_region_id(addr);
>> 
>> -               if (WARN_ON_ONCE(region != VMALLOC_REGION_ID && region != IO_REGION_ID))
>> +               if (region != VMALLOC_REGION_ID && region != IO_REGION_ID) {
>> +                       pr_warn_once("%s: possible use of set_memory_* on linear map on Hash from (%ps)\n",
>> +                                       __func__, __builtin_return_address(0));
>
> Is it really only linear map ?
>
> What about "possible user of set_memory_* outside of vmalloc or io region.

"warning: possible user of set_memory_* outside of vmalloc and io region."

I am thinking of adding a word "warning" too. I can make above change and send v2.

>
> Maybe a show_stack() would also be worth it ?

IMO, since we have the caller, we need not pollute the dmesg with the
entire call stack. Besides I am not aware of dump_stack_once() style prints.

>
>
> But in principle I think it would be better to keep the WARN_ONCE until 
> we can add __must_check to set_memory_xxx() functions to be sure all 
> callers check the result, as mandated by 
> https://github.com/KSPP/linux/issues/7

Fixing the callers to check the return value is something that need not
depend on this change no?

The intention of this change is to mainly remove the heavy WARN_ON_ONCE
from powerpc specific change_memory_attr() and convert to printk warn.

-ritesh

Re: [PATCH] powerpc: Use printk instead of WARN in change_memory_attr

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 27/08/2024 à 11:12, Ritesh Harjani (IBM) a écrit :
>> [Vous ne recevez pas souvent de courriers de ritesh.list@gmail.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>> 
>> Use pr_warn_once instead of WARN_ON_ONCE as discussed here [1]
>> for printing possible use of set_memory_* on linear map on Hash.
>> 
>> [1]: https://lore.kernel.org/all/877cc2fpi2.fsf@mail.lhotse/#t
>> 
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> ---
>>   arch/powerpc/mm/pageattr.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
>> index ac22bf28086f..c8c2d664c6f3 100644
>> --- a/arch/powerpc/mm/pageattr.c
>> +++ b/arch/powerpc/mm/pageattr.c
>> @@ -94,8 +94,11 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
>>          if (!radix_enabled()) {
>>                  int region = get_region_id(addr);
>> 
>> -               if (WARN_ON_ONCE(region != VMALLOC_REGION_ID && region != IO_REGION_ID))
>> +               if (region != VMALLOC_REGION_ID && region != IO_REGION_ID) {
>> +                       pr_warn_once("%s: possible use of set_memory_* on linear map on Hash from (%ps)\n",
>> +                                       __func__, __builtin_return_address(0));
>
> Is it really only linear map ?
>
> What about "possible user of set_memory_* outside of vmalloc or io region.

Yeah that's more accurate. Can drop "possible" too I think? We're pretty
sure that's what happened.

> Maybe a show_stack() would also be worth it ?
>
>
> But in principle I think it would be better to keep the WARN_ONCE until 
> we can add __must_check to set_memory_xxx() functions to be sure all 
> callers check the result, as mandated by 
> https://github.com/KSPP/linux/issues/7

Will that help in this case though?

If the caller checks the return value they'll see that the call failed,
and presumably error out, possibly breaking boot or something else crucial.

Unless we change change_memory_attr() to return EOPNOTSUPP or something
in this case, and expect callers to handle that.

cheers