Messages in this thread | | | Subject | Re: [PATCH arm/aspeed/ast2500 v2] ipmi: add an Aspeed KCS IPMI BMC driver | From | "Wang, Haiyue" <> | Date | Fri, 26 Jan 2018 14:26:04 +0800 |
| |
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; >> +} >
| |