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 <nmant...@amazon.de>
>> Signed-off-by: David Woodhouse <d...@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
