| Message ID | 1405df8415d3bff446c22753d0e9b91ff246eb0f.1562616169.git.alifm@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some vfio-ccw fixes | expand |
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>
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
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
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 > >
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 > > > >
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 >>> >>> > >
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.
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 > > > >
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 >>> >>> >
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
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);
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(-)