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

Matthew Rosato
2022-06-02T19:51:22Z

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