[PATCH] io_apic: initialize irq with -EINVAL

[PATCH] io_apic: initialize irq with -EINVAL

[Norbert Manthey]

To catch the case where the uninitialized variable irq might be
returned. 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.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 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;
-- 
2.7.4




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

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

[Thomas Gleixner]

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@amazon.de>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

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, ... ?

Thanks,

	tglx

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@amazon.de>
>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> 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