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

[Wang, Haiyue]

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@xxxxxxxxxxxxxxx>
> >
> > ---
>
> > +
> > +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;
> > +}