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