linux-kernel - Re: [PATCH] clockevents/drivers/cs5535: improve resilience to spurious interrupts

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]

Message-ID: <alpine.LRH.2.21.1710200930370.3927@linux.fjfi.cvut.cz>
Date:   Fri, 20 Oct 2017 09:38:57 +0200 (CEST)
From:   David Kozub <zub@...ux.fjfi.cvut.cz>
To:     Thomas Gleixner <tglx@...utronix.de>
cc:     Daniel Lezcano <daniel.lezcano@...aro.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] clockevents/drivers/cs5535: improve resilience to spurious
 interrupts

On Fri, 20 Oct 2017, Thomas Gleixner wrote:

> On Thu, 19 Oct 2017, David Kozub wrote:
>
>> This solves a BUG on ALIX 2c3 where mfgpt_tick is called before
>> clockevents_config_and_register returns. This caused mfgpt_tick to call a
>> null function pointer.
>>
>> Thanks to Daniel Lezcano and Thomas Gleixner for helping me analyze this
>> and suggesting a solution.
>
> Thanks for sending this! Though I have two minor issues with this:
>
> 1) The changelog.
>
> We try to structure changelogs by explaining the context, the problem and
> its consequences and the fix. If the fix is non obvious it wants an
> elaborate explanation. Let me give you an example:
>
>   The interrupt handler mfgpt_tick() is not robust versus spurious
>   interrupts which happen before the clock event device is registered and
>   fully initialized.
>
>   The reason is that the safe guard against spurious interrupts solely
>   checks for the clockevents shutdown state, but lacks a check for
>   detached state. If the interrupt hits while the device is in detached
>   state it passes the safe guard and dereferences the event handler call
>   back which is NULL.
>
>   Add the missing state check.
>
> See?

Thanks for the feedback. Yes, that is better.

Now that I'm neither the author of the code nor the author of the commit 
message it doesn't seem right that I should be submitting this.

>> -	if (clockevent_state_shutdown(&cs5535_clockevent))
>> +	if (clockevent_state_detached(&cs5535_clockevent) ||
>> +			clockevent_state_shutdown(&cs5535_clockevent))
>
> Please do not use random indentation for multiline conditionals
>
> 	if (clockevent_state_detached(&cs5535_clockevent) ||
> 	    clockevent_state_shutdown(&cs5535_clockevent))
>
> is the style we use through out the code.

OK. I noticed the four spaces. I was swayed by process/coding-style.rst:

Outside of comments, documentation and except in Kconfig, spaces are never 
used for indentation,

But now I think I just misunderstood that. The spaces on the second line 
of the "if" are not regarded as ident.


Best regards,
David