Some vfio-ccw fixes

The comment is misleading because it tells us that we should set orb.cmd.c64 before calling ccwchain_calc_length, otherwise the function ccwchain_calc_length would return an error. This is not completely accurate.

We want to allow an orb without cmd.c64, and this is fine as long as the channel program does not use IDALs. But we do want to reject any channel program that uses IDALs and does not set the flag, which what we do in ccwchain_calc_length.

After we have done the ccw processing, it should be safe to set cmd.c64, since we will convert them into IDALs.

Signed-off-by Farhan Ali <[email protected]>

---
 drivers/s390/cio/vfio_ccw_cp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/s390/cio/vfio_ccw_cp.c +0 -0

index d6a8dff..7622b72 100644

We don't set cp->initialized to true so calling cp_free will just return and not do anything.

Also fix a memory leak where we fail to free a ccwchain on an error.

Fixes 812271b910 ("s390/cio: Squash cp_free() and cp_unpin_free()")

Signed-off-by Farhan Ali <[email protected]>

---
 drivers/s390/cio/vfio_ccw_cp.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

So we don't call try to call vfio_unpin_pages() incorrectly.

Fixes 0a19e61e6d4c ("vfio: ccw: introduce channel program interfaces")

Signed-off-by Farhan Ali <[email protected]>

Reviewed-by Eric Farman <[email protected]>

---
 drivers/s390/cio/vfio_ccw_cp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/s390/cio/vfio_ccw_cp.c +0 -0

index 31a04a5..1b93863 100644
@@ -72,8 +72,10 @@ static int pfn_array_alloc(struct pfn_array *pa, u64 iova, unsigned int len)
 				  sizeof(*pa->pa_iova_pfn) +

We now support CLEAR SUBCHANNEL and HALT SUBCHANNEL via ccw_cmd_region.

Signed-off-by Farhan Ali <[email protected]>

---
 Documentation/s390/vfio-ccw.rst | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

Documentation/s390/vfio-ccw.rst +15 -0

index 1f6d0b5..40e4110 100644
@@ -180,6 +180,13 @@ The process of how these work together.
    add it to an iommu_group and a vfio_group. Then we could pass through
    the mdev to a guest.
 
+
+VFIO-CCW Regions
+----------------
+
+The vfio-ccw driver exposes MMIO regions to accept requests from and return
+results to userspace.
+
 vfio-ccw I/O region
 -------------------
 

@@ -205,6 +212,25 @@ irb_area stores the I/O result.
 
 ret_code stores a return code for each access of the region.
 
+This region is always available.
+
+vfio-ccw cmd region
+-------------------
+
+The vfio-ccw cmd region is used to accept asynchronous instructions
+from userspace.
+

Documentation/s390/vfio-ccw.rst +0 -0

 vfio-ccw supports classic (command mode) channel I/O only. Transport
 mode (HPF) is not supported.

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 <[email protected]>

---
 drivers/s390/cio/vfio_ccw_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/s390/cio/vfio_ccw_drv.c +0 -0

index 4e3a903..0357165 100644

Quoted 3 lines · On Mon, 8 Jul 2019 16:10:38 -0400 Farhan Ali <[email protected]> wrote:

We now support CLEAR SUBCHANNEL and HALT SUBCHANNEL via ccw_cmd_region.

Thanks, I forgot about this.

Fixes d5afd5d135c8 ("vfio-ccw: add handling for async channel instructions")

Quoted 39 lines

Signed-off-by Farhan Ali <[email protected]>

---
  Documentation/s390/vfio-ccw.rst | 31 ++++++++++++++++++++++++++++---
  1 file changed, 28 insertions(+), 3 deletions(-)

Documentation/s390/vfio-ccw.rst +19 -0

index 1f6d0b5..40e4110 100644
@@ -180,6 +180,13 @@ The process of how these work together.
     add it to an iommu_group and a vfio_group. Then we could pass through
     the mdev to a guest.
 
+
+VFIO-CCW Regions
+----------------
+
+The vfio-ccw driver exposes MMIO regions to accept requests from and return
+results to userspace.
+
  vfio-ccw I/O region
  -------------------
 

@@ -205,6 +212,25 @@ irb_area stores the I/O result.
 
  ret_code stores a return code for each access of the region.
 
+This region is always available.
+
+vfio-ccw cmd region
+-------------------
+
+The vfio-ccw cmd region is used to accept asynchronous instructions
+from userspace.
+
+#define VFIO_CCW_ASYNC_CMD_HSCH (1 << 0)
+#define VFIO_CCW_ASYNC_CMD_CSCH (1 << 1)
+struct ccw_cmd_region {
+       __u32 command;

Documentation/s390/vfio-ccw.rst irb_area stores the I/O result. · line 232

Quoted 6 lines

Documentation/s390/vfio-ccw.rst +6 -0

+       __u32 ret_code;
+} __packed;
+
+This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD.
+
+CLEAR SUBCHANNEL and HALT SUBCHANNEL currently uses this region.

"Currently, CLEAR SUBCHANNEL and HALT SUBCHANNEL use this region". ?

Quoted 8 lines

Documentation/s390/vfio-ccw.rst +1 -0

+
  vfio-ccw operation details
  --------------------------
 

@@ -306,9 +332,8 @@ Together with the corresponding work in QEMU, we can bring the passed
  through DASD/ECKD device online in a guest now and use it as a block
  device.
 

Documentation/s390/vfio-ccw.rst Together with the corresponding work in QEMU, we can bring the passed · line 336

Quoted 5 lines

Documentation/s390/vfio-ccw.rst +2 -3

-While the current code allows the guest to start channel programs via
-START SUBCHANNEL, support for HALT SUBCHANNEL or CLEAR SUBCHANNEL is
-not yet implemented.
+The current code allows the guest to start channel programs via
+START SUBCHANNEL, and supports HALT SUBCHANNEL or CLEAR SUBCHANNEL.

"The current code allows the guest to start channel programs via START SUBCHANNEL, and to issue HALT SUBCHANNEL and CLEAR SUBCHANNEL." ?

Documentation/s390/vfio-ccw.rst Together with the corresponding work in QEMU, we can bring the passed · line 339

Quoted 3 lines

Documentation/s390/vfio-ccw.rst +0 -0

  vfio-ccw supports classic (command mode) channel I/O only. Transport
  mode (HPF) is not supported.

Quoted 6 lines · On Mon, 8 Jul 2019 16:10:37 -0400 Farhan Ali <[email protected]> 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.

Quoted 3 lines

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?

Quoted 6 lines

Signed-off-by Farhan Ali <[email protected]>

---
  drivers/s390/cio/vfio_ccw_drv.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 98

Quoted 13 lines

drivers/s390/cio/vfio_ccw_drv.c +1 -1

index 4e3a903..0357165 100644
@@ -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 <[email protected]>

Quoted 11 lines · On Mon, 8 Jul 2019 16:10:35 -0400 Farhan Ali <[email protected]> wrote:

We don't set cp->initialized to true so calling cp_free will just return and not do anything.

Also fix a memory leak where we fail to free a ccwchain on an error.

Fixes 812271b910 ("s390/cio: Squash cp_free() and cp_unpin_free()")

Signed-off-by Farhan Ali <[email protected]>

---
  drivers/s390/cio/vfio_ccw_cp.c | 11 +++++++----
  1 file changed, 7 insertions(+), 4 deletions(-)

(...)

Quoted 2 lines

(quoted diff) +0 -0

@@ -642,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
 

drivers/s390/cio/vfio_ccw_cp.c int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) · line 646

Quoted 4 lines

drivers/s390/cio/vfio_ccw_cp.c +0 -2

  	/* Build a ccwchain for the first CCW segment */
  	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
-	if (ret)
-		cp_free(cp);

Now that I look again: it's a bit odd that we set the bit in all cases, even if we have an error. We could move that into the !ret branch that sets ->initialized; but it does not really hurt.

drivers/s390/cio/vfio_ccw_cp.c int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) · line 652

Quoted 4 lines

drivers/s390/cio/vfio_ccw_cp.c +0 -0

  	/* It is safe to force: if it was not set but idals used
  	 * ccwchain_calc_length would have returned an error.

The rest of the patch looks good to me.

Quoted 19 lines · Cornelia Huck

Quoted 11 lines · On Mon, 8 Jul 2019 16:10:35 -0400 Farhan Ali <[email protected]> wrote:

We don't set cp->initialized to true so calling cp_free will just return and not do anything.

Also fix a memory leak where we fail to free a ccwchain on an error.

Fixes 812271b910 ("s390/cio: Squash cp_free() and cp_unpin_free()")

Signed-off-by Farhan Ali <[email protected]>

---
  drivers/s390/cio/vfio_ccw_cp.c | 11 +++++++----
  1 file changed, 7 insertions(+), 4 deletions(-)  

(...)

Quoted 2 lines

(quoted diff) +0 -0

@@ -642,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
 

Quoted 8 lines

drivers/s390/cio/vfio_ccw_cp.c int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) · line 646

Quoted 4 lines

drivers/s390/cio/vfio_ccw_cp.c +0 -2

  	/* Build a ccwchain for the first CCW segment */
  	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
-	if (ret)
-		cp_free(cp);  

Now that I look again: it's a bit odd that we set the bit in all cases, even if we have an error. We could move that into the !ret branch that sets ->initialized; but it does not really hurt.

By setting the bit, I am assuming you meant cmd.c64?

Yes, it doesn't harm anything but for better code readability you have a good point. I will move it into !ret branch in the first patch since I think it would be more appropriate there, no?

Quoted 9 lines

drivers/s390/cio/vfio_ccw_cp.c int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) · line 652

Quoted 5 lines

drivers/s390/cio/vfio_ccw_cp.c +0 -0

  	/* It is safe to force: if it was not set but idals used
  	 * ccwchain_calc_length would have returned an error.  

The rest of the patch looks good to me.

Quoted 13 lines · Cornelia Huck

Quoted 9 lines · On Mon, 8 Jul 2019 16:10:37 -0400 Farhan Ali <[email protected]> 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.

Quoted 3 lines

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

I think the problem was always there.

Quoted 6 lines

Quoted 6 lines

Signed-off-by Farhan Ali <[email protected]>

---
  drivers/s390/cio/vfio_ccw_drv.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 95

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c +1 -1

index 4e3a903..0357165 100644
@@ -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)

Quoted 7 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 98

Quoted 3 lines

drivers/s390/cio/vfio_ccw_drv.c +0 -0

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

Reviewed-by Cornelia Huck <[email protected]>

Thanks for reviewing.

Thanks
Farhan

Quoted 16 lines · On Tue, 9 Jul 2019 09:46:51 -0400 Farhan Ali <[email protected]> wrote:

Quoted 13 lines · Cornelia Huck

Quoted 9 lines · On Mon, 8 Jul 2019 16:10:37 -0400 Farhan Ali <[email protected]> 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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Signed-off-by Farhan Ali <[email protected]>

---
   drivers/s390/cio/vfio_ccw_drv.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 10 lines

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 95

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c +1 -1

index 4e3a903..0357165 100644
@@ -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?

Quoted 12 lines

Quoted 8 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 98

Quoted 4 lines

drivers/s390/cio/vfio_ccw_drv.c +0 -0

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

Reviewed-by Cornelia Huck <[email protected]>

Thanks for reviewing.

Thanks
Farhan

Quoted 36 lines · Halil Pasic

Quoted 16 lines · On Tue, 9 Jul 2019 09:46:51 -0400 Farhan Ali <[email protected]> wrote:

Quoted 13 lines · Cornelia Huck

Quoted 9 lines · On Mon, 8 Jul 2019 16:10:37 -0400 Farhan Ali <[email protected]> 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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Signed-off-by Farhan Ali <[email protected]>

---
   drivers/s390/cio/vfio_ccw_drv.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 12 lines

Quoted 10 lines

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 95

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c +1 -1

index 4e3a903..0357165 100644
@@ -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.

Quoted 3 lines

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?

Quoted 15 lines

Quoted 13 lines

Quoted 9 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 98

Quoted 5 lines

drivers/s390/cio/vfio_ccw_drv.c +0 -0

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

Reviewed-by Cornelia Huck <[email protected]>

Thanks for reviewing.

Thanks
Farhan

Quoted 52 lines · On Tue, 9 Jul 2019 17:27:47 -0400 Farhan Ali <[email protected]> wrote:

Quoted 36 lines · Halil Pasic

Quoted 16 lines · On Tue, 9 Jul 2019 09:46:51 -0400 Farhan Ali <[email protected]> wrote:

Quoted 13 lines · Cornelia Huck

Quoted 9 lines · On Mon, 8 Jul 2019 16:10:37 -0400 Farhan Ali <[email protected]> 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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Signed-off-by Farhan Ali <[email protected]>

---
    drivers/s390/cio/vfio_ccw_drv.c | 2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 12 lines

Quoted 10 lines

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 95

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c +1 -1

index 4e3a903..0357165 100644
@@ -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

Quoted 3 lines

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...

Quoted 7 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 16 lines

Quoted 16 lines

Quoted 14 lines

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 98

Quoted 6 lines

drivers/s390/cio/vfio_ccw_drv.c +0 -0

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

Reviewed-by Cornelia Huck <[email protected]>

Thanks for reviewing.

Thanks
Farhan

Quoted 4 lines · Cornelia Huck

Quoted 1 line · On Tue, 9 Jul 2019 17:27:47 -0400 Farhan Ali <[email protected]> wrote:

On 07/09/2019 10:21 AM, Halil Pasic wrote:

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 23 lines · Halil Pasic

Quoted 20 lines · On Tue, 9 Jul 2019 09:46:51 -0400 Farhan Ali <[email protected]> wrote:

Quoted 16 lines · Cornelia Huck

Quoted 9 lines · On Mon, 8 Jul 2019 16:10:37 -0400 Farhan Ali <[email protected]> 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?

Quoted 7 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Signed-off-by Farhan Ali <[email protected]>

---
   drivers/s390/cio/vfio_ccw_drv.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 12 lines

Quoted 10 lines

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 95

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c +1 -1

index 4e3a903..0357165 100644
@@ -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.

Quoted 2 lines

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?

Quoted 14 lines

Quoted 12 lines

Quoted 8 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 98

Quoted 4 lines

drivers/s390/cio/vfio_ccw_drv.c +0 -0

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

Reviewed-by Cornelia Huck <[email protected]>

Thanks for reviewing.

Thanks
Farhan

Quoted 29 lines · On Tue, 9 Jul 2019 17:27:47 -0400 Farhan Ali <[email protected]> wrote:

Quoted 26 lines · Halil Pasic

Quoted 22 lines · On Tue, 9 Jul 2019 09:46:51 -0400 Farhan Ali <[email protected]> wrote:

Quoted 9 lines · Cornelia Huck

Quoted 6 lines · On Mon, 8 Jul 2019 16:10:37 -0400 Farhan Ali <[email protected]> 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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 4 lines

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.

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?

Quoted 25 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

Signed-off-by Farhan Ali <[email protected]>

---
   drivers/s390/cio/vfio_ccw_drv.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 12 lines

Quoted 10 lines

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 95

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c +1 -1

index 4e3a903..0357165 100644
@@ -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.

Quoted 9 lines

Quoted 2 lines

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.

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

Quoted 14 lines

Quoted 14 lines

Quoted 12 lines

Quoted 8 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 98

Quoted 4 lines

drivers/s390/cio/vfio_ccw_drv.c +0 -0

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

Reviewed-by Cornelia Huck <[email protected]>

Thanks for reviewing.

Thanks
Farhan

Quoted 32 lines · Halil Pasic

Quoted 29 lines · On Tue, 9 Jul 2019 17:27:47 -0400 Farhan Ali <[email protected]> wrote:

Quoted 26 lines · Halil Pasic

Quoted 22 lines · On Tue, 9 Jul 2019 09:46:51 -0400 Farhan Ali <[email protected]> wrote:

Quoted 9 lines · Cornelia Huck

Quoted 6 lines · On Mon, 8 Jul 2019 16:10:37 -0400 Farhan Ali <[email protected]> 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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

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.

Quoted 12 lines

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?

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

Signed-off-by Farhan Ali <[email protected]>

---
   drivers/s390/cio/vfio_ccw_drv.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 94

Quoted 8 lines

drivers/s390/cio/vfio_ccw_drv.c +0 -0

index 4e3a903..0357165 100644
@@ -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);

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 95

Quoted 2 lines

drivers/s390/cio/vfio_ccw_drv.c +1 -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

Quoted 4 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

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?).

Quoted 11 lines

Quoted 3 lines

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.

Quoted 7 lines

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)

Quoted 7 lines

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. :(

Quoted 5 lines

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.

Quoted 2 lines

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.