[RFC,v2,4/5] vfio-ccw: Don't call cp_free if we are processing a channel program
diff mbox series

Message ID 1405df8415d3bff446c22753d0e9b91ff246eb0f.1562616169.git.alifm@linux.ibm.com
State New
Headers show
Series
  • Some vfio-ccw fixes
Related show

Commit Message

Farhan Ali July 8, 2019, 8:10 p.m. UTC
There is a small window where it's possible that we could be working
on an interrupt (queued in the workqueue) and setting up a channel
program (i.e allocating memory, pinning pages, translating address).
This can lead to allocating and freeing the channel program at the
same time and can cause memory corruption.

Let's not call cp_free if we are currently processing a channel program.
The only way we know for sure that we don't have a thread setting
up a channel program is when the state is set to VFIO_CCW_STATE_CP_PENDING.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Cornelia Huck July 9, 2019, 10:16 a.m. UTC | #1
On Mon,  8 Jul 2019 16:10:37 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> There is a small window where it's possible that we could be working
> on an interrupt (queued in the workqueue) and setting up a channel
> program (i.e allocating memory, pinning pages, translating address).
> This can lead to allocating and freeing the channel program at the
> same time and can cause memory corruption.
> 
> Let's not call cp_free if we are currently processing a channel program.
> The only way we know for sure that we don't have a thread setting
> up a channel program is when the state is set to VFIO_CCW_STATE_CP_PENDING.

Can we pinpoint a commit that introduced this bug, or has it been there
since the beginning?

> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  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 4e3a903..0357165 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -92,7 +92,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);
> -		if (is_final)
> +		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)
>  			cp_free(&private->cp);
>  	}
>  	mutex_lock(&private->io_mutex);

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Farhan Ali July 9, 2019, 1:46 p.m. UTC | #2
On 07/09/2019 06:16 AM, Cornelia Huck wrote:
> On Mon,  8 Jul 2019 16:10:37 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>> There is a small window where it's possible that we could be working
>> on an interrupt (queued in the workqueue) and setting up a channel
>> program (i.e allocating memory, pinning pages, translating address).
>> This can lead to allocating and freeing the channel program at the
>> same time and can cause memory corruption.
>>
>> Let's not call cp_free if we are currently processing a channel program.
>> The only way we know for sure that we don't have a thread setting
>> up a channel program is when the state is set to VFIO_CCW_STATE_CP_PENDING.
> 
> Can we pinpoint a commit that introduced this bug, or has it been there
> since the beginning?
> 

I think the problem was always there.

>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> ---
>>   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 4e3a903..0357165 100644
>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>> @@ -92,7 +92,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);
>> -		if (is_final)
>> +		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)
>>   			cp_free(&private->cp);
>>   	}
>>   	mutex_lock(&private->io_mutex);
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
> 
Thanks for reviewing.

Thanks
Farhan
Halil Pasic July 9, 2019, 2:21 p.m. UTC | #3
On Tue, 9 Jul 2019 09:46:51 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> 
> 
> On 07/09/2019 06:16 AM, Cornelia Huck wrote:
> > On Mon,  8 Jul 2019 16:10:37 -0400
> > Farhan Ali <alifm@linux.ibm.com> wrote:
> > 
> >> There is a small window where it's possible that we could be working
> >> on an interrupt (queued in the workqueue) and setting up a channel
> >> program (i.e allocating memory, pinning pages, translating address).
> >> This can lead to allocating and freeing the channel program at the
> >> same time and can cause memory corruption.
> >>
> >> Let's not call cp_free if we are currently processing a channel program.
> >> The only way we know for sure that we don't have a thread setting
> >> up a channel program is when the state is set to VFIO_CCW_STATE_CP_PENDING.
> > 
> > Can we pinpoint a commit that introduced this bug, or has it been there
> > since the beginning?
> > 
> 
> I think the problem was always there.
> 

I think it became relevant with the async stuff. Because after the async
stuff was added we start getting solicited interrupts that are not about
channel program is done. At least this is how I remember the discussion.

> >>
> >> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> >> ---
> >>   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 4e3a903..0357165 100644
> >> --- a/drivers/s390/cio/vfio_ccw_drv.c
> >> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> >> @@ -92,7 +92,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);
> >> -		if (is_final)
> >> +		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)

Ain't private->state potentially used by multiple threads of execution?
Do we need to use atomic operations or external synchronization to avoid
this being another gamble? Or am I missing something?

> >>   			cp_free(&private->cp);
> >>   	}
> >>   	mutex_lock(&private->io_mutex);
> > 
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > 
> > 
> Thanks for reviewing.
> 
> Thanks
> Farhan
Farhan Ali July 9, 2019, 9:27 p.m. UTC | #4
On 07/09/2019 10:21 AM, Halil Pasic wrote:
> On Tue, 9 Jul 2019 09:46:51 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>>
>>
>> On 07/09/2019 06:16 AM, Cornelia Huck wrote:
>>> On Mon,  8 Jul 2019 16:10:37 -0400
>>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>>
>>>> There is a small window where it's possible that we could be working
>>>> on an interrupt (queued in the workqueue) and setting up a channel
>>>> program (i.e allocating memory, pinning pages, translating address).
>>>> This can lead to allocating and freeing the channel program at the
>>>> same time and can cause memory corruption.
>>>>
>>>> Let's not call cp_free if we are currently processing a channel program.
>>>> The only way we know for sure that we don't have a thread setting
>>>> up a channel program is when the state is set to VFIO_CCW_STATE_CP_PENDING.
>>>
>>> Can we pinpoint a commit that introduced this bug, or has it been there
>>> since the beginning?
>>>
>>
>> I think the problem was always there.
>>
> 
> I think it became relevant with the async stuff. Because after the async
> stuff was added we start getting solicited interrupts that are not about
> channel program is done. At least this is how I remember the discussion.
> 
>>>>
>>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>>>> ---
>>>>    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 4e3a903..0357165 100644
>>>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>>>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>>>> @@ -92,7 +92,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);
>>>> -		if (is_final)
>>>> +		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)
> 
> Ain't private->state potentially used by multiple threads of execution?

yes

One of the paths I can think of is a machine check from the host which 
will ultimately call vfio_ccw_sch_event callback which could set state 
to NOT_OPER or IDLE.

> Do we need to use atomic operations or external synchronization to avoid
> this being another gamble? Or am I missing something?

I think we probably should think about atomic operations for 
synchronizing the state (and it could be a separate add on patch?).

But for preventing 2 threads from stomping on the cp the check should be 
enough, unless I am missing something?

> 
>>>>    			cp_free(&private->cp);
>>>>    	}
>>>>    	mutex_lock(&private->io_mutex);
>>>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>
>>>
>> Thanks for reviewing.
>>
>> Thanks
>> Farhan
> 
>
Cornelia Huck July 10, 2019, 1:45 p.m. UTC | #5
On Tue, 9 Jul 2019 17:27:47 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> On 07/09/2019 10:21 AM, Halil Pasic wrote:
> > On Tue, 9 Jul 2019 09:46:51 -0400
> > Farhan Ali <alifm@linux.ibm.com> wrote:
> >   
> >>
> >>
> >> On 07/09/2019 06:16 AM, Cornelia Huck wrote:  
> >>> On Mon,  8 Jul 2019 16:10:37 -0400
> >>> Farhan Ali <alifm@linux.ibm.com> wrote:
> >>>  
> >>>> There is a small window where it's possible that we could be working
> >>>> on an interrupt (queued in the workqueue) and setting up a channel
> >>>> program (i.e allocating memory, pinning pages, translating address).
> >>>> This can lead to allocating and freeing the channel program at the
> >>>> same time and can cause memory corruption.
> >>>>
> >>>> Let's not call cp_free if we are currently processing a channel program.
> >>>> The only way we know for sure that we don't have a thread setting
> >>>> up a channel program is when the state is set to VFIO_CCW_STATE_CP_PENDING.  
> >>>
> >>> Can we pinpoint a commit that introduced this bug, or has it been there
> >>> since the beginning?
> >>>  
> >>
> >> I think the problem was always there.
> >>  
> > 
> > I think it became relevant with the async stuff. Because after the async
> > stuff was added we start getting solicited interrupts that are not about
> > channel program is done. At least this is how I remember the discussion.
> >   
> >>>>
> >>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> >>>> ---
> >>>>    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 4e3a903..0357165 100644
> >>>> --- a/drivers/s390/cio/vfio_ccw_drv.c
> >>>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> >>>> @@ -92,7 +92,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);
> >>>> -		if (is_final)
> >>>> +		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)  
> > 
> > Ain't private->state potentially used by multiple threads of execution?  
> 
> yes
> 
> One of the paths I can think of is a machine check from the host which 
> will ultimately call vfio_ccw_sch_event callback which could set state 
> to NOT_OPER or IDLE.

Now I went through the machine check rabbit hole because I thought
freeing the cp in there might be a good idea, but it's not that easy
(who'd have thought...)

If I read the POP correctly, an IPI or IPR in the subchannel CRW will
indicate that the subchannel has been restored to a state after an I/O
reset; in particular, that means that the subchannel does not have any
I/O pending. However, that does not seem to be the case e.g. for an IPM
(the doc does not seem to be very clear on that, though.) We can't
unconditionally do something, as we do not know what event we're being
called for (please disregard the positively ancient "we're called for
IPI" comment in css_process_crw(), I think I added that one in the
Linux 2.4 or 2.5 timeframe...) tl;dr We can't rely on anything...

> 
> > Do we need to use atomic operations or external synchronization to avoid
> > this being another gamble? Or am I missing something?  
> 
> I think we probably should think about atomic operations for 
> synchronizing the state (and it could be a separate add on patch?).

+1 to thinking about some atomicity changes later.

> 
> But for preventing 2 threads from stomping on the cp the check should be 
> enough, unless I am missing something?

I think so. Plus, the patch is small enough that we can merge it right
away, and figure out a more generic change later.

> 
> >   
> >>>>    			cp_free(&private->cp);
> >>>>    	}
> >>>>    	mutex_lock(&private->io_mutex);  
> >>>
> >>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> >>>
> >>>  
> >> Thanks for reviewing.
> >>
> >> Thanks
> >> Farhan  
> > 
> >
Farhan Ali July 10, 2019, 4:10 p.m. UTC | #6
On 07/10/2019 09:45 AM, Cornelia Huck wrote:
> On Tue, 9 Jul 2019 17:27:47 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>> On 07/09/2019 10:21 AM, Halil Pasic wrote:
>>> On Tue, 9 Jul 2019 09:46:51 -0400
>>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>>    
>>>>
>>>>
>>>> On 07/09/2019 06:16 AM, Cornelia Huck wrote:
>>>>> On Mon,  8 Jul 2019 16:10:37 -0400
>>>>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>>>>   
>>>>>> There is a small window where it's possible that we could be working
>>>>>> on an interrupt (queued in the workqueue) and setting up a channel
>>>>>> program (i.e allocating memory, pinning pages, translating address).
>>>>>> This can lead to allocating and freeing the channel program at the
>>>>>> same time and can cause memory corruption.
>>>>>>
>>>>>> Let's not call cp_free if we are currently processing a channel program.
>>>>>> The only way we know for sure that we don't have a thread setting
>>>>>> up a channel program is when the state is set to VFIO_CCW_STATE_CP_PENDING.
>>>>>
>>>>> Can we pinpoint a commit that introduced this bug, or has it been there
>>>>> since the beginning?
>>>>>   
>>>>
>>>> I think the problem was always there.
>>>>   
>>>
>>> I think it became relevant with the async stuff. Because after the async
>>> stuff was added we start getting solicited interrupts that are not about
>>> channel program is done. At least this is how I remember the discussion.
>>>    
>>>>>>
>>>>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>>>>>> ---
>>>>>>     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 4e3a903..0357165 100644
>>>>>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>>>>>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>>>>>> @@ -92,7 +92,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);
>>>>>> -		if (is_final)
>>>>>> +		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)
>>>
>>> Ain't private->state potentially used by multiple threads of execution?
>>
>> yes
>>
>> One of the paths I can think of is a machine check from the host which
>> will ultimately call vfio_ccw_sch_event callback which could set state
>> to NOT_OPER or IDLE.
> 
> Now I went through the machine check rabbit hole because I thought
> freeing the cp in there might be a good idea, but it's not that easy
> (who'd have thought...)

Thanks for taking a deeper look :)

> 
> If I read the POP correctly, an IPI or IPR in the subchannel CRW will
> indicate that the subchannel has been restored to a state after an I/O
> reset; in particular, that means that the subchannel does not have any
> I/O pending. However, that does not seem to be the case e.g. for an IPM
> (the doc does not seem to be very clear on that, though.) We can't
> unconditionally do something, as we do not know what event we're being
> called for (please disregard the positively ancient "we're called for
> IPI" comment in css_process_crw(), I think I added that one in the
> Linux 2.4 or 2.5 timeframe...) tl;dr We can't rely on anything...

Yes, the CRW infrastructure in Linux does not convey the exact event 
back to the subchannel driver.

> 
>>
>>> Do we need to use atomic operations or external synchronization to avoid
>>> this being another gamble? Or am I missing something?
>>
>> I think we probably should think about atomic operations for
>> synchronizing the state (and it could be a separate add on patch?).
> 
> +1 to thinking about some atomicity changes later.
> 
>>
>> But for preventing 2 threads from stomping on the cp the check should be
>> enough, unless I am missing something?
> 
> I think so. Plus, the patch is small enough that we can merge it right
> away, and figure out a more generic change later.

I will send out a v3 soon if no one else has any other suggestions.

> 
>>
>>>    
>>>>>>     			cp_free(&private->cp);
>>>>>>     	}
>>>>>>     	mutex_lock(&private->io_mutex);
>>>>>
>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>>>
>>>>>   
>>>> Thanks for reviewing.
>>>>
>>>> Thanks
>>>> Farhan
>>>
>>>    
> 
>
Eric Farman July 11, 2019, 12:28 p.m. UTC | #7
On 7/10/19 12:10 PM, Farhan Ali wrote:
> 
> 
> On 07/10/2019 09:45 AM, Cornelia Huck wrote:
>> On Tue, 9 Jul 2019 17:27:47 -0400
>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>
>>> On 07/09/2019 10:21 AM, Halil Pasic wrote:
>>>> Do we need to use atomic operations or external synchronization to
>>>> avoid
>>>> this being another gamble? Or am I missing something?
>>>
>>> I think we probably should think about atomic operations for
>>> synchronizing the state (and it could be a separate add on patch?).
>>
>> +1 to thinking about some atomicity changes later.

+1

>>
>>>
>>> But for preventing 2 threads from stomping on the cp the check should be
>>> enough, unless I am missing something?
>>
>> I think so. Plus, the patch is small enough that we can merge it right
>> away, and figure out a more generic change later.
> 
> I will send out a v3 soon if no one else has any other suggestions.
> 

I thumbed through them and think they look good with Conny's
suggestions.  Nothing else jumps to mind for me.
Halil Pasic July 11, 2019, 2:57 p.m. UTC | #8
On Tue, 9 Jul 2019 17:27:47 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> 
> 
> On 07/09/2019 10:21 AM, Halil Pasic wrote:
> > On Tue, 9 Jul 2019 09:46:51 -0400
> > Farhan Ali <alifm@linux.ibm.com> wrote:
> > 
> >>
> >>
> >> On 07/09/2019 06:16 AM, Cornelia Huck wrote:
> >>> On Mon,  8 Jul 2019 16:10:37 -0400
> >>> Farhan Ali <alifm@linux.ibm.com> wrote:
> >>>
> >>>> There is a small window where it's possible that we could be working
> >>>> on an interrupt (queued in the workqueue) and setting up a channel
> >>>> program (i.e allocating memory, pinning pages, translating address).
> >>>> This can lead to allocating and freeing the channel program at the
> >>>> same time and can cause memory corruption.
> >>>>
> >>>> Let's not call cp_free if we are currently processing a channel program.
> >>>> The only way we know for sure that we don't have a thread setting
> >>>> up a channel program is when the state is set to VFIO_CCW_STATE_CP_PENDING.
> >>>
> >>> Can we pinpoint a commit that introduced this bug, or has it been there
> >>> since the beginning?
> >>>
> >>
> >> I think the problem was always there.
> >>
> > 
> > I think it became relevant with the async stuff. Because after the async
> > stuff was added we start getting solicited interrupts that are not about
> > channel program is done. At least this is how I remember the discussion.
> > 

You seem to have ignored this comment. BTW wasn't the cp->is_initialized
make 'Make it safe to call the cp accessors in any case, so we can call
them unconditionally.'?

@Connie: Your opinion as the author of that patch and of the cited
sentence?

> >>>>
> >>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> >>>> ---
> >>>>    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 4e3a903..0357165 100644
> >>>> --- a/drivers/s390/cio/vfio_ccw_drv.c
> >>>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> >>>> @@ -92,7 +92,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);
> >>>> -		if (is_final)
> >>>> +		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)
> > 
> > Ain't private->state potentially used by multiple threads of execution?
> 
> yes
> 
> One of the paths I can think of is a machine check from the host which 
> will ultimately call vfio_ccw_sch_event callback which could set state 
> to NOT_OPER or IDLE.
> 
> > Do we need to use atomic operations or external synchronization to avoid
> > this being another gamble? Or am I missing something?
> 
> I think we probably should think about atomic operations for 
> synchronizing the state (and it could be a separate add on patch?).
> 
> But for preventing 2 threads from stomping on the cp the check should be 
> enough, unless I am missing something?
> 

Usually programming languages don't like incorrectly synchronized
programs. One tends to end up in undefined behavior land -- form language
perspective. That doesn't actually mean you are bound to see strange
stuff. With implementation spec + ABI spec + platform/architecture
spec one may end up with things being well defined. But it that is a much
deeper rabbit hole.

The nice thing about condition state == VFIO_CCW_STATE_CP_PENDING is
that it can tolerate stale state values. The bad case at hand
(you free but you should not) would be we see a stale
VFIO_CCW_STATE_CP_PENDING but we are actually
VFIO_CCW_STATE_CP_PROCESSING. That is pretty difficult to imagine
because one can enter VFIO_CCW_STATE_CP_PROCESSING only form
VFIO_CCW_STATE_CP_PENDING afair. On s390x torn reads/writes (i.e.
observing something that ain't either the old nor the new value) on an
int shouldn't be a concern.

The other bad case (where you don't free albeit you should) looks a
bit trickier.

I'm not a fan of keeping races around without good reasons. And I don't
see good reasons here. I'm no fan of needlessly complicated solutions
either.

But seems, at least with my beliefs about races, I'm the oddball
here. 

Regards,
Halil

> > 
> >>>>    			cp_free(&private->cp);
> >>>>    	}
> >>>>    	mutex_lock(&private->io_mutex);
> >>>
> >>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> >>>
> >>>
> >> Thanks for reviewing.
> >>
> >> Thanks
> >> Farhan
> > 
> >
Eric Farman July 11, 2019, 8:09 p.m. UTC | #9
On 7/11/19 10:57 AM, Halil Pasic wrote:
> On Tue, 9 Jul 2019 17:27:47 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>>
>>
>> On 07/09/2019 10:21 AM, Halil Pasic wrote:
>>> On Tue, 9 Jul 2019 09:46:51 -0400
>>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>>
>>>>
>>>>
>>>> On 07/09/2019 06:16 AM, Cornelia Huck wrote:
>>>>> On Mon,  8 Jul 2019 16:10:37 -0400
>>>>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>>>>
>>>>>> There is a small window where it's possible that we could be working
>>>>>> on an interrupt (queued in the workqueue) and setting up a channel
>>>>>> program (i.e allocating memory, pinning pages, translating address).
>>>>>> This can lead to allocating and freeing the channel program at the
>>>>>> same time and can cause memory corruption.
>>>>>>
>>>>>> Let's not call cp_free if we are currently processing a channel program.
>>>>>> The only way we know for sure that we don't have a thread setting
>>>>>> up a channel program is when the state is set to VFIO_CCW_STATE_CP_PENDING.
>>>>>
>>>>> Can we pinpoint a commit that introduced this bug, or has it been there
>>>>> since the beginning?
>>>>>
>>>>
>>>> I think the problem was always there.
>>>>
>>>
>>> I think it became relevant with the async stuff. Because after the async
>>> stuff was added we start getting solicited interrupts that are not about
>>> channel program is done. At least this is how I remember the discussion.
>>>
> 
> You seem to have ignored this comment. 

I read both comments as being in agreement with one another.  The
problem has always been there, but didn't mean anything until we had
another mechanism (async) to drive additional interrupts.  Hence the v3
patch including the async patch in a Fixes tag.

BTW wasn't the cp->is_initialized
> make 'Make it safe to call the cp accessors in any case, so we can call
> them unconditionally.'?
> 
> @Connie: Your opinion as the author of that patch and of the cited
> sentence?
> >>>>>>
>>>>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>>>>>> ---
>>>>>>    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 4e3a903..0357165 100644
>>>>>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>>>>>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>>>>>> @@ -92,7 +92,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);
>>>>>> -		if (is_final)
>>>>>> +		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)
>>>
>>> Ain't private->state potentially used by multiple threads of execution?
>>
>> yes
>>
>> One of the paths I can think of is a machine check from the host which 
>> will ultimately call vfio_ccw_sch_event callback which could set state 
>> to NOT_OPER or IDLE.
>>
>>> Do we need to use atomic operations or external synchronization to avoid
>>> this being another gamble? Or am I missing something?
>>
>> I think we probably should think about atomic operations for 
>> synchronizing the state (and it could be a separate add on patch?).
>>
>> But for preventing 2 threads from stomping on the cp the check should be 
>> enough, unless I am missing something?
>>
> 
> Usually programming languages don't like incorrectly synchronized
> programs. One tends to end up in undefined behavior land -- form language
> perspective. That doesn't actually mean you are bound to see strange
> stuff. With implementation spec + ABI spec + platform/architecture
> spec one may end up with things being well defined. But it that is a much
> deeper rabbit hole.
> 
> The nice thing about condition state == VFIO_CCW_STATE_CP_PENDING is
> that it can tolerate stale state values. The bad case at hand
> (you free but you should not) would be we see a stale
> VFIO_CCW_STATE_CP_PENDING but we are actually
> VFIO_CCW_STATE_CP_PROCESSING. That is pretty difficult to imagine
> because one can enter VFIO_CCW_STATE_CP_PROCESSING only form
> VFIO_CCW_STATE_CP_PENDING afair. 

I think you're backwards here.  The path is IDLE -> CP_PROCESSING ->
(CP_PENDING | IDLE)

On s390x torn reads/writes (i.e.
> observing something that ain't either the old nor the new value) on an
> int shouldn't be a concern.
> 
> The other bad case (where you don't free albeit you should) looks a
> bit trickier.

I'm afraid I don't understand your intention with the above paragraphs.  :(

> 
> I'm not a fan of keeping races around without good reasons. And I don't
> see good reasons here. I'm no fan of needlessly complicated solutions
> either.
> 
> But seems, at least with my beliefs about races, I'm the oddball
> here. 

The "race" here is that we have one synchronous operation (SSCH) and two
asynchronous operations (HSCH, CSCH), both of which interact with one
another and generate interrupts that pass through this chunk of code.

I have not fully considered this patch yet, but the race is a concern to
all of us oddballs.  I have not chimed in any great detail because I
only got through the first couple patches in v1 before going on holiday,
and the discussions on v1/v2 are numerous.

 - Eric

> 
> Regards,
> Halil
> 
>>>
>>>>>>    			cp_free(&private->cp);
>>>>>>    	}
>>>>>>    	mutex_lock(&private->io_mutex);
>>>>>
>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>>>
>>>>>
>>>> Thanks for reviewing.
>>>>
>>>> Thanks
>>>> Farhan
>>>
>>>
>
Halil Pasic July 12, 2019, 1:59 p.m. UTC | #10
On Thu, 11 Jul 2019 16:09:22 -0400
Eric Farman <farman@linux.ibm.com> wrote:

> 
> 
> On 7/11/19 10:57 AM, Halil Pasic wrote:
> > On Tue, 9 Jul 2019 17:27:47 -0400
> > Farhan Ali <alifm@linux.ibm.com> wrote:
> > 
> >>
> >>
> >> On 07/09/2019 10:21 AM, Halil Pasic wrote:
> >>> On Tue, 9 Jul 2019 09:46:51 -0400
> >>> Farhan Ali <alifm@linux.ibm.com> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 07/09/2019 06:16 AM, Cornelia Huck wrote:
> >>>>> On Mon,  8 Jul 2019 16:10:37 -0400
> >>>>> Farhan Ali <alifm@linux.ibm.com> wrote:
> >>>>>
> >>>>>> There is a small window where it's possible that we could be working
> >>>>>> on an interrupt (queued in the workqueue) and setting up a channel
> >>>>>> program (i.e allocating memory, pinning pages, translating address).
> >>>>>> This can lead to allocating and freeing the channel program at the
> >>>>>> same time and can cause memory corruption.
> >>>>>>
> >>>>>> Let's not call cp_free if we are currently processing a channel program.
> >>>>>> The only way we know for sure that we don't have a thread setting
> >>>>>> up a channel program is when the state is set to VFIO_CCW_STATE_CP_PENDING.
> >>>>>
> >>>>> Can we pinpoint a commit that introduced this bug, or has it been there
> >>>>> since the beginning?
> >>>>>
> >>>>
> >>>> I think the problem was always there.
> >>>>
> >>>
> >>> I think it became relevant with the async stuff. Because after the async
> >>> stuff was added we start getting solicited interrupts that are not about
> >>> channel program is done. At least this is how I remember the discussion.
> >>>
> > 
> > You seem to have ignored this comment. 
> 
> I read both comments as being in agreement with one another.

Which both comments do you see in agreement? The one that states 'was
always there' and the one that states 'was introduced by the async
series'?

> The
> problem has always been there, but didn't mean anything until we had
> another mechanism (async) to drive additional interrupts.  Hence the v3
> patch including the async patch in a Fixes tag.
> 

Sorry, when I started writing this response, there was no v3 out yet.
Later I've seen the Fixes tag in v3. I'm not sure it is the correct one
though. Hence my question about cp->is_initialized.

> > BTW wasn't the cp->is_initialized
> > make 'Make it safe to call the cp accessors in any case, so we can call
> > them unconditionally.'?
> > 
> > @Connie: Your opinion as the author of that patch and of the cited
> > sentence?
> > >>>>>>
> >>>>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> >>>>>> ---
> >>>>>>    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 4e3a903..0357165 100644
> >>>>>> --- a/drivers/s390/cio/vfio_ccw_drv.c
> >>>>>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> >>>>>> @@ -92,7 +92,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);

<BACKREF_1>

> >>>>>> -		if (is_final)
> >>>>>> +		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)
> >>>
> >>> Ain't private->state potentially used by multiple threads of execution?
> >>
> >> yes
> >>

</BACKREF_1>

> >> One of the paths I can think of is a machine check from the host which 
> >> will ultimately call vfio_ccw_sch_event callback which could set state 
> >> to NOT_OPER or IDLE.
> >>

<BACKREF_2>

> >>> Do we need to use atomic operations or external synchronization to avoid
> >>> this being another gamble? Or am I missing something?
> >>
> >> I think we probably should think about atomic operations for 
> >> synchronizing the state (and it could be a separate add on patch?).
> >>

</BACKREF_2>

> >> But for preventing 2 threads from stomping on the cp the check should be 
> >> enough, unless I am missing something?
> >>
> > 
> > Usually programming languages don't like incorrectly synchronized
> > programs. One tends to end up in undefined behavior land -- form language
> > perspective. That doesn't actually mean you are bound to see strange
> > stuff. With implementation spec + ABI spec + platform/architecture
> > spec one may end up with things being well defined. But it that is a much
> > deeper rabbit hole.
> > 
> > The nice thing about condition state == VFIO_CCW_STATE_CP_PENDING is
> > that it can tolerate stale state values. The bad case at hand
> > (you free but you should not) would be we see a stale
> > VFIO_CCW_STATE_CP_PENDING but we are actually
> > VFIO_CCW_STATE_CP_PROCESSING. That is pretty difficult to imagine
> > because one can enter VFIO_CCW_STATE_CP_PROCESSING only form
> > VFIO_CCW_STATE_CP_PENDING afair. 
> 
> I think you're backwards here.  The path is IDLE -> CP_PROCESSING ->
> (CP_PENDING | IDLE)

That is what I tried to say. The backwards twist probably comes from
the fact that I'm discussing what can happen if we read stale value
from state.

> 
> > On s390x torn reads/writes (i.e.
> > observing something that ain't either the old nor the new value) on an
> > int shouldn't be a concern.
> > 
> > The other bad case (where you don't free albeit you should) looks a
> > bit trickier.
> 
> I'm afraid I don't understand your intention with the above paragraphs.  :(
> 

All three paragraphs are about discussing what can happen or can not
happen in practice. The paragraph before those three is about the so
called academic aspects.

> > 
> > I'm not a fan of keeping races around without good reasons. And I don't
> > see good reasons here. I'm no fan of needlessly complicated solutions
> > either.
> > 
> > But seems, at least with my beliefs about races, I'm the oddball
> > here. 
> 
> The "race" here is that we have one synchronous operation (SSCH) and two
> asynchronous operations (HSCH, CSCH), both of which interact with one
> another and generate interrupts that pass through this chunk of code.
> 

I don't agree. Please consider the stuff between the <BACKREF_[12]> tags.
In my reading Farhan agrees that we have a data race on private->state.

If you did not get that, no wonder my email makes little sense.

> I have not fully considered this patch yet, but the race is a concern to
> all of us oddballs.  

Yes, but seems to a different extent. The rest of the guys are fine with
just plainly accessing private->state in <BACKREF_1>, and do different
logic based on the value, even though nobody seems to argue that the
accesses to private->state involve race.

> I have not chimed in any great detail because I
> only got through the first couple patches in v1 before going on holiday,
> and the discussions on v1/v2 are numerous.

My email was mostly addressed to Farhan, the author of the patch. No
need to for apologies. :)

Regards,
Halil

Patch
diff mbox series

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 4e3a903..0357165 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -92,7 +92,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);
-		if (is_final)
+		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)
 			cp_free(&private->cp);
 	}
 	mutex_lock(&private->io_mutex);