Re: [PATCH arm/aspeed/ast2500 v2] ipmi: add an Aspeed KCS IPMI BMC driver

["Wang, Haiyue" <haiyue.wang@linux.intel.com>]

On 2018-01-25 01:48, Corey Minyard wrote:
> On 01/24/2018 10:06 AM, Haiyue Wang wrote:
>> The KCS (Keyboard Controller Style) interface is used to perform in-band
>> IPMI communication between a server host and its BMC (BaseBoard 
>> Management
>> Controllers).
>>
>> This driver exposes the KCS interface on ASpeed SOCs (AST2400 and 
>> AST2500)
>> as a character device. Such SOCs are commonly used as BMCs and this 
>> driver
>> implements the BMC side of the KCS interface.
>>
>> Signed-off-by: Haiyue Wang <haiyue.wang@linux.intel.com>
>>
>> ---
>
>> +
>> +static ssize_t kcs_bmc_read(struct file *filp, char *buf,
>> +                size_t count, loff_t *offset)
>> +{
>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>> +    ssize_t ret = -EAGAIN;
>> +
>
> This function still has some issues.
>
> You can't call copy_to_user() with a spinlock held or interrupts 
> disabled.
> To handle readers, you probably need a separate mutex.
>
> Also, this function can return -EAGAIN even if O_NONBLOCK is not set if
> kcs_bmc->data_in_avail changes between when you wait on the event
> and when you check it under the lock.
>
> You also clear data_in_avail even if the copy_to_user() fails, which is
> wrong.
>
> I believe the best way to handle this would be to have the spinlock
> protect the inner workings of the state machine and a mutex handle
> copying data out, setting/clearing the running flag (thus a mutex
> instead of spinlock in open and release) and the ioctl settings (except
> for abort where you will need to grab the spinlock).
>
> After the wait event below, grab the mutex.  If data is not available
> and O_NONBLOCK is not set, drop the mutex and retry.  Otherwise
> this is the only place (besides release) that sets data_in_avail to 
> false.
> Do the copy_to_user(), grab the spinlock, clear data_in_avail and
> data_in_idx, then release the lock and mutex.  If you are really
> adventurous you can do this without grabbing the lock using
> barriers, but it's probably not necessary here.
>
The main race is data_in and data_out memory copy from & to between one 
user-land (ipmid) and
the irq handler. If separates the copy_to_user into two parts: check the 
'access_ok(VERIFY_WRITE, to, n)',
if no errors, then grap the spinlock and irq disabled, then 
'memcpy((void __force *)to, from, n);' It it right
calling ?

I will add a mutex to avoid spinlcok using as possible.
>> +    if (!(filp->f_flags & O_NONBLOCK))
>> +        wait_event_interruptible(kcs_bmc->queue,
>> +                     kcs_bmc->data_in_avail);
>> +
>> +    spin_lock_irq(&kcs_bmc->lock);
>> +
>> +    if (kcs_bmc->data_in_avail) {
>> +        kcs_bmc->data_in_avail = false;
>> +
>> +        if (count > kcs_bmc->data_in_idx)
>> +            count = kcs_bmc->data_in_idx;
>> +
>> +        if (!copy_to_user(buf, kcs_bmc->data_in, count))
>> +            ret = count;
>> +        else
>> +            ret = -EFAULT;
>> +    }
>> +
>> +    spin_unlock_irq(&kcs_bmc->lock);
>> +
>> +    return ret;
>> +}
>> +
>
>> +    }
>> +
>> +    spin_unlock_irq(&kcs_bmc->lock);
>> +
>> +    return ret;
>> +}
>