linux-kernel - Re: [PATCH V1 1/5] irqchip/loongson-eiointc: Fix returned value on parsing MADT

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 27 Mar 2023 11:58:57 +0800
From:   Jianmin Lv <lvjianmin@...ngson.cn>
To:     loongson-kernel@...ts.loongnix.cn
Cc:     Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org,
        loongarch@...ts.linux.dev,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Jiaxun Yang <jiaxun.yang@...goat.com>,
        Huacai Chen <chenhuacai@...ngson.cn>
Subject: Re: [PATCH V1 1/5] irqchip/loongson-eiointc: Fix returned value on
 parsing MADT



On 2023/3/24 下午11:36, Marc Zyngier wrote:
> On Fri, 24 Mar 2023 06:08:50 +0000,
> Jianmin Lv <lvjianmin@...ngson.cn> wrote:
>>
>> In pch_pic_parse_madt(), a NULL parent pointer will be
>> returned from acpi_get_vec_parent() for second pch-pic domain
>> related to second bridge while calling eiointc_acpi_init() at
>> first time, where the parent of it has not been initialized
>> yet, and will be initialized during second time calling
>> eiointc_acpi_init(). So, it's reasonable to return zero so
>> that failure of acpi_table_parse_madt() will be avoided, or else
>> acpi_cascade_irqdomain_init() will return and initialization of
>> followed pch_msi domain will be skipped.
>>
>> Although it does not matter when pch_msi_parse_madt() returns
>> -EINVAL if no invalid parent is found, it's also reasonable to
>> return zero for that.
>>
>> Change-Id: I4d278534999ec3e5c8db6d40155ba2665d9de86f
>> Signed-off-by: Jianmin Lv <lvjianmin@...ngson.cn>
>> ---
>>   drivers/irqchip/irq-loongson-eiointc.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-loongson-eiointc.c b/drivers/irqchip/irq-loongson-eiointc.c
>> index d15fd38c1756..62a632d73991 100644
>> --- a/drivers/irqchip/irq-loongson-eiointc.c
>> +++ b/drivers/irqchip/irq-loongson-eiointc.c
>> @@ -343,7 +343,7 @@ static int __init pch_pic_parse_madt(union acpi_subtable_headers *header,
>>   	if (parent)
>>   		return pch_pic_acpi_init(parent, pchpic_entry);
>>   
>> -	return -EINVAL;
>> +	return 0;
> 
> Why can't you detect this particular case instead of blindly
> suppressing the error?
> 

For dual-bridges scenario, parent handle for pch_pic IRQ domain will be 
set by acpi_set_vec_parent() during each eiointc initialization. And
the parent handle for one pch_pic will be set during *first* eiointc
entry parsing, and the parent handle for another pch_pic will be
set during *second* eiointc entry parsing. But two pch_pic entries
will be parsed during each eiointc entry parsing, so a NULL parent
pointer must be returned druing first eiointc initialization, which
is reasonable and should not be treated as an error.

The calling stack of pch_pic_parse_madt (where acpi_get_vec_parent is 
called to get parent handle) is like this:

cpuintc_acpi_init
   acpi_cascade_irqdomain_init(in cpuintc driver)
     acpi_table_parse_madt
       eiointc_parse_madt
         eiointc_acpi_init /* this will be called two times 
corresponding  to parsing two eiointc entries in MADT under dual-bridges 
scenario*/
           acpi_set_vec_parent /* set parent handle for one pch_pic 
during first eiointc entry parsing, and set parent handle for another 
pch_pic during second eiointc entry parsing*/
             acpi_cascade_irqdomain_init(in eiointc driver)
               acpi_table_parse_madt
                 pch_pic_parse_madt /* this will be called twice because 
of two pchpic entries, and only one valid parent handle will be returned 
from acpi_get_vec_parent() during first eiointc entry parsing (another 
parent handle is not set yet), so a NULL parent pointer must be returned 
during first eiointc entry parsing, which is reasonable and should not 
be treated as an error.*/

Thanks!

> 	M.
>