Re: [PATCH v1 01/18] vfio/ccw: Remove UUID from s390 debug log

Matthew Rosato <mjrosato@xxxxxxxxxxxxx> · Thu, 2 Jun 2022 15:51:11 -0400

On 6/2/22 1:19 PM, Eric Farman wrote:
From: Michael Kawano <mkawano@xxxxxxxxxxxxx>

As vfio-ccw devices are created/destroyed, the uuid of the associated
mdevs that are recorded in $S390DBF/vfio_ccw_msg/sprintf get lost as
they are created using pointers passed by reference.

This is a deliberate design point of s390dbf, but it leaves the uuid

This wording is confusing, maybe some re-wording would help here.

Basically, s390dbf doesn't support values passed by reference today 
(e.g. %pUl), it will just store that pointer (e.g. &mdev->uuid) and not 
its contents -- so a subsequent viewing of the s390dbf log at any point 
in the future will go peek at that referenced memory -- which might have 
been freed (e.g. mdev was removed).  So this change will fix potential 
garbage data viewed from the log or worse an oops when viewing the log 
-- the latter of which should probably be mentioned in the commit message.

I'm not sure if it was a deliberate design decision of s390dbf or just a 
feature that was never implemented, so I'd omit that altogether -- but 
it IS pointed out in the s390dbf documentation as a limitation anyway.

The code itself is fine:

Reviewed-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>

in these traces less than useful. Since the subchannels are more
constant, and are mapped 1:1 with the mdevs, the associated mdev can
be discerned by looking at the device configuration (e.g., mdevctl)
and places, such as kernel messages, where it is statically stored.

Thus, let's just remove the uuid from s390dbf traces. As we were
the only consumer of mdev_uuid(), remove that too.

Cc: Kirti Wankhede <kwankhede@xxxxxxxxxx>
Signed-off-by: Michael Kawano <mkawano@xxxxxxxxxxxxx>
Fixes: 60e05d1cf0875 ("vfio-ccw: add some logging")
Fixes: b7701dfbf9832 ("vfio-ccw: Register a chp_event callback for vfio-ccw")
[farman: reworded commit message, added Fixes: tags]
Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
---
  drivers/s390/cio/vfio_ccw_drv.c |  5 ++---
  drivers/s390/cio/vfio_ccw_fsm.c | 24 ++++++++++++------------
  drivers/s390/cio/vfio_ccw_ops.c |  8 ++++----
  include/linux/mdev.h            |  4 ----
  4 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index ee182cfb467d..35055eb94115 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -14,7 +14,6 @@
  #include <linux/init.h>
  #include <linux/device.h>
  #include <linux/slab.h>
-#include <linux/uuid.h>
  #include <linux/mdev.h>
  
  #include <asm/isc.h>
@@ -358,8 +357,8 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
  		return 0;
  
  	trace_vfio_ccw_chp_event(private->sch->schid, mask, event);
-	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
-			   mdev_uuid(private->mdev), sch->schid.cssid,
+	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: mask=0x%x event=%d\n",
+			   sch->schid.cssid,
  			   sch->schid.ssid, sch->schid.sch_no,
  			   mask, event);
  
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index e435a9cd92da..86b23732d899 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -256,8 +256,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
  		if (orb->tm.b) {
  			io_region->ret_code = -EOPNOTSUPP;
  			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): transport mode\n",
-					   mdev_uuid(mdev), schid.cssid,
+					   "sch %x.%x.%04x: transport mode\n",
+					   schid.cssid,
  					   schid.ssid, schid.sch_no);
  			errstr = "transport mode";
  			goto err_out;
@@ -266,8 +266,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
  					      orb);
  		if (io_region->ret_code) {
  			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): cp_init=%d\n",
-					   mdev_uuid(mdev), schid.cssid,
+					   "sch %x.%x.%04x: cp_init=%d\n",
+					   schid.cssid,
  					   schid.ssid, schid.sch_no,
  					   io_region->ret_code);
  			errstr = "cp init";
@@ -277,8 +277,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
  		io_region->ret_code = cp_prefetch(&private->cp);
  		if (io_region->ret_code) {
  			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): cp_prefetch=%d\n",
-					   mdev_uuid(mdev), schid.cssid,
+					   "sch %x.%x.%04x: cp_prefetch=%d\n",
+					   schid.cssid,
  					   schid.ssid, schid.sch_no,
  					   io_region->ret_code);
  			errstr = "cp prefetch";
@@ -290,8 +290,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
  		io_region->ret_code = fsm_io_helper(private);
  		if (io_region->ret_code) {
  			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): fsm_io_helper=%d\n",
-					   mdev_uuid(mdev), schid.cssid,
+					   "sch %x.%x.%04x: fsm_io_helper=%d\n",
+					   schid.cssid,
  					   schid.ssid, schid.sch_no,
  					   io_region->ret_code);
  			errstr = "cp fsm_io_helper";
@@ -301,16 +301,16 @@ static void fsm_io_request(struct vfio_ccw_private *private,
  		return;
  	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
  		VFIO_CCW_MSG_EVENT(2,
-				   "%pUl (%x.%x.%04x): halt on io_region\n",
-				   mdev_uuid(mdev), schid.cssid,
+				   "sch %x.%x.%04x: halt on io_region\n",
+				   schid.cssid,
  				   schid.ssid, schid.sch_no);
  		/* halt is handled via the async cmd region */
  		io_region->ret_code = -EOPNOTSUPP;
  		goto err_out;
  	} else if (scsw->cmd.fctl & SCSW_FCTL_CLEAR_FUNC) {
  		VFIO_CCW_MSG_EVENT(2,
-				   "%pUl (%x.%x.%04x): clear on io_region\n",
-				   mdev_uuid(mdev), schid.cssid,
+				   "sch %x.%x.%04x: clear on io_region\n",
+				   schid.cssid,
  				   schid.ssid, schid.sch_no);
  		/* clear is handled via the async cmd region */
  		io_region->ret_code = -EOPNOTSUPP;
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index d8589afac272..bebae21228aa 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -131,8 +131,8 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
  	private->mdev = mdev;
  	private->state = VFIO_CCW_STATE_IDLE;
  
-	VFIO_CCW_MSG_EVENT(2, "mdev %pUl, sch %x.%x.%04x: create\n",
-			   mdev_uuid(mdev), private->sch->schid.cssid,
+	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
+			   private->sch->schid.cssid,
  			   private->sch->schid.ssid,
  			   private->sch->schid.sch_no);
  
@@ -154,8 +154,8 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
  {
  	struct vfio_ccw_private *private = dev_get_drvdata(mdev->dev.parent);
  
-	VFIO_CCW_MSG_EVENT(2, "mdev %pUl, sch %x.%x.%04x: remove\n",
-			   mdev_uuid(mdev), private->sch->schid.cssid,
+	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: remove\n",
+			   private->sch->schid.cssid,
  			   private->sch->schid.ssid,
  			   private->sch->schid.sch_no);
  
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index 15d03f6532d0..a5788f592817 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -139,10 +139,6 @@ static inline void mdev_set_drvdata(struct mdev_device *mdev, void *data)
  {
  	mdev->driver_data = data;
  }
-static inline const guid_t *mdev_uuid(struct mdev_device *mdev)
-{
-	return &mdev->uuid;
-}
  
  extern struct bus_type mdev_bus_type;
  







