linux-kernel - Re: [PATCH arm/aspeed/ast2500 v1] ipmi: add an Aspeed KCS IPMI BMC driver

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: <39da73d5-23da-c2b3-7fd2-b9c6c7e293ac@linux.intel.com>
Date:   Thu, 18 Jan 2018 08:57:43 +0800
From:   "Wang, Haiyue" <haiyue.wang@...ux.intel.com>
To:     minyard@....org, joel@....id.au, openbmc@...ts.ozlabs.org,
        openipmi-developer@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org
Cc:     andriy.shevchenko@...el.com
Subject: Re: [PATCH arm/aspeed/ast2500 v1] ipmi: add an Aspeed KCS IPMI BMC
 driver



On 2018-01-18 00:31, Corey Minyard wrote:
> On 01/17/2018 08:31 AM, Wang, Haiyue wrote:
>>
>>
>
> Snip...
>
>>>>>
>>>>> +
>>>>> +struct kcs_bmc {
>>>>> +    struct regmap *map;
>>>>> +    spinlock_t     lock;
>>>>
>>>> This lock is only used in threads, as far as I can tell. Couldn't 
>>>> it just be a normal mutex?
>>>> But more on this later.
>>>>
>> I missed this lock using in KCS ISR function, for AST2500 is single 
>> core CPU. The critical data such as
>> data_in_avail is shared between ISR and user thread, spinlock_t 
>> related API should be the right one ?
>> especially for SMP ?
>>
>
> Sort of.  In the case below, you need to use spin_lock_irqsave(), you 
> don't necessarily get
> here with interrupts disabled.
>
> In the ones called from user context, you should really use 
> spin_lock_irq().  Interrupts
> should always be on at that point, so it's better.
>
Understood, will change it with the right API call.
>> static irqreturn_t kcs_bmc_irq(int irq, void *arg)
>> {
>>     ....
>>     spin_lock(&kcs_bmc->lock);  // <-- MISSED
>>
>>     switch (sts) {
>>     case KCS_STR_IBF | KCS_STR_CMD_DAT:
>>         kcs_rx_cmd(kcs_bmc);
>>         break;
>>
>>     case KCS_STR_IBF:
>>         kcs_rx_data(kcs_bmc);
>>         break;
>>
>>     default:
>>         ret = IRQ_NONE;
>>         break;
>>     }
>>
>>     spin_unlock(&kcs_bmc->lock); // <-- MISSED
>>
>>     return ret;
>> }
>>
>>
>
>>>>> + spin_lock_irqsave(&kcs_bmc->lock, flags);
>>>>> +    if (kcs_bmc->kcs_phase == KCS_PHASE_READ) {
>>>>
>>>> If you don't modify kcs_phase here, you have a race condition. You 
>>>> probably
>>>> need a KCS_WAIT_READ condition.  Also, the nomenclature of "read" 
>>>> and "write"
>>>> here is a little confusing, since your phases are from the host's 
>>>> point of view,
>>>> not this driver's point of view.  You might want to document that 
>>>> explicitly.
>>>>
>> The race condition means that the user MAY write the duplicated 
>> response ?
>
> Not exactly.  Two threads can call this, and if it hasn't transitions 
> from the read phase,
> the data out will be overwritten.
>
OK, will add new state KCS_WAIT_READ handling.