diff mbox series

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

Message ID 20220602171948.2790690-2-farman@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series VFIO ccw/mdev rework | expand

Commit Message

Eric Farman June 2, 2022, 5:19 p.m. UTC
From: Michael Kawano <mkawano@linux.ibm.com>

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
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@nvidia.com>
Signed-off-by: Michael Kawano <mkawano@linux.ibm.com>
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@linux.ibm.com>
---
 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(-)

Comments

Jason Gunthorpe June 2, 2022, 6:55 p.m. UTC | #1
On Thu, Jun 02, 2022 at 07:19:31PM +0200, Eric Farman wrote:
> From: Michael Kawano <mkawano@linux.ibm.com>
> 
> 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
> 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.

I don't quite understand these two paragraphs, but the change looks OK

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason
Matthew Rosato June 2, 2022, 7:51 p.m. UTC | #2
On 6/2/22 1:19 PM, Eric Farman wrote:
> From: Michael Kawano <mkawano@linux.ibm.com>
> 
> 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@linux.ibm.com>

> 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@nvidia.com>
> Signed-off-by: Michael Kawano <mkawano@linux.ibm.com>
> 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@linux.ibm.com>
> ---
>   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;
>
Eric Farman June 3, 2022, 7:03 p.m. UTC | #3
On Thu, 2022-06-02 at 15:51 -0400, Matthew Rosato wrote:
> On 6/2/22 1:19 PM, Eric Farman wrote:
> > From: Michael Kawano <mkawano@linux.ibm.com>
> > 
> > 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.

@Jason, @Matt...  All fair, I obviously got too verbose in whatever I
was writing at the time. I've changed this to:

As vfio-ccw devices are created/destroyed, the uuid of the associated
mdevs that are recorded in $S390DBF/vfio_ccw_msg/sprintf get lost.
This is because a pointer to the UUID is stored instead of the UUID
itself, and that memory may have been repurposed if/when the logs are
examined. The result is usually garbage UUID data in the logs, though
there is an outside chance of an oops happening here.

Simply remove the UUID from the traces, as the subchannel number will
provide useful configuration information for problem determination,
and is stored directly into the log instead of a pointer.

As we were the only consumer of mdev_uuid(), remove that too.

> 
> The code itself is fine:
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> > 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@nvidia.com>
> > Signed-off-by: Michael Kawano <mkawano@linux.ibm.com>
> > 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@linux.ibm.com>
> > ---
> >   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;
> >
Matthew Rosato June 6, 2022, 8:45 p.m. UTC | #4
On 6/3/22 3:03 PM, Eric Farman wrote:
> On Thu, 2022-06-02 at 15:51 -0400, Matthew Rosato wrote:
>> On 6/2/22 1:19 PM, Eric Farman wrote:
>>> From: Michael Kawano <mkawano@linux.ibm.com>
>>>
>>> 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.
> 
> @Jason, @Matt...  All fair, I obviously got too verbose in whatever I
> was writing at the time. I've changed this to:
> 
> As vfio-ccw devices are created/destroyed, the uuid of the associated
> mdevs that are recorded in $S390DBF/vfio_ccw_msg/sprintf get lost.
> This is because a pointer to the UUID is stored instead of the UUID
> itself, and that memory may have been repurposed if/when the logs are
> examined. The result is usually garbage UUID data in the logs, though
> there is an outside chance of an oops happening here.
> 
> Simply remove the UUID from the traces, as the subchannel number will
> provide useful configuration information for problem determination,
> and is stored directly into the log instead of a pointer.
> 
> As we were the only consumer of mdev_uuid(), remove that too.
> 

Sounds good
diff mbox series

Patch

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;