[PATCH] vfio/ccw: Remove WARN_ON during shutdown

[PATCH] vfio/ccw: Remove WARN_ON during shutdown

[Eric Farman]

The logic in vfio_ccw_sch_shutdown() always assumed that the input
subchannel would point to a vfio_ccw_private struct, without checking
that one exists. The blamed commit put in a check for this scenario,
to prevent the possibility of a missing private.

The trouble is that check was put alongside a WARN_ON(), presuming
that such a scenario would be a cause for concern. But this can be
triggered by binding a subchannel to vfio-ccw, and rebooting the
system before starting the mdev (via "mdevctl start" or similar)
or after stopping it. In those cases, shutdown doesn't need to
worry because either the private was never allocated, or it was
cleaned up by vfio_ccw_mdev_remove().

Remove the WARN_ON() piece of this check, since there are plausible
scenarios where private would be NULL in this path.

Fixes: 9e6f07cd1eaa ("vfio/ccw: create a parent struct")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 54aba7cceb33..ff538a086fc7 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -225,7 +225,7 @@ static void vfio_ccw_sch_shutdown(struct subchannel *sch)
 	struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
 	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
 
-	if (WARN_ON(!private))
+	if (!private)
 		return;
 
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
-- 
2.37.2

Re: [PATCH] vfio/ccw: Remove WARN_ON during shutdown

[Matthew Rosato]

On 2/10/23 12:42 PM, Eric Farman wrote:
> The logic in vfio_ccw_sch_shutdown() always assumed that the input
> subchannel would point to a vfio_ccw_private struct, without checking
> that one exists. The blamed commit put in a check for this scenario,
> to prevent the possibility of a missing private.
> 
> The trouble is that check was put alongside a WARN_ON(), presuming
> that such a scenario would be a cause for concern. But this can be
> triggered by binding a subchannel to vfio-ccw, and rebooting the
> system before starting the mdev (via "mdevctl start" or similar)
> or after stopping it. In those cases, shutdown doesn't need to
> worry because either the private was never allocated, or it was
> cleaned up by vfio_ccw_mdev_remove().
> 
> Remove the WARN_ON() piece of this check, since there are plausible
> scenarios where private would be NULL in this path.
> 
> Fixes: 9e6f07cd1eaa ("vfio/ccw: create a parent struct")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

The other ops in vfio_ccw_sch_driver look OK in that they quietly tolerate this scenario -- with .irq being an exception but the rationale and what we log there makes sense to me (we shouldn't get an interrupt on a disabled subchannel) plus it's only a debug log not a WARN.

Re: [PATCH] vfio/ccw: Remove WARN_ON during shutdown

[Alex Williamson]

On Fri, 10 Feb 2023 18:42:27 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> The logic in vfio_ccw_sch_shutdown() always assumed that the input
> subchannel would point to a vfio_ccw_private struct, without checking
> that one exists. The blamed commit put in a check for this scenario,
> to prevent the possibility of a missing private.
> 
> The trouble is that check was put alongside a WARN_ON(), presuming
> that such a scenario would be a cause for concern. But this can be
> triggered by binding a subchannel to vfio-ccw, and rebooting the
> system before starting the mdev (via "mdevctl start" or similar)
> or after stopping it. In those cases, shutdown doesn't need to
> worry because either the private was never allocated, or it was
> cleaned up by vfio_ccw_mdev_remove().
> 
> Remove the WARN_ON() piece of this check, since there are plausible
> scenarios where private would be NULL in this path.
> 
> Fixes: 9e6f07cd1eaa ("vfio/ccw: create a parent struct")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 54aba7cceb33..ff538a086fc7 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -225,7 +225,7 @@ static void vfio_ccw_sch_shutdown(struct subchannel *sch)
>  	struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
>  	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
>  
> -	if (WARN_ON(!private))
> +	if (!private)
>  		return;
>  
>  	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);

I see I'm on the To: line here, is this intended to go through the vfio
tree rather than s390?  Thanks,

Alex

Re: [PATCH] vfio/ccw: Remove WARN_ON during shutdown

[Eric Farman]

On Fri, 2023-02-10 at 14:30 -0700, Alex Williamson wrote:
> On Fri, 10 Feb 2023 18:42:27 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
> > The logic in vfio_ccw_sch_shutdown() always assumed that the input
> > subchannel would point to a vfio_ccw_private struct, without
> > checking
> > that one exists. The blamed commit put in a check for this
> > scenario,
> > to prevent the possibility of a missing private.
> > 
> > The trouble is that check was put alongside a WARN_ON(), presuming
> > that such a scenario would be a cause for concern. But this can be
> > triggered by binding a subchannel to vfio-ccw, and rebooting the
> > system before starting the mdev (via "mdevctl start" or similar)
> > or after stopping it. In those cases, shutdown doesn't need to
> > worry because either the private was never allocated, or it was
> > cleaned up by vfio_ccw_mdev_remove().
> > 
> > Remove the WARN_ON() piece of this check, since there are plausible
> > scenarios where private would be NULL in this path.
> > 
> > Fixes: 9e6f07cd1eaa ("vfio/ccw: create a parent struct")
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >  drivers/s390/cio/vfio_ccw_drv.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_drv.c
> > b/drivers/s390/cio/vfio_ccw_drv.c
> > index 54aba7cceb33..ff538a086fc7 100644
> > --- a/drivers/s390/cio/vfio_ccw_drv.c
> > +++ b/drivers/s390/cio/vfio_ccw_drv.c
> > @@ -225,7 +225,7 @@ static void vfio_ccw_sch_shutdown(struct
> > subchannel *sch)
> >         struct vfio_ccw_parent *parent = dev_get_drvdata(&sch-
> > >dev);
> >         struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> > >dev);
> >  
> > -       if (WARN_ON(!private))
> > +       if (!private)
> >                 return;
> >  
> >         vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
> 
> I see I'm on the To: line here, is this intended to go through the
> vfio
> tree rather than s390? 

Either way. I put you as "to" as the blamed commit went via vfio, but I
could ask Heiko or Vasili to take it if that's easier.

Eric

Re: [PATCH] vfio/ccw: Remove WARN_ON during shutdown

[Heiko Carstens]

On Fri, Feb 10, 2023 at 08:24:05PM -0500, Eric Farman wrote:
> On Fri, 2023-02-10 at 14:30 -0700, Alex Williamson wrote:
> > On Fri, 10 Feb 2023 18:42:27 +0100
> > Eric Farman <farman@linux.ibm.com> wrote:
> > 
> > > The logic in vfio_ccw_sch_shutdown() always assumed that the input
> > > subchannel would point to a vfio_ccw_private struct, without
> > > checking
> > > that one exists. The blamed commit put in a check for this
> > > scenario,
> > > to prevent the possibility of a missing private.
> > > 
> > > The trouble is that check was put alongside a WARN_ON(), presuming
> > > that such a scenario would be a cause for concern. But this can be
> > > triggered by binding a subchannel to vfio-ccw, and rebooting the
> > > system before starting the mdev (via "mdevctl start" or similar)
> > > or after stopping it. In those cases, shutdown doesn't need to
> > > worry because either the private was never allocated, or it was
> > > cleaned up by vfio_ccw_mdev_remove().
> > > 
> > > Remove the WARN_ON() piece of this check, since there are plausible
> > > scenarios where private would be NULL in this path.
> > > 
> > > Fixes: 9e6f07cd1eaa ("vfio/ccw: create a parent struct")
> > > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > > ---
> > >  drivers/s390/cio/vfio_ccw_drv.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
...
> > I see I'm on the To: line here, is this intended to go through the
> > vfio
> > tree rather than s390? 
> 
> Either way. I put you as "to" as the blamed commit went via vfio, but I
> could ask Heiko or Vasili to take it if that's easier.

I picked it up, so it will go via s390. Thanks!