Re: [RFC v1 1/1] vfio-ccw: Don't call cp_free if we are processing a channel program

[Farhan Ali <alifm@linux.ibm.com>]

On 06/21/2019 01:40 PM, Eric Farman wrote:
> 
> 
> On 6/21/19 10:17 AM, Farhan Ali wrote:
>>
>>
>> On 06/20/2019 04:27 PM, Eric Farman wrote:
>>>
>>>
>>> On 6/20/19 3:40 PM, Farhan Ali wrote:
>>>> There is a small window where it's possible that an interrupt can
>>>> arrive and can call cp_free, while we are still processing a channel
>>>> program (i.e allocating memory, pinnging pages, translating
>>>
>>> s/pinnging/pinning/
>>>
>>>> addresses etc). This can lead to allocating and freeing at the same
>>>> time and can cause memory corruption.
>>>>
>>>> Let's not call cp_free if we are currently processing a channel program.
>>>
>>> The check around this cp_free() call is for a solicited interrupt, so
>>> it's presumably in response to a SSCH we issued.  But if we're still
>>> processing a CP, then we hadn't issued the SSCH to the hardware yet.  So
>>> what is this interrupt for?  Do the contents of irb.cpa provide any
>>> clues, perhaps if it's in the current cp or for someone else?
>>>
>>
>> I don't think the interrupt is in response to an ssch but rather due to
>> an csch/hsch.
>>
>>>>
>>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>>>> ---
>>>>
>>>> I have been running my test overnight with this patch and I haven't
>>>> seen the stack traces that I mentioned about earlier. I would like
>>>> to get some reviews on this and also if this is the right thing to
>>>> do?
>>>>
>>>> Thanks
>>>> Farhan
>>>>
>>>>    drivers/s390/cio/vfio_ccw_drv.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c
>>>> b/drivers/s390/cio/vfio_ccw_drv.c
>>>> index 66a66ac..61ece3f 100644
>>>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>>>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>>>> @@ -88,7 +88,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct
>>>> *work)
>>>>                 (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));
>>>>        if (scsw_is_solicited(&irb->scsw)) {
>>>>            cp_update_scsw(&private->cp, &irb->scsw);
>>>
>>> As I alluded earlier, do we know this irb is for this cp?  If no, what
>>> does this function end up putting in the scsw?
>>>
>>>> -        if (is_final)
>>>> +        if (is_final && private->state != VFIO_CCW_STATE_CP_PROCESSING)
>>>
>>> In looking at how we set this state, and how we exit it, I see we do:
>>>
>>> if SSCH got CC0, CP_PROCESSING -> CP_PENDING
>>> if SSCH got !CC0, CP_PROCESSING -> IDLE
>>>
>>> While the first scenario happens immediately after the SSCH instruction,
>>> I guess it could be just tiny enough, like the io_trigger FSM patch I
>>> sent a few weeks ago.
>>>
>>> Meanwhile, the latter happens way after we return from the jump table.
>>> So that scenario leaves considerable time for such an interrupt to
>>> occur, though I don't understand why it would if we got a CC(1-3) on the
>>> SSCH.
>>>
>>> And anyway, the return from fsm_io_helper() in that case will also call
>>> cp_free().  So why does the cp->initialized check provide protection
>>> from a double-free in that direction, but not here?  I'm confused.
>>
>> I have a theory where I think it's possible to have 2 different threads
>> executing cp_free
>>
>> If we start with private->state == IDLE and the guest issues a
>> clear/halt and then an ssch
>>
>> - clear/halt will be issued to hardware, and if succeeds we will return
>> cc=0 to guest
>>
>> - the guest can then issue ssch
> 
> It can issue whatever it wants, but shouldn't the SSCH get a CC2 until
> the halt/clear pending state is cleared?  Hrm, fsm_io_request() doesn't
> look.  Rather, it (fsm_io_helper()) relies on the CC2 to be signalled
> from the SSCH issued to the device.  There's a lot of stuff that happens
> before we get to that point.

Yes, and stuff that happens is memory allocation, pinning and other 
things which can take time.

> 
> I'm wondering if there's a way we could/should return the SSCH here
> before we do any processing.  After all, until the interrupt on the
> workqueue is processed, we are busy.
> 

you mean return the csch/hsch before processing the ssch? Maybe we could 
extend CP_PENDING state, to just PENDING and use that to reject any ssch 
if we have a pending csch/hsch?

>>
>> - we get an interrupt for csch/hsch and we queue the interrupt in the
>> workqueue
>>
>> - we start processing the ssch and then at the same time another cpu
>> could be working on the
>> interrupt>
>>
>> Thread 1                                        Thread 2
>> --------                                        --------
>>
>> fsm_io_request                                  vfio_ccw_sch_io_todo
>>      cp_init                                         cp_free
>>      cp_prefetch
>>      fsm_io_helper
>>          cp_free
>>
>>
>>
>> The test that I am trying is with a guest running an fio workload, while
>> at the same time stressing the error recovery path in the guest. So
>> there is a lot of ssch and lot of csch.
>>
>> Of course I don't think my patch completely solves the problem, I think
>> it just makes the window narrower. I just wanted to get a discussion
>> started :)
>>
>>
>> Now that I am thinking more about it, I think we might have to protect
>> cp with it's own mutex.
> 
> That seems like a big hammer, and I wonder if the existing SCHIB/FSM/CP
> state data doesn't provide that information to us.  But I gotta wander
> around some code before I say.

Any ideas are welcome :)

> 
>>
>> Thanks
>> Farhan
>>
>>
>>>
>>>>                cp_free(&private->cp);
>>>>        }
>>>>        mutex_lock(&private->io_mutex);
>>>>
>>>
>>
>