| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6f86e0d5-338e-6321-dacd-8666a30e8f09@gmail.com>
Date: Fri, 14 May 2021 23:15:16 +0200
From: Heiner Kallweit <hkallweit1@...il.com>
To: Jason Baron <jbaron@...mai.com>, Joe Perches <joe@...ches.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] x86/e820: Use pr_debug to avoid spamming dmesg log
with debug messages
On 14.05.2021 22:32, Jason Baron wrote:
>
>
> On 5/14/21 4:22 PM, Joe Perches wrote:
>> On Fri, 2021-05-14 at 15:56 -0400, Jason Baron wrote:
>>>
>>> On 5/14/21 1:38 PM, Joe Perches wrote:
>>>> On Tue, 2021-05-11 at 17:31 -0400, Jason Baron wrote:
>>>>
>>>>> That said, I do see the value in not having to open code the branch stuff, and
>>>>> making pr_debug() consistent with printk which does return a value. So that
>>>>> makes sense to me.
>>>>
>>>> IMO: printk should not return a value.
>>>>
>>>
>>> Ok, the issue we are trying to resolve is to control whether a 'pr_debug()' statement
>>> is enabled and thus use that to control subsequent output. The proposed patch does:
>>>
>>>
>>> + printed = pr_debug("e820: update [mem %#010Lx-%#010Lx] ", start, end - 1);
>>> + if (printed > 0) {
>>> + e820_print_type(old_type);
>>> + pr_cont(" ==> ");
>>> + e820_print_type(new_type);
>>> + pr_cont("\n");
>>> + }
>>>
>>> I do think pr_debug() here is different from printk() b/c it can be explicitly
>>> toggled.
>>>
>>> I also suggested an alternative, which is possible with the current code which
>>> is to use DYNAMIC_DEBUG_BRANCH().
>>>
>>> if (DYNAMIC_DEBUG_BRANCH(e820_debg)) {
>>> printk(KERN_DEBUG "e820: update [mem %#010Lx-%#010Lx] ", start, end - 1);
>>> e820_print_type(old_type);
>>> pr_cont(" ==> ");
>>> e820_print_type(new_type);
>>> pr_cont("\n");
>>> }
>>>
>>> That however does require one to do something like this first:
>>>
>>> DEFINE_DYNAMIC_DEBUG_METADATA(e820_dbg, "e820 verbose mode");
>>>
>>> So I don't feel too strongly either way, but maybe others do?
>>
>> Why not avoid the problem by using temporaries on the stack
>> and not use pr_cont altogether?
>
> Nice. That's fine with me Heiner?
>
I once tested a similar approach, just using kasprintf() to dynamically
allocate the substrings. Still using pr_cont() looks a little bit
more elegant to me, but it has its drawbacks. Having said that:
Yes, fine with me (with pr_debug).
> I think though Heiner wants to use pr_debug() below instead of printk
> with KERN_DEBUG, at least that was my understanding. But that would
> be a simpple s/printk/pr_debug().
>
> Thanks,
>
> -Jason
>
Heiner
>
>
>
>> ---
>> arch/x86/kernel/e820.c | 71 ++++++++++++++++++++++++++++++++------------------
>> 1 file changed, 46 insertions(+), 25 deletions(-)
>>
>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>> index bc0657f0deed..a6e7ab4b522b 100644
>> --- a/arch/x86/kernel/e820.c
>> +++ b/arch/x86/kernel/e820.c
>> @@ -184,20 +184,38 @@ void __init e820__range_add(u64 start, u64 size, enum e820_type type)
>> __e820__range_add(e820_table, start, size, type);
>> }
>>
>> -static void __init e820_print_type(enum e820_type type)
>> +static char * __init e820_fmt_type(enum e820_type type, char *buf, size_t size)
>> {
>> switch (type) {
>> - case E820_TYPE_RAM: /* Fall through: */
>> - case E820_TYPE_RESERVED_KERN: pr_cont("usable"); break;
>> - case E820_TYPE_RESERVED: pr_cont("reserved"); break;
>> - case E820_TYPE_SOFT_RESERVED: pr_cont("soft reserved"); break;
>> - case E820_TYPE_ACPI: pr_cont("ACPI data"); break;
>> - case E820_TYPE_NVS: pr_cont("ACPI NVS"); break;
>> - case E820_TYPE_UNUSABLE: pr_cont("unusable"); break;
>> - case E820_TYPE_PMEM: /* Fall through: */
>> - case E820_TYPE_PRAM: pr_cont("persistent (type %u)", type); break;
>> - default: pr_cont("type %u", type); break;
>> + case E820_TYPE_RAM:
>> + case E820_TYPE_RESERVED_KERN:
>> + strscpy(buf, "usable", size);
>> + break;
>> + case E820_TYPE_RESERVED:
>> + strscpy(buf, "reserved", size);
>> + break;
>> + case E820_TYPE_SOFT_RESERVED:
>> + strscpy(buf, "soft reserved", size);
>> + break;
>> + case E820_TYPE_ACPI:
>> + strscpy(buf, "ACPI data", size);
>> + break;
>> + case E820_TYPE_NVS:
>> + strscpy(buf, "ACPI NVS", size);
>> + break;
>> + case E820_TYPE_UNUSABLE:
>> + strscpy(buf, "unusable", size);
>> + break;
>> + case E820_TYPE_PMEM:
>> + case E820_TYPE_PRAM:
>> + scnprintf(buf, size, "persistent (type %u)", type);
>> + break;
>> + default:
>> + scnprintf(buf, size, "type %u", type);
>> + break;
>> }
>> +
>> + return buf;
>> }
>>
>> void __init e820__print_table(char *who)
>> @@ -205,13 +223,14 @@ void __init e820__print_table(char *who)
>> int i;
>>
>> for (i = 0; i < e820_table->nr_entries; i++) {
>> - pr_info("%s: [mem %#018Lx-%#018Lx] ",
>> + char type[32];
>> +
>> + pr_info("%s: [mem %#018Lx-%#018Lx] %s\n",
>> who,
>> e820_table->entries[i].addr,
>> - e820_table->entries[i].addr + e820_table->entries[i].size - 1);
>> -
>> - e820_print_type(e820_table->entries[i].type);
>> - pr_cont("\n");
>> + e820_table->entries[i].addr + e820_table->entries[i].size - 1,
>> + e820_fmt_type(e820_table->entries[i].type,
>> + type, sizeof(type)));
>> }
>> }
>>
>> @@ -465,6 +484,8 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty
>> u64 end;
>> unsigned int i;
>> u64 real_updated_size = 0;
>> + char type1[32];
>> + char type2[32];
>>
>> BUG_ON(old_type == new_type);
>>
>> @@ -472,11 +493,10 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty
>> size = ULLONG_MAX - start;
>>
>> end = start + size;
>> - printk(KERN_DEBUG "e820: update [mem %#010Lx-%#010Lx] ", start, end - 1);
>> - e820_print_type(old_type);
>> - pr_cont(" ==> ");
>> - e820_print_type(new_type);
>> - pr_cont("\n");
>> + printk(KERN_DEBUG "e820: update [mem %#010Lx-%#010Lx] %s ==> %s\n",
>> + start, end - 1,
>> + e820_fmt_type(old_type, type1, sizeof(type1)),
>> + e820_fmt_type(new_type, type2, sizeof(type2)));
>>
>> for (i = 0; i < table->nr_entries; i++) {
>> struct e820_entry *entry = &table->entries[i];
>> @@ -543,15 +563,16 @@ u64 __init e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool
>> int i;
>> u64 end;
>> u64 real_removed_size = 0;
>> + char type[32];
>>
>> if (size > (ULLONG_MAX - start))
>> size = ULLONG_MAX - start;
>>
>> end = start + size;
>> - printk(KERN_DEBUG "e820: remove [mem %#010Lx-%#010Lx] ", start, end - 1);
>> - if (check_type)
>> - e820_print_type(old_type);
>> - pr_cont("\n");
>> + printk(KERN_DEBUG "e820: remove [mem %#010Lx-%#010Lx]%s%s\n",
>> + start, end - 1,
>> + check_type ? " " : "",
>> + check_type ? e820_fmt_type(old_type, type, sizeof(type)) : "");
>>
>> for (i = 0; i < e820_table->nr_entries; i++) {
>> struct e820_entry *entry = &e820_table->entries[i];
>>
>>
Powered by blists - more mailing lists