On Thu, 24 Jan 2019 21:37:44 -0500
Eric Farman <farman@xxxxxxxxxxxxx> wrote:
> >> @@ -188,25 +192,30 @@ static ssize_t vfio_ccw_mdev_write(struct
> >> mdev_device *mdev,
> >> {
> >> struct vfio_ccw_private *private;
> >> struct ccw_io_region *region;
> >> + int ret;
> >> if (*ppos + count > sizeof(*region))
> >> return -EINVAL;
> >> private = dev_get_drvdata(mdev_parent_dev(mdev));
> >> - if (private->state != VFIO_CCW_STATE_IDLE)
> >> + if (private->state == VFIO_CCW_STATE_NOT_OPER ||
> >> + private->state == VFIO_CCW_STATE_STANDBY)
> >> return -EACCES;
> >> + if (!mutex_trylock(&private->io_mutex))
> >> + return -EAGAIN;
> >
> > Ah, I see Halil's difficulty here.
> >
> > It is true there is a race condition today, and that this doesn't
> > address it. That's fine, add it to the todo list. But even with that,
> > I don't see what the mutex is enforcing? Two simultaneous SSCHs will be
> > serialized (one will get kicked out with a failed trylock() call), while
> > still leaving the window open between cc=0 on the SSCH and the
> > subsequent interrupt. In the latter case, a second SSCH will come
> > through here, do the copy_from_user below, and then jump to fsm_io_busy
> > to return EAGAIN. Do we really want to stomp on io_region in that case?
> > Why can't we simply return EAGAIN if state==BUSY?
>
> (Answering my own questions as I skim patch 5...)
>
> Because of course this series is for async handling, while I was looking
> specifically at the synchronous code that exists today. I guess then my
> question just remains on how the mutex is adding protection in the sync
> case, because that's still not apparent to me. (Perhaps I missed it in
> a reply to Halil; if so I apologize, there were a lot when I returned.)
Careful, at the end we have vfio_ccw_mdev_write_io_region() and the
write callback for the capchain regions. We could return EAGAIN if
state==BUSY in the vfio_ccw_mdev_write_io_region() (but I would prefer a
different error code -- see my other response).
I answered your mutex question as well. Just a small addendum the mutex
is not only for the cases the userspace acts sane (but also when it acts
insane;).
Halil
