Re: [qemu-s390x] [PATCH v3 1/6] vfio-ccw: make it safe to access channel programs

[Eric Farman]

On 02/05/2019 07:03 AM, Cornelia Huck wrote:
> On Mon, 4 Feb 2019 14:25:34 -0500
> Eric Farman <address@hidden> wrote:
>
> > On 01/30/2019 08:22 AM, Cornelia Huck wrote:
> > > When we get a solicited interrupt, the start function may have
> > > been cleared by a csch, but we still have a channel program
> > > structure allocated. Make it safe to call the cp accessors in
> > > any case, so we can call them unconditionally.
> > >
> > > While at it, also make sure that functions called from other parts
> > > of the code return gracefully if the channel program structure
> > > has not been initialized (even though that is a bug in the caller).
> > >
> > > Signed-off-by: Cornelia Huck <address@hidden>
> > > ---
> > >    drivers/s390/cio/vfio_ccw_cp.c  | 20 +++++++++++++++++++-
> > >    drivers/s390/cio/vfio_ccw_cp.h  |  2 ++
> > >    drivers/s390/cio/vfio_ccw_fsm.c |  5 +++++
> > >    3 files changed, 26 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> > > index ba08fe137c2e..0bc0c38edda7 100644
> > > --- a/drivers/s390/cio/vfio_ccw_cp.c
> > > +++ b/drivers/s390/cio/vfio_ccw_cp.c
> > > @@ -335,6 +335,7 @@ static void cp_unpin_free(struct channel_program *cp)
> > >         struct ccwchain *chain, *temp;
> > >         int i;
> > >    
> > > +	cp->initialized = false;
> > >         list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
> > >                 for (i = 0; i < chain->ch_len; i++) {
> > >                         pfn_array_table_unpin_free(chain->ch_pat + i,
> > > @@ -701,6 +702,8 @@ int cp_init(struct channel_program *cp, struct device 
> > > *mdev, union orb *orb)
> > >          */
> > >         cp->orb.cmd.c64 = 1;
> > >    
> > > +	cp->initialized = true;
> > > +
> >
> > Not seen in this hunk, but we call ccwchain_loop_tic() just prior to
> > this point.  If that returns non-zero, we call cp_unpin_free()[1] (and
> > set initailized to false), and then fall through to here.  So this is
> > going to set initialized to true, even though we're taking an error
> > path.  :-(
>
> Eek, setting c64 unconditionally threw me off. This needs to check
> for !ret, of course.
>
> > [1] Wait, why is it calling cp_unpin_free()?  Oh, I had proposed
> > squashing cp_free() and cp_unpin_free() back in November[2], got an r-b
> > from Pierre but haven't gotten back to tidy up the series for a v2.
> > Okay, I'll try to do that again soon.  :-)
>
> :)
>
> > [2] https://patchwork.kernel.org/patch/10675261/
> >
> > >         return ret;
> > >    }
> > >    
> > > @@ -715,7 +718,8 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
> > >     */
> > >    void cp_free(struct channel_program *cp)
> > >    {
> > > -       cp_unpin_free(cp);
> > > +       if (cp->initialized)
> > > +               cp_unpin_free(cp);
> > >    }
> > >    
> > >    /**
> > > @@ -760,6 +764,10 @@ int cp_prefetch(struct channel_program *cp)
> > >         struct ccwchain *chain;
> > >         int len, idx, ret;
> > >    
> > > +	/* this is an error in the caller */
> > > +       if (!cp || !cp->initialized)
> > > +               return -EINVAL;
> > > +
> > >         list_for_each_entry(chain, &cp->ccwchain_list, next) {
> > >                 len = chain->ch_len;
> > >                 for (idx = 0; idx < len; idx++) {
> > > @@ -795,6 +803,10 @@ union orb *cp_get_orb(struct channel_program *cp, u32 
> > > intparm, u8 lpm)
> > >         struct ccwchain *chain;
> > >         struct ccw1 *cpa;
> > >    
> > > +	/* this is an error in the caller */
> > > +       if (!cp || !cp->initialized)
> > > +               return NULL;
> > > +
> > >         orb = &cp->orb;
> > >    
> > >    	orb->cmd.intparm = intparm;
> > > @@ -831,6 +843,9 @@ void cp_update_scsw(struct channel_program *cp, union scsw 
> > > *scsw)
> > >         u32 cpa = scsw->cmd.cpa;
> > >         u32 ccw_head, ccw_tail;
> > >    
> > > +	if (!cp->initialized)
> > > +               return;
> > > +
> > >         /*
> > >          * LATER:
> > >          * For now, only update the cmd.cpa part. We may need to deal with
> > > @@ -869,6 +884,9 @@ bool cp_iova_pinned(struct channel_program *cp, u64 iova)
> > >         struct ccwchain *chain;
> > >         int i;
> > >    
> > > +	if (!cp->initialized)
> >
> > So, two of the checks added above look for a nonzero cp pointer prior to
> > checking initialized, while two don't.  I guess cp can't be NULL, since
> > it's embedded in the private struct directly and that's only free'd when
> > we do vfio_ccw_sch_remove() ... But I guess some consistency in how we
> > look would be nice.
>
> The idea was: In which context is this called? Is there a legitimate
> reason for the caller to pass in an uninitialized cp, or would that
> mean the caller had messed up (and we should not trust cp to be !NULL
> either?)
>
> But you're right, that does look inconsistent. Always checking for
> cp != NULL probably looks least odd, although it is overkill. Opinions?

My opinion?  Since cp is embedded in vfio_ccw_private, rather than a 
pointer to a separately malloc'd struct, we pass &private->cp to those 
functions.  So a check for !cp doesn't really buy us anything because 
what we are actually concerned about is whether or not private is NULL, 
which only changes on the probe/remove boundaries.

> > > +               return false;
> > > +
> > >         list_for_each_entry(chain, &cp->ccwchain_list, next) {
> > >                 for (i = 0; i < chain->ch_len; i++)
> > >                         if (pfn_array_table_iova_pinned(chain->ch_pat + i,