Re: [PATCH] io_apic: initialize irq with -EINVAL

[Norbert Manthey]

On 11/30/18 13:14, Thomas Gleixner wrote:
> Norbert,
>
> On Wed, 28 Nov 2018, Norbert Manthey wrote:
>
> thanks for the patch.
>
>> Subject: [PATCH] io_apic: initialize irq with -EINVAL
> io_apic is not a proper subsystem prefix. git log should give you a hint:
>
>    x86/ioapic: ....
>
> Also please start the first word after the colon with an uppercase letter.
>
> Now 'Initialize irq with -EINVAL' is not really informative. It tells what
> the patch does, but does not give a consise hint, what this is about and
> why you want to do it. Something like:
>
>    x86/ioapic: Prevent uninitialized return value
>
> provides precise information about the scope of the patch.
>
>> To catch the case where the uninitialized variable irq might be
>> returned.
> I had to read this incomplete sentence 3 times until I discovered that this
> is the second part of the subject line. Please don't do that.
>
>> As the path that might lead to this situation can only
>> occur based on invalid arguments, we initialize this variable with
>> the value -EINVAL, so that callers are notified accordingly, and no
>> uninitialized value is returned.
>>
>> The path that would allow to return an uninitialized value for the
>> variable irq would require legacy IRQs without the ALLOC flag.
> Ideally you describe the problem first and not elaborate lengthy on the
> solution, because the solution is obvious once the problem is
> clear. Something like this:
>
>   mp_map_pin_to_irq() has an exit path which returns an uninitialized
>   variable. This is reached, when  called with arguments ......
>
>   Initialize 'irq' with -EINVAL to prevent that.
>
>> Signed-off-by: Norbert Manthey <nmanthey@xxxxxxxxx>
>> Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
> This SOB chain is wrong. How is David involved in this? If he co-developed
> the patch, then a 'Co-Developed-by: David...' tag is missing. If not, then
> his SOB is just wrong here.
>
>> ---
>>  arch/x86/kernel/apic/io_apic.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
>> index 2953bbf..219dbc1 100644
>> --- a/arch/x86/kernel/apic/io_apic.c
>> +++ b/arch/x86/kernel/apic/io_apic.c
>> @@ -1031,7 +1031,7 @@ static int alloc_isa_irq_from_domain(struct irq_domain *domain,
>>  static int mp_map_pin_to_irq(u32 gsi, int idx, int ioapic, int pin,
>>  			     unsigned int flags, struct irq_alloc_info *info)
>>  {
>> -	int irq;
>> +	int irq = -EINVAL;
>>  	bool legacy = false;
>>  	struct irq_alloc_info tmp;
>>  	struct mp_chip_data *data;
> Now, lets look at the actual error path:
>
>         if (!(flags & IOAPIC_MAP_ALLOC)) {
>                 if (!legacy) {
>                         irq = irq_find_mapping(domain, pin);
>                         if (irq == 0)
>                                 irq = -ENOENT;
>                 }
> 		 <----------- (i.e. if legacy == true)
> 	}
>
> That looks about right, but to get there something must set legacy to
> 'true' and the only code path which does so is:
>
>         if (idx >= 0 && test_bit(mp_irqs[idx].srcbus, mp_bus_not_pci)) {
>                 irq = mp_irqs[idx].srcbusirq;
>                 legacy = mp_is_legacy_irq(irq);
>         }
>
> and this code path actually initializes 'irq'. So returning uninitialized
> 'irq' cannot happen.
>
> How did you find that? Code inspection, static checker, ... ?

I did a manual code inspection, actually reasoning about something else
and during that just spotted the irq variable. Apparently, I missed the
case that handles writing to the variable "legacy" and hence blocks the
faulty exit path.

Thanks for all the constructive criticism!

Best,
Norbert

>
> Thanks,
>
> 	tglx




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B