| Message ID | 20220602171948.2790690-9-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VFIO ccw/mdev rework | expand |
On 6/2/22 1:19 PM, Eric Farman wrote: > The subchannel->dev->drvdata pointer is set in vfio_ccw_sch_probe() > and cleared in vfio_ccw_sch_remove(). In a purely defensive move, > let's check that the resultant pointer exists before operating on it > any way, since some lifecycle changes are forthcoming. > > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > drivers/s390/cio/vfio_ccw_drv.c | 12 ++++++++++++ > drivers/s390/cio/vfio_ccw_ops.c | 3 +++ > 2 files changed, 15 insertions(+) > > diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c > index 3784eb4cda85..f8226db26e54 100644 > --- a/drivers/s390/cio/vfio_ccw_drv.c > +++ b/drivers/s390/cio/vfio_ccw_drv.c > @@ -41,6 +41,9 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch) > DECLARE_COMPLETION_ONSTACK(completion); > int iretry, ret = 0; > > + if (!private) > + return 0; > + > spin_lock_irq(sch->lock); > if (!sch->schib.pmcw.ena) > goto out_unlock; > @@ -132,6 +135,9 @@ static void vfio_ccw_sch_irq(struct subchannel *sch) > { > struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev); > > + if (WARN_ON(!private)) > + return; > + I wonder, why a warning here vs quietly ignoring in all the other cases? (maybe add a small comment) Also, kind of wondering what serialization is protecting the state of private (e.g. is it possible that you got a nonzero pointer from dev_get_drvdata but now it's been freed by a remove) > inc_irq_stat(IRQIO_CIO); > vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT); > } > @@ -260,6 +266,9 @@ static void vfio_ccw_sch_remove(struct subchannel *sch) > { > struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev); > > + if (!private) > + return; > + > vfio_ccw_sch_quiesce(sch); > mdev_unregister_device(&sch->dev); > > @@ -293,6 +302,9 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process) > unsigned long flags; > int rc = -EAGAIN; > > + if (!private) > + return 0; > + > spin_lock_irqsave(sch->lock, flags); > if (!device_is_registered(&sch->dev)) > goto out_unlock; > diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c > index 497e1b7ffd61..9e5c184eab89 100644 > --- a/drivers/s390/cio/vfio_ccw_ops.c > +++ b/drivers/s390/cio/vfio_ccw_ops.c > @@ -152,6 +152,9 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev) > { > struct vfio_ccw_private *private = dev_get_drvdata(mdev->dev.parent); > > + if (!private) > + return; > + > VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: remove\n", > private->sch->schid.cssid, > private->sch->schid.ssid,
On Thu, Jun 02, 2022 at 07:19:38PM +0200, Eric Farman wrote: > The subchannel->dev->drvdata pointer is set in vfio_ccw_sch_probe() > and cleared in vfio_ccw_sch_remove(). In a purely defensive move, > let's check that the resultant pointer exists before operating on it > any way, since some lifecycle changes are forthcoming. There should be no possibility for the drvdata to be NULL in any of these routines, if that happens there is some kernel race bug with removal. Thus all of these should be WARN_ON to document that they cannot happen. Jason
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c index 3784eb4cda85..f8226db26e54 100644 --- a/drivers/s390/cio/vfio_ccw_drv.c +++ b/drivers/s390/cio/vfio_ccw_drv.c @@ -41,6 +41,9 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch) DECLARE_COMPLETION_ONSTACK(completion); int iretry, ret = 0; + if (!private) + return 0; + spin_lock_irq(sch->lock); if (!sch->schib.pmcw.ena) goto out_unlock; @@ -132,6 +135,9 @@ static void vfio_ccw_sch_irq(struct subchannel *sch) { struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev); + if (WARN_ON(!private)) + return; + inc_irq_stat(IRQIO_CIO); vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT); } @@ -260,6 +266,9 @@ static void vfio_ccw_sch_remove(struct subchannel *sch) { struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev); + if (!private) + return; + vfio_ccw_sch_quiesce(sch); mdev_unregister_device(&sch->dev); @@ -293,6 +302,9 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process) unsigned long flags; int rc = -EAGAIN; + if (!private) + return 0; + spin_lock_irqsave(sch->lock, flags); if (!device_is_registered(&sch->dev)) goto out_unlock; diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c index 497e1b7ffd61..9e5c184eab89 100644 --- a/drivers/s390/cio/vfio_ccw_ops.c +++ b/drivers/s390/cio/vfio_ccw_ops.c @@ -152,6 +152,9 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev) { struct vfio_ccw_private *private = dev_get_drvdata(mdev->dev.parent); + if (!private) + return; + VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: remove\n", private->sch->schid.cssid, private->sch->schid.ssid,
The subchannel->dev->drvdata pointer is set in vfio_ccw_sch_probe() and cleared in vfio_ccw_sch_remove(). In a purely defensive move, let's check that the resultant pointer exists before operating on it any way, since some lifecycle changes are forthcoming. Signed-off-by: Eric Farman <farman@linux.ibm.com> --- drivers/s390/cio/vfio_ccw_drv.c | 12 ++++++++++++ drivers/s390/cio/vfio_ccw_ops.c | 3 +++ 2 files changed, 15 insertions(+)